[PATCH net-next 0/3] nfp: extend firmware request logic

[PATCH net-next 0/3] nfp: extend firmware request logic

[Jakub Kicinski]

Hi!

We have been pondering for some time how to support loading different
application firmwares onto NFP.  We want to support both users selecting
one of the firmware images provided by Netronome (which are optimized
for different use cases each) as well as firmware images created  by 
users themselves or other companies.

In the end we decided to go with the simplest solution - depending on
the right firmware image being placed in /lib/firmware.  This vastly
simplifies driver logic and also doesn't require any new API.

Different NICs on one system may want to run different applications
therefore we first try to load firmware specific to the device (by 
serial number of PCI slot) and if not present try the device model
based name we have been using so far.


Jakub Kicinski (3):
  nfp: remove the probe deferral when FW not present
  nfp: look for firmware image by device serial number and PCI name
  nfp: only use direct firmware requests

 drivers/net/ethernet/netronome/nfp/nfp_main.c     | 48 ++++++++++++++++++-----
 drivers/net/ethernet/netronome/nfp/nfp_net_main.c |  2 +-
 2 files changed, 39 insertions(+), 11 deletions(-)

-- 
2.11.0

[PATCH net-next 1/3] nfp: remove the probe deferral when FW not present

[Jakub Kicinski]

We use a hack to defer probe when firmware was not pre-loaded
or found on disk.  This helps in case users forgot to include
firmware in initramfs, the driver will most likely get another
shot at probing after real root is mounted.

This is not for what EPROBE_DEFER is supposed to be used, and
when FW is completely missing every time new device is probed
NFP will reprobe spamming kernel logs.

Remove this hack, users will now have to make sure the right
firmware image is present in initramfs if nfp.ko is placed
there or built in.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/nfp_net_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
index 5797dbf2b507..d5e2361f0e86 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
@@ -704,7 +704,7 @@ int nfp_net_pci_probe(struct nfp_pf *pf)
 	if (!pf->rtbl) {
 		nfp_err(pf->cpp, "No %s, giving up.\n",
 			pf->fw_loaded ? "symbol table" : "firmware found");
-		return -EPROBE_DEFER;
+		return -EINVAL;
 	}
 
 	mutex_lock(&pf->lock);
-- 
2.11.0

[PATCH net-next 2/3] nfp: look for firmware image by device serial number and PCI name

[Jakub Kicinski]

We generally look up firmware by card type, but that doesn't allow
users who have more than one card of the same type in their system
to select firmware per adapter.

Unfortunately user space firmware helper seems fraught with
difficulties and to be on its way out.  In particular support for
handling firmware uevents have been dropped from systemd and most
distributions don't enable the FW fallback by default any more.

To allow users selecting firmware for a particular device look up
firmware names by serial and pci_name().  Use the direct lookup to
disable generating uevents when enabled in Kconfig and not print
any warnings to logs if adapter-specific files are missing.  Users
can place in /lib/firmware/netronome files named:

pci-${pci_name}.nffw
serial-${serial}.nffw

to target a specific card.  E.g.:

pci-0000:04:00.0.nffw
pci-0000:82:00.0.nffw
serial-00-aa-bb-11-22-33-10-ff.nffw

We use the full serial number including the interface id, as it
appears in lspci output (bytes separated by '-').

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/nfp_main.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.c b/drivers/net/ethernet/netronome/nfp/nfp_main.c
index d67969d3e484..13d056da0765 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_main.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_main.c
@@ -188,9 +188,27 @@ nfp_net_fw_find(struct pci_dev *pdev, struct nfp_pf *pf)
 	struct nfp_eth_table_port *port;
 	const char *fw_model;
 	char fw_name[256];
+	const u8 *serial;
 	int spc, err = 0;
+	u16 interface;
 	int i, j;
 
