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

[Grant Grundler <grundler@parisc-linux.org>]

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