LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] Add PCI_DEVICE_TABLE macro
@ 2008-02-18  8:03 Jonas Bonn
  0 siblings, 0 replies; 9+ messages in thread
From: Jonas Bonn @ 2008-02-18  8:03 UTC (permalink / raw)
  To: linux-kernel, greg; +Cc: olof, randy.dunlap, Jonas Bonn

The definitions of struct pci_device_id arrays should generally follow
the same pattern across the entire kernel.  This macro defines this
array as const and puts it into the __devinitconst section.

Signed-off-by: Jonas Bonn <jonas@southpole.se>
---
 include/linux/pci.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 87195b6..c7a91b1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -389,6 +389,15 @@ struct pci_driver {
 #define	to_pci_driver(drv) container_of(drv, struct pci_driver, driver)
 
 /**
+ * PCI_DEVICE_TABLE - macro used to describe a pci device table
+ * @_table: device table name
+ *
+ * This macro is used to create a struct pci_device_id array (a device table) 
+ * in a generic manner.
+ */
+#define PCI_DEVICE_TABLE(_table) const struct pci_device_id _table[] __devinitconst
+
+/**
  * PCI_DEVICE - macro used to describe a specific pci device
  * @vend: the 16 bit PCI Vendor ID
  * @dev: the 16 bit PCI Device ID
-- 
1.5.3.8



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

* Re: [PATCH] Add PCI_DEVICE_TABLE macro
  2008-02-20 16:11       ` Greg KH
@ 2008-02-20 17:05         ` Jeff Garzik
  0 siblings, 0 replies; 9+ messages in thread
From: Jeff Garzik @ 2008-02-20 17:05 UTC (permalink / raw)
  To: Greg KH; +Cc: Jonas Bonn, linux-kernel, olof, sam

Greg KH wrote:
> On Mon, Feb 18, 2008 at 08:34:42AM +0100, Jonas Bonn wrote:
>>> And again, what does this buy us?
>> Clarity and simplicity, I hope... there are a bunch of definitions 
>> scattered about the kernel that omit the __devinitdata modifier despite the 
>> documentation stating that it should always be there.  The definition 
>> really should have been const, which wasn't possible before but has become 
>> so with the addition of the __devinitconst attribute.
>>
>> Furthermore, there are definitions that use "const" and __devinitdata, 
>> which is explicitly wrong but the compiler doesn't catch section mismatches 
>> if there's only one such one case in the module (which is often the case).
>>
>> Adding the __devinitconst modifier where there was nothing before buys us 
>> memory.  Adding the const modifier gives the compiler a chance to do its 
>> thing.  Changing __devinitdata to __devinitconst where it was wrong 
>> actually fixes some compiler errors in older (mid-release) kernels that 
>> were patched over by "removing" the section attribute altogether (which 
>> wastes memory).
>>
>> Adding the macro (Olof's idea, not mine) makes it pretty difficult to get 
>> this definition wrong... I'll do the rest of the cleanup, but I need to 
>> know whether it's better to use a macro like this, or to open code the 
>> definitions.  I prefer the macro approach...
>>
>> Hope this makes some sense...
> 
> Ok, yes it does, thanks for the explaination.
> 
> Please, can you add this very good text to the changelog entry for the
> addition of the macro, and to the documentation somewhere?  I'd be glad
> to take the patch if that was done.

I would suggest having a DECLARE_ prefix in there, like other subsystems 
do...

	Jeff





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

* Re: [PATCH] Add PCI_DEVICE_TABLE macro
  2008-02-18  7:34     ` Jonas Bonn
@ 2008-02-20 16:11       ` Greg KH
  2008-02-20 17:05         ` Jeff Garzik
  0 siblings, 1 reply; 9+ messages in thread
From: Greg KH @ 2008-02-20 16:11 UTC (permalink / raw)
  To: Jonas Bonn; +Cc: linux-kernel, olof, sam

On Mon, Feb 18, 2008 at 08:34:42AM +0100, Jonas Bonn wrote:
>> And again, what does this buy us?
>
> Clarity and simplicity, I hope... there are a bunch of definitions 
> scattered about the kernel that omit the __devinitdata modifier despite the 
> documentation stating that it should always be there.  The definition 
> really should have been const, which wasn't possible before but has become 
> so with the addition of the __devinitconst attribute.
>
> Furthermore, there are definitions that use "const" and __devinitdata, 
> which is explicitly wrong but the compiler doesn't catch section mismatches 
> if there's only one such one case in the module (which is often the case).
>
> Adding the __devinitconst modifier where there was nothing before buys us 
> memory.  Adding the const modifier gives the compiler a chance to do its 
> thing.  Changing __devinitdata to __devinitconst where it was wrong 
> actually fixes some compiler errors in older (mid-release) kernels that 
> were patched over by "removing" the section attribute altogether (which 
> wastes memory).
>
> Adding the macro (Olof's idea, not mine) makes it pretty difficult to get 
> this definition wrong... I'll do the rest of the cleanup, but I need to 
> know whether it's better to use a macro like this, or to open code the 
> definitions.  I prefer the macro approach...
>
> Hope this makes some sense...

Ok, yes it does, thanks for the explaination.

Please, can you add this very good text to the changelog entry for the
addition of the macro, and to the documentation somewhere?  I'd be glad
to take the patch if that was done.

thanks,

greg k-h

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

* [PATCH] Add PCI_DEVICE_TABLE macro
  2008-02-20 12:53 pci_device_id cleanups Jonas Bonn
@ 2008-02-20 12:53 ` Jonas Bonn
  0 siblings, 0 replies; 9+ messages in thread
From: Jonas Bonn @ 2008-02-20 12:53 UTC (permalink / raw)
  To: linux-kernel, netdev; +Cc: greg, Jonas Bonn

The definitions of struct pci_device_id arrays should generally follow
the same pattern across the entire kernel.  This macro defines this
array as const and puts it into the __devinitconst section.

Signed-off-by: Jonas Bonn <jonas@southpole.se>
---
 include/linux/pci.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 87195b6..c7a91b1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -389,6 +389,15 @@ struct pci_driver {
 #define	to_pci_driver(drv) container_of(drv, struct pci_driver, driver)
 
 /**
+ * PCI_DEVICE_TABLE - macro used to describe a pci device table
+ * @_table: device table name
+ *
+ * This macro is used to create a struct pci_device_id array (a device table) 
+ * in a generic manner.
+ */
+#define PCI_DEVICE_TABLE(_table) const struct pci_device_id _table[] __devinitconst
+
+/**
  * PCI_DEVICE - macro used to describe a specific pci device
  * @vend: the 16 bit PCI Vendor ID
  * @dev: the 16 bit PCI Device ID
-- 
1.5.3.8



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

* Re: [PATCH] Add PCI_DEVICE_TABLE macro
  2008-02-18  4:48   ` Greg KH
@ 2008-02-18  7:34     ` Jonas Bonn
  2008-02-20 16:11       ` Greg KH
  0 siblings, 1 reply; 9+ messages in thread
From: Jonas Bonn @ 2008-02-18  7:34 UTC (permalink / raw)
  To: Greg KH; +Cc: linux-kernel, olof, sam

> 
> And again, what does this buy us?
> 

Clarity and simplicity, I hope... there are a bunch of definitions 
scattered about the kernel that omit the __devinitdata modifier despite 
the documentation stating that it should always be there.  The 
definition really should have been const, which wasn't possible before 
but has become so with the addition of the __devinitconst attribute.

Furthermore, there are definitions that use "const" and __devinitdata, 
which is explicitly wrong but the compiler doesn't catch section 
mismatches if there's only one such one case in the module (which is 
often the case).

Adding the __devinitconst modifier where there was nothing before buys 
us memory.  Adding the const modifier gives the compiler a chance to do 
its thing.  Changing __devinitdata to __devinitconst where it was wrong 
actually fixes some compiler errors in older (mid-release) kernels that 
were patched over by "removing" the section attribute altogether (which 
wastes memory).

Adding the macro (Olof's idea, not mine) makes it pretty difficult to 
get this definition wrong... I'll do the rest of the cleanup, but I need 
to know whether it's better to use a macro like this, or to open code 
the definitions.  I prefer the macro approach...

Hope this makes some sense...

/Jonas


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

* Re: [PATCH] Add PCI_DEVICE_TABLE macro
  2008-02-17 12:13 ` Jonas Bonn
@ 2008-02-18  4:48   ` Greg KH
  2008-02-18  7:34     ` Jonas Bonn
  0 siblings, 1 reply; 9+ messages in thread
From: Greg KH @ 2008-02-18  4:48 UTC (permalink / raw)
  To: Jonas Bonn; +Cc: linux-kernel, olof, sam

On Sun, Feb 17, 2008 at 01:13:24PM +0100, Jonas Bonn wrote:
> I'm a bit uncertain whether the definition really should include the
> 'static' modifier... for most definitions of these tables this is ok,
> but there are a couple of cases where it should not be static, so the
> line would need to be open-coded again...

Yes, please don't make it static.

And again, what does this buy us?

thanks,

greg k-h

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

* Re: [PATCH] Add PCI_DEVICE_TABLE macro
  2008-02-17 12:10 Jonas Bonn
  2008-02-17 12:13 ` Jonas Bonn
@ 2008-02-17 18:14 ` Randy Dunlap
  1 sibling, 0 replies; 9+ messages in thread
From: Randy Dunlap @ 2008-02-17 18:14 UTC (permalink / raw)
  To: Jonas Bonn; +Cc: linux-kernel, greg, olof, sam

On Sun, 17 Feb 2008 13:10:05 +0100 Jonas Bonn wrote:

> The definitions of struct pci_device_id arrays should generally follow
> the same pattern across the entire kernel.  This macro defines this
> array as static const and puts it into the __devinitconst section.
> 
> Signed-off-by: Jonas Bonn <jonas@southpole.se>
> ---
>  include/linux/pci.h |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 87195b6..487d31c 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -389,6 +389,14 @@ struct pci_driver {
>  #define	to_pci_driver(drv) container_of(drv, struct pci_driver, driver)
>  
>  /**
> + * PCI_DEVICE_TABLE - macro used to describe a pci device table
> + * 
> + * This macro is used to create a struct pci_device_id array in a generic
> + * manner.
> + */
> +#define PCI_DEVICE_TABLE(_table) static const struct pci_device_id _table[] __devinitconst
> +
> +/**
>   * PCI_DEVICE - macro used to describe a specific pci device
>   * @vend: the 16 bit PCI Vendor ID
>   * @dev: the 16 bit PCI Device ID
> -- 

Missing kernel-doc notation for the _table parameter.  See the
parameters for PCI_DEVICE() below for an example.

---
~Randy

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

* Re: [PATCH] Add PCI_DEVICE_TABLE macro
  2008-02-17 12:10 Jonas Bonn
@ 2008-02-17 12:13 ` Jonas Bonn
  2008-02-18  4:48   ` Greg KH
  2008-02-17 18:14 ` Randy Dunlap
  1 sibling, 1 reply; 9+ messages in thread
From: Jonas Bonn @ 2008-02-17 12:13 UTC (permalink / raw)
  To: linux-kernel; +Cc: greg, olof, sam

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

I'm a bit uncertain whether the definition really should include the
'static' modifier... for most definitions of these tables this is ok,
but there are a couple of cases where it should not be static, so the
line would need to be open-coded again...

/Jonas

On Sun, 2008-02-17 at 13:10 +0100, Jonas Bonn wrote:
> The definitions of struct pci_device_id arrays should generally follow
> the same pattern across the entire kernel.  This macro defines this
> array as static const and puts it into the __devinitconst section.
> 
> Signed-off-by: Jonas Bonn <jonas@southpole.se>
> ---
>  include/linux/pci.h |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 87195b6..487d31c 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -389,6 +389,14 @@ struct pci_driver {
>  #define	to_pci_driver(drv) container_of(drv, struct pci_driver, driver)
>  
>  /**
> + * PCI_DEVICE_TABLE - macro used to describe a pci device table
> + * 
> + * This macro is used to create a struct pci_device_id array in a generic
> + * manner.
> + */
> +#define PCI_DEVICE_TABLE(_table) static const struct pci_device_id _table[] __devinitconst
> +
> +/**
>   * PCI_DEVICE - macro used to describe a specific pci device
>   * @vend: the 16 bit PCI Vendor ID
>   * @dev: the 16 bit PCI Device ID

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

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

* [PATCH] Add PCI_DEVICE_TABLE macro
@ 2008-02-17 12:10 Jonas Bonn
  2008-02-17 12:13 ` Jonas Bonn
  2008-02-17 18:14 ` Randy Dunlap
  0 siblings, 2 replies; 9+ messages in thread
From: Jonas Bonn @ 2008-02-17 12:10 UTC (permalink / raw)
  To: linux-kernel, greg; +Cc: olof, sam, Jonas Bonn

The definitions of struct pci_device_id arrays should generally follow
the same pattern across the entire kernel.  This macro defines this
array as static const and puts it into the __devinitconst section.

Signed-off-by: Jonas Bonn <jonas@southpole.se>
---
 include/linux/pci.h |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 87195b6..487d31c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -389,6 +389,14 @@ struct pci_driver {
 #define	to_pci_driver(drv) container_of(drv, struct pci_driver, driver)
 
 /**
+ * PCI_DEVICE_TABLE - macro used to describe a pci device table
+ * 
+ * This macro is used to create a struct pci_device_id array in a generic
+ * manner.
+ */
+#define PCI_DEVICE_TABLE(_table) static const struct pci_device_id _table[] __devinitconst
+
+/**
  * PCI_DEVICE - macro used to describe a specific pci device
  * @vend: the 16 bit PCI Vendor ID
  * @dev: the 16 bit PCI Device ID
-- 
1.5.3.8



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

end of thread, other threads:[~2008-02-20 17:05 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-02-18  8:03 [PATCH] Add PCI_DEVICE_TABLE macro Jonas Bonn
  -- strict thread matches above, loose matches on Subject: below --
2008-02-20 12:53 pci_device_id cleanups Jonas Bonn
2008-02-20 12:53 ` [PATCH] Add PCI_DEVICE_TABLE macro Jonas Bonn
2008-02-17 12:10 Jonas Bonn
2008-02-17 12:13 ` Jonas Bonn
2008-02-18  4:48   ` Greg KH
2008-02-18  7:34     ` Jonas Bonn
2008-02-20 16:11       ` Greg KH
2008-02-20 17:05         ` Jeff Garzik
2008-02-17 18:14 ` Randy Dunlap

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