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://http://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,
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://http://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
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://http://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
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://http://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
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://http://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
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://http://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
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://http://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