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 <[email protected]>
---
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);
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,
Add ids module parameter which allows specifying initial IDs for the
pci-stub driver. When built into the kernel, pci-stub is linked
before any real pci drivers and by setting up IDs from initialization
it can prevent built-in drivers from attaching to specific devices.
While at it, make pci_stub_probe() print out about devices it grabbed
to weed out "but my controller isn't being probed" bug reports.
Signed-off-by: Tejun Heo <[email protected]>
Cc: Chris Wright <[email protected]>
---
Sorry the first one was missing 1/2 in the title.
drivers/pci/pci-stub.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 44 insertions(+), 1 deletion(-)
Index: ata/drivers/pci/pci-stub.c
===================================================================
--- ata.orig/drivers/pci/pci-stub.c
+++ ata/drivers/pci/pci-stub.c
@@ -19,8 +19,16 @@
#include <linux/module.h>
#include <linux/pci.h>
+static char ids[1024] __initdata;
+
+module_param_string(ids, ids, sizeof(ids), 0);
+MODULE_PARM_DESC(ids, "Initial PCI IDs to add to the stub driver, format is "
+ "\"vendor:device[:subvendor[:subdevice[:class[:class_mask]]]]\""
+ " and multiple comma separated entries can be specified");
+
static int pci_stub_probe(struct pci_dev *dev, const struct pci_device_id *id)
{
+ dev_printk(KERN_INFO, &dev->dev, "claimed by stub\n");
return 0;
}
@@ -32,7 +40,42 @@ static struct pci_driver stub_driver = {
static int __init pci_stub_init(void)
{
- return pci_register_driver(&stub_driver);
+ char *p, *id;
+ int rc;
+
+ rc = pci_register_driver(&stub_driver);
+ if (rc)
+ return rc;
+
+ /* add ids specified in the module parameter */
+ p = ids;
+ while ((id = strsep(&p, ","))) {
+ unsigned int vendor, device, subvendor = PCI_ANY_ID,
+ subdevice = PCI_ANY_ID, class=0, class_mask=0;
+ int fields;
+
+ fields = sscanf(id, "%x:%x:%x:%x:%x:%x",
+ &vendor, &device, &subvendor, &subdevice,
+ &class, &class_mask);
+
+ if (fields < 2) {
+ printk(KERN_WARNING
+ "pci-stub: invalid id string \"%s\"\n", id);
+ continue;
+ }
+
+ printk(KERN_INFO
+ "pci-stub: add %04X:%04X sub=%04X:%04X cls=%08X/%08X\n",
+ vendor, device, subvendor, subdevice, class, class_mask);
+
+ rc = pci_add_dynid(&stub_driver, vendor, device,
+ subvendor, subdevice, class, class_mask, 0);
+ if (rc)
+ printk(KERN_WARNING
+ "pci-stub: failed to add dynamic id (%d)\n", rc);
+ }
+
+ return 0;
}
static void __exit pci_stub_exit(void)
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 <[email protected]>
> ---
> 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 <[email protected]>
thanks,
greg k-h
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 <[email protected]>
> ---
> 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 <[email protected]>
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
Thanks.
--
tejun
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 <[email protected]>
>
> This patch seems pretty straight forward.
Thanks.
--
tejun
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.
--
tejun
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