Re: [dpdk-stable] patch 'bus/pci: use given name as generic name' has been queued to stable release 17.05.2

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

Hi,

On Tue, Aug 29, 2017 at 03:47:28PM +0800, Yuanhan Liu wrote:
> On Mon, Aug 21, 2017 at 05:30:20PM +0800, Yuanhan Liu wrote:
> > Hi,
> > 
> > FYI, your patch has been queued to stable release 17.05.2
> > 
> > Note it hasn't been pushed to http://dpdk.org/browse/dpdk-stable yet.
> > It will be pushed if I get no objections before 08/24/17. So please
> > shout if anyone has objections.
> 
> There is a build error:
> 
>   lib/librte_eal/common/eal_common_pci.c:133:41: error: ‘struct rte_devargs’ has no member named ‘name’
>      dev->device.name = dev->device.devargs->name;
> 
> I usually will do a build test before sending this out. Not quite sure
> why I missed that this time :/
> 
> So I will drop it in this release. Let me know if you think a backport
> is needed.
> 
> Sorry for that!
> 

No, it's my fault I think.
I referenced a commit to fix, however it is dependent on rte_devargs
evolutions made in the meantime.

I think it's safe to drop this patch completely from a stable branch for
17.05, as those rte_devargs evolutions are not meant to be backported I
think.

Moreover, the bug fixed with this commit only makes sense in light of
these rte_devargs evolutions (meaning: rte_devargs now holds the
"user-given" name of a device, and only once this name is defined does
it make sense to distinguish the generated name of a PCI device from its
"user-given" one.).

