Re: [PATCH v4 01/15] scripts: checkpatch: Add __aligned to the list of attribute notes

[Jonathan Cameron]

On Sat, 16 Dec 2023 10:07:24 -0800
Joe Perches <joe@xxxxxxxxxxx> wrote:

> On Sat, 2023-12-16 at 14:45 -0300, Marcelo Schmitt wrote:
> > Checkpatch presumes attributes marked with __aligned(alignment) are part
> > of a function declaration and throws a warning stating that those
> > compiler attributes should have an identifier name which is not correct.
> > Add __aligned compiler attributes to the list of attribute notes
> > so they don't cause warnings anymore.
> > 
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
> > ---
> > Any expression that evaluates to an integer that is a power of 2 can be
> > within __aligned parenthesis.
> > I can't see how we could use a regex to check code meets such constraint (if possible).  
> 
> You can't because if a #define is used for the alignment
> value it is not necessarily available to a patch fragment
> or even a file if the #define is in an #include.
> 
> > Some additional exotic uses of __aligned are:
> > drivers/net/wireless/quantenna/qtnfmac/bus.h:72:   char bus_priv[] __aligned(sizeof(void *));
> > include/linux/netdevice.h:225:} __aligned(4 * sizeof(unsigned long));   
> 
> Right, then there are random uses like:
> 
> drivers/firmware/qcom/qcom_qseecom_uefisecapp.c:		size_t __aligned;						\
> drivers/firmware/qcom/qcom_qseecom_uefisecapp.c:			*__offset = __aligned;					\
> 
> so there's always some false positive/negative issue
> with checkpatch.
> 
> Anyway:
> 
> Acked-by: Joe Perches <joe@xxxxxxxxxxx>
Given this should cut down on false postives I've picked this one up now.

Applied to the togreg branch of iio.git and pushed out briefly as testing to
see what blows up.

I'll see if I can pick up some more parts of this series to avoid us going round
again with quite so many patches.


Jonathan

> 
> > 
> > The regex
> > 			__aligned\s*\(.*\)
> > seems to match all use cases. 
> > 
> > We might not catch invalid arguments to __aligned, but it looks like making
> > checkpath confidently report those wouldn't be feasible anyway.
> > 
> > The patch that would trigger the mentioned warning in now
> > patch number 13 (iio: adc: Add support for AD7091R-8).
> > 
> >  scripts/checkpatch.pl | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 25fdb7fda112..d56c98146da3 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -512,6 +512,7 @@ our $Attribute	= qr{
> >  			__ro_after_init|
> >  			__kprobes|
> >  			$InitAttribute|
> > +			__aligned\s*\(.*\)|
> >  			____cacheline_aligned|
> >  			____cacheline_aligned_in_smp|
> >  			____cacheline_internodealigned_in_smp|  
> 
>