+	/* First try to find a firmware image specific for this device */
+	interface = nfp_cpp_interface(pf->cpp);
+	nfp_cpp_serial(pf->cpp, &serial);
+	sprintf(fw_name, "netronome/serial-%pMF-%02hhx-%02hhx.nffw",
+		serial, interface >> 8, interface & 0xff);
+	err = request_firmware_direct(&fw, fw_name, &pdev->dev);
+	if (!err)
+		goto done;
+
+	/* Then try the PCI name */
+	sprintf(fw_name, "netronome/pci-%s.nffw", pci_name(pdev));
+	err = request_firmware_direct(&fw, fw_name, &pdev->dev);
+	if (!err)
+		goto done;
+
+	/* Finally try the card type and media */
 	if (!pf->eth_tbl) {
 		dev_err(&pdev->dev, "Error: can't identify media config\n");
 		return NULL;
@@ -226,7 +244,7 @@ nfp_net_fw_find(struct pci_dev *pdev, struct nfp_pf *pf)
 	err = request_firmware(&fw, fw_name, &pdev->dev);
 	if (err)
 		return NULL;
-
+done:
 	dev_info(&pdev->dev, "Loading FW image: %s\n", fw_name);
 
 	return fw;
-- 
2.11.0

RE: [PATCH net-next 2/3] nfp: look for firmware image by device serial number and PCI name

[David Laight]

From: Jakub Kicinski
> Sent: 26 July 2017 19:10
> We generally look up firmware by card type, but that doesn't allow
> users who have more than one card of the same type in their system
> to select firmware per adapter.
> 
> Unfortunately user space firmware helper seems fraught with
> difficulties and to be on its way out.  In particular support for
> handling firmware uevents have been dropped from systemd and most
> distributions don't enable the FW fallback by default any more.
> 
> To allow users selecting firmware for a particular device look up
> firmware names by serial and pci_name().  Use the direct lookup to
> disable generating uevents when enabled in Kconfig and not print
> any warnings to logs if adapter-specific files are missing.  Users
> can place in /lib/firmware/netronome files named:
> 
> pci-${pci_name}.nffw
> serial-${serial}.nffw
> 
> to target a specific card.  E.g.:
> 
> pci-0000:04:00.0.nffw
> pci-0000:82:00.0.nffw
> serial-00-aa-bb-11-22-33-10-ff.nffw
> 
> We use the full serial number including the interface id, as it
> appears in lspci output (bytes separated by '-').

Where does lspci get that from?

> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
>  drivers/net/ethernet/netronome/nfp/nfp_main.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.c
> b/drivers/net/ethernet/netronome/nfp/nfp_main.c
> index d67969d3e484..13d056da0765 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_main.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_main.c
> @@ -188,9 +188,27 @@ nfp_net_fw_find(struct pci_dev *pdev, struct nfp_pf *pf)
>  	struct nfp_eth_table_port *port;
>  	const char *fw_model;
>  	char fw_name[256];
> +	const u8 *serial;
>  	int spc, err = 0;
> +	u16 interface;
>  	int i, j;
> 
> +	/* First try to find a firmware image specific for this device */
> +	interface = nfp_cpp_interface(pf->cpp);
> +	nfp_cpp_serial(pf->cpp, &serial);
> +	sprintf(fw_name, "netronome/serial-%pMF-%02hhx-%02hhx.nffw",
> +		serial, interface >> 8, interface & 0xff);

WTF???
- use snprintf().
- kill those hh, the arguments are of type 'int'.
In fact make 'interface' 'unsigned int' as well.

	David

Re: [PATCH net-next 2/3] nfp: look for firmware image by device serial number and PCI name

[Jakub Kicinski]

On Thu, 27 Jul 2017 08:20:22 +0000, David Laight wrote:
> From: Jakub Kicinski
> > Sent: 26 July 2017 19:10
> > We generally look up firmware by card type, but that doesn't allow
> > users who have more than one card of the same type in their system
> > to select firmware per adapter.
> > 
> > Unfortunately user space firmware helper seems fraught with
> > difficulties and to be on its way out.  In particular support for
> > handling firmware uevents have been dropped from systemd and most
> > distributions don't enable the FW fallback by default any more.
> > 
> > To allow users selecting firmware for a particular device look up
> > firmware names by serial and pci_name().  Use the direct lookup to
> > disable generating uevents when enabled in Kconfig and not print
> > any warnings to logs if adapter-specific files are missing.  Users
> > can place in /lib/firmware/netronome files named:
> > 
> > pci-${pci_name}.nffw
> > serial-${serial}.nffw
> > 
> > to target a specific card.  E.g.:
> > 
> > pci-0000:04:00.0.nffw
> > pci-0000:82:00.0.nffw
> > serial-00-aa-bb-11-22-33-10-ff.nffw
> > 
> > We use the full serial number including the interface id, as it
> > appears in lspci output (bytes separated by '-').  
> 
> Where does lspci get that from?

Extended capability type 3, Device Serial Number.

> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > ---
> >  drivers/net/ethernet/netronome/nfp/nfp_main.c | 20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.c
> > b/drivers/net/ethernet/netronome/nfp/nfp_main.c
> > index d67969d3e484..13d056da0765 100644
> > --- a/drivers/net/ethernet/netronome/nfp/nfp_main.c
> > +++ b/drivers/net/ethernet/netronome/nfp/nfp_main.c
> > @@ -188,9 +188,27 @@ nfp_net_fw_find(struct pci_dev *pdev, struct nfp_pf *pf)
> >  	struct nfp_eth_table_port *port;
> >  	const char *fw_model;
> >  	char fw_name[256];
> > +	const u8 *serial;
> >  	int spc, err = 0;
> > +	u16 interface;
> >  	int i, j;
> > 
> > +	/* First try to find a firmware image specific for this device */
> > +	interface = nfp_cpp_interface(pf->cpp);
> > +	nfp_cpp_serial(pf->cpp, &serial);
> > +	sprintf(fw_name, "netronome/serial-%pMF-%02hhx-%02hhx.nffw",
> > +		serial, interface >> 8, interface & 0xff);  
> 
> WTF???

Please be civil.

> - use snprintf().

To effectively print an integer into an amply sized array?  I need to
guarantee that the string will fit otherwise I would request a FW image
with a wrong name.  snprintf() would only mask such a bug.

> - kill those hh, the arguments are of type 'int'.

It doesn't matter.  I will be more careful in the future, though.

> In fact make 'interface' 'unsigned int' as well.

It's a value read from the hardware, and it's 16 bits wide, therefore
my preference it to explicitly size the variable.

RE: [PATCH net-next 2/3] nfp: look for firmware image by device serial number and PCI name

[David Laight]

From: Jakub Kicinski
> Sent: 27 July 2017 10:26
...
> > - use snprintf().
> 
> To effectively print an integer into an amply sized array?  I need to
> guarantee that the string will fit otherwise I would request a FW image
> with a wrong name.  snprintf() would only mask such a bug.

Eh?
If, for any reason, the buffer isn't long enough snprintf() won't
write over random memory - sprint() will, and you may not notice.

> > - kill those hh, the arguments are of type 'int'.
> 
> It doesn't matter.  I will be more careful in the future, though.
> 
> > In fact make 'interface' 'unsigned int' as well.
> 
> It's a value read from the hardware, and it's 16 bits wide, therefore
> my preference it to explicitly size the variable.

Right, so you keep asking the compiler to generate code to mask
any arithmetic results (written into the variable) back down to
16 bits.

OTOH, looking at some of the functions in nfp_cppcore.c you don't
care about performance at all.

	David

[PATCH net-next 3/3] nfp: only use direct firmware requests

[Jakub Kicinski]

request_firmware() will fallback to user space helper and may cause
long delays when driver is loaded if udev doesn't correctly handle
FW requests.  Since we never really made use of the user space
helper functionality switch to the simpler request_firmware_direct()
call.  The side effect of this change is that no warning will be
printed when the FW image does not exists.  To help users figure
out which FW file is missing print a info message when we request
each file.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/nfp_main.c | 42 +++++++++++++++++----------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.c b/drivers/net/ethernet/netronome/nfp/nfp_main.c
index 13d056da0765..dd769eceb33d 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_main.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_main.c
@@ -174,6 +174,21 @@ static int nfp_pcie_sriov_configure(struct pci_dev *pdev, int num_vfs)
 		return nfp_pcie_sriov_enable(pdev, num_vfs);
 }
 
