checkpatch.pl false positive?

checkpatch.pl false positive?

[Michele Sorcinelli]

I tried to run the checkpatch.pl script as follows:

	scripts/checkpatch.pl -f drivers/hwmon/dell-smm-hwmon.c

And I noticed this kind of warnings:

	WARNING: Missing a blank line after declarations
	#122: FILE: drivers/hwmon/dell-smm-hwmon.c:122:
	+       unsigned int eax;
	+       unsigned int ebx __packed;

	WARNING: Missing a blank line after declarations
	#123: FILE: drivers/hwmon/dell-smm-hwmon.c:123:
	+       unsigned int ebx __packed;
	+       unsigned int ecx __packed;

The offending code looks like this:

	struct smm_regs {
		unsigned int eax;
		unsigned int ebx __packed;
		unsigned int ecx __packed;
		unsigned int edx __packed;
		unsigned int esi __packed;
		unsigned int edi __packed;
	};

It looks like a false positive as it excepts a blank line after every struct
field declaration.

Should it be reported to the checkpatch.pl maintainers?


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: checkpatch.pl false positive?

[Greg KH]

On Thu, Apr 02, 2020 at 01:08:01PM +0100, Michele Sorcinelli wrote:
> I tried to run the checkpatch.pl script as follows:
> 
> 	scripts/checkpatch.pl -f drivers/hwmon/dell-smm-hwmon.c
> 
> And I noticed this kind of warnings:
> 
> 	WARNING: Missing a blank line after declarations
> 	#122: FILE: drivers/hwmon/dell-smm-hwmon.c:122:
> 	+       unsigned int eax;
> 	+       unsigned int ebx __packed;
> 
> 	WARNING: Missing a blank line after declarations
> 	#123: FILE: drivers/hwmon/dell-smm-hwmon.c:123:
> 	+       unsigned int ebx __packed;
> 	+       unsigned int ecx __packed;
> 
> The offending code looks like this:
> 
> 	struct smm_regs {
> 		unsigned int eax;
> 		unsigned int ebx __packed;
> 		unsigned int ecx __packed;
> 		unsigned int edx __packed;
> 		unsigned int esi __packed;
> 		unsigned int edi __packed;
> 	};
> 
> It looks like a false positive as it excepts a blank line after every struct
> field declaration.
> 
> Should it be reported to the checkpatch.pl maintainers?

No, the structure should be changed to actually be correct, as that is
not how it should be declared in order to not have any padding between
variables in the structure :)

But note, running checkpatch on code outside of drivers/staging/ is not
always welcome by some subsystem maintainers, so be careful about doing
lots of these types of changes in subsystems you do not have experience
contributing to.

good luck!

greg k-h

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: checkpatch.pl false positive?

[Michele Sorcinelli]

On 4/2/20 1:13 PM, Greg KH wrote:
> No, the structure should be changed to actually be correct, as that is
> not how it should be declared in order to not have any padding between
> variables in the structure :)

What would be the correct way of declaring that structure?


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: checkpatch.pl false positive?

[Greg KH]

On Thu, Apr 02, 2020 at 01:16:27PM +0100, Michele Sorcinelli wrote:
> On 4/2/20 1:13 PM, Greg KH wrote:
> > No, the structure should be changed to actually be correct, as that is
> > not how it should be declared in order to not have any padding between
> > variables in the structure :)
> 
> What would be the correct way of declaring that structure?
> 

See any of the structures in include/linux/* that have:
	} __packed;
as their closing bracket.

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies