[PATCH 1/1] PCI: Dynids - passing driver data

[PATCH 1/1] PCI: Dynids - passing driver data

[brking]

Currently, code exists in the pci layer to allow userspace to specify
driver data when adding a pci dynamic id from sysfs. However, this data
is never used and there exists no way in the existing code to use it.
This patch allows device drivers to indicate that they want driver data
passed to them on dynamic id adds by initializing use_driver_data in their
pci_driver->pci_dynids struct. The documentation has also been updated
to reflect this.

Signed-off-by: Brian King <brking@us.ibm.com>
---

 linux-2.6.11-rc3-bk4-bjking1/Documentation/pci.txt    |    8 ++++----
 linux-2.6.11-rc3-bk4-bjking1/drivers/pci/pci-driver.c |    1 -
 2 files changed, 4 insertions(+), 5 deletions(-)

diff -puN drivers/pci/pci-driver.c~pci_dynids_driver_data drivers/pci/pci-driver.c
--- linux-2.6.11-rc3-bk4/drivers/pci/pci-driver.c~pci_dynids_driver_data	2005-02-07 15:58:21.000000000 -0600
+++ linux-2.6.11-rc3-bk4-bjking1/drivers/pci/pci-driver.c	2005-02-07 15:58:21.000000000 -0600
@@ -115,7 +115,6 @@ static DRIVER_ATTR(new_id, S_IWUSR, NULL
 static inline void
 pci_init_dynids(struct pci_dynids *dynids)
 {
-	memset(dynids, 0, sizeof(*dynids));
 	spin_lock_init(&dynids->lock);
 	INIT_LIST_HEAD(&dynids->list);
 }
diff -puN Documentation/pci.txt~pci_dynids_driver_data Documentation/pci.txt
--- linux-2.6.11-rc3-bk4/Documentation/pci.txt~pci_dynids_driver_data	2005-02-07 15:58:21.000000000 -0600
+++ linux-2.6.11-rc3-bk4-bjking1/Documentation/pci.txt	2005-02-07 15:58:21.000000000 -0600
@@ -99,10 +99,10 @@ where all fields are passed in as hexade
 Users need pass only as many fields as necessary; vendor, device,
 subvendor, and subdevice fields default to PCI_ANY_ID (FFFFFFFF),
 class and classmask fields default to 0, and driver_data defaults to
-0UL.  Device drivers must call
-   pci_dynids_set_use_driver_data(pci_driver *, 1)
-in order for the driver_data field to get passed to the driver.
-Otherwise, only a 0 is passed in that field.
+0UL.  Device drivers must initialize use_driver_data in the dynids struct
+in their pci_driver struct prior to calling pci_register_driver in order
+for the driver_data field to get passed to the driver. Otherwise, only a
+0 is passed in that field.
 
 When the driver exits, it just calls pci_unregister_driver() and the PCI layer
 automatically calls the remove hook for all devices handled by the driver.
_

Re: [PATCH 1/1] PCI: Dynids - passing driver data

[Greg KH]

On Mon, Feb 07, 2005 at 04:00:27PM -0600, brking@us.ibm.com wrote:
> 
> Currently, code exists in the pci layer to allow userspace to specify
> driver data when adding a pci dynamic id from sysfs. However, this data
> is never used and there exists no way in the existing code to use it.

Which is a good thing, right?  "driver_data" is usually a pointer to
somewhere.  Having userspace specify it would not be a good thing.

> This patch allows device drivers to indicate that they want driver data
> passed to them on dynamic id adds by initializing use_driver_data in their
> pci_driver->pci_dynids struct. The documentation has also been updated
> to reflect this.

What driver wants to use this?

thanks,

greg k-h

Re: [PATCH 1/1] PCI: Dynids - passing driver data

[Brian King]

Greg KH wrote:
> On Mon, Feb 07, 2005 at 04:00:27PM -0600, brking@us.ibm.com wrote:
> 
>>Currently, code exists in the pci layer to allow userspace to specify
>>driver data when adding a pci dynamic id from sysfs. However, this data
>>is never used and there exists no way in the existing code to use it.
> 
> 
> Which is a good thing, right?  "driver_data" is usually a pointer to
> somewhere.  Having userspace specify it would not be a good thing.

That depends on the driver usage, and the patch allows it to be 
configurable and defaults to not being used.

>>This patch allows device drivers to indicate that they want driver data
>>passed to them on dynamic id adds by initializing use_driver_data in their
>>pci_driver->pci_dynids struct. The documentation has also been updated
>>to reflect this.
> 
> 
> What driver wants to use this?

I am in the process of adding dynids support into the ipr scsi driver. I 
originally was using driver_data as a pointer, but am changing it to be 
an index instead, so that it can be specified by the user.

There are essentially 2 different types of chipsets that ipr controls, 
the primary difference being the register offsets. I am using 
driver_data to figure that out today.

My other option is to somehow change the driver to cope with having no 
driver data, but that will result in more driver code and will 
ultimately be less flexible in the new chipsets that can be added using 
dynids.


-Brian


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [PATCH 1/1] PCI: Dynids - passing driver data

[Martin Mares]

Hello!

> >Which is a good thing, right?  "driver_data" is usually a pointer to
> >somewhere.  Having userspace specify it would not be a good thing.
> 
> That depends on the driver usage, and the patch allows it to be 
> configurable and defaults to not being used.

Maybe we could just define the operation as cloning of an entry
for another device ID, including its driver_data.

				Have a nice fortnight
-- 
Martin `MJ' Mares   <mj@ucw.cz>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
Only dead fish swim with the stream.

Re: [PATCH 1/1] PCI: Dynids - passing driver data

[Brian King]

Martin Mares wrote:
> Hello!
> 
> 
>>>Which is a good thing, right?  "driver_data" is usually a pointer to
>>>somewhere.  Having userspace specify it would not be a good thing.
>>
>>That depends on the driver usage, and the patch allows it to be 
>>configurable and defaults to not being used.
> 
> 
> Maybe we could just define the operation as cloning of an entry
> for another device ID, including its driver_data.

Possibly. That would potentially require a lot of parameters to 
userspace. We would really need to duplicate all the currently existing 
sysfs parms to accomplish this.

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [PATCH 1/1] PCI: Dynids - passing driver data

[Adam Belay]

On Mon, Feb 07, 2005 at 02:18:20PM -0800, Greg KH wrote:
> On Mon, Feb 07, 2005 at 04:00:27PM -0600, brking@us.ibm.com wrote:
> > 
> > Currently, code exists in the pci layer to allow userspace to specify
> > driver data when adding a pci dynamic id from sysfs. However, this data
> > is never used and there exists no way in the existing code to use it.
> 
> Which is a good thing, right?  "driver_data" is usually a pointer to
> somewhere.  Having userspace specify it would not be a good thing.

Yeah, I don't think it's safe to use "driver_data".  Although it can be a
set of flags, it's also often used as an index in an array, or as a
pointer.  An invalid value could result in an oops.

Most drivers don't use "driver_data".  For those that do, it might be
helpful to have the capability of setting a few static configuration values
before probing the device. Currently "driver_data" fills this role.
Perhaps we need a new mechanism that would be more useable with sysfs?
The current code is limiting because the configuration options in
"driver_data" are not well defined.  Any ideas?

Thanks,
Adam

P.S.: The pci serial driver is a good example.

Re: [PATCH 1/1] PCI: Dynids - passing driver data

[Brian King]

Adam Belay wrote:
> On Mon, Feb 07, 2005 at 02:18:20PM -0800, Greg KH wrote:
> 
>>On Mon, Feb 07, 2005 at 04:00:27PM -0600, brking@us.ibm.com wrote:
>>
>>>Currently, code exists in the pci layer to allow userspace to specify
>>>driver data when adding a pci dynamic id from sysfs. However, this data
>>>is never used and there exists no way in the existing code to use it.
>>
>>Which is a good thing, right?  "driver_data" is usually a pointer to
>>somewhere.  Having userspace specify it would not be a good thing.
> 
> 
> Yeah, I don't think it's safe to use "driver_data".  Although it can be a
> set of flags, it's also often used as an index in an array, or as a
> pointer.  An invalid value could result in an oops.
> 
> Most drivers don't use "driver_data".  For those that do, it might be
> helpful to have the capability of setting a few static configuration values
> before probing the device. Currently "driver_data" fills this role.
> Perhaps we need a new mechanism that would be more useable with sysfs?
> The current code is limiting because the configuration options in
> "driver_data" are not well defined.  Any ideas?

Unfortunately, from what I have gathered so far, driver_data is pretty 
unique for different drivers, especially when comparing the usage in the 
pci serial driver to a scsi hba driver like ipr.

Certainly, a driver that uses driver_data as a pointer cannot allow it 
to be passed in from userspace. But a device driver that uses it as an 
index into an array can easily allow it to be passed in and do some 
simple range checking on it to verify it is a valid value before 
indexing into the array.


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center