LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* pci_device_id definition cleanups
@ 2008-02-15 23:21 Jonas Bonn
  2008-02-16  0:05 ` Greg KH
  2008-02-16  2:23 ` Sam Ravnborg
  0 siblings, 2 replies; 7+ messages in thread
From: Jonas Bonn @ 2008-02-15 23:21 UTC (permalink / raw)
  To: linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]

I've done some work on cleaning up the definitions of pci_device_id to
make them "static const" (where possible) and to make sure they go into
__devinitconst.  There are about 350 changes of the type shown in the
diff at the end of this mail.

All these changes are in my public GIT tree at:

git://www.southpole.se/~jonas/git/linux.git

(Based on 2.6.25-rc2)

In addition to these pci_device_id changes, there are a few changesets
that move "const" data from __devinitdata to __devinitconst.

The tree above builds with both allmodconfig and allyesconfig.

/Jonas

------

Representative example change:

diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
index 4d59ae8..90c7820 100644
--- a/drivers/scsi/megaraid.c
+++ b/drivers/scsi/megaraid.c
@@ -4967,7 +4967,7 @@ megaraid_shutdown(struct pci_dev *pdev)
 	__megaraid_shutdown(adapter);
 }
 
-static struct pci_device_id megaraid_pci_tbl[] = {
+static const struct pci_device_id megaraid_pci_tbl[] __devinitconst = {
 	{PCI_VENDOR_ID_AMI, PCI_DEVICE_ID_AMI_MEGARAID,
 		PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
 	{PCI_VENDOR_ID_AMI, PCI_DEVICE_ID_AMI_MEGARAID2,



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: pci_device_id definition cleanups
  2008-02-15 23:21 pci_device_id definition cleanups Jonas Bonn
@ 2008-02-16  0:05 ` Greg KH
  2008-02-16  0:55   ` Jeff Garzik
  2008-02-16  2:23 ` Sam Ravnborg
  1 sibling, 1 reply; 7+ messages in thread
From: Greg KH @ 2008-02-16  0:05 UTC (permalink / raw)
  To: Jonas Bonn; +Cc: linux-kernel

On Sat, Feb 16, 2008 at 12:21:40AM +0100, Jonas Bonn wrote:
> I've done some work on cleaning up the definitions of pci_device_id to
> make them "static const" (where possible) and to make sure they go into
> __devinitconst.  There are about 350 changes of the type shown in the
> diff at the end of this mail.
> 
> ???All these changes are in my public GIT tree at:
> 
> git://www.southpole.se/~jonas/git/linux.git
> 
> (Based on 2.6.25-rc2)
> 
> In addition to these pci_device_id changes, there are a few changesets
> that move "const" data from __devinitdata to __devinitconst.
> 
> The tree above builds with both allmodconfig and allyesconfig.

Hm, does this save us any memory on any type of configuration?

What about drivers that end up writing to these structures (I know some
USB drivers do, not sure about PCI ones.)

You're going to need to send out patches for these to the different
developers, a git tree isn't going to help much.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: pci_device_id definition cleanups
  2008-02-16  0:05 ` Greg KH
@ 2008-02-16  0:55   ` Jeff Garzik
  2008-02-16  0:57     ` Greg KH
  0 siblings, 1 reply; 7+ messages in thread
From: Jeff Garzik @ 2008-02-16  0:55 UTC (permalink / raw)
  To: Greg KH; +Cc: Jonas Bonn, linux-kernel

Greg KH wrote:
> On Sat, Feb 16, 2008 at 12:21:40AM +0100, Jonas Bonn wrote:
>> I've done some work on cleaning up the definitions of pci_device_id to
>> make them "static const" (where possible) and to make sure they go into
>> __devinitconst.  There are about 350 changes of the type shown in the
>> diff at the end of this mail.
>>
>> ???All these changes are in my public GIT tree at:
>>
>> git://www.southpole.se/~jonas/git/linux.git
>>
>> (Based on 2.6.25-rc2)
>>
>> In addition to these pci_device_id changes, there are a few changesets
>> that move "const" data from __devinitdata to __devinitconst.
>>
>> The tree above builds with both allmodconfig and allyesconfig.
> 
> Hm, does this save us any memory on any type of configuration?
> 
> What about drivers that end up writing to these structures (I know some
> USB drivers do, not sure about PCI ones.)

I don't recall ever seeing a PCI driver do that... and if it exists on 
the PCI side, I would be motivated to create patches to "fix" such 
behavior :)

That information is exported to utilities that expect that table to be 
static.  Messing around with it is just hacky, and bound to produce 
unwanted edge cases.


> You're going to need to send out patches for these to the different
> developers, a git tree isn't going to help much.

Agreed.

	Jeff





^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: pci_device_id definition cleanups
  2008-02-16  0:55   ` Jeff Garzik
@ 2008-02-16  0:57     ` Greg KH
  0 siblings, 0 replies; 7+ messages in thread
From: Greg KH @ 2008-02-16  0:57 UTC (permalink / raw)
  To: Jeff Garzik; +Cc: Jonas Bonn, linux-kernel

On Fri, Feb 15, 2008 at 07:55:07PM -0500, Jeff Garzik wrote:
> Greg KH wrote:
>> On Sat, Feb 16, 2008 at 12:21:40AM +0100, Jonas Bonn wrote:
>>> I've done some work on cleaning up the definitions of pci_device_id to
>>> make them "static const" (where possible) and to make sure they go into
>>> __devinitconst.  There are about 350 changes of the type shown in the
>>> diff at the end of this mail.
>>>
>>> ???All these changes are in my public GIT tree at:
>>>
>>> git://www.southpole.se/~jonas/git/linux.git
>>>
>>> (Based on 2.6.25-rc2)
>>>
>>> In addition to these pci_device_id changes, there are a few changesets
>>> that move "const" data from __devinitdata to __devinitconst.
>>>
>>> The tree above builds with both allmodconfig and allyesconfig.
>> Hm, does this save us any memory on any type of configuration?
>> What about drivers that end up writing to these structures (I know some
>> USB drivers do, not sure about PCI ones.)
>
> I don't recall ever seeing a PCI driver do that... and if it exists on the 
> PCI side, I would be motivated to create patches to "fix" such behavior :)
>
> That information is exported to utilities that expect that table to be 
> static.  Messing around with it is just hacky, and bound to produce 
> unwanted edge cases.

Well, the USB drivers used to add an additional NULL entry at the end of
the list, and then populate it based on module parameters.  That way the
userspace tools would always work just fine, and you could add
additional device ids at run-time.

Now we have a way to do this for all USB and PCI drivers through sysfs,
so it's not needed, but we have to keep the backwards compatiblity for a
while in a number of USB drivers.

Hopefully PCI drivers didn't do the same thing, but I haven't looked at
all of them to verify this or not :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: pci_device_id definition cleanups
  2008-02-15 23:21 pci_device_id definition cleanups Jonas Bonn
  2008-02-16  0:05 ` Greg KH
@ 2008-02-16  2:23 ` Sam Ravnborg
  2008-02-16  5:27   ` Olof Johansson
  1 sibling, 1 reply; 7+ messages in thread
From: Sam Ravnborg @ 2008-02-16  2:23 UTC (permalink / raw)
  To: Jonas Bonn; +Cc: linux-kernel

On Sat, Feb 16, 2008 at 12:21:40AM +0100, Jonas Bonn wrote:
> I've done some work on cleaning up the definitions of pci_device_id to
> make them "static const" (where possible) and to make sure they go into
> __devinitconst.  There are about 350 changes of the type shown in the
> diff at the end of this mail.
> 
> All these changes are in my public GIT tree at:
> 
> git://www.southpole.se/~jonas/git/linux.git
> 
> (Based on 2.6.25-rc2)
> 
> In addition to these pci_device_id changes, there are a few changesets
> that move "const" data from __devinitdata to __devinitconst.
> 
> The tree above builds with both allmodconfig and allyesconfig.

Hi Jonas.

Can I ask you to try the same with ARCH=powerpc
(or alpha or ia64).
Becasue it is for these architectures we see issues with
defining data const.

	Sam

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: pci_device_id definition cleanups
  2008-02-16  2:23 ` Sam Ravnborg
@ 2008-02-16  5:27   ` Olof Johansson
  2008-02-16 12:24     ` Sam Ravnborg
  0 siblings, 1 reply; 7+ messages in thread
From: Olof Johansson @ 2008-02-16  5:27 UTC (permalink / raw)
  To: Sam Ravnborg; +Cc: Jonas Bonn, linux-kernel

On Sat, Feb 16, 2008 at 03:23:36AM +0100, Sam Ravnborg wrote:
> On Sat, Feb 16, 2008 at 12:21:40AM +0100, Jonas Bonn wrote:
> > I've done some work on cleaning up the definitions of pci_device_id to
> > make them "static const" (where possible) and to make sure they go into
> > __devinitconst.  There are about 350 changes of the type shown in the
> > diff at the end of this mail.
> > 
> > ???All these changes are in my public GIT tree at:
> > 
> > git://www.southpole.se/~jonas/git/linux.git
> > 
> > (Based on 2.6.25-rc2)
> > 
> > In addition to these pci_device_id changes, there are a few changesets
> > that move "const" data from __devinitdata to __devinitconst.
> > 
> > The tree above builds with both allmodconfig and allyesconfig.
> 
> Hi Jonas.
> 
> Can I ask you to try the same with ARCH=powerpc
> (or alpha or ia64).
> Becasue it is for these architectures we see issues with
> defining data const.

I pulled his tree and tried building on powerpc w/ gcc 4.3, it passed.

I'm not too excited about the extremely long open-coded variable
definitions everywhere now though. Wouldn't it be better to just do a
macro for it?

Something like:

#define PCI_DEVICE_TABLE(_var) static const struct pci_device_id _var[] __devinitconst

And then just:

PCI_DEVICE_TABLE(mydevice_tbl) = {
	...
};


-Olof

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: pci_device_id definition cleanups
  2008-02-16  5:27   ` Olof Johansson
@ 2008-02-16 12:24     ` Sam Ravnborg
  0 siblings, 0 replies; 7+ messages in thread
From: Sam Ravnborg @ 2008-02-16 12:24 UTC (permalink / raw)
  To: Olof Johansson; +Cc: Jonas Bonn, linux-kernel

On Fri, Feb 15, 2008 at 11:27:38PM -0600, Olof Johansson wrote:
> On Sat, Feb 16, 2008 at 03:23:36AM +0100, Sam Ravnborg wrote:
> > On Sat, Feb 16, 2008 at 12:21:40AM +0100, Jonas Bonn wrote:
> > > I've done some work on cleaning up the definitions of pci_device_id to
> > > make them "static const" (where possible) and to make sure they go into
> > > __devinitconst.  There are about 350 changes of the type shown in the
> > > diff at the end of this mail.
> > > 
> > > ???All these changes are in my public GIT tree at:
> > > 
> > > git://www.southpole.se/~jonas/git/linux.git
> > > 
> > > (Based on 2.6.25-rc2)
> > > 
> > > In addition to these pci_device_id changes, there are a few changesets
> > > that move "const" data from __devinitdata to __devinitconst.
> > > 
> > > The tree above builds with both allmodconfig and allyesconfig.
> > 
> > Hi Jonas.
> > 
> > Can I ask you to try the same with ARCH=powerpc
> > (or alpha or ia64).
> > Becasue it is for these architectures we see issues with
> > defining data const.
> 
> I pulled his tree and tried building on powerpc w/ gcc 4.3, it passed.

Thanks for testing!

> 
> I'm not too excited about the extremely long open-coded variable
> definitions everywhere now though. Wouldn't it be better to just do a
> macro for it?
> 
> Something like:
> 
> #define PCI_DEVICE_TABLE(_var) static const struct pci_device_id _var[] __devinitconst
Fully agreed - but this is Greg's area I guess. Try submitting him a patch.

	Sam

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2008-02-16 12:24 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-02-15 23:21 pci_device_id definition cleanups Jonas Bonn
2008-02-16  0:05 ` Greg KH
2008-02-16  0:55   ` Jeff Garzik
2008-02-16  0:57     ` Greg KH
2008-02-16  2:23 ` Sam Ravnborg
2008-02-16  5:27   ` Olof Johansson
2008-02-16 12:24     ` Sam Ravnborg

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).