> 	--yliu
> > 
> > ---
> > >From abbdc42771b8999624afe375b4d5d8e036d7ab71 Mon Sep 17 00:00:00 2001
> > From: Gaetan Rivet <gaetan.rivet@6wind.com>
> > Date: Sat, 15 Jul 2017 19:56:39 +0200
> > Subject: [PATCH] bus/pci: use given name as generic name
> > 
> > [ upstream commit 23eaa9059ec24e95e32361f333ed0686f82bea74 ]
> > 
> > When an application requests the use of a PCI device, it can currently
> > interchangeably use either the longform DomBDF format (0000:00:00.0) or
> > the shorter BDF format (00:00.0).
> > 
> > When a device is inserted via the hotplug API, it must first be scanned
> > and then will be identified by its name using `find_device`. The name of
> > the device must match the name given by the user to be found and then
> > probed.
> > 
> > A new function sets the expected name for a scanned PCI device. It was
> > previously generated from parsing the PCI address. This canonical name
> > is superseded when an rte_devargs exists describing the device. In such
> > case, the device takes the given name found within the rte_devargs.
> > 
> > As the rte_devargs is linked to the rte_pci_device during scanning, it
> > can be avoided during the probe. Additionally, this fixes the issue of
> > the rte_devargs lookup not being done within rte_pci_probe_one.
> > 
> > Fixes: beec692c5157 ("eal: add name field to generic device")
> > 
> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > ---
> >  lib/librte_eal/bsdapp/eal/eal_pci.c    |  4 ++--
> >  lib/librte_eal/common/eal_common_pci.c | 29 ++++++++++++++++++++++++-----
> >  lib/librte_eal/common/eal_private.h    |  5 +++++
> >  lib/librte_eal/linuxapp/eal/eal_pci.c  |  4 ++--
> >  4 files changed, 33 insertions(+), 9 deletions(-)
> > 
> > diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c b/lib/librte_eal/bsdapp/eal/eal_pci.c
> > index e321461..97a88ec 100644
> > --- a/lib/librte_eal/bsdapp/eal/eal_pci.c
> > +++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
> > @@ -282,8 +282,7 @@ pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
> >  	/* FreeBSD has no NUMA support (yet) */
> >  	dev->device.numa_node = 0;
> >  
> > -	rte_pci_device_name(&dev->addr, dev->name, sizeof(dev->name));
> > -	dev->device.name = dev->name;
> > +	pci_name_set(dev);
> >  
> >  	/* FreeBSD has only one pass through driver */
> >  	dev->kdrv = RTE_KDRV_NIC_UIO;
> > @@ -334,6 +333,7 @@ pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
> >  			} else { /* already registered */
> >  				dev2->kdrv = dev->kdrv;
> >  				dev2->max_vfs = dev->max_vfs;
> > +				pci_name_set(dev2);
> >  				memmove(dev2->mem_resource,
> >  					dev->mem_resource,
> >  					sizeof(dev->mem_resource));
> > diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
> > index b749991..8f8627e 100644
> > --- a/lib/librte_eal/common/eal_common_pci.c
> > +++ b/lib/librte_eal/common/eal_common_pci.c
> > @@ -113,6 +113,29 @@ static struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *dev)
> >  	return NULL;
> >  }
> >  
> > +void
> > +pci_name_set(struct rte_pci_device *dev)
> > +{
> > +	struct rte_devargs *devargs;
> > +
> > +	/* Each device has its internal, canonical name set. */
> > +	rte_pci_device_name(&dev->addr,
> > +			dev->name, sizeof(dev->name));
> > +	devargs = pci_devargs_lookup(dev);
> > +	dev->device.devargs = devargs;
> > +	/* In blacklist mode, if the device is not blacklisted, no
> > +	 * rte_devargs exists for it.
> > +	 */
> > +	if (devargs != NULL)
> > +		/* If an rte_devargs exists, the generic rte_device uses the
> > +		 * given name as its namea
> > +		 */
> > +		dev->device.name = dev->device.devargs->name;
> > +	else
> > +		/* Otherwise, it uses the internal, canonical form. */
> > +		dev->device.name = dev->name;
> > +}
> > +
> >  /* map a particular resource from a file */
> >  void *
> >  pci_map_resource(void *requested_addr, int fd, off_t offset, size_t size,
> > @@ -419,11 +442,7 @@ rte_pci_probe(void)
> >  	FOREACH_DEVICE_ON_PCIBUS(dev) {
> >  		probed++;
> >  
> > -		/* set devargs in PCI structure */
> > -		devargs = pci_devargs_lookup(dev);
> > -		if (devargs != NULL)
> > -			dev->device.devargs = devargs;
> > -
> > +		devargs = dev->device.devargs;
> >  		/* probe all or only whitelisted devices */
> >  		if (probe_all)
> >  			ret = pci_probe_all_drivers(dev);
> > diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
> > index 6cacce0..0a1c3a7 100644
> > --- a/lib/librte_eal/common/eal_private.h
> > +++ b/lib/librte_eal/common/eal_private.h
> > @@ -113,6 +113,11 @@ struct rte_pci_driver;
> >  struct rte_pci_device;
> >  
> >  /**
> > + * Find the name of a PCI device.
> > + */
> > +void pci_name_set(struct rte_pci_device *dev);
> > +
> > +/**
> >   * Add a PCI device to the PCI Bus (append to PCI Device list). This function
> >   * also updates the bus references of the PCI Device (and the generic device
> >   * object embedded within.
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
> > index 595622b..816b19d 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> > @@ -324,8 +324,7 @@ pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
> >  		dev->device.numa_node = tmp;
> >  	}
> >  
> > -	rte_pci_device_name(addr, dev->name, sizeof(dev->name));
> > -	dev->device.name = dev->name;
> > +	pci_name_set(dev);
> >  
> >  	/* parse resources */
> >  	snprintf(filename, sizeof(filename), "%s/resource", dirname);
> > @@ -373,6 +372,7 @@ pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
> >  			} else { /* already registered */
> >  				dev2->kdrv = dev->kdrv;
> >  				dev2->max_vfs = dev->max_vfs;
> > +				pci_name_set(dev2);
> >  				memmove(dev2->mem_resource, dev->mem_resource,
> >  					sizeof(dev->mem_resource));
> >  				free(dev);
> > -- 
> > 2.7.4

-- 
Gaëtan Rivet
6WIND