Displaying/modifying PCI device id tables via sysfs

Displaying/modifying PCI device id tables via sysfs

[Matt Domsch]

A lot of PCI drivers today use the pci_device_id table model to specify 
what IDs the driver supports.  I'd like to be able to do 2 things with 
this information:
1) display it in sysfs
2) Add new IDs at runtime and have the drivers probe for the new IDs.

To this end, I've started thinking about how to properly display the table 
via sysfs.  So far I've got:


/sys
|-- bus
|   |-- pci
|   |   `-- drivers
|   |       |-- 3c59x
|   |       |   `-- id_table
|   |       |-- PIIX IDE
|   |       |   `-- id_table
|   |       |-- RZ1000 IDE
|   |       |   `-- id_table
|   |       |-- aic7xxx
|   |       |   `-- id_table
|   |       |-- e100
|   |       |   `-- id_table
|   |       |-- e1000
|   |       |   `-- id_table
|   |       |-- eepro100
|   |       |   `-- id_table
|   |       `-- serial
|   |           `-- id_table


where id_table is a file that looks like:

  vendor,   device, subvendor, subdevice,    class, class_mask
00009004, ffffffff,  ffffffff,  ffffffff, 00010000,   00ffff00
00009005, ffffffff,  ffffffff,  ffffffff, 00010000,   00ffff00

Now, this isn't ideal.
a) It violates sysfs rule of one value per file
b) for drivers with lots of IDs (like 3c59x), it could be longer than
PAGE_SIZE.

So, I've thought about a directory model:


/sys
`-- bus
    `-- pci
        `-- drivers
            `-- 3c59x
                |-- dynamic_id_table
                |   |-- 0
                |   |   |-- class
                |   |   |-- class_mask
                |   |   |-- device
                |   |   |-- subdevice
                |   |   |-- subvendor
                |   |   `-- vendor
                |   `-- new_id
                `-- static_id_table
                    |-- 0
                    |   |-- class
                    |   |-- class_mask
                    |   |-- device
                    |   |-- subdevice
                    |   |-- subvendor
                    |   `-- vendor


The new_id file in the dynamic_id_table would be writable, used for
adding new IDs.

This solves a and b both, at the expense of adding two additional
directories to the hierarchy.

The current sysfs implementation doesn't cleanly handle this type of
data display however.  sysfs_create_dir() takes a kobject*, but I
don't think we want to instantiate kobjects which are simply 
groupings for attribute entries in a table, do we?

I've started down this road, but before I go too far want validation that 
I'm on the right track instantiating kobjects for these table entries.  
Something doesn't feel quite right about it.

I've so far had to:
a) add a struct kobject and a struct list_head (for the dynamic list) to 
pci_device_id.
b) add 2 struct kobjects and 1 struct list_head to struct pci_driver
c) create them all in drivers/pci/pci-sysfs.c.

I also think I need some type of ATTR_ATTR macro, as I don't have whole 
devices for these, just kobjects.  That's where it starts getting really 
weird.


Feedback requested.

Thanks,
Matt

-- 
Matt Domsch
Sr. Software Engineer, Lead Engineer, Architect
Dell Linux Solutions www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Re: Displaying/modifying PCI device id tables via sysfs

[Greg KH]

On Mon, Mar 03, 2003 at 11:57:05AM -0600, Matt Domsch wrote:
> A lot of PCI drivers today use the pci_device_id table model to specify 
> what IDs the driver supports.  I'd like to be able to do 2 things with 
> this information:
> 1) display it in sysfs

That info is already exported to userspace through the modules.pcimap
file.  It's also available in raw form in the .o file if you want to
export it to any other format that you may want to use.  Why export it
again through sysfs?

> 2) Add new IDs at runtime and have the drivers probe for the new IDs.

Ick, no.  If a driver really wants to have a user provide new ids on the
fly, they usually provide a module paramater to do this.  A number of
the USB drivers do this already (and to be honest, they have caused
nothing but trouble, as users use that option for new devices, that the
driver can't control at all due to protocol or register location
changes.)

In short, it's not a good idea to allow users to change these values on
the fly, the driver author usually knows best here :)

thanks,

greg k-h

Re: Displaying/modifying PCI device id tables via sysfs

[Alan Cox]

On Mon, 2003-03-03 at 18:25, Greg KH wrote:
> > 2) Add new IDs at runtime and have the drivers probe for the new IDs.
> 
> Ick, no.  If a driver really wants to have a user provide new ids on the
> fly, they usually provide a module paramater to do this.  A number of
> the USB drivers do this already (and to be honest, they have caused
> nothing but trouble, as users use that option for new devices, that the
> driver can't control at all due to protocol or register location
> changes.)
> 
> In short, it's not a good idea to allow users to change these values on
> the fly, the driver author usually knows best here :)

I would strongly disagree with that statement. The majority of users run
kernels a little behind the times, and in may cases adding the PCI id is
just fine. It doens't want to be casual and uncontrolled but it does want
to be possible

Re: Displaying/modifying PCI device id tables via sysfs

[Matt Domsch]

> That info is already exported to userspace through the modules.pcimap
> file.

OK.

> > 2) Add new IDs at runtime and have the drivers probe for the new IDs.
> 
> Ick, no.  If a driver really wants to have a user provide new ids on the
> fly, they usually provide a module paramater to do this.

Yes, I've done this kind of thing too with aacraid.  I was hoping to 
generalize the process and build upon the ID table already present.

> A number of the USB drivers do this already (and to be honest, they
> have caused nothing but trouble, as users use that option for new
> devices, that the driver can't control at all due to protocol or
> register location changes.)
> 
> In short, it's not a good idea to allow users to change these values on
> the fly, the driver author usually knows best here :)

Indeed, it only works if simply adding PCI IDs is the only change needed
to support a new device.

Not everyone can run a recent kernel with the most recent IDs in the
driver.  Take for example an distribution release on CD.  It's very hard
to update the kernel running at OS install time to handle a new device,
even if the driver would otherwise handle that device if only it had the
PCI IDs.  This can easily happen if the hardware subsystem vendor/device
IDs change, but the driver has a hard-coded list of IDs to test which
include specific subsystem vendor/device IDs.

Built-in drivers (e.g. IDE today) are even harder - there you can't just
replace a driver module with one built with additional IDs, you've got to
replace the whole kernel.  It would be nice, for the cases where it works, 
to add IDs at runtime and continue, and later upgrade to a newer kernel 
that includes the IDs in the driver source.


Adding IDs to drivers at runtime is definitely a stop-gap measure, and 
only works when drivers don't need other changes, but it solves an 
important subset of the problem space.

DKMS, announced last Friday on lkml, is the next step in this progression.  
It can help driver maintainers build and release driver modules for the
case when driver updates are necessary, but whole kernel updates are not
necessary, or perhaps even possible.

The last step in this progression is the release of whole new kernels,
which include updated device drivers to match.  Certainly we encourage
this from a developer perspective, but it isn't always feasible from a
distributor or customer perspective.


Thanks,
Matt

-- 
Matt Domsch
Sr. Software Engineer, Lead Engineer, Architect
Dell Linux Solutions www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Re: Displaying/modifying PCI device id tables via sysfs

[Greg KH]

On Mon, Mar 03, 2003 at 02:56:23PM -0600, Matt Domsch wrote:
> > > 2) Add new IDs at runtime and have the drivers probe for the new IDs.
> > 
> > Ick, no.  If a driver really wants to have a user provide new ids on the
> > fly, they usually provide a module paramater to do this.
> 
> Yes, I've done this kind of thing too with aacraid.  I was hoping to 
> generalize the process and build upon the ID table already present.

Ok, you and Alan have convinced me :)

I'd like to see what your patch looks like to add this kind of support.

thanks,

greg k-h

Re: Displaying/modifying PCI device id tables via sysfs

[Matt Domsch]

> I'd like to see what your patch looks like to add this kind of support.

I don't have any code that I'm proud enough of to post quite yet, I'm
just trying to get the overall design concept right first.  Your
feedback is appreciated.

Jeff said:
> Field-replacement of PCI id tables is a todo item for a while now :)

Replacement, or addition?  I've been picturing addition, not
modification or replacement.  For modification, we would have to export
the entries via sysfs, which Greg had just convinced me I didn't need to
do. :-)

As far as the sysfs layout could go, here's what I've in mind for
simplicity sake.

/sys
`-- bus
    `-- pci
        `-- drivers
            `-- 3c59x
                |-- dynamic_id_0  (these are simple DRIVER_ATTRs)
                |-- dynamic_id_1
                |-- dynamic_id_2
                `-- new_id

Where dynamic_id_[012] are new dynamic entries, created by writing
values into new_id.  Both file types would be of the format (analogous
to pci_show_resources):
echo "0x0000 0x0000 0x0000 0x0000 0x0000 0x0000" > new_id
with fields being vendor, device, subvendor, subdevice, class,
class_mask.

To be safe, the store method for new_id should walk the static table and
the dynamic ID table, ensuring it's not creating a duplicate.

The new entries would get added to a new list in struct pci_driver, and
struct pci_id_table would grow a struct list_head so new list entries
could be easily created and added to the list.  The N in dynamic_id_N
would be a per-driver atomic_t, monotonically increasing as new IDs are
given.

It's then simple to have a script run at shutdown/startup to
save/restore IDs if desired.

The next step is to get the system to probe for devices matching the new
IDs.  Two new functions similar to pci_device_probe() and
pci_match_device() need implementing, where given a struct pci_driver
and a struct pci_device_id, it would probe.  The logic of
pci_match_device could be used with two wrappers, one to walk the static
ID table, one to match a new ID.


If you'd prefer to see this:
3c59x/
`-- dynamic_id_table
    |-- 0
    |   |-- class
    |   |-- class_mask
    |   |-- device
    |   |-- subdevice
    |   |-- subvendor
    |   `-- vendor
    `-- new_id

then there needs to be a simple way in sysfs to export an attribute
hierarchy, beneath an object in the kobject hierarchy.  Right now it's
assumed that each kobject can have multiple attributes, but all are at a
single level.  Pat, is this hard to do?


How far off base am I? :-)

Thanks,
Matt

-- 
Matt Domsch
Sr. Software Engineer, Lead Engineer, Architect
Dell Linux Solutions www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Re: Displaying/modifying PCI device id tables via sysfs

[Patrick Mochel]

> then there needs to be a simple way in sysfs to export an attribute
> hierarchy, beneath an object in the kobject hierarchy.  Right now it's
> assumed that each kobject can have multiple attributes, but all are at a
> single level.  Pat, is this hard to do?

It's 1 kobject == 1 sysfs directory. I recommend putting a kobject into 
the ID structure and registering it (by the PCI core) once the driver has 
been registered. Then, create files for each of the static IDs. 

When new IDs are added, I don't see why you need to differentiate between 
them in userspace. Couldn't you have e.g.:

/sys/bus/pci/drivers/3c59x/
`-- id
    |-- 0
    |-- 1
    `-- new_id

Then, when someone writes a new ID, have:

/sys/bus/pci/drivers/3c59x/
`-- id
    |-- 0
    |-- 1
    |-- 2
    `-- new_id

Internally, we of course need to treat them differently, but to the user, 
they're just IDs the driver supports..

	-pat

Re: Displaying/modifying PCI device id tables via sysfs

[Kai Germaschewski]

On 3 Mar 2003, Matt Domsch wrote:

> /sys
> `-- bus
>     `-- pci
>         `-- drivers
>             `-- 3c59x
>                 |-- dynamic_id_0  (these are simple DRIVER_ATTRs)
>                 |-- dynamic_id_1
>                 |-- dynamic_id_2
>                 `-- new_id
> 
> Where dynamic_id_[012] are new dynamic entries, created by writing
> values into new_id.  Both file types would be of the format (analogous
> to pci_show_resources):
> echo "0x0000 0x0000 0x0000 0x0000 0x0000 0x0000" > new_id
> with fields being vendor, device, subvendor, subdevice, class,
> class_mask.

I dont' think what you actually want is changing the id table - after all, 
it's only walked when registering the driver (+ hotplug).

What you really want is a way to call the drivers' probe routine for a 
device which isn't in its tables.

So why not simply

echo "0x0000 0x0000 0x0000 0x0000 0x0000 0x0000" > .../3c59x/probe

It shares one caveat with the approach above, i.e. struct pci_device_id
has a field called "unsigned long driver_data", and as such is really hard
to fill from userspace. I think in the common case it's not used and can
be just set to zero, but if the driver e.g. expects it to point to a
driver-private structure describing the device, you lose.

And you need one more thing, i.e. the ability to load a driver without
hardware being present. E.g. pci_module_init() is supposed to return
-ENODEV when modular and no hardware is found. Actually, it doesn't
anymore, since someone (I think you, Pat ;) broke it.

--Kai

Re: Displaying/modifying PCI device id tables via sysfs

[Alan Cox]

On Tue, 2003-03-04 at 20:22, Kai Germaschewski wrote:
> It shares one caveat with the approach above, i.e. struct pci_device_id
> has a field called "unsigned long driver_data", and as such is really hard
> to fill from userspace. I think in the common case it's not used and can
> be just set to zero, but if the driver e.g. expects it to point to a
> driver-private structure describing the device, you lose.

Lots of drivers will simply do the wrong thing if you do that. Version
equivalence is rather more important. Of course if you know what equivalence
you are claiming you can look it up in the table to get the pci_device_id

Re: Displaying/modifying PCI device id tables via sysfs

[Jeff Garzik]

On Tue, Mar 04, 2003 at 02:22:13PM -0600, Kai Germaschewski wrote:
> On 3 Mar 2003, Matt Domsch wrote:
> 
> > /sys
> > `-- bus
> >     `-- pci
> >         `-- drivers
> >             `-- 3c59x
> >                 |-- dynamic_id_0  (these are simple DRIVER_ATTRs)
> >                 |-- dynamic_id_1
> >                 |-- dynamic_id_2
> >                 `-- new_id
> > 
> > Where dynamic_id_[012] are new dynamic entries, created by writing
> > values into new_id.  Both file types would be of the format (analogous
> > to pci_show_resources):
> > echo "0x0000 0x0000 0x0000 0x0000 0x0000 0x0000" > new_id
> > with fields being vendor, device, subvendor, subdevice, class,
> > class_mask.
> 
> I dont' think what you actually want is changing the id table - after all, 
> it's only walked when registering the driver (+ hotplug).
> 
> What you really want is a way to call the drivers' probe routine for a 
> device which isn't in its tables.
> 
> So why not simply
> 
> echo "0x0000 0x0000 0x0000 0x0000 0x0000 0x0000" > .../3c59x/probe

I think there is value in decoupling the two operations:

	echo "0x0000 0x0000 0x0000 0x0000 0x0000 0x0000" > .../3c59x/table
	echo 1 > .../3c59x/probe_it

Because you want the id table additions to be persistant in the face of
cardbus unplug/replug, and for the case where cardbus card may not be
present yet, but {will,may} be soon.

	Jeff

Re: Displaying/modifying PCI device id tables via sysfs

[Jeff Garzik]

Matt Domsch wrote:
> Adding IDs to drivers at runtime is definitely a stop-gap measure, and 
> only works when drivers don't need other changes, but it solves an 
> important subset of the problem space.


Agreed on both points -- it's a stopgap measure, and also one that 
happens solve quite a few cases in the field.

Field-replacement of PCI id tables is a todo item for a while now :)

However, anything beyond PCI id table replacement requires code changes 
and recompilation, and that can be handled by existing patch submission 
procedures...

	Jeff

Re: Displaying/modifying PCI device id tables via sysfs

[Rusty Russell]

On Mon, 3 Mar 2003 10:25:53 -0800
Greg KH <greg@kroah.com> wrote:

> On Mon, Mar 03, 2003 at 11:57:05AM -0600, Matt Domsch wrote:
> > A lot of PCI drivers today use the pci_device_id table model to specify 
> > what IDs the driver supports.  I'd like to be able to do 2 things with 
> > this information:
> > 1) display it in sysfs
> 
> That info is already exported to userspace through the modules.pcimap
> file.

Matt's patch just went into Linus' tree, so I'll comment on this.

More importantly, the modules now contain suitable aliases embedded within
them: modules.pcimap et al will vanish before 2.6 whenever Greg (hint hint)
updates the hotplug scripts to use them instead of modules.XXXmap.

So the question is, how do you add PCI IDs to a module which isn't loaded?
You can trivially add a new alias for it, which will cause modprobe to
find it, but the module won't know it can handle the new PCI ID, and
will fail to load.

Obvious options include:
	just update the damn module
	inserting the new PCI entry in the module (simple, but icky)
	adding a module param to say "add these IDs"

I agree with Alan (and Jeff Garzik who pointed this out to me before)
that it's neat to be able to update these tables on the fly, but that
is probably even more important for modules which aren't loaded.

Matt, do you have thoughts on this?

Cheers,
Rusty.
-- 
   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy

Re: Displaying/modifying PCI device id tables via sysfs

[Greg KH]

On Fri, May 23, 2003 at 10:53:51AM +1000, Rusty Russell wrote:
> 
> More importantly, the modules now contain suitable aliases embedded within
> them: modules.pcimap et al will vanish before 2.6 whenever Greg (hint hint)
> updates the hotplug scripts to use them instead of modules.XXXmap.

Yeah, yeah, yeah, I'll get to it soon, sorry :)

> So the question is, how do you add PCI IDs to a module which isn't loaded?

Do we really care about this question?  I mean, if a user knows that
they want to use a specific module for their device (as they must know
this somehow), then they can just load the module today, and use the
dynamic id feature to add the new id.  At that time the device is bound
to the driver.

I don't really see this as a problem, but I might be missing something
obvious.

> You can trivially add a new alias for it, which will cause modprobe to
> find it, but the module won't know it can handle the new PCI ID, and
> will fail to load.

Not true.  The module will load just fine, just not bind to the device.
Well, old-style pci modules will not load, but I don't feel sorry about
them at all.  They have had about 3 years to convert to the "new" pci
api.

thanks,

greg k-h

Re: Displaying/modifying PCI device id tables via sysfs

[Rusty Russell]

In message <20030524003712.GA15086@kroah.com> you write:
> On Fri, May 23, 2003 at 10:53:51AM +1000, Rusty Russell wrote:
> > So the question is, how do you add PCI IDs to a module which isn't loaded?
> 
> Do we really care about this question?  I mean, if a user knows that
> they want to use a specific module for their device (as they must know
> this somehow), then they can just load the module today, and use the
> dynamic id feature to add the new id.  At that time the device is bound
> to the driver.

Jeff's once complained about a distro having to update modules simply
to update the PCI tables, which he indicated happened frequently.

But adding new aliases is trivial, so this covers it.

> > You can trivially add a new alias for it, which will cause modprobe to
> > find it, but the module won't know it can handle the new PCI ID, and
> > will fail to load.
> 
> Not true.  The module will load just fine, just not bind to the device.

Yes, Matt pointed this out, which makes it work fine.

Thanks,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: Displaying/modifying PCI device id tables via sysfs

[Matt_Domsch]

> So the question is, how do you add PCI IDs to a module which isn't
> loaded?

I think the key is here, in drivers/pci/pci-driver.c:

/**
 * pci_register_driver - register a new pci driver
 * @drv: the driver structure to register
 * 
 * Adds the driver structure to the list of registered drivers
 * Returns the number of pci devices which were claimed by the driver
 * during registration.  The driver remains registered even if the
 * return value is zero.
 */
int
pci_register_driver(struct pci_driver *drv)


So it's perfectly legal to have a driver loaded, for which no device
instances are present.  Then you can add the new ID, and it gets probed
for automatically.  When I was testing this, I had the e100 driver built
in statically, and though it didn't have the ID for my actual device in
the table, it remained available.  When I used the new_id interface to
add the ID, then it probed for devices again, the new ID matched, and it
attached itself to the hardware instance.

Granted, today not all the drivers do this.  But I think it's part of
the grand hotplug scheme to allow them to do so.

This does also mean that a whole pile of drivers, for which hardware may
*never* be present, could be loaded.  My sysfs tree shows drivers for
several PCI devices, which I left as =y in my .config file, which don't
exist in my system.

I'll be out of touch the next few days, but will join back in any
discussion next week.

Thanks,
Matt

-- 
Matt Domsch
Sr. Software Engineer, Lead Engineer, Architect
Dell Linux Solutions www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com