+static const struct firmware *
+nfp_net_fw_request(struct pci_dev *pdev, struct nfp_pf *pf, const char *name)
+{
+	const struct firmware *fw = NULL;
+	int err;
+
+	err = request_firmware_direct(&fw, name, &pdev->dev);
+	nfp_info(pf->cpp, "  %s: %s\n",
+		 name, err ? "not found" : "found, loading...");
+	if (err)
+		return NULL;
+
+	return fw;
+}
+
 /**
  * nfp_net_fw_find() - Find the correct firmware image for netdev mode
  * @pdev:	PCI Device structure
@@ -184,29 +199,30 @@ static int nfp_pcie_sriov_configure(struct pci_dev *pdev, int num_vfs)
 static const struct firmware *
 nfp_net_fw_find(struct pci_dev *pdev, struct nfp_pf *pf)
 {
-	const struct firmware *fw = NULL;
 	struct nfp_eth_table_port *port;
+	const struct firmware *fw;
 	const char *fw_model;
 	char fw_name[256];
 	const u8 *serial;
-	int spc, err = 0;
 	u16 interface;
-	int i, j;
+	int spc, i, j;
+
+	nfp_info(pf->cpp, "Looking for firmware file in order of priority:\n");
 
 	/* First try to find a firmware image specific for this device */
 	interface = nfp_cpp_interface(pf->cpp);
 	nfp_cpp_serial(pf->cpp, &serial);
 	sprintf(fw_name, "netronome/serial-%pMF-%02hhx-%02hhx.nffw",
 		serial, interface >> 8, interface & 0xff);
-	err = request_firmware_direct(&fw, fw_name, &pdev->dev);
-	if (!err)
-		goto done;
+	fw = nfp_net_fw_request(pdev, pf, fw_name);
+	if (fw)
+		return fw;
 
 	/* Then try the PCI name */
 	sprintf(fw_name, "netronome/pci-%s.nffw", pci_name(pdev));
-	err = request_firmware_direct(&fw, fw_name, &pdev->dev);
-	if (!err)
-		goto done;
+	fw = nfp_net_fw_request(pdev, pf, fw_name);
+	if (fw)
+		return fw;
 
 	/* Finally try the card type and media */
 	if (!pf->eth_tbl) {
@@ -241,13 +257,7 @@ nfp_net_fw_find(struct pci_dev *pdev, struct nfp_pf *pf)
 	if (spc <= 0)
 		return NULL;
 
-	err = request_firmware(&fw, fw_name, &pdev->dev);
-	if (err)
-		return NULL;
-done:
-	dev_info(&pdev->dev, "Loading FW image: %s\n", fw_name);
-
-	return fw;
+	return nfp_net_fw_request(pdev, pf, fw_name);
 }
 
 /**
-- 
2.11.0

Re: [oss-drivers] [PATCH net-next 0/3] nfp: extend firmware request logic

[Simon Horman]

On Wed, Jul 26, 2017 at 11:09:45AM -0700, Jakub Kicinski wrote:
> Hi!
> 
> We have been pondering for some time how to support loading different
> application firmwares onto NFP.  We want to support both users selecting
> one of the firmware images provided by Netronome (which are optimized
> for different use cases each) as well as firmware images created  by 
> users themselves or other companies.
> 
> In the end we decided to go with the simplest solution - depending on
> the right firmware image being placed in /lib/firmware.  This vastly
> simplifies driver logic and also doesn't require any new API.
> 
> Different NICs on one system may want to run different applications
> therefore we first try to load firmware specific to the device (by 
> serial number of PCI slot) and if not present try the device model
> based name we have been using so far.
> 
> 
> Jakub Kicinski (3):
>   nfp: remove the probe deferral when FW not present
>   nfp: look for firmware image by device serial number and PCI name
>   nfp: only use direct firmware requests

Thanks Jakub, this looks good to me.

All patches:

Reviewed-by: Simon Horman <simon.horman@netronome.com>

Re: [PATCH net-next 0/3] nfp: extend firmware request logic

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Wed, 26 Jul 2017 11:09:45 -0700

> We have been pondering for some time how to support loading different
> application firmwares onto NFP.  We want to support both users selecting
> one of the firmware images provided by Netronome (which are optimized
> for different use cases each) as well as firmware images created  by 
> users themselves or other companies.
> 
> In the end we decided to go with the simplest solution - depending on
> the right firmware image being placed in /lib/firmware.  This vastly
> simplifies driver logic and also doesn't require any new API.
> 
> Different NICs on one system may want to run different applications
> therefore we first try to load firmware specific to the device (by 
> serial number of PCI slot) and if not present try the device model
> based name we have been using so far.

Series applied, thanks Jakub.