Re: [PATCH] pci: separate out pci_add_dynid()

[Greg KH <greg@kroah.com>]

On Wed, Sep 02, 2009 at 10:10:48AM -0600, Grant Grundler wrote:
> On Wed, Sep 02, 2009 at 06:20:58PM +0900, Tejun Heo wrote:
> > Separate out pci_add_dynid() from store_new_id() and export it so that
> > in-kernel code can add PCI IDs dynamically.  As the function will be
> > available regardless of HOTPLUG, put it and pull pci_free_dynids()
> > outside of CONFIG_HOTPLUG.
> > 
> > This will be used by pci-stub to initialize initial IDs via module
> > param.
> > 
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > ---
> >  drivers/pci/pci-driver.c |  119 +++++++++++++++++++++++++++++------------------
> >  include/linux/pci.h      |    5 +
> >  2 files changed, 79 insertions(+), 45 deletions(-)
> > 
> > Index: ata/drivers/pci/pci-driver.c
> > ===================================================================
> > --- ata.orig/drivers/pci/pci-driver.c
> > +++ ata/drivers/pci/pci-driver.c
> > @@ -19,37 +19,98 @@
> >  #include <linux/cpu.h>
> >  #include "pci.h"
> > 
> > -/*
> > - * Dynamic device IDs are disabled for !CONFIG_HOTPLUG
> > - */
> 
> The comment was removed here. Ok.
> 
> > -
> >  struct pci_dynid {
> >  	struct list_head node;
> >  	struct pci_device_id id;
> >  };
> > 
> > -#ifdef CONFIG_HOTPLUG
> > +/**
> > + * pci_add_dynid - add a new PCI device ID to this driver and re-probe devices
> > + * @drv: target pci driver
> > + * @vendor: PCI vendor ID
> > + * @device: PCI device ID
> > + * @subvendor: PCI subvendor ID
> > + * @subdevice: PCI subdevice ID
> > + * @class: PCI class
> > + * @class_mask: PCI class mask
> > + * @driver_data: private driver data
> > + *
> > + * Adds a new dynamic pci device ID to this driver and causes the
> > + * driver to probe for all devices again.  @drv must have been
> > + * registered prior to calling this function.
> > + *
> > + * CONTEXT:
> > + * Does GFP_KERNEL allocation.
> > + *
> > + * RETURNS:
> > + * 0 on success, -errno on failure.
> > + */
> > +int pci_add_dynid(struct pci_driver *drv,
> > +		  unsigned int vendor, unsigned int device,
> > +		  unsigned int subvendor, unsigned int subdevice,
> > +		  unsigned int class, unsigned int class_mask,
> > +		  unsigned long driver_data)
> > +{
> > +	struct pci_dynid *dynid;
> > +	int retval;
> > +
> > +	dynid = kzalloc(sizeof(*dynid), GFP_KERNEL);
> > +	if (!dynid)
> > +		return -ENOMEM;
> > +
> > +	dynid->id.vendor = vendor;
> > +	dynid->id.device = device;
> > +	dynid->id.subvendor = subvendor;
> > +	dynid->id.subdevice = subdevice;
> > +	dynid->id.class = class;
> > +	dynid->id.class_mask = class_mask;
> > +	dynid->id.driver_data = driver_data;
> > 
> > +	spin_lock(&drv->dynids.lock);
> > +	list_add_tail(&dynid->node, &drv->dynids.list);
> > +	spin_unlock(&drv->dynids.lock);
> > +
> > +	get_driver(&drv->driver);
> 
> Return value ignored caught my attention... But I can't say if that's wrong.
> 
> I'm not finding any documentation on get_driver() in Documentation/ .
> Looking at drivers/base/driver.c:get_driver() code, I'm confused why this
> function bothers returning struct device_driver *. My expectation is 
> the input parameter "drv" is the same as "priv->driver" that gets returned.
> No?

Yes.

> In any case, if the Docbook comments for get_driver() could explain
> whatever subtle difference there is, that would be helpful.

There is no difference, we were just trying to be kind way back when it
was first written, 6+ years ago :)


thanks,

greg k-h