diff mbox

pci: separate out pci_add_dynid()

Message ID 4A9E38FA.3000506@kernel.org (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Tejun Heo Sept. 2, 2009, 9:20 a.m. UTC 
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(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Greg KH Sept. 2, 2009, 1:30 p.m. UTC | #1 
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
> - */
> -
>  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);
> +	retval = driver_attach(&drv->driver);
> +	put_driver(&drv->driver);
> +
> +	return retval;
> +}
> +
> +static void pci_free_dynids(struct pci_driver *drv)
> +{
> +	struct pci_dynid *dynid, *n;
> +
> +	spin_lock(&drv->dynids.lock);
> +	list_for_each_entry_safe(dynid, n, &drv->dynids.list, node) {
> +		list_del(&dynid->node);
> +		kfree(dynid);
> +	}
> +	spin_unlock(&drv->dynids.lock);
> +}
> +
> +/*
> + * Dynamic device ID manipulation via sysfs is disabled for !CONFIG_HOTPLUG
> + */
> +#ifdef CONFIG_HOTPLUG
>  /**
> - * store_new_id - add a new PCI device ID to this driver and re-probe devices
> + * store_new_id - sysfs frontend to pci_add_dynid()
>   * @driver: target device driver
>   * @buf: buffer for scanning device ID data
>   * @count: input size
>   *
> - * Adds a new dynamic pci device ID to this driver,
> - * and causes the driver to probe for all devices again.
> + * Allow PCI IDs to be added to an existing driver via sysfs.
>   */
>  static ssize_t
>  store_new_id(struct device_driver *driver, const char *buf, size_t count)
>  {
> -	struct pci_dynid *dynid;
>  	struct pci_driver *pdrv = to_pci_driver(driver);
>  	const struct pci_device_id *ids = pdrv->id_table;
>  	__u32 vendor, device, subvendor=PCI_ANY_ID,
>  		subdevice=PCI_ANY_ID, class=0, class_mask=0;
>  	unsigned long driver_data=0;
>  	int fields=0;
> -	int retval=0;
> +	int retval;
> 
>  	fields = sscanf(buf, "%x %x %x %x %x %x %lx",
>  			&vendor, &device, &subvendor, &subdevice,
> @@ -72,27 +133,8 @@ store_new_id(struct device_driver *drive
>  			return 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(&pdrv->dynids.lock);
> -	list_add_tail(&dynid->node, &pdrv->dynids.list);
> -	spin_unlock(&pdrv->dynids.lock);
> -
> -	if (get_driver(&pdrv->driver)) {
> -		retval = driver_attach(&pdrv->driver);
> -		put_driver(&pdrv->driver);
> -	}
> -
> +	retval = pci_add_dynid(pdrv, vendor, device, subvendor, subdevice,
> +			       class, class_mask, driver_data);
>  	if (retval)
>  		return retval;
>  	return count;
> @@ -145,19 +187,6 @@ store_remove_id(struct device_driver *dr
>  }
>  static DRIVER_ATTR(remove_id, S_IWUSR, NULL, store_remove_id);
> 
> -static void
> -pci_free_dynids(struct pci_driver *drv)
> -{
> -	struct pci_dynid *dynid, *n;
> -
> -	spin_lock(&drv->dynids.lock);
> -	list_for_each_entry_safe(dynid, n, &drv->dynids.list, node) {
> -		list_del(&dynid->node);
> -		kfree(dynid);
> -	}
> -	spin_unlock(&drv->dynids.lock);
> -}
> -
>  static int
>  pci_create_newid_file(struct pci_driver *drv)
>  {
> @@ -186,7 +215,6 @@ static void pci_remove_removeid_file(str
>  	driver_remove_file(&drv->driver, &driver_attr_remove_id);
>  }
>  #else /* !CONFIG_HOTPLUG */
> -static inline void pci_free_dynids(struct pci_driver *drv) {}
>  static inline int pci_create_newid_file(struct pci_driver *drv)
>  {
>  	return 0;
> @@ -1106,6 +1134,7 @@ static int __init pci_driver_init(void)
> 
>  postcore_initcall(pci_driver_init);
> 
> +EXPORT_SYMBOL(pci_add_dynid);

scripts/checkpatch.pl will complain about this :)

Care to mark it EXPORT_SYMBOL_GPL() please?

With that change, then feel free to add:
	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grant Grundler Sept. 2, 2009, 4:10 p.m. UTC | #2 
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?

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

> +	retval = driver_attach(&drv->driver);
> +	put_driver(&drv->driver);
> +
> +	return retval;
> +}
> +
> +static void pci_free_dynids(struct pci_driver *drv)
> +{
> +	struct pci_dynid *dynid, *n;
> +
> +	spin_lock(&drv->dynids.lock);
> +	list_for_each_entry_safe(dynid, n, &drv->dynids.list, node) {
> +		list_del(&dynid->node);
> +		kfree(dynid);
> +	}
> +	spin_unlock(&drv->dynids.lock);
> +}
> +
> +/*
> + * Dynamic device ID manipulation via sysfs is disabled for !CONFIG_HOTPLUG
> + */

But same comment added back here. Is this comment correct?

If comments are the only thing that change, please add:
  Reviewed-by: Grant Grundler <grundler@parisc-linux.org>

This patch seems pretty straight forward.

thanks
grant

> +#ifdef CONFIG_HOTPLUG
>  /**
> - * store_new_id - add a new PCI device ID to this driver and re-probe devices
> + * store_new_id - sysfs frontend to pci_add_dynid()
>   * @driver: target device driver
>   * @buf: buffer for scanning device ID data
>   * @count: input size
>   *
> - * Adds a new dynamic pci device ID to this driver,
> - * and causes the driver to probe for all devices again.
> + * Allow PCI IDs to be added to an existing driver via sysfs.
>   */
>  static ssize_t
>  store_new_id(struct device_driver *driver, const char *buf, size_t count)
>  {
> -	struct pci_dynid *dynid;
>  	struct pci_driver *pdrv = to_pci_driver(driver);
>  	const struct pci_device_id *ids = pdrv->id_table;
>  	__u32 vendor, device, subvendor=PCI_ANY_ID,
>  		subdevice=PCI_ANY_ID, class=0, class_mask=0;
>  	unsigned long driver_data=0;
>  	int fields=0;
> -	int retval=0;
> +	int retval;
> 
>  	fields = sscanf(buf, "%x %x %x %x %x %x %lx",
>  			&vendor, &device, &subvendor, &subdevice,
> @@ -72,27 +133,8 @@ store_new_id(struct device_driver *drive
>  			return 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(&pdrv->dynids.lock);
> -	list_add_tail(&dynid->node, &pdrv->dynids.list);
> -	spin_unlock(&pdrv->dynids.lock);
> -
> -	if (get_driver(&pdrv->driver)) {
> -		retval = driver_attach(&pdrv->driver);
> -		put_driver(&pdrv->driver);
> -	}
> -
> +	retval = pci_add_dynid(pdrv, vendor, device, subvendor, subdevice,
> +			       class, class_mask, driver_data);
>  	if (retval)
>  		return retval;
>  	return count;
> @@ -145,19 +187,6 @@ store_remove_id(struct device_driver *dr
>  }
>  static DRIVER_ATTR(remove_id, S_IWUSR, NULL, store_remove_id);
> 
> -static void
> -pci_free_dynids(struct pci_driver *drv)
> -{
> -	struct pci_dynid *dynid, *n;
> -
> -	spin_lock(&drv->dynids.lock);
> -	list_for_each_entry_safe(dynid, n, &drv->dynids.list, node) {
> -		list_del(&dynid->node);
> -		kfree(dynid);
> -	}
> -	spin_unlock(&drv->dynids.lock);
> -}
> -
>  static int
>  pci_create_newid_file(struct pci_driver *drv)
>  {
> @@ -186,7 +215,6 @@ static void pci_remove_removeid_file(str
>  	driver_remove_file(&drv->driver, &driver_attr_remove_id);
>  }
>  #else /* !CONFIG_HOTPLUG */
> -static inline void pci_free_dynids(struct pci_driver *drv) {}
>  static inline int pci_create_newid_file(struct pci_driver *drv)
>  {
>  	return 0;
> @@ -1106,6 +1134,7 @@ static int __init pci_driver_init(void)
> 
>  postcore_initcall(pci_driver_init);
> 
> +EXPORT_SYMBOL(pci_add_dynid);
>  EXPORT_SYMBOL(pci_match_id);
>  EXPORT_SYMBOL(__pci_register_driver);
>  EXPORT_SYMBOL(pci_unregister_driver);
> Index: ata/include/linux/pci.h
> ===================================================================
> --- ata.orig/include/linux/pci.h
> +++ ata/include/linux/pci.h
> @@ -794,6 +794,11 @@ int __must_check __pci_register_driver(s
>  void pci_unregister_driver(struct pci_driver *dev);
>  void pci_remove_behind_bridge(struct pci_dev *dev);
>  struct pci_driver *pci_dev_driver(const struct pci_dev *dev);
> +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);
>  const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
>  					 struct pci_dev *dev);
>  int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg KH Sept. 2, 2009, 5:56 p.m. UTC | #3 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo Sept. 2, 2009, 10:29 p.m. UTC | #4 
Hello, Greg.

Greg KH wrote:
>> +EXPORT_SYMBOL(pci_add_dynid);
> 
> scripts/checkpatch.pl will complain about this :)
> 
> Care to mark it EXPORT_SYMBOL_GPL() please?

Yeap, I just moved the code a bit so was a bit hesitant to add _GPL
there when all other functions didn't have it.  Oh well, it's not like
external drivers would it.  I'll.

> With that change, then feel free to add:
> 	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

Thanks.
Tejun Heo Sept. 2, 2009, 10:38 p.m. UTC | #5 
Hello, Grant.

Grant Grundler wrote:
>> -/*
>> - * Dynamic device IDs are disabled for !CONFIG_HOTPLUG
>> - */
> 
> The comment was removed here. Ok.

Yeap, because dynids as a whole are no longer dependent on
CONFIG_HOTPLUG.

>> +	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, it is.

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

It's one of the gripes I've had with the driver core interface.
Returning the object which had its reference bumped up is nice to do
do_something(get_driver(driver)); but it should have been made clear
that the return value always equals the input parameter.  IIRC, in the
early days of the current driver model and kref, there were intentions
to make kref somehow more capable and handle the last reference
problem from within it so that if (get_driver(driver)) test is
actually meaningful but that turned out to be not too feasible, so we
ended up with unnecessary if () around driver model get functions.  I
try to remove them whenever an affected piece of code is being
modified but there still are plenty left.  I'll update comment on
driver core get functions.

>> +/*
>> + * Dynamic device ID manipulation via sysfs is disabled for !CONFIG_HOTPLUG
>> + */
> 
> But same comment added back here. Is this comment correct?

Yes, dynid in general is enabled regardless but sysfs access is
available iff CONFIG_HOTPLUG.

> If comments are the only thing that change, please add:
>   Reviewed-by: Grant Grundler <grundler@parisc-linux.org>
> 
> This patch seems pretty straight forward.

Thanks.
Tejun Heo Sept. 2, 2009, 10:43 p.m. UTC | #6 
Ummm... another thought.

Tejun Heo wrote:
> It's one of the gripes I've had with the driver core interface.
> Returning the object which had its reference bumped up is nice to do
> do_something(get_driver(driver)); but it should have been made clear
> that the return value always equals the input parameter.  IIRC, in the
> early days of the current driver model and kref, there were intentions
> to make kref somehow more capable and handle the last reference
> problem from within it so that if (get_driver(driver)) test is
> actually meaningful but that turned out to be not too feasible, so we
> ended up with unnecessary if () around driver model get functions.  I
> try to remove them whenever an affected piece of code is being
> modified but there still are plenty left.  I'll update comment on
> driver core get functions.

Greg, I think I suggested this before.  What do you think about just
making those get functions return void?  The current interface
combined with widespread if() around them is quite misleading.  Most
device/driver structures are inherited and extended when actually in
use, so the driver or its subsystem code usually can't do much with
the return value other than checking for bogus error condition anyway.

Thanks.
Greg KH Sept. 3, 2009, 12:05 a.m. UTC | #7 
On Thu, Sep 03, 2009 at 07:43:28AM +0900, Tejun Heo wrote:
> Ummm... another thought.
> 
> Tejun Heo wrote:
> > It's one of the gripes I've had with the driver core interface.
> > Returning the object which had its reference bumped up is nice to do
> > do_something(get_driver(driver)); but it should have been made clear
> > that the return value always equals the input parameter.  IIRC, in the
> > early days of the current driver model and kref, there were intentions
> > to make kref somehow more capable and handle the last reference
> > problem from within it so that if (get_driver(driver)) test is
> > actually meaningful but that turned out to be not too feasible, so we
> > ended up with unnecessary if () around driver model get functions.  I
> > try to remove them whenever an affected piece of code is being
> > modified but there still are plenty left.  I'll update comment on
> > driver core get functions.
> 
> Greg, I think I suggested this before.  What do you think about just
> making those get functions return void?

I have no objection to that at all.

> The current interface combined with widespread if() around them is
> quite misleading.

I agree.

> Most device/driver structures are inherited and extended when actually
> in use, so the driver or its subsystem code usually can't do much with
> the return value other than checking for bogus error condition anyway.

I totally agree.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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
- */
-
 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);
+	retval = driver_attach(&drv->driver);
+	put_driver(&drv->driver);
+
+	return retval;
+}
+
+static void pci_free_dynids(struct pci_driver *drv)
+{
+	struct pci_dynid *dynid, *n;
+
+	spin_lock(&drv->dynids.lock);
+	list_for_each_entry_safe(dynid, n, &drv->dynids.list, node) {
+		list_del(&dynid->node);
+		kfree(dynid);
+	}
+	spin_unlock(&drv->dynids.lock);
+}
+
+/*
+ * Dynamic device ID manipulation via sysfs is disabled for !CONFIG_HOTPLUG
+ */
+#ifdef CONFIG_HOTPLUG
 /**
- * store_new_id - add a new PCI device ID to this driver and re-probe devices
+ * store_new_id - sysfs frontend to pci_add_dynid()
  * @driver: target device driver
  * @buf: buffer for scanning device ID data
  * @count: input size
  *
- * Adds a new dynamic pci device ID to this driver,
- * and causes the driver to probe for all devices again.
+ * Allow PCI IDs to be added to an existing driver via sysfs.
  */
 static ssize_t
 store_new_id(struct device_driver *driver, const char *buf, size_t count)
 {
-	struct pci_dynid *dynid;
 	struct pci_driver *pdrv = to_pci_driver(driver);
 	const struct pci_device_id *ids = pdrv->id_table;
 	__u32 vendor, device, subvendor=PCI_ANY_ID,
 		subdevice=PCI_ANY_ID, class=0, class_mask=0;
 	unsigned long driver_data=0;
 	int fields=0;
-	int retval=0;
+	int retval;

 	fields = sscanf(buf, "%x %x %x %x %x %x %lx",
 			&vendor, &device, &subvendor, &subdevice,
@@ -72,27 +133,8 @@  store_new_id(struct device_driver *drive
 			return 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(&pdrv->dynids.lock);
-	list_add_tail(&dynid->node, &pdrv->dynids.list);
-	spin_unlock(&pdrv->dynids.lock);
-
-	if (get_driver(&pdrv->driver)) {
-		retval = driver_attach(&pdrv->driver);
-		put_driver(&pdrv->driver);
-	}
-
+	retval = pci_add_dynid(pdrv, vendor, device, subvendor, subdevice,
+			       class, class_mask, driver_data);
 	if (retval)
 		return retval;
 	return count;
@@ -145,19 +187,6 @@  store_remove_id(struct device_driver *dr
 }
 static DRIVER_ATTR(remove_id, S_IWUSR, NULL, store_remove_id);

-static void
-pci_free_dynids(struct pci_driver *drv)
-{
-	struct pci_dynid *dynid, *n;
-
-	spin_lock(&drv->dynids.lock);
-	list_for_each_entry_safe(dynid, n, &drv->dynids.list, node) {
-		list_del(&dynid->node);
-		kfree(dynid);
-	}
-	spin_unlock(&drv->dynids.lock);
-}
-
 static int
 pci_create_newid_file(struct pci_driver *drv)
 {
@@ -186,7 +215,6 @@  static void pci_remove_removeid_file(str
 	driver_remove_file(&drv->driver, &driver_attr_remove_id);
 }
 #else /* !CONFIG_HOTPLUG */
-static inline void pci_free_dynids(struct pci_driver *drv) {}
 static inline int pci_create_newid_file(struct pci_driver *drv)
 {
 	return 0;
@@ -1106,6 +1134,7 @@  static int __init pci_driver_init(void)

 postcore_initcall(pci_driver_init);

+EXPORT_SYMBOL(pci_add_dynid);
 EXPORT_SYMBOL(pci_match_id);
 EXPORT_SYMBOL(__pci_register_driver);
 EXPORT_SYMBOL(pci_unregister_driver);
Index: ata/include/linux/pci.h
===================================================================
--- ata.orig/include/linux/pci.h
+++ ata/include/linux/pci.h
@@ -794,6 +794,11 @@  int __must_check __pci_register_driver(s
 void pci_unregister_driver(struct pci_driver *dev);
 void pci_remove_behind_bridge(struct pci_dev *dev);
 struct pci_driver *pci_dev_driver(const struct pci_dev *dev);
+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);
 const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
 					 struct pci_dev *dev);
 int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,