LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Greg KH <greg@kroah.com>
To: Grant Grundler <grundler@parisc-linux.org>
Cc: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>,
	Linux Kernel list <linux-kernel@vger.kernel.org>,
	linux-pci@atrey.karlin.mff.cuni.cz, Andrew Morton <akpm@osdl.org>,
	e1000-devel@lists.sourceforge.net, linux-scsi@vger.kernel.org,
	Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Subject: Re: [PATCH 1/5] Update Documentation/pci.txt
Date: Tue, 5 Dec 2006 23:26:51 -0800	[thread overview]
Message-ID: <20061206072651.GG17199@kroah.com> (raw)
In-Reply-To: <20061124051217.GB8202@colo.lackof.org>

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

  parent reply	other threads:[~2006-12-06  7:27 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2006-11-22  8:05 Hidetoshi Seto
2006-11-22  8:54 ` Arjan van de Ven
2006-11-22 18:28 ` Grant Grundler
2006-11-24  0:38   ` Hidetoshi Seto
2006-11-24  5:12     ` Grant Grundler
2006-11-24  6:05       ` Hidetoshi Seto
2006-12-06  7:26       ` Greg KH [this message]
2006-12-07  3:55         ` Grant Grundler
2006-12-10  7:25         ` Grant Grundler
2006-12-15 17:02           ` Greg KH
2006-12-18  7:11             ` Grant Grundler
2006-12-22 19:46               ` Randy Dunlap
2006-12-22 21:52                 ` Stefan Richter
2006-12-24  6:11                 ` Grant Grundler
2006-12-24  6:07               ` [PATCH] Update Documentation/pci.txt v7 Grant Grundler
2006-12-24 19:16                 ` Randy Dunlap
2006-12-25  7:59                   ` Grant Grundler
2006-12-25  8:06                 ` Grant Grundler
2006-12-25  8:08                   ` Grant Grundler
2007-01-02 21:45                     ` Greg KH
2007-01-03  7:15                       ` Grant Grundler
2006-12-25  9:04                   ` Kenji Kaneshige
2007-01-16 22:26                   ` patch pci-rework-documentation-pci.txt.patch added to gregkh-2.6 tree gregkh
2007-01-17  9:10                     ` Jiri Slaby
2007-01-17 19:21                       ` Greg KH

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20061206072651.GG17199@kroah.com \
    --to=greg@kroah.com \
    --cc=akpm@osdl.org \
    --cc=e1000-devel@lists.sourceforge.net \
    --cc=grundler@parisc-linux.org \
    --cc=kaneshige.kenji@jp.fujitsu.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@atrey.karlin.mff.cuni.cz \
    --cc=linux-scsi@vger.kernel.org \
    --cc=seto.hidetoshi@jp.fujitsu.com \
    --subject='Re: [PATCH 1/5] Update Documentation/pci.txt' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).