[PATCH] checkpatch.pl: Add warning for new __packed additions

[PATCH] checkpatch.pl: Add warning for new __packed additions

[Tom Rini]

While there are valid reasons to use __packed, often the answer is that
you should be doing something else here instead.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Joe Perches <joe@perches.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Signed-off-by: Tom Rini <trini@ti.com>
---
 scripts/checkpatch.pl |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 0ea2a1e..fef3b13 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4010,6 +4010,11 @@ sub process {
 			WARN("PREFER_PACKED",
 			     "__packed is preferred over __attribute__((packed))\n" . $herecurr);
 		}
+# Check for new packed usage, warn to use care
+		if ($line =~ /\b(__attribute__\s*\(\s*\(.*\bpacked|__packed)\b/) {
+			WARN("NEW_PACKED",
+			     "Adding new packed members is to be done with care\n" . $herecurr);
+		}
 
 # Check for __attribute__ aligned, prefer __aligned
 		if ($realfile !~ m@\binclude/uapi/@ &&
-- 
1.7.9.5

Re: [PATCH] checkpatch.pl: Add warning for new __packed additions

[Joe Perches]

On Mon, 2014-02-24 at 15:38 -0500, Tom Rini wrote:
> While there are valid reasons to use __packed, often the answer is that
> you should be doing something else here instead.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -4010,6 +4010,11 @@ sub process {
>  			WARN("PREFER_PACKED",
>  			     "__packed is preferred over __attribute__((packed))\n" . $herecurr);
>  		}
> +# Check for new packed usage, warn to use care
> +		if ($line =~ /\b(__attribute__\s*\(\s*\(.*\bpacked|__packed)\b/) {
> +			WARN("NEW_PACKED",
> +			     "Adding new packed members is to be done with care\n" . $herecurr);
> +		}

Hi Tom.

This might be too noisy as it will warn on any use of __packed,
including existing ones if checking using -f/--file.

(there are some odd, probably broken ones in drivers/char/tpm/tpm_acpi.c)

How often is this actually a problem?

It'll warn on any use of packed not just packed members.

This may be better as
	"Using 'packed' can impact performance\n"
and only tested when not in --file mode.

Re: [PATCH] checkpatch.pl: Add warning for new __packed additions

[Tom Rini]

On 02/24/2014 04:00 PM, Joe Perches wrote:
> On Mon, 2014-02-24 at 15:38 -0500, Tom Rini wrote:
>> While there are valid reasons to use __packed, often the answer is that
>> you should be doing something else here instead.
> []
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>> @@ -4010,6 +4010,11 @@ sub process {
>>  			WARN("PREFER_PACKED",
>>  			     "__packed is preferred over __attribute__((packed))\n" . $herecurr);
>>  		}
>> +# Check for new packed usage, warn to use care
>> +		if ($line =~ /\b(__attribute__\s*\(\s*\(.*\bpacked|__packed)\b/) {
>> +			WARN("NEW_PACKED",
>> +			     "Adding new packed members is to be done with care\n" . $herecurr);
>> +		}
> 
> Hi Tom.
> 
> This might be too noisy as it will warn on any use of __packed,
> including existing ones if checking using -f/--file.

Well, in my mind this is like typedef and some other things that can be
warned on in existing files.  I did a quick and lazy git grep and got
~1100 files in v3.14-rc1 out of ~36000 files.

> (there are some odd, probably broken ones in drivers/char/tpm/tpm_acpi.c)
> 
> How often is this actually a problem?

I think the first line answers the second one, honestly.  If one wants
to get pedantic about things and really investigate there's probably
some unneeded usages scattered about, and that's generally the type of
thing one wants to address when checking whole files, right?

> It'll warn on any use of packed not just packed members.
> 
> This may be better as
> 	"Using 'packed' can impact performance\n"
> and only tested when not in --file mode.

I can also make this change, sure, just point me off-list for an example
to crib from and test?

-- 
Tom

Re: [PATCH] checkpatch.pl: Add warning for new __packed additions

[Joe Perches]

On Mon, 2014-02-24 at 16:11 -0500, Tom Rini wrote:
> On 02/24/2014 04:00 PM, Joe Perches wrote:
> > On Mon, 2014-02-24 at 15:38 -0500, Tom Rini wrote:
> >> While there are valid reasons to use __packed, often the answer is that
> >> you should be doing something else here instead.
[]
> > How often is this actually a problem?
> 
> I think the first line answers the second one, honestly.  If one wants
> to get pedantic about things and really investigate there's probably
> some unneeded usages scattered about, and that's generally the type of
> thing one wants to address when checking whole files, right?

Maybe not.

That entirely depends on the correct and necessary uses of
packed vs the incorrect usage rates.

I think almost all packed uses are correct and there might
be a lot of patches submitted to remove them by over-zealous
advocates of checkpatch -f.

> > This may be better as
> > 	"Using 'packed' can impact performance\n"
> > and only tested when not in --file mode.
> 
> I can also make this change, sure, just point me off-list for an example
> to crib from and test?

Look at the FSF mailing address test as an example:

			my $msg_type = \&ERROR;
			$msg_type = \&CHK if ($file);
			&{$msg_type}("FSF_MAILING_ADDRESS",

Re: [PATCH] checkpatch.pl: Add warning for new __packed additions

[josh]

On Mon, Feb 24, 2014 at 03:38:16PM -0500, Tom Rini wrote:
> While there are valid reasons to use __packed, often the answer is that
> you should be doing something else here instead.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Joe Perches <joe@perches.com>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Signed-off-by: Tom Rini <trini@ti.com>
> ---
>  scripts/checkpatch.pl |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 0ea2a1e..fef3b13 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -4010,6 +4010,11 @@ sub process {
>  			WARN("PREFER_PACKED",
>  			     "__packed is preferred over __attribute__((packed))\n" . $herecurr);
>  		}
> +# Check for new packed usage, warn to use care
> +		if ($line =~ /\b(__attribute__\s*\(\s*\(.*\bpacked|__packed)\b/) {
> +			WARN("NEW_PACKED",
> +			     "Adding new packed members is to be done with care\n" . $herecurr);
> +		}

This seems wrong; "is to be done with care" is the very definition of a
false positive.  At *best* this should always be CHK, and even then it
seems excessive.

- Josh Triplett

Re: [PATCH] checkpatch.pl: Add warning for new __packed additions

[Tom Rini]

On 02/24/2014 04:28 PM, Joe Perches wrote:
> On Mon, 2014-02-24 at 16:11 -0500, Tom Rini wrote:
>> On 02/24/2014 04:00 PM, Joe Perches wrote:
>>> On Mon, 2014-02-24 at 15:38 -0500, Tom Rini wrote:
>>>> While there are valid reasons to use __packed, often the answer is that
>>>> you should be doing something else here instead.
> []
>>> How often is this actually a problem?
>>
>> I think the first line answers the second one, honestly.  If one wants
>> to get pedantic about things and really investigate there's probably
>> some unneeded usages scattered about, and that's generally the type of
>> thing one wants to address when checking whole files, right?
> 
> Maybe not.
> 
> That entirely depends on the correct and necessary uses of
> packed vs the incorrect usage rates.
> 
> I think almost all packed uses are correct and there might
> be a lot of patches submitted to remove them by over-zealous
> advocates of checkpatch -f.

To try and also answer Josh's feedback as well, I've been lead to
believe that most cases now people should be using regmap instead, which
just leaves the case of having to match on-disk formats or similar cases
I believe as the things that must stay __packed.

>>> This may be better as
>>> 	"Using 'packed' can impact performance\n"
>>> and only tested when not in --file mode.
>>
>> I can also make this change, sure, just point me off-list for an example
>> to crib from and test?
> 
> Look at the FSF mailing address test as an example:
> 
> 			my $msg_type = \&ERROR;
> 			$msg_type = \&CHK if ($file);
> 			&{$msg_type}("FSF_MAILING_ADDRESS",

OK, thanks, I'll make something happen, and drop it to a CHK too.

-- 
Tom

Re: [PATCH] checkpatch.pl: Add warning for new __packed additions

[Joe Perches]

On Mon, 2014-02-24 at 16:52 -0500, Tom Rini wrote:
> I've been lead to
> believe that most cases now people should be using regmap instead, which
> just leaves the case of having to match on-disk formats or similar cases
> I believe as the things that must stay __packed.

__packed is also necessary for on-wire networking protocols.

Re: [PATCH] checkpatch.pl: Add warning for new __packed additions

[Tom Rini]

On 02/24/2014 05:02 PM, Joe Perches wrote:
> On Mon, 2014-02-24 at 16:52 -0500, Tom Rini wrote:
>> I've been lead to
>> believe that most cases now people should be using regmap instead, which
>> just leaves the case of having to match on-disk formats or similar cases
>> I believe as the things that must stay __packed.
> 
> __packed is also necessary for on-wire networking protocols.

Indeed, and there's probably a few other valid cases I'm also
forgetting.  But the common case is hardware, where regmap is now preferred.

I've got this modified to a CHK and only for non-file usage.  Anything
else we want to talk about before I repost?

-- 
Tom

Re: [PATCH] checkpatch.pl: Add warning for new __packed additions

[Joe Perches]

On Mon, 2014-02-24 at 17:04 -0500, Tom Rini wrote:
> I've got this modified to a CHK and only for non-file usage.  Anything
> else we want to talk about before I repost?

Probably not, but I'm still not convinced it's useful.

Have you found a case where it's currently specified
but not useful?

Re: [PATCH] checkpatch.pl: Add warning for new __packed additions

[Tom Rini]

On 02/24/2014 05:08 PM, Joe Perches wrote:
> On Mon, 2014-02-24 at 17:04 -0500, Tom Rini wrote:
>> I've got this modified to a CHK and only for non-file usage.  Anything
>> else we want to talk about before I repost?
> 
> Probably not, but I'm still not convinced it's useful.
> 
> Have you found a case where it's currently specified
> but not useful?

Well, U-Boot and the kernel both share the dubious to incorrect __packed
horror of cros_ec things (see include/linux/mfd/cros_ec_commands.h).

If this is really not seen as useful for the kernel, that's fine, I'll
drop it.  I mainly did this for U-Boot where we do want a bit more loud
screaming going on when people add __packed to make sure it's for a good
reason.  Wanted to be a good neighbor so to speak and see if upstream
wants it too.

-- 
Tom

Re: [PATCH] checkpatch.pl: Add warning for new __packed additions

[Joe Perches]

On Mon, 2014-02-24 at 17:20 -0500, Tom Rini wrote:
> On 02/24/2014 05:08 PM, Joe Perches wrote:
> > On Mon, 2014-02-24 at 17:04 -0500, Tom Rini wrote:
> >> I've got this modified to a CHK and only for non-file usage.  Anything
> >> else we want to talk about before I repost?
> > 
> > Probably not, but I'm still not convinced it's useful.
> > 
> > Have you found a case where it's currently specified
> > but not useful?
> 
> Well, U-Boot and the kernel both share the dubious to incorrect __packed
> horror of cros_ec things (see include/linux/mfd/cros_ec_commands.h).

Are the __packed entries in cros_ec dubious?

Maybe the ones that don't seem to need them
because the are naturally 32 bit aligned, but
the others that are u16 aligned probably _do_
need __packed.

> If this is really not seen as useful for the kernel, that's fine, I'll
> drop it.  I mainly did this for U-Boot where we do want a bit more loud
> screaming going on when people add __packed to make sure it's for a good
> reason.  Wanted to be a good neighbor so to speak and see if upstream
> wants it too.

I don't mind keeping checkpatch compatible with U-Boot
requirements, but probably not on by default.

Maybe there would be some "UBOOT-<foo>" type specific entries
that could be default off but enabled with some wildcard or
a .checkpatch.conf specific file for U-Boot.

Re: [PATCH] checkpatch.pl: Add warning for new __packed additions

[Tom Rini]

On 02/24/2014 05:31 PM, Joe Perches wrote:
> On Mon, 2014-02-24 at 17:20 -0500, Tom Rini wrote:
>> On 02/24/2014 05:08 PM, Joe Perches wrote:
>>> On Mon, 2014-02-24 at 17:04 -0500, Tom Rini wrote:
>>>> I've got this modified to a CHK and only for non-file usage.  Anything
>>>> else we want to talk about before I repost?
>>>
>>> Probably not, but I'm still not convinced it's useful.
>>>
>>> Have you found a case where it's currently specified
>>> but not useful?
>>
>> Well, U-Boot and the kernel both share the dubious to incorrect __packed
>> horror of cros_ec things (see include/linux/mfd/cros_ec_commands.h).
> 
> Are the __packed entries in cros_ec dubious?
> 
> Maybe the ones that don't seem to need them
> because the are naturally 32 bit aligned, but
> the others that are u16 aligned probably _do_
> need __packed.

There's so many unused entries in there that I cannot honestly say if
things need to be packed or not.

>> If this is really not seen as useful for the kernel, that's fine, I'll
>> drop it.  I mainly did this for U-Boot where we do want a bit more loud
>> screaming going on when people add __packed to make sure it's for a good
>> reason.  Wanted to be a good neighbor so to speak and see if upstream
>> wants it too.
> 
> I don't mind keeping checkpatch compatible with U-Boot
> requirements, but probably not on by default.
> 
> Maybe there would be some "UBOOT-<foo>" type specific entries
> that could be default off but enabled with some wildcard or
> a .checkpatch.conf specific file for U-Boot.

My perl is quite limited, so however much effort you're interested in
putting in here is greatly appreciated (even if it's pointing out
something else already in the script to copy and modify).  We already
ship a .checkpatch.conf so having to enable something by default is
fine.  Thanks!

-- 
Tom

Re: [PATCH] checkpatch.pl: Add warning for new __packed additions

[Joe Perches]

On Mon, 2014-02-24 at 17:43 -0500, Tom Rini wrote:
> My perl is quite limited, so however much effort you're interested in
> putting in here is greatly appreciated (even if it's pointing out
> something else already in the script to copy and modify).  We already
> ship a .checkpatch.conf so having to enable something by default is
> fine.  Thanks!

The U-Boot checkpatch is not the same as the current
linux version and would need updating.

Tell me a little more please about how often or possible
it would be to update the current linux version of
checkpatch to the version used in U-Boot.

Re: [PATCH] checkpatch.pl: Add warning for new __packed additions

[Heiko Carstens]

On Mon, Feb 24, 2014 at 05:04:57PM -0500, Tom Rini wrote:
> On 02/24/2014 05:02 PM, Joe Perches wrote:
> > On Mon, 2014-02-24 at 16:52 -0500, Tom Rini wrote:
> >> I've been lead to
> >> believe that most cases now people should be using regmap instead, which
> >> just leaves the case of having to match on-disk formats or similar cases
> >> I believe as the things that must stay __packed.
> > 
> > __packed is also necessary for on-wire networking protocols.
> 
> Indeed, and there's probably a few other valid cases I'm also
> forgetting.  But the common case is hardware, where regmap is now preferred.

Except on s390 which doesn't use regmap. So this will give us a lot of false
positives with a warning message that isn't helpful at all.
It doesn't even tell us why it could be problematic and what would be the
better choice.
 
> I've got this modified to a CHK and only for non-file usage.  Anything
> else we want to talk about before I repost?

/me thinks this patch should be dropped. It would cause too many false
positives, since there are lot of valid use cases for __packed.

Re: [PATCH] checkpatch.pl: Add warning for new __packed additions

[Tom Rini]

On 02/25/2014 12:23 AM, Joe Perches wrote:
> On Mon, 2014-02-24 at 17:43 -0500, Tom Rini wrote:
>> My perl is quite limited, so however much effort you're interested in
>> putting in here is greatly appreciated (even if it's pointing out
>> something else already in the script to copy and modify).  We already
>> ship a .checkpatch.conf so having to enable something by default is
>> fine.  Thanks!
> 
> The U-Boot checkpatch is not the same as the current
> linux version and would need updating.
> 
> Tell me a little more please about how often or possible
> it would be to update the current linux version of
> checkpatch to the version used in U-Boot.

The only intentional differences we have today are adding debug/printf
to the list of log functions and pointing people at boards.cfg not
CHECKPATCH in MAINTAINERS, so I can re-sync us no problem as, yup, we're
about a year out of date there.  Oh and I see now someone snuck in a
check for whitespace before semi-colon to catch 'return 0 ;' which I'll
check real quick isn't something already caught and if not post that as
well.

-- 
Tom

Re: [PATCH] checkpatch.pl: Add warning for new __packed additions

[Joe Perches]

On Tue, 2014-02-25 at 07:30 -0500, Tom Rini wrote:
> The only intentional differences we have today are adding debug/printf
> to the list of log functions

That seems fine and trivial to keep current.

> and pointing people at boards.cfg not
> CHECKPATCH in MAINTAINERS,

That seems wrong.

The idea of that message is to fix defects in checkpatch,
not any other code.

Maybe that could be reworded to
"See the CHECKPATCH entry in the MAINTAINERS file of linux git"
or some such.

>  so I can re-sync us no problem as, yup, we're
> about a year out of date there.  Oh and I see now someone snuck in a
> check for whitespace before semi-colon to catch 'return 0 ;' which I'll
> check real quick isn't something already caught and if not post that as
> well.

Does anyone know of other projects that use checkpatch?

Re: [PATCH] checkpatch.pl: Add warning for new __packed additions

[Tom Rini]

On 02/26/2014 05:04 PM, Joe Perches wrote:
> On Tue, 2014-02-25 at 07:30 -0500, Tom Rini wrote:
>> The only intentional differences we have today are adding debug/printf
>> to the list of log functions
> 
> That seems fine and trivial to keep current.

Agreed,

>> and pointing people at boards.cfg not
>> CHECKPATCH in MAINTAINERS,
> 
> That seems wrong.
> 
> The idea of that message is to fix defects in checkpatch,
> not any other code.
> 
> Maybe that could be reworded to
> "See the CHECKPATCH entry in the MAINTAINERS file of linux git"
> or some such.

I've dropped that change now in a re-sync to current checkpatch.pl.
Happy to pull in a re-wording of this part from a near-future resync
with the kernel :)

-- 
Tom