On Thu, Nov 23, 2006 at 10:12:17PM -0700, Grant Grundler wrote:
> On Fri, Nov 24, 2006 at 09:38:00AM +0900, Hidetoshi Seto wrote:
> > Grant Grundler wrote:
> > >Hidetoshi,
> > >I have a nearly finished rewrite of Documentation/pci.txt.
> > >Can you drop this patch for now on my promise to integrate
> > >your proposed text?
> > 
> > No problem at all.
> 
> Thanks - I've posted pci.txt-05 on:
>       http://www.parisc-linux.org/~grundler/pci.txt-05
> 
> and appended it below.
> 
> pci.txt-03 is the last version I posted.
> pci.txt-04 contains all feedback from Andi Kleen and Randi Dunlap
>            (plus a few other minor changes)
> pci.txt-05 reverts pci_enable_device/pci_request_resource ordering to
>       reflect current reality. But I've added a comment to remind us
>       about the issue. Also added Section 10/11 from Hidetoshi-san.
>       A few minor other changes as well.
> 
> If this looks good, I'll post a diff for gregkh.

This looks very good, thanks for doing this work.  I do have a few minor
comments:

> 1. pci_register_driver() call
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> PCI device drivers call pci_register_driver() during their
> initialization with a pointer to a structure describing the driver
> (struct pci_driver):
> 
>       field name      Description
>       ----------      ------------------------------------------------------
>       id_table        Pointer to table of device ID's the driver is
>                       interested in.  Most drivers should export this
>                       table using MODULE_DEVICE_TABLE(pci,...).
> 
>       probe           This probing function gets called (during execution
>                       of pci_register_driver() for already existing
>                       devices or later if a new device gets inserted) for
>                       all PCI devices which match the ID table and are not
>                       "owned" by the other drivers yet. This function gets
>                       passed a "struct pci_dev *" for each device whose
>                       entry in the ID table matches the device. The probe
>                       function returns zero when the driver chooses to
>                       take "ownership" of the device or an error code
>                       (negative number) otherwise.
>                       The probe function always gets called from process
>                       context, so it can sleep.
> 
>       remove          The remove() function gets called whenever a device
>                       being handled by this driver is removed (either during
>                       deregistration of the driver or when it's manually
>                       pulled out of a hot-pluggable slot).
>                       The remove function always gets called from process
>                       context, so it can sleep.
> 
>       save_state      Save a device's state before it is suspended.

There is no such callback.  We have "suspend", "suspend_late",
"resume_early", and "resume".  You might want to update this.

> 
>       suspend         Put device into low power state.
> 
>       resume          Wake device from low power state.
> 
>       enable_wake     Enable device to generate wake events from a low power
>                       state.
> 
>                       (Please see Documentation/power/pci.txt for descriptions
>                       of PCI Power Management and the related functions.)


> 
> 
> The ID table is an array of struct pci_device_id entries ending with an
> all-zero entry.  Each entry consists of:
> 
>       vendor,device   Vendor and device ID to match (or PCI_ANY_ID)
> 
>       subvendor,      Subsystem vendor and device ID to match (or PCI_ANY_ID)
>       subdevice,
> 
>       class           Device class, subclass, and "interface" to match.
>                       See Appendix D of the PCI Local Bus Spec or
>                       include/linux/pci_ids.h for a full list of classes.
>                       Most drivers do not need to specify class/class_mask
>                       as vendor/device is normally sufficient.
> 
>       class_mask      limit which sub-fields of the class field are compared.
>                       See drivers/scsi/sym53c8xx_2/ for example of usage.
> 
>       driver_data     Data private to the driver.
>                       Most drivers don't need to use driver_data field.
>                       Best practice is to use driver_data as an index
>                       into a static list of equivalent device types,
>                       instead of using it as a pointer.

Perhaps mention the PCI_DEVICE() and PCI_DEVICE_CLASS() macros to set
these fields properly?

> Have a table entry {PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID}

PCI_DEVICE(PCI_ANY_ID, PCI_ANY_ID) is a bit smaller :)

> to have probe() called for every PCI device known to the system.
> 
> New PCI IDs may be added to a device driver pci_ids table at runtime
> as shown below:
> 
> echo "vendor device subvendor subdevice class class_mask driver_data" > \
> /sys/bus/pci/drivers/{driver}/new_id
> 
> All fields are passed in as hexadecimal values (no leading 0x).
> Users need pass only as many fields as necessary:
>       ovendor, device, subvendor, and subdevice fields default
>       to PCI_ANY_ID (FFFFFFFF),
>       oclass and classmask fields default to 0
>       odriver_data defaults to 0UL.

What's with the "o"s here?

> Once added, the driver probe routine will be invoked for any unclaimed
> PCI devices listed in its (newly updated) pci_ids list.
> 
> 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 (zero) is passed in that field.

Note that _no one_ uses this field, so it might go away soon...

> 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.
> 
> Please mark the initialization and cleanup functions where appropriate
> (the corresponding macros are defined in <linux/init.h>):
> 
>       __init          Initialization code. Thrown away after the driver
>                       initializes.
>       __exit          Exit code. Ignored for non-modular drivers.
>       __devinit       Device initialization code. Identical to __init if
>                       the kernel is not compiled with CONFIG_HOTPLUG, normal
>                       function otherwise.
>       __devexit       The same for __exit.
> 
> Tips on marks:
>       o The module_init()/module_exit() functions (and all initialization
>           functions called _only_ from these) should be marked __init/__exit.
> 
>       o The struct pci_driver shouldn't be marked with any of these tags.
> 
>       o The ID table array should be marked __devinitdata.
> 
>       o The probe() and remove() functions (and all initialization
>         functions called only from these) should be marked __devinit
>         and __devexit.
> 
>       o If the driver is not a hotplug driver then use only
>         __init/__exit and __initdata/__exitdata.

No, don't say this, pci drivers are not "hotplug drivers", and since
CONFIG_HOTPLUG is really hard to turn off these days, don't confuse
people with the devinit stuff.  Everyone gets it wrong...

>       o Pointers to functions marked as __devexit must be created using
>         __devexit_p(function_name).  That will generate the function
>         name or NULL if the __devexit function will be discarded.

I really recommend just not using any of these for almost all PCI
drivers, as the space savings just really isn't there...

thanks,

greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to