[lm-sensors] [PATCH 1/1] hwmon: (acpi_power_meter) Fix incorrect placement of __initdata

[lm-sensors] [PATCH 1/1] hwmon: (acpi_power_meter) Fix incorrect placement of __initdata

[Sachin Kamat]

__initdata should be placed between the variable name and equal
sign for the variable to be placed in the intended section.

Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
Cc: Darrick J. Wong <djwong@us.ibm.com>
---
 drivers/hwmon/acpi_power_meter.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
index 6351aba..d7d9b2f 100644
--- a/drivers/hwmon/acpi_power_meter.c
+++ b/drivers/hwmon/acpi_power_meter.c
@@ -970,7 +970,7 @@ static int __init enable_cap_knobs(const struct dmi_system_id *d)
 	return 0;
 }
 
-static struct dmi_system_id __initdata pm_dmi_table[] = {
+static struct dmi_system_id pm_dmi_table[] __initdata = {
 	{
 		enable_cap_knobs, "IBM Active Energy Manager",
 		{
-- 
1.7.9.5


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/1] hwmon: (acpi_power_meter) Fix incorrect placement of __initdata

[Guenter Roeck]

On 08/07/2013 02:47 AM, Sachin Kamat wrote:
> __initdata should be placed between the variable name and equal
> sign for the variable to be placed in the intended section.
>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> Cc: Darrick J. Wong <djwong@us.ibm.com>
> ---
>   drivers/hwmon/acpi_power_meter.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
> index 6351aba..d7d9b2f 100644
> --- a/drivers/hwmon/acpi_power_meter.c
> +++ b/drivers/hwmon/acpi_power_meter.c
> @@ -970,7 +970,7 @@ static int __init enable_cap_knobs(const struct dmi_system_id *d)
>   	return 0;
>   }
>
> -static struct dmi_system_id __initdata pm_dmi_table[] = {
> +static struct dmi_system_id pm_dmi_table[] __initdata = {
>   	{
>   		enable_cap_knobs, "IBM Active Energy Manager",
>   		{
>
Applied to -next.

Thanks,
Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/1] hwmon: (acpi_power_meter) Fix incorrect placement of __initdata

[Jean Delvare]

Hi Sachin,

On Wed,  7 Aug 2013 15:17:13 +0530, Sachin Kamat wrote:
> __initdata should be placed between the variable name and equal
> sign for the variable to be placed in the intended section.

Really? With gcc 4.7.2 of openSUSE 12.3/x86-64, I see no difference
with and without this change. pm_dmi_table is in section .init.data in
both cases. So when/where/how does it actually matter?

I see that there are hundreds of other occurrences of this in the
kernel tree, so I admit I have a hard time believing it is actually
wrong, and I would appreciate extra explanations.

> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> Cc: Darrick J. Wong <djwong@us.ibm.com>
> ---
>  drivers/hwmon/acpi_power_meter.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
> index 6351aba..d7d9b2f 100644
> --- a/drivers/hwmon/acpi_power_meter.c
> +++ b/drivers/hwmon/acpi_power_meter.c
> @@ -970,7 +970,7 @@ static int __init enable_cap_knobs(const struct dmi_system_id *d)
>  	return 0;
>  }
>  
> -static struct dmi_system_id __initdata pm_dmi_table[] = {
> +static struct dmi_system_id pm_dmi_table[] __initdata = {
>  	{
>  		enable_cap_knobs, "IBM Active Energy Manager",
>  		{

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/1] hwmon: (acpi_power_meter) Fix incorrect placement of __initdata

[Guenter Roeck]

On Mon, Aug 26, 2013 at 10:47:34PM +0200, Jean Delvare wrote:
> Hi Sachin,
> 
> On Wed,  7 Aug 2013 15:17:13 +0530, Sachin Kamat wrote:
> > __initdata should be placed between the variable name and equal
> > sign for the variable to be placed in the intended section.
> 
> Really? With gcc 4.7.2 of openSUSE 12.3/x86-64, I see no difference
> with and without this change. pm_dmi_table is in section .init.data in
> both cases. So when/where/how does it actually matter?
> 
> I see that there are hundreds of other occurrences of this in the
> kernel tree, so I admit I have a hard time believing it is actually
> wrong, and I would appreciate extra explanations.
> 

There is this:

http://permalink.gmane.org/gmane.linux.ports.arm.kernel/258149

Maybe target and/or compiler version specific ?

Guenter

> > Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> > Cc: Darrick J. Wong <djwong@us.ibm.com>
> > ---
> >  drivers/hwmon/acpi_power_meter.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
> > index 6351aba..d7d9b2f 100644
> > --- a/drivers/hwmon/acpi_power_meter.c
> > +++ b/drivers/hwmon/acpi_power_meter.c
> > @@ -970,7 +970,7 @@ static int __init enable_cap_knobs(const struct dmi_system_id *d)
> >  	return 0;
> >  }
> >  
> > -static struct dmi_system_id __initdata pm_dmi_table[] = {
> > +static struct dmi_system_id pm_dmi_table[] __initdata = {
> >  	{
> >  		enable_cap_knobs, "IBM Active Energy Manager",
> >  		{
> 
> -- 
> Jean Delvare
> 

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/1] hwmon: (acpi_power_meter) Fix incorrect placement of __initdata

[Jean Delvare]

On Mon, 26 Aug 2013 15:48:43 -0700, Guenter Roeck wrote:
> On Mon, Aug 26, 2013 at 10:47:34PM +0200, Jean Delvare wrote:
> > On Wed,  7 Aug 2013 15:17:13 +0530, Sachin Kamat wrote:
> > > __initdata should be placed between the variable name and equal
> > > sign for the variable to be placed in the intended section.
> > 
> > Really? With gcc 4.7.2 of openSUSE 12.3/x86-64, I see no difference
> > with and without this change. pm_dmi_table is in section .init.data in
> > both cases. So when/where/how does it actually matter?
> > 
> > I see that there are hundreds of other occurrences of this in the
> > kernel tree, so I admit I have a hard time believing it is actually
> > wrong, and I would appreciate extra explanations.
> 
> There is this:
> 
> http://permalink.gmane.org/gmane.linux.ports.arm.kernel/258149
> 
> Maybe target and/or compiler version specific ?

Russell, can you explain? Is this an old gcc misbehavior which has been
fixed meanwhile, or...?

Me, I can't see how a section attribute could apply to a variable type
in the first place, so I'd expect gcc to either transparently apply it
to the variable instead (which it apparently does for me) or emit a
warning.

OTOH if the problem is real then a check for it should be added to
checkpatch.pl because I don't think a lot of people know about it. A
quick grep suggests that 29% of the use cases (1260 occurrences) have
it wrong.

Thanks,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/1] hwmon: (acpi_power_meter) Fix incorrect placement of __initdata

[Russell King - ARM Linux]

On Tue, Aug 27, 2013 at 01:14:58PM +0200, Jean Delvare wrote:
> On Mon, 26 Aug 2013 15:48:43 -0700, Guenter Roeck wrote:
> > On Mon, Aug 26, 2013 at 10:47:34PM +0200, Jean Delvare wrote:
> > > On Wed,  7 Aug 2013 15:17:13 +0530, Sachin Kamat wrote:
> > > > __initdata should be placed between the variable name and equal
> > > > sign for the variable to be placed in the intended section.
> > > 
> > > Really? With gcc 4.7.2 of openSUSE 12.3/x86-64, I see no difference
> > > with and without this change. pm_dmi_table is in section .init.data in
> > > both cases. So when/where/how does it actually matter?
> > > 
> > > I see that there are hundreds of other occurrences of this in the
> > > kernel tree, so I admit I have a hard time believing it is actually
> > > wrong, and I would appreciate extra explanations.
> > 
> > There is this:
> > 
> > http://permalink.gmane.org/gmane.linux.ports.arm.kernel/258149
> > 
> > Maybe target and/or compiler version specific ?
> 
> Russell, can you explain? Is this an old gcc misbehavior which has been
> fixed meanwhile, or...?
> 
> Me, I can't see how a section attribute could apply to a variable type
> in the first place, so I'd expect gcc to either transparently apply it
> to the variable instead (which it apparently does for me) or emit a
> warning.
> 
> OTOH if the problem is real then a check for it should be added to
> checkpatch.pl because I don't think a lot of people know about it. A
> quick grep suggests that 29% of the use cases (1260 occurrences) have
> it wrong.

If it's gcc misbehaviour, then it's documented gcc misbehaviour, because
my description in the above link comes from the GCC info pages.  See:

C Extensions -> Attribute Syntax

This is because attributes can be either function attributes, variable
attributes or type attributes.  Which kind they are depends on where
they are placed.

When they are placed immediately after 'struct', 'enum' or 'union', or
immediately after the closing brace of such a declaration, they are a
type attribute.  It may be possible to argue that a section-specifying
attribute should be allowable as part of the type, but that doesn't
appear to be the case (and it doesn't error out either.)

The version online at: http://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html
looks very similar to the version I've been reading for my gcc version,
so I assume nothing has changed in this regard.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/1] hwmon: (acpi_power_meter) Fix incorrect placement of __initdata

[Guenter Roeck]

On 08/27/2013 04:30 AM, Russell King - ARM Linux wrote:
> On Tue, Aug 27, 2013 at 01:14:58PM +0200, Jean Delvare wrote:
>> On Mon, 26 Aug 2013 15:48:43 -0700, Guenter Roeck wrote:
>>> On Mon, Aug 26, 2013 at 10:47:34PM +0200, Jean Delvare wrote:
>>>> On Wed,  7 Aug 2013 15:17:13 +0530, Sachin Kamat wrote:
>>>>> __initdata should be placed between the variable name and equal
>>>>> sign for the variable to be placed in the intended section.
>>>>
>>>> Really? With gcc 4.7.2 of openSUSE 12.3/x86-64, I see no difference
>>>> with and without this change. pm_dmi_table is in section .init.data in
>>>> both cases. So when/where/how does it actually matter?
>>>>
>>>> I see that there are hundreds of other occurrences of this in the
>>>> kernel tree, so I admit I have a hard time believing it is actually
>>>> wrong, and I would appreciate extra explanations.
>>>
>>> There is this:
>>>
>>> http://permalink.gmane.org/gmane.linux.ports.arm.kernel/258149
>>>
>>> Maybe target and/or compiler version specific ?
>>
>> Russell, can you explain? Is this an old gcc misbehavior which has been
>> fixed meanwhile, or...?
>>
>> Me, I can't see how a section attribute could apply to a variable type
>> in the first place, so I'd expect gcc to either transparently apply it
>> to the variable instead (which it apparently does for me) or emit a
>> warning.
>>
>> OTOH if the problem is real then a check for it should be added to
>> checkpatch.pl because I don't think a lot of people know about it. A
>> quick grep suggests that 29% of the use cases (1260 occurrences) have
>> it wrong.
>
> If it's gcc misbehaviour, then it's documented gcc misbehaviour, because
> my description in the above link comes from the GCC info pages.  See:
>
> C Extensions -> Attribute Syntax
>
> This is because attributes can be either function attributes, variable
> attributes or type attributes.  Which kind they are depends on where
> they are placed.
>
> When they are placed immediately after 'struct', 'enum' or 'union', or
> immediately after the closing brace of such a declaration, they are a
> type attribute.  It may be possible to argue that a section-specifying
> attribute should be allowable as part of the type, but that doesn't
> appear to be the case (and it doesn't error out either.)
>
> The version online at: http://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html
> looks very similar to the version I've been reading for my gcc version,
> so I assume nothing has changed in this regard.
>

If I understand the document correctly,
	static struct dmi_system_id __initdata pm_dmi_table[] = {
applies the attribute to the type used to declare the variable,
thus the variable will be of type 'struct dmi_system_id __initdata'
and be placed in initdata.

	static struct dmi_system_id pm_dmi_table[] __initdata = {
applies the attribute to the variable, with the same result.

So I wonder - is this a real problem ? The result should be the same
in either case.

Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/1] hwmon: (acpi_power_meter) Fix incorrect placement of __initdata

[Jean Delvare]

Hi Guenter, Russell,

On Tue, 27 Aug 2013 06:37:27 -0700, Guenter Roeck wrote:
> On 08/27/2013 04:30 AM, Russell King - ARM Linux wrote:
> > On Tue, Aug 27, 2013 at 01:14:58PM +0200, Jean Delvare wrote:
> >> Russell, can you explain? Is this an old gcc misbehavior which has been
> >> fixed meanwhile, or...?
> >>
> >> Me, I can't see how a section attribute could apply to a variable type
> >> in the first place, so I'd expect gcc to either transparently apply it
> >> to the variable instead (which it apparently does for me) or emit a
> >> warning.
> >>
> >> OTOH if the problem is real then a check for it should be added to
> >> checkpatch.pl because I don't think a lot of people know about it. A
> >> quick grep suggests that 29% of the use cases (1260 occurrences) have
> >> it wrong.
> >
> > If it's gcc misbehaviour, then it's documented gcc misbehaviour, because
> > my description in the above link comes from the GCC info pages.  See:
> >
> > C Extensions -> Attribute Syntax
> >
> > This is because attributes can be either function attributes, variable
> > attributes or type attributes.  Which kind they are depends on where
> > they are placed.
> >
> > When they are placed immediately after 'struct', 'enum' or 'union', or
> > immediately after the closing brace of such a declaration, they are a
> > type attribute.  It may be possible to argue that a section-specifying
> > attribute should be allowable as part of the type, but that doesn't
> > appear to be the case (and it doesn't error out either.)
> >
> > The version online at: http://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html
> > looks very similar to the version I've been reading for my gcc version,
> > so I assume nothing has changed in this regard.
> >
> 
> If I understand the document correctly,
> 	static struct dmi_system_id __initdata pm_dmi_table[] = {
> applies the attribute to the type used to declare the variable,
> thus the variable will be of type 'struct dmi_system_id __initdata'
> and be placed in initdata.
> 
> 	static struct dmi_system_id pm_dmi_table[] __initdata = {
> applies the attribute to the variable, with the same result.

Not even sure, as
http://gcc.gnu.org/onlinedocs/gcc/Type-Attributes.html#Type-Attributes
doesn't list "section" as a valid attribute for types.

> So I wonder - is this a real problem ? The result should be the same
> in either case.

I think that what applies here is this little note at the end of the
"Attribute Syntax" documentation:

"If an attribute that only applies to declarations is applied to the
type of a declaration, it will be treated as applying to that
declaration."

I believe this is exactly what I'm seeing in my tests.

Also note that the fix Sachin sent for acpi_power_meter is slightly
different from what Russell complained about originally. Russell
complained about:

struct __initdata foo bar;

While acpi_power_meter uses:

struct foo __initdata bar;

The former does indeed NOT work in my tests, so Russell was right
complaining. The good news is that there are only a few dozen
occurrences of this in the kernel tree.

The later OTOH works fine in my tests so I see no reason to change it -
unless someone can find a gcc version and platform combination where it
does not work. Hope this clarifies the whole situation...

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/1] hwmon: (acpi_power_meter) Fix incorrect placement of __initdata

[Russell King - ARM Linux]

On Tue, Aug 27, 2013 at 06:37:27AM -0700, Guenter Roeck wrote:
> On 08/27/2013 04:30 AM, Russell King - ARM Linux wrote:
>> If it's gcc misbehaviour, then it's documented gcc misbehaviour, because
>> my description in the above link comes from the GCC info pages.  See:
>>
>> C Extensions -> Attribute Syntax
>>
>> This is because attributes can be either function attributes, variable
>> attributes or type attributes.  Which kind they are depends on where
>> they are placed.
>>
>> When they are placed immediately after 'struct', 'enum' or 'union', or
>> immediately after the closing brace of such a declaration, they are a
>> type attribute.  It may be possible to argue that a section-specifying
>> attribute should be allowable as part of the type, but that doesn't
>> appear to be the case (and it doesn't error out either.)
>>
>> The version online at: http://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html
>> looks very similar to the version I've been reading for my gcc version,
>> so I assume nothing has changed in this regard.
>>
>
> If I understand the document correctly,
> 	static struct dmi_system_id __initdata pm_dmi_table[] = {
> applies the attribute to the type used to declare the variable,
> thus the variable will be of type 'struct dmi_system_id __initdata'
> and be placed in initdata.
>
> 	static struct dmi_system_id pm_dmi_table[] __initdata = {
> applies the attribute to the variable, with the same result.
>
> So I wonder - is this a real problem ? The result should be the same
> in either case.

Well, there's an easy way to find out.  Try it with GCC - build it to
assembly and see what section it gets placed into.

Here's the test and results I just ran:

struct foo { unsigned blah; };
struct foo foo1[1] __attribute__((__section__(".foo"))) = { 1 };
struct foo foo_normal1 = { 1 };
struct __attribute__((__section__(".foo"))) foo foo2[1] = { 2 };
struct foo foo_normal2 = { 2 };
struct foo __attribute__((__section__(".foo"))) foo3[1] = { 3 };
struct foo foo_normal3 = { 3 };
__attribute__((__section__(".foo"))) struct foo foo4[1] = { 4 };

	.file	"tt.c"
.globl foo1
	.section	.foo,"aw",@progbits
	.align 4
	.type	foo1, @object
	.size	foo1, 4
foo1:
	.long	1
.globl foo_normal1
	.data
	.align 4
	.type	foo_normal1, @object
	.size	foo_normal1, 4
foo_normal1:
	.long	1
.globl foo2
	.align 4
	.type	foo2, @object
	.size	foo2, 4
foo2:
	.long	2
.globl foo_normal2
	.align 4
	.type	foo_normal2, @object
	.size	foo_normal2, 4
foo_normal2:
	.long	2
.globl foo3
	.section	.foo
	.align 4
	.type	foo3, @object
	.size	foo3, 4
foo3:
	.long	3
.globl foo_normal3
	.data
	.align 4
	.type	foo_normal3, @object
	.size	foo_normal3, 4
foo_normal3:
	.long	3
.globl foo4
	.section	.foo
	.align 4
	.type	foo4, @object
	.size	foo4, 4
foo4:
	.long	4
	.ident	"GCC: (GNU) 4.5.1 20100924 (Red Hat 4.5.1-4)"
	.section	.note.GNU-stack,"",@progbits

Out of those, the only ones which ends up in section ".foo" are foo1, foo3
foo4.  foo2 doesn't, which is the section specifier as a type attribute.
This is entirely inline with what the GCC docs say IMHO.

However, given the attributes are sensitive to where they're placed, isn't
it better to have one convention about their placement and use that as a
stylistic requirement, so that everyone understands where they should be
placed without having to read that murky documentation?

From what I remember of the early days of adding section attributes for
init section marking, the convention always was to add them after the
variable identifier, and that's the convension I've always used - and I've
yet to be caught out by it.  Those who place it elsewhere seem to end up
with janitorial patches moving it to the end or end up putting it after
'struct' and not realising that it doesn't have the desired effect.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/1] hwmon: (acpi_power_meter) Fix incorrect placement of __initdata

[Russell King - ARM Linux]

On Tue, Aug 27, 2013 at 03:57:53PM +0200, Jean Delvare wrote:
> Also note that the fix Sachin sent for acpi_power_meter is slightly
> different from what Russell complained about originally. Russell
> complained about:
> 
> struct __initdata foo bar;
> 
> While acpi_power_meter uses:
> 
> struct foo __initdata bar;
> 
> The former does indeed NOT work in my tests, so Russell was right
> complaining. The good news is that there are only a few dozen
> occurrences of this in the kernel tree.

Indeed - and these "don't work" ones are definitely worth fixing.

> The later OTOH works fine in my tests so I see no reason to change it -
> unless someone can find a gcc version and platform combination where it
> does not work. Hope this clarifies the whole situation...

Yes, better not to change what's already there if it works fine.
However, as I mentioned in my previous email, it's probably better to
have a convention here for new code rather than having people stick
these markers in random places within the declaration and hit the "it
silently doesn't work" case.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/1] hwmon: (acpi_power_meter) Fix incorrect placement of __initdata

[Jean Delvare]

On Tue, 27 Aug 2013 15:10:43 +0100, Russell King - ARM Linux wrote:
> On Tue, Aug 27, 2013 at 03:57:53PM +0200, Jean Delvare wrote:
> > Also note that the fix Sachin sent for acpi_power_meter is slightly
> > different from what Russell complained about originally. Russell
> > complained about:
> > 
> > struct __initdata foo bar;
> > 
> > While acpi_power_meter uses:
> > 
> > struct foo __initdata bar;
> > 
> > The former does indeed NOT work in my tests, so Russell was right
> > complaining. The good news is that there are only a few dozen
> > occurrences of this in the kernel tree.
> 
> Indeed - and these "don't work" ones are definitely worth fixing.

Agreed.

> > The later OTOH works fine in my tests so I see no reason to change it -
> > unless someone can find a gcc version and platform combination where it
> > does not work. Hope this clarifies the whole situation...
> 
> Yes, better not to change what's already there if it works fine.
> However, as I mentioned in my previous email, it's probably better to
> have a convention here for new code rather than having people stick
> these markers in random places within the declaration and hit the "it
> silently doesn't work" case.

I totally agree, I think we want a checkpatch rule for this. I'll ask
for it in a separate thread.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/1] hwmon: (acpi_power_meter) Fix incorrect placement of __initdata

[Sachin Kamat]

On 27 August 2013 19:40, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Aug 27, 2013 at 03:57:53PM +0200, Jean Delvare wrote:

> Yes, better not to change what's already there if it works fine.
> However, as I mentioned in my previous email, it's probably better to
> have a convention here for new code rather than having people stick
> these markers in random places within the declaration and hit the "it
> silently doesn't work" case.

 init.h header file mentions the following about the attribute placement:

 For initialized data:
* You should insert __initdata between the variable name and equal
* sign followed by value, e.g.:
*
* static int init_variable __initdata = 0;
* static const char linux_logo[] __initconst = { 0x32, 0x36, ... };

-- 
With warm regards,
Sachin

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/1] hwmon: (acpi_power_meter) Fix incorrect placement of __initdata

[Sachin Kamat]

Hi Jean,

On 27 August 2013 19:43, Jean Delvare <khali@linux-fr.org> wrote:
> On Tue, 27 Aug 2013 15:10:43 +0100, Russell King - ARM Linux wrote:
>> On Tue, Aug 27, 2013 at 03:57:53PM +0200, Jean Delvare wrote:
>> > Also note that the fix Sachin sent for acpi_power_meter is slightly
>> > different from what Russell complained about originally. Russell
>> > complained about:
>> >
>> > struct __initdata foo bar;
>> >
>> > While acpi_power_meter uses:
>> >
>> > struct foo __initdata bar;
>> >
>> > The former does indeed NOT work in my tests, so Russell was right
>> > complaining. The good news is that there are only a few dozen
>> > occurrences of this in the kernel tree.
>>
>> Indeed - and these "don't work" ones are definitely worth fixing.
>
> Agreed.
>
>> > The later OTOH works fine in my tests so I see no reason to change it -
>> > unless someone can find a gcc version and platform combination where it
>> > does not work. Hope this clarifies the whole situation...
>>
>> Yes, better not to change what's already there if it works fine.
>> However, as I mentioned in my previous email, it's probably better to
>> have a convention here for new code rather than having people stick
>> these markers in random places within the declaration and hit the "it
>> silently doesn't work" case.
>
> I totally agree, I think we want a checkpatch rule for this. I'll ask
> for it in a separate thread.

I had already asked Joe to add this as a checkpatch rule. He mentioned
the following (which I quote):
"There's no way for checkpatch to look at an __<foo> use
and determine it should be before or after a variable.

__scanf, __printf, __cold, etc are often place before
declarations."


-- 
With warm regards,
Sachin

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/1] hwmon: (acpi_power_meter) Fix incorrect placement of __initdata

[Jean Delvare]

On Tue, 27 Aug 2013 20:18:08 +0530, Sachin Kamat wrote:
> On 27 August 2013 19:40, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > Yes, better not to change what's already there if it works fine.
> > However, as I mentioned in my previous email, it's probably better to
> > have a convention here for new code rather than having people stick
> > these markers in random places within the declaration and hit the "it
> > silently doesn't work" case.
> 
>  init.h header file mentions the following about the attribute placement:
> 
>  For initialized data:
> * You should insert __initdata between the variable name and equal
> * sign followed by value, e.g.:
> *
> * static int init_variable __initdata = 0;
> * static const char linux_logo[] __initconst = { 0x32, 0x36, ... };

Yes, sure, but who reads that?

Not me.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/1] hwmon: (acpi_power_meter) Fix incorrect placement of __initdata

[Jean Delvare]

On Tue, 27 Aug 2013 20:13:46 +0530, Sachin Kamat wrote:
> Hi Jean,
> 
> On 27 August 2013 19:43, Jean Delvare <khali@linux-fr.org> wrote:
> > I totally agree, I think we want a checkpatch rule for this. I'll ask
> > for it in a separate thread.
> 
> I had already asked Joe to add this as a checkpatch rule. He mentioned
> the following (which I quote):
> "There's no way for checkpatch to look at an __<foo> use
> and determine it should be before or after a variable.
> 
> __scanf, __printf, __cold, etc are often place before
> declarations."

Just because we can't cover all the cases is no good reason to not
cover this one specific problematic case. It's not about __<foo>, only
about "struct __initdata" and "struct __initconst". If grep can find
them, checkpatch should catch them too.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/1] hwmon: (acpi_power_meter) Fix incorrect placement of __initdata

[Guenter Roeck]

On Tue, Aug 27, 2013 at 03:10:43PM +0100, Russell King - ARM Linux wrote:
> On Tue, Aug 27, 2013 at 03:57:53PM +0200, Jean Delvare wrote:
> > Also note that the fix Sachin sent for acpi_power_meter is slightly
> > different from what Russell complained about originally. Russell
> > complained about:
> > 
> > struct __initdata foo bar;
> > 
> > While acpi_power_meter uses:
> > 
> > struct foo __initdata bar;
> > 
> > The former does indeed NOT work in my tests, so Russell was right
> > complaining. The good news is that there are only a few dozen
> > occurrences of this in the kernel tree.
> 
> Indeed - and these "don't work" ones are definitely worth fixing.
> 
> > The later OTOH works fine in my tests so I see no reason to change it -
> > unless someone can find a gcc version and platform combination where it
> > does not work. Hope this clarifies the whole situation...
> 
> Yes, better not to change what's already there if it works fine.

Since the patch discussed here is a working case and doesn't need to get fixed,
I'll drop it from my queue.

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/1] hwmon: (acpi_power_meter) Fix incorrect placement of __initdata

[Jean Delvare]

On Tue, 27 Aug 2013 08:25:01 -0700, Guenter Roeck wrote:
> On Tue, Aug 27, 2013 at 03:10:43PM +0100, Russell King - ARM Linux wrote:
> > On Tue, Aug 27, 2013 at 03:57:53PM +0200, Jean Delvare wrote:
> > > Also note that the fix Sachin sent for acpi_power_meter is slightly
> > > different from what Russell complained about originally. Russell
> > > complained about:
> > > 
> > > struct __initdata foo bar;
> > > 
> > > While acpi_power_meter uses:
> > > 
> > > struct foo __initdata bar;
> > > 
> > > The former does indeed NOT work in my tests, so Russell was right
> > > complaining. The good news is that there are only a few dozen
> > > occurrences of this in the kernel tree.
> > 
> > Indeed - and these "don't work" ones are definitely worth fixing.
> > 
> > > The later OTOH works fine in my tests so I see no reason to change it -
> > > unless someone can find a gcc version and platform combination where it
> > > does not work. Hope this clarifies the whole situation...
> > 
> > Yes, better not to change what's already there if it works fine.
> 
> Since the patch discussed here is a working case and doesn't need to get fixed,
> I'll drop it from my queue.

Yes please. Same for:

[PATCH 1/1] hwmon: (asus_atk0110) Fix incorrect placement of __initconst

Thanks,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 1/1] hwmon: (acpi_power_meter) Fix incorrect placement of __initdata

[Guenter Roeck]

On Tue, Aug 27, 2013 at 05:30:46PM +0200, Jean Delvare wrote:
> On Tue, 27 Aug 2013 08:25:01 -0700, Guenter Roeck wrote:
> > On Tue, Aug 27, 2013 at 03:10:43PM +0100, Russell King - ARM Linux wrote:
> > > On Tue, Aug 27, 2013 at 03:57:53PM +0200, Jean Delvare wrote:
> > > > Also note that the fix Sachin sent for acpi_power_meter is slightly
> > > > different from what Russell complained about originally. Russell
> > > > complained about:
> > > > 
> > > > struct __initdata foo bar;
> > > > 
> > > > While acpi_power_meter uses:
> > > > 
> > > > struct foo __initdata bar;
> > > > 
> > > > The former does indeed NOT work in my tests, so Russell was right
> > > > complaining. The good news is that there are only a few dozen
> > > > occurrences of this in the kernel tree.
> > > 
> > > Indeed - and these "don't work" ones are definitely worth fixing.
> > > 
> > > > The later OTOH works fine in my tests so I see no reason to change it -
> > > > unless someone can find a gcc version and platform combination where it
> > > > does not work. Hope this clarifies the whole situation...
> > > 
> > > Yes, better not to change what's already there if it works fine.
> > 
> > Since the patch discussed here is a working case and doesn't need to get fixed,
> > I'll drop it from my queue.
> 
> Yes please. Same for:
> 
> [PATCH 1/1] hwmon: (asus_atk0110) Fix incorrect placement of __initconst
> 
Yes, I did that already.

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors