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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/