diff mbox series

[v3,1/2] net/ice: factorize firmware loading

Message ID 20210629080632.30964-2-david.marchand@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers show
Series Support compressed firmwares | expand

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

David Marchand June 29, 2021, 8:06 a.m. UTC
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

Wang, Haiyue July 5, 2021, 1:43 a.m. UTC | #1
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
Wang, Haiyue July 5, 2021, 3:33 a.m. UTC | #2
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
David Marchand July 5, 2021, 7:08 a.m. UTC | #3
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?
Wang, Haiyue July 5, 2021, 8:02 a.m. UTC | #4
> -----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
David Marchand July 5, 2021, 8:33 a.m. UTC | #5
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?
Zhang, Qi Z July 5, 2021, 9:59 a.m. UTC | #6
> -----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
Wang, Haiyue July 5, 2021, 11:44 a.m. UTC | #7
> -----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
Wang, Haiyue July 5, 2021, 11:46 a.m. UTC | #8
> -----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
>
Wang, Haiyue July 5, 2021, 1:18 p.m. UTC | #9
> -----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
David Marchand July 5, 2021, 1:34 p.m. UTC | #10
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!
diff mbox series

Patch

diff --git a/drivers/net/ice/base/ice_osdep.h b/drivers/net/ice/base/ice_osdep.h
index 878c5597d4..78093adb00 100644
--- a/drivers/net/ice/base/ice_osdep.h
+++ b/drivers/net/ice/base/ice_osdep.h
@@ -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)
 
diff --git a/drivers/net/ice/ice_dcf_parent.c b/drivers/net/ice/ice_dcf_parent.c
index 19420a0f58..318239abdc 100644
--- a/drivers/net/ice/ice_dcf_parent.c
+++ b/drivers/net/ice/ice_dcf_parent.c
@@ -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,
diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 09e38590e5..d180b73c32 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -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,"
diff --git a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h
index 7f3c26fb6f..edafdf168b 100644
--- a/drivers/net/ice/ice_ethdev.h
+++ b/drivers/net/ice/ice_ethdev.h
@@ -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