[v3,1/2] net/ice: factorize firmware loading
Checks
Commit Message
Both "normal" and "dcf" inits have their copy of some firmware loading
code.
The DSN query is moved in specific parts for the "normal" and "dcf" init.
A common helper ice_load_pkg is then introduced and takes an adapter
pointer as its main input.
This helper takes care of finding the right firmware file and loading
it.
The adapter active_pkg_type field is set by this helper.
The ice_access macro is removed from the osdep.h header: osdep.h should
only hosts wrappers for base driver code.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
drivers/net/ice/base/ice_osdep.h | 6 --
drivers/net/ice/ice_dcf_parent.c | 97 ++-----------------
drivers/net/ice/ice_ethdev.c | 161 +++++++++++++++----------------
drivers/net/ice/ice_ethdev.h | 3 +-
4 files changed, 88 insertions(+), 179 deletions(-)
Comments
Hi David,
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of David Marchand
> Sent: Tuesday, June 29, 2021 16:07
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: [dpdk-dev] [PATCH v3 1/2] net/ice: factorize firmware loading
>
> Both "normal" and "dcf" inits have their copy of some firmware loading
> code.
>
> The DSN query is moved in specific parts for the "normal" and "dcf" init.
>
> A common helper ice_load_pkg is then introduced and takes an adapter
> pointer as its main input.
>
> This helper takes care of finding the right firmware file and loading
> it.
> The adapter active_pkg_type field is set by this helper.
>
> The ice_access macro is removed from the osdep.h header: osdep.h should
> only hosts wrappers for base driver code.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> drivers/net/ice/base/ice_osdep.h | 6 --
> drivers/net/ice/ice_dcf_parent.c | 97 ++-----------------
> drivers/net/ice/ice_ethdev.c | 161 +++++++++++++++----------------
> drivers/net/ice/ice_ethdev.h | 3 +-
> 4 files changed, 88 insertions(+), 179 deletions(-)
>
> + if (!use_dsn)
> + goto no_dsn;
> +
> + memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
> + snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
> + "ice-%016" PRIx64 ".pkg", dsn);
> + strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
> + ICE_MAX_PKG_FILENAME_SIZE);
> + if (!ice_access(strcat(pkg_file, opt_ddp_filename), 0))
> + goto load_fw;
> +
> + strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_DEFAULT,
> + ICE_MAX_PKG_FILENAME_SIZE);
> + if (!ice_access(strcat(pkg_file, opt_ddp_filename), 0))
> + goto load_fw;
> +
> +no_dsn:
> + strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
> + if (!ice_access(pkg_file, 0))
> + goto load_fw;
> + strncpy(pkg_file, ICE_PKG_FILE_DEFAULT, ICE_MAX_PKG_FILENAME_SIZE);
> + if (ice_access(pkg_file, 0)) {
> PMD_INIT_LOG(ERR, "failed to search file path\n");
> - return err;
> + return -1;
> }
>
> +load_fw:
> file = fopen(pkg_file, "rb");
> if (!file) {
> PMD_INIT_LOG(ERR, "failed to open file: %s\n", pkg_file);
> return -1;
> }
>
I'm wondering what's full name for ice firmware in F34, has any *.xz
postfix ? If so, the search method will also needs to be updated, since
we will check each file can be accessed:
#define ICE_PKG_FILE_DEFAULT "/lib/firmware/intel/ice/ddp/ice.pkg"
#define ICE_PKG_FILE_UPDATES "/lib/firmware/updates/intel/ice/ddp/ice.pkg"
> 2.23.0
Hi David & Qi,
> -----Original Message-----
> From: Wang, Haiyue
> Sent: Monday, July 5, 2021 09:43
> To: David Marchand <david.marchand@redhat.com>; dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v3 1/2] net/ice: factorize firmware loading
>
> Hi David,
>
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of David Marchand
> > Sent: Tuesday, June 29, 2021 16:07
> > To: dev@dpdk.org
> > Cc: Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > Subject: [dpdk-dev] [PATCH v3 1/2] net/ice: factorize firmware loading
> >
> > Both "normal" and "dcf" inits have their copy of some firmware loading
> > code.
> >
> > The DSN query is moved in specific parts for the "normal" and "dcf" init.
> >
> > A common helper ice_load_pkg is then introduced and takes an adapter
> > pointer as its main input.
> >
> > This helper takes care of finding the right firmware file and loading
> > it.
> > The adapter active_pkg_type field is set by this helper.
> >
> > The ice_access macro is removed from the osdep.h header: osdep.h should
> > only hosts wrappers for base driver code.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> > drivers/net/ice/base/ice_osdep.h | 6 --
> > drivers/net/ice/ice_dcf_parent.c | 97 ++-----------------
> > drivers/net/ice/ice_ethdev.c | 161 +++++++++++++++----------------
> > drivers/net/ice/ice_ethdev.h | 3 +-
> > 4 files changed, 88 insertions(+), 179 deletions(-)
> >
>
>
> > + if (!use_dsn)
> > + goto no_dsn;
> > +
> > + memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
> > + snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
> > + "ice-%016" PRIx64 ".pkg", dsn);
> > + strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
> > + ICE_MAX_PKG_FILENAME_SIZE);
> > + if (!ice_access(strcat(pkg_file, opt_ddp_filename), 0))
> > + goto load_fw;
> > +
> > + strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_DEFAULT,
> > + ICE_MAX_PKG_FILENAME_SIZE);
> > + if (!ice_access(strcat(pkg_file, opt_ddp_filename), 0))
> > + goto load_fw;
> > +
> > +no_dsn:
> > + strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
> > + if (!ice_access(pkg_file, 0))
> > + goto load_fw;
> > + strncpy(pkg_file, ICE_PKG_FILE_DEFAULT, ICE_MAX_PKG_FILENAME_SIZE);
> > + if (ice_access(pkg_file, 0)) {
> > PMD_INIT_LOG(ERR, "failed to search file path\n");
> > - return err;
> > + return -1;
> > }
> >
> > +load_fw:
> > file = fopen(pkg_file, "rb");
> > if (!file) {
> > PMD_INIT_LOG(ERR, "failed to open file: %s\n", pkg_file);
> > return -1;
> > }
> >
>
> I'm wondering what's full name for ice firmware in F34, has any *.xz
> postfix ? If so, the search method will also needs to be updated, since
> we will check each file can be accessed:
>
> #define ICE_PKG_FILE_DEFAULT "/lib/firmware/intel/ice/ddp/ice.pkg"
> #define ICE_PKG_FILE_UPDATES "/lib/firmware/updates/intel/ice/ddp/ice.pkg"
>
We need to update the default/comms search method:
I try the F34:
tree /lib/firmware/intel/ice/
/lib/firmware/intel/ice/
├── ddp
│ ├── ice-1.3.16.0.pkg.xz
│ └── ice.pkg.xz -> ice-1.3.16.0.pkg.xz
└── ddp-comms
└── ice_comms-1.3.20.0.pkg.xz
It matches the upstream version:
https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/intel/ice
d--------- ddp-comms 50 logstatsplain
d--------- ddp 44 logstatsplain
>
> > 2.23.0
On Mon, Jul 5, 2021 at 3:43 AM Wang, Haiyue <haiyue.wang@intel.com> wrote:
>
> Hi David,
>
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of David Marchand
> > Sent: Tuesday, June 29, 2021 16:07
> > To: dev@dpdk.org
> > Cc: Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > Subject: [dpdk-dev] [PATCH v3 1/2] net/ice: factorize firmware loading
> >
> > Both "normal" and "dcf" inits have their copy of some firmware loading
> > code.
> >
> > The DSN query is moved in specific parts for the "normal" and "dcf" init.
> >
> > A common helper ice_load_pkg is then introduced and takes an adapter
> > pointer as its main input.
> >
> > This helper takes care of finding the right firmware file and loading
> > it.
> > The adapter active_pkg_type field is set by this helper.
> >
> > The ice_access macro is removed from the osdep.h header: osdep.h should
> > only hosts wrappers for base driver code.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> > drivers/net/ice/base/ice_osdep.h | 6 --
> > drivers/net/ice/ice_dcf_parent.c | 97 ++-----------------
> > drivers/net/ice/ice_ethdev.c | 161 +++++++++++++++----------------
> > drivers/net/ice/ice_ethdev.h | 3 +-
> > 4 files changed, 88 insertions(+), 179 deletions(-)
> >
>
>
> > + if (!use_dsn)
> > + goto no_dsn;
> > +
> > + memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
> > + snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
> > + "ice-%016" PRIx64 ".pkg", dsn);
> > + strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
> > + ICE_MAX_PKG_FILENAME_SIZE);
> > + if (!ice_access(strcat(pkg_file, opt_ddp_filename), 0))
> > + goto load_fw;
> > +
> > + strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_DEFAULT,
> > + ICE_MAX_PKG_FILENAME_SIZE);
> > + if (!ice_access(strcat(pkg_file, opt_ddp_filename), 0))
> > + goto load_fw;
> > +
> > +no_dsn:
> > + strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
> > + if (!ice_access(pkg_file, 0))
> > + goto load_fw;
> > + strncpy(pkg_file, ICE_PKG_FILE_DEFAULT, ICE_MAX_PKG_FILENAME_SIZE);
> > + if (ice_access(pkg_file, 0)) {
> > PMD_INIT_LOG(ERR, "failed to search file path\n");
> > - return err;
> > + return -1;
> > }
> >
> > +load_fw:
> > file = fopen(pkg_file, "rb");
> > if (!file) {
> > PMD_INIT_LOG(ERR, "failed to open file: %s\n", pkg_file);
> > return -1;
> > }
> >
>
> I'm wondering what's full name for ice firmware in F34, has any *.xz
> postfix ? If so, the search method will also needs to be updated, since
> we will check each file can be accessed:
>
> #define ICE_PKG_FILE_DEFAULT "/lib/firmware/intel/ice/ddp/ice.pkg"
> #define ICE_PKG_FILE_UPDATES "/lib/firmware/updates/intel/ice/ddp/ice.pkg"
This first patch is a preparation to have a single helper to
select/open the firmware.
I don't get what you mean.
Is there a change in behavior with this patch?
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday, July 5, 2021 15:08
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 1/2] net/ice: factorize firmware loading
>
> On Mon, Jul 5, 2021 at 3:43 AM Wang, Haiyue <haiyue.wang@intel.com> wrote:
> >
> > Hi David,
> >
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of David Marchand
> > > Sent: Tuesday, June 29, 2021 16:07
> > > To: dev@dpdk.org
> > > Cc: Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Subject: [dpdk-dev] [PATCH v3 1/2] net/ice: factorize firmware loading
> > >
> > > Both "normal" and "dcf" inits have their copy of some firmware loading
> > > code.
> > >
> > > The DSN query is moved in specific parts for the "normal" and "dcf" init.
> > >
> > > A common helper ice_load_pkg is then introduced and takes an adapter
> > > pointer as its main input.
> > >
> > > This helper takes care of finding the right firmware file and loading
> > > it.
> > > The adapter active_pkg_type field is set by this helper.
> > >
> > > The ice_access macro is removed from the osdep.h header: osdep.h should
> > > only hosts wrappers for base driver code.
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > ---
> > > drivers/net/ice/base/ice_osdep.h | 6 --
> > > drivers/net/ice/ice_dcf_parent.c | 97 ++-----------------
> > > drivers/net/ice/ice_ethdev.c | 161 +++++++++++++++----------------
> > > drivers/net/ice/ice_ethdev.h | 3 +-
> > > 4 files changed, 88 insertions(+), 179 deletions(-)
> > >
> >
> >
> > > + if (!use_dsn)
> > > + goto no_dsn;
> > > +
> > > + memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
> > > + snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
> > > + "ice-%016" PRIx64 ".pkg", dsn);
> > > + strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
> > > + ICE_MAX_PKG_FILENAME_SIZE);
> > > + if (!ice_access(strcat(pkg_file, opt_ddp_filename), 0))
> > > + goto load_fw;
> > > +
> > > + strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_DEFAULT,
> > > + ICE_MAX_PKG_FILENAME_SIZE);
> > > + if (!ice_access(strcat(pkg_file, opt_ddp_filename), 0))
> > > + goto load_fw;
> > > +
> > > +no_dsn:
> > > + strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
> > > + if (!ice_access(pkg_file, 0))
> > > + goto load_fw;
> > > + strncpy(pkg_file, ICE_PKG_FILE_DEFAULT, ICE_MAX_PKG_FILENAME_SIZE);
> > > + if (ice_access(pkg_file, 0)) {
> > > PMD_INIT_LOG(ERR, "failed to search file path\n");
> > > - return err;
> > > + return -1;
> > > }
> > >
> > > +load_fw:
> > > file = fopen(pkg_file, "rb");
> > > if (!file) {
> > > PMD_INIT_LOG(ERR, "failed to open file: %s\n", pkg_file);
> > > return -1;
> > > }
> > >
> >
> > I'm wondering what's full name for ice firmware in F34, has any *.xz
> > postfix ? If so, the search method will also needs to be updated, since
> > we will check each file can be accessed:
> >
> > #define ICE_PKG_FILE_DEFAULT "/lib/firmware/intel/ice/ddp/ice.pkg"
> > #define ICE_PKG_FILE_UPDATES "/lib/firmware/updates/intel/ice/ddp/ice.pkg"
>
> This first patch is a preparation to have a single helper to
> select/open the firmware.
> I don't get what you mean.
Since the pkg file has the *.xz, now the search method doesn't work. You fix
the read only. ;-)
>
> Is there a change in behavior with this patch?
>
>
> --
> David Marchand
On Mon, Jul 5, 2021 at 10:02 AM Wang, Haiyue <haiyue.wang@intel.com> wrote:
> > > I'm wondering what's full name for ice firmware in F34, has any *.xz
> > > postfix ? If so, the search method will also needs to be updated, since
> > > we will check each file can be accessed:
> > >
> > > #define ICE_PKG_FILE_DEFAULT "/lib/firmware/intel/ice/ddp/ice.pkg"
> > > #define ICE_PKG_FILE_UPDATES "/lib/firmware/updates/intel/ice/ddp/ice.pkg"
> >
> > This first patch is a preparation to have a single helper to
> > select/open the firmware.
> > I don't get what you mean.
>
> Since the pkg file has the *.xz, now the search method doesn't work. You fix
> the read only. ;-)
This patch fixes nothing wrt to F34.
It simply prepares for the next patch, because I did not want to fix
in two places with the same change.
I can squash everything in a single patch if you prefer.
In the end, I end up with the same question, but for the whole series:
> > Is there a change in behavior with this patch?
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday, July 5, 2021 4:34 PM
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 1/2] net/ice: factorize firmware loading
>
> On Mon, Jul 5, 2021 at 10:02 AM Wang, Haiyue <haiyue.wang@intel.com>
> wrote:
> > > > I'm wondering what's full name for ice firmware in F34, has any
> > > > *.xz postfix ? If so, the search method will also needs to be
> > > > updated, since we will check each file can be accessed:
> > > >
> > > > #define ICE_PKG_FILE_DEFAULT "/lib/firmware/intel/ice/ddp/ice.pkg"
> > > > #define ICE_PKG_FILE_UPDATES
> "/lib/firmware/updates/intel/ice/ddp/ice.pkg"
> > >
> > > This first patch is a preparation to have a single helper to
> > > select/open the firmware.
> > > I don't get what you mean.
> >
> > Since the pkg file has the *.xz, now the search method doesn't work.
> > You fix the read only. ;-)
>
> This patch fixes nothing wrt to F34.
> It simply prepares for the next patch, because I did not want to fix in two places
> with the same change.
I agree with this , the patch just do code refactory, it looks good to have , no matter if we need this for F34 or not.
Haiyue, did you see any problem on F34 with both patches be applied or just this one be applied?
> I can squash everything in a single patch if you prefer.
>
>
> In the end, I end up with the same question, but for the whole series:
> > > Is there a change in behavior with this patch?
>
>
> --
> David Marchand
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday, July 5, 2021 16:34
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 1/2] net/ice: factorize firmware loading
>
> On Mon, Jul 5, 2021 at 10:02 AM Wang, Haiyue <haiyue.wang@intel.com> wrote:
> > > > I'm wondering what's full name for ice firmware in F34, has any *.xz
> > > > postfix ? If so, the search method will also needs to be updated, since
> > > > we will check each file can be accessed:
> > > >
> > > > #define ICE_PKG_FILE_DEFAULT "/lib/firmware/intel/ice/ddp/ice.pkg"
> > > > #define ICE_PKG_FILE_UPDATES "/lib/firmware/updates/intel/ice/ddp/ice.pkg"
> > >
> > > This first patch is a preparation to have a single helper to
> > > select/open the firmware.
> > > I don't get what you mean.
> >
> > Since the pkg file has the *.xz, now the search method doesn't work. You fix
> > the read only. ;-)
>
> This patch fixes nothing wrt to F34.
> It simply prepares for the next patch, because I did not want to fix
> in two places with the same change.
> I can squash everything in a single patch if you prefer.
>
>
> In the end, I end up with the same question, but for the whole series:
> > > Is there a change in behavior with this patch?
Sorry, after applied the whole patch to read the change, I get the real
design.
Looks like one patch is clear, since I'm lost in Patch#1, I thought it used
'*.pkg' to get the pkg name.... after applied, I read the source code directly,
yes, it’s clear in ice_load_pkg: the rte_firmware_read has handled "*.xz" right.
static int
ice_dcf_load_pkg(struct ice_adapter *adapter)
{
struct ice_dcf_adapter *dcf_adapter =
container_of(&adapter->hw, struct ice_dcf_adapter, parent.hw);
struct virtchnl_pkg_info pkg_info;
struct dcf_virtchnl_cmd vc_cmd;
bool use_dsn;
uint64_t dsn = 0;
vc_cmd.v_op = VIRTCHNL_OP_DCF_GET_PKG_INFO;
vc_cmd.req_msglen = 0;
vc_cmd.req_msg = NULL;
vc_cmd.rsp_buflen = sizeof(pkg_info);
vc_cmd.rsp_msgbuf = (uint8_t *)&pkg_info;
use_dsn = ice_dcf_execute_virtchnl_cmd(&dcf_adapter->real_hw, &vc_cmd) == 0;
if (use_dsn)
rte_memcpy(&dsn, pkg_info.dsn, sizeof(dsn));
return ice_load_pkg(adapter, use_dsn, dsn);
}
>
>
> --
> David Marchand
> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Monday, July 5, 2021 18:00
> To: David Marchand <david.marchand@redhat.com>; Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v3 1/2] net/ice: factorize firmware loading
>
>
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Monday, July 5, 2021 4:34 PM
> > To: Wang, Haiyue <haiyue.wang@intel.com>
> > Cc: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH v3 1/2] net/ice: factorize firmware loading
> >
> > On Mon, Jul 5, 2021 at 10:02 AM Wang, Haiyue <haiyue.wang@intel.com>
> > wrote:
> > > > > I'm wondering what's full name for ice firmware in F34, has any
> > > > > *.xz postfix ? If so, the search method will also needs to be
> > > > > updated, since we will check each file can be accessed:
> > > > >
> > > > > #define ICE_PKG_FILE_DEFAULT "/lib/firmware/intel/ice/ddp/ice.pkg"
> > > > > #define ICE_PKG_FILE_UPDATES
> > "/lib/firmware/updates/intel/ice/ddp/ice.pkg"
> > > >
> > > > This first patch is a preparation to have a single helper to
> > > > select/open the firmware.
> > > > I don't get what you mean.
> > >
> > > Since the pkg file has the *.xz, now the search method doesn't work.
> > > You fix the read only. ;-)
> >
> > This patch fixes nothing wrt to F34.
> > It simply prepares for the next patch, because I did not want to fix in two places
> > with the same change.
>
> I agree with this , the patch just do code refactory, it looks good to have , no matter if we need
> this for F34 or not.
>
> Haiyue, did you see any problem on F34 with both patches be applied or just this one be applied?
Just install a VM to see the firmware name, not test yet. ;-)
>
>
>
> > I can squash everything in a single patch if you prefer.
>
>
> >
> >
> > In the end, I end up with the same question, but for the whole series:
> > > > Is there a change in behavior with this patch?
> >
> >
> > --
> > David Marchand
>
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of David Marchand
> Sent: Tuesday, June 29, 2021 16:07
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: [dpdk-dev] [PATCH v3 1/2] net/ice: factorize firmware loading
>
> Both "normal" and "dcf" inits have their copy of some firmware loading
> code.
>
> The DSN query is moved in specific parts for the "normal" and "dcf" init.
>
> A common helper ice_load_pkg is then introduced and takes an adapter
> pointer as its main input.
>
> This helper takes care of finding the right firmware file and loading
> it.
> The adapter active_pkg_type field is set by this helper.
>
> The ice_access macro is removed from the osdep.h header: osdep.h should
> only hosts wrappers for base driver code.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> drivers/net/ice/base/ice_osdep.h | 6 --
> drivers/net/ice/ice_dcf_parent.c | 97 ++-----------------
> drivers/net/ice/ice_ethdev.c | 161 +++++++++++++++----------------
> drivers/net/ice/ice_ethdev.h | 3 +-
> 4 files changed, 88 insertions(+), 179 deletions(-)
>
Squash into a single patch seems great help to understand the compress firmware
context. ;-)
But it's OK as two patches.
Acked-by: Haiyue Wang <haiyue.wang@intel.com>
> --
> 2.23.0
On Mon, Jul 5, 2021 at 3:18 PM Wang, Haiyue <haiyue.wang@intel.com> wrote:
>
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of David Marchand
> > Sent: Tuesday, June 29, 2021 16:07
> > To: dev@dpdk.org
> > Cc: Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > Subject: [dpdk-dev] [PATCH v3 1/2] net/ice: factorize firmware loading
> >
> > Both "normal" and "dcf" inits have their copy of some firmware loading
> > code.
> >
> > The DSN query is moved in specific parts for the "normal" and "dcf" init.
> >
> > A common helper ice_load_pkg is then introduced and takes an adapter
> > pointer as its main input.
> >
> > This helper takes care of finding the right firmware file and loading
> > it.
> > The adapter active_pkg_type field is set by this helper.
> >
> > The ice_access macro is removed from the osdep.h header: osdep.h should
> > only hosts wrappers for base driver code.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> > drivers/net/ice/base/ice_osdep.h | 6 --
> > drivers/net/ice/ice_dcf_parent.c | 97 ++-----------------
> > drivers/net/ice/ice_ethdev.c | 161 +++++++++++++++----------------
> > drivers/net/ice/ice_ethdev.h | 3 +-
> > 4 files changed, 88 insertions(+), 179 deletions(-)
> >
>
> Squash into a single patch seems great help to understand the compress firmware
> context. ;-)
>
> But it's OK as two patches.
>
> Acked-by: Haiyue Wang <haiyue.wang@intel.com>
Cool, thanks for the review and test!
@@ -74,12 +74,6 @@ typedef uint64_t s64;
#define min(a, b) RTE_MIN(a, b)
#define max(a, b) RTE_MAX(a, b)
-#ifdef RTE_EXEC_ENV_WINDOWS
-#define ice_access _access
-#else
-#define ice_access access
-#endif
-
#define FIELD_SIZEOF(t, f) RTE_SIZEOF_FIELD(t, f)
#define ARRAY_SIZE(arr) RTE_DIM(arr)
@@ -298,13 +298,14 @@ static void ice_dcf_uninit_parent_hw(struct ice_hw *hw)
}
static int
-ice_dcf_request_pkg_name(struct ice_hw *hw, char *pkg_name)
+ice_dcf_load_pkg(struct ice_adapter *adapter)
{
struct ice_dcf_adapter *dcf_adapter =
- container_of(hw, struct ice_dcf_adapter, parent.hw);
+ container_of(&adapter->hw, struct ice_dcf_adapter, parent.hw);
struct virtchnl_pkg_info pkg_info;
struct dcf_virtchnl_cmd vc_cmd;
- uint64_t dsn;
+ bool use_dsn;
+ uint64_t dsn = 0;
vc_cmd.v_op = VIRTCHNL_OP_DCF_GET_PKG_INFO;
vc_cmd.req_msglen = 0;
@@ -312,90 +313,11 @@ ice_dcf_request_pkg_name(struct ice_hw *hw, char *pkg_name)
vc_cmd.rsp_buflen = sizeof(pkg_info);
vc_cmd.rsp_msgbuf = (uint8_t *)&pkg_info;
- if (ice_dcf_execute_virtchnl_cmd(&dcf_adapter->real_hw, &vc_cmd))
- goto pkg_file_direct;
+ use_dsn = ice_dcf_execute_virtchnl_cmd(&dcf_adapter->real_hw, &vc_cmd) == 0;
+ if (use_dsn)
+ rte_memcpy(&dsn, pkg_info.dsn, sizeof(dsn));
- rte_memcpy(&dsn, pkg_info.dsn, sizeof(dsn));
-
- snprintf(pkg_name, ICE_MAX_PKG_FILENAME_SIZE,
- ICE_PKG_FILE_SEARCH_PATH_UPDATES "ice-%016llx.pkg",
- (unsigned long long)dsn);
- if (!ice_access(pkg_name, 0))
- return 0;
-
- snprintf(pkg_name, ICE_MAX_PKG_FILENAME_SIZE,
- ICE_PKG_FILE_SEARCH_PATH_DEFAULT "ice-%016llx.pkg",
- (unsigned long long)dsn);
- if (!ice_access(pkg_name, 0))
- return 0;
-
-pkg_file_direct:
- snprintf(pkg_name,
- ICE_MAX_PKG_FILENAME_SIZE, "%s", ICE_PKG_FILE_UPDATES);
- if (!ice_access(pkg_name, 0))
- return 0;
-
- snprintf(pkg_name,
- ICE_MAX_PKG_FILENAME_SIZE, "%s", ICE_PKG_FILE_DEFAULT);
- if (!ice_access(pkg_name, 0))
- return 0;
-
- return -1;
-}
-
-static int
-ice_dcf_load_pkg(struct ice_hw *hw)
-{
- char pkg_name[ICE_MAX_PKG_FILENAME_SIZE];
- uint8_t *pkg_buf;
- uint32_t buf_len;
- struct stat st;
- FILE *fp;
- int err;
-
- if (ice_dcf_request_pkg_name(hw, pkg_name)) {
- PMD_INIT_LOG(ERR, "Failed to locate the package file");
- return -ENOENT;
- }
-
- PMD_INIT_LOG(DEBUG, "DDP package name: %s", pkg_name);
-
- err = stat(pkg_name, &st);
- if (err) {
- PMD_INIT_LOG(ERR, "Failed to get file status");
- return err;
- }
-
- buf_len = st.st_size;
- pkg_buf = rte_malloc(NULL, buf_len, 0);
- if (!pkg_buf) {
- PMD_INIT_LOG(ERR, "failed to allocate buffer of size %u for package",
- buf_len);
- return -1;
- }
-
- fp = fopen(pkg_name, "rb");
- if (!fp) {
- PMD_INIT_LOG(ERR, "failed to open file: %s", pkg_name);
- err = -1;
- goto ret;
- }
-
- err = fread(pkg_buf, buf_len, 1, fp);
- fclose(fp);
- if (err != 1) {
- PMD_INIT_LOG(ERR, "failed to read package data");
- err = -1;
- goto ret;
- }
-
- err = ice_copy_and_init_pkg(hw, pkg_buf, buf_len);
- if (err)
- PMD_INIT_LOG(ERR, "ice_copy_and_init_hw failed: %d", err);
-
-ret:
- rte_free(pkg_buf);
- return err;
+ return ice_load_pkg(adapter, use_dsn, dsn);
}
int
@@ -435,13 +357,12 @@ ice_dcf_init_parent_adapter(struct rte_eth_dev *eth_dev)
return err;
}
- err = ice_dcf_load_pkg(parent_hw);
+ err = ice_dcf_load_pkg(parent_adapter);
if (err) {
PMD_INIT_LOG(ERR, "failed to load package with error %d",
err);
goto uninit_hw;
}
- parent_adapter->active_pkg_type = ice_load_pkg_type(parent_hw);
parent_adapter->pf.main_vsi->idx = hw->num_vfs;
ice_dcf_update_pf_vsi_map(parent_hw,
@@ -1649,57 +1649,7 @@ ice_pf_setup(struct ice_pf *pf)
return 0;
}
-/*
- * Extract device serial number from PCIe Configuration Space and
- * determine the pkg file path according to the DSN.
- */
-#ifndef RTE_EXEC_ENV_WINDOWS
-static int
-ice_pkg_file_search_path(struct rte_pci_device *pci_dev, char *pkg_file)
-{
- off_t pos;
- char opt_ddp_filename[ICE_MAX_PKG_FILENAME_SIZE];
- uint32_t dsn_low, dsn_high;
- memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
-
- pos = rte_pci_find_ext_capability(pci_dev, RTE_PCI_EXT_CAP_ID_DSN);
-
- if (pos) {
- if (rte_pci_read_config(pci_dev, &dsn_low, 4, pos + 4) < 0) {
- PMD_INIT_LOG(ERR, "Failed to read pci config space\n");
- return -1;
- }
- if (rte_pci_read_config(pci_dev, &dsn_high, 4, pos + 8) < 0) {
- PMD_INIT_LOG(ERR, "Failed to read pci config space\n");
- return -1;
- }
- snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
- "ice-%08x%08x.pkg", dsn_high, dsn_low);
- } else {
- PMD_INIT_LOG(ERR, "Failed to read device serial number\n");
- goto fail_dsn;
- }
-
- strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
- ICE_MAX_PKG_FILENAME_SIZE);
- if (!ice_access(strcat(pkg_file, opt_ddp_filename), 0))
- return 0;
-
- strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_DEFAULT,
- ICE_MAX_PKG_FILENAME_SIZE);
- if (!ice_access(strcat(pkg_file, opt_ddp_filename), 0))
- return 0;
-
-fail_dsn:
- strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
- if (!ice_access(pkg_file, 0))
- return 0;
- strncpy(pkg_file, ICE_PKG_FILE_DEFAULT, ICE_MAX_PKG_FILENAME_SIZE);
- return 0;
-}
-#endif
-
-enum ice_pkg_type
+static enum ice_pkg_type
ice_load_pkg_type(struct ice_hw *hw)
{
enum ice_pkg_type package_type;
@@ -1723,37 +1673,62 @@ ice_load_pkg_type(struct ice_hw *hw)
return package_type;
}
-#ifndef RTE_EXEC_ENV_WINDOWS
-static int ice_load_pkg(struct rte_eth_dev *dev)
+#ifdef RTE_EXEC_ENV_WINDOWS
+#define ice_access _access
+#else
+#define ice_access access
+#endif
+
+int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
{
- struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+ struct ice_hw *hw = &adapter->hw;
char pkg_file[ICE_MAX_PKG_FILENAME_SIZE];
+ char opt_ddp_filename[ICE_MAX_PKG_FILENAME_SIZE];
int err;
- uint8_t *buf;
+ uint8_t *buf = NULL;
int buf_len;
FILE *file;
struct stat fstat;
- struct rte_pci_device *pci_dev = RTE_DEV_TO_PCI(dev->device);
- struct ice_adapter *ad =
- ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
- err = ice_pkg_file_search_path(pci_dev, pkg_file);
- if (err) {
+ if (!use_dsn)
+ goto no_dsn;
+
+ memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
+ snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
+ "ice-%016" PRIx64 ".pkg", dsn);
+ strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
+ ICE_MAX_PKG_FILENAME_SIZE);
+ if (!ice_access(strcat(pkg_file, opt_ddp_filename), 0))
+ goto load_fw;
+
+ strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_DEFAULT,
+ ICE_MAX_PKG_FILENAME_SIZE);
+ if (!ice_access(strcat(pkg_file, opt_ddp_filename), 0))
+ goto load_fw;
+
+no_dsn:
+ strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
+ if (!ice_access(pkg_file, 0))
+ goto load_fw;
+ strncpy(pkg_file, ICE_PKG_FILE_DEFAULT, ICE_MAX_PKG_FILENAME_SIZE);
+ if (ice_access(pkg_file, 0)) {
PMD_INIT_LOG(ERR, "failed to search file path\n");
- return err;
+ return -1;
}
+load_fw:
file = fopen(pkg_file, "rb");
if (!file) {
PMD_INIT_LOG(ERR, "failed to open file: %s\n", pkg_file);
return -1;
}
+ PMD_INIT_LOG(DEBUG, "DDP package name: %s", pkg_file);
+
err = stat(pkg_file, &fstat);
if (err) {
PMD_INIT_LOG(ERR, "failed to get file stats\n");
- fclose(file);
- return err;
+ goto out;
}
buf_len = fstat.st_size;
@@ -1762,44 +1737,33 @@ static int ice_load_pkg(struct rte_eth_dev *dev)
if (!buf) {
PMD_INIT_LOG(ERR, "failed to allocate buf of size %d for package\n",
buf_len);
- fclose(file);
- return -1;
+ err = -1;
+ goto out;
}
err = fread(buf, buf_len, 1, file);
if (err != 1) {
PMD_INIT_LOG(ERR, "failed to read package data\n");
- fclose(file);
err = -1;
- goto fail_exit;
+ goto out;
}
- fclose(file);
-
err = ice_copy_and_init_pkg(hw, buf, buf_len);
if (err) {
PMD_INIT_LOG(ERR, "ice_copy_and_init_hw failed: %d\n", err);
- goto fail_exit;
+ goto out;
}
/* store the loaded pkg type info */
- ad->active_pkg_type = ice_load_pkg_type(hw);
+ adapter->active_pkg_type = ice_load_pkg_type(hw);
- err = ice_init_hw_tbls(hw);
- if (err) {
- PMD_INIT_LOG(ERR, "ice_init_hw_tbls failed: %d\n", err);
- goto fail_init_tbls;
- }
-
- return 0;
-
-fail_init_tbls:
- rte_free(hw->pkg_copy);
-fail_exit:
+out:
+ fclose(file);
rte_free(buf);
return err;
}
-#endif
+
+#undef ice_access
static void
ice_base_queue_get(struct ice_pf *pf)
@@ -2030,6 +1994,12 @@ ice_dev_init(struct rte_eth_dev *dev)
ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
struct ice_vsi *vsi;
int ret;
+#ifndef RTE_EXEC_ENV_WINDOWS
+ off_t pos;
+ uint32_t dsn_low, dsn_high;
+ uint64_t dsn;
+ bool use_dsn;
+#endif
dev->dev_ops = &ice_eth_dev_ops;
dev->rx_queue_count = ice_rx_queue_count;
@@ -2080,7 +2050,30 @@ ice_dev_init(struct rte_eth_dev *dev)
}
#ifndef RTE_EXEC_ENV_WINDOWS
- ret = ice_load_pkg(dev);
+ use_dsn = false;
+ dsn = 0;
+ pos = rte_pci_find_ext_capability(pci_dev, RTE_PCI_EXT_CAP_ID_DSN);
+ if (pos) {
+ if (rte_pci_read_config(pci_dev, &dsn_low, 4, pos + 4) < 0 ||
+ rte_pci_read_config(pci_dev, &dsn_high, 4, pos + 8) < 0) {
+ PMD_INIT_LOG(ERR, "Failed to read pci config space\n");
+ } else {
+ use_dsn = true;
+ dsn = (uint64_t)dsn_high << 32 | dsn_low;
+ }
+ } else {
+ PMD_INIT_LOG(ERR, "Failed to read device serial number\n");
+ }
+
+ ret = ice_load_pkg(pf->adapter, use_dsn, dsn);
+ if (ret == 0) {
+ ret = ice_init_hw_tbls(hw);
+ if (ret) {
+ PMD_INIT_LOG(ERR, "ice_init_hw_tbls failed: %d\n", ret);
+ rte_free(hw->pkg_copy);
+ }
+ }
+
if (ret) {
if (ad->devargs.safe_mode_support == 0) {
PMD_INIT_LOG(ERR, "Failed to load the DDP package,"
@@ -534,7 +534,8 @@ struct ice_vsi_vlan_pvid_info {
#define ICE_PF_TO_ETH_DEV(pf) \
(((struct ice_pf *)pf)->adapter->eth_dev)
-enum ice_pkg_type ice_load_pkg_type(struct ice_hw *hw);
+int
+ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn);
struct ice_vsi *
ice_setup_vsi(struct ice_pf *pf, enum ice_vsi_type type);
int