2022-03-28 08:49:08

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [PATCH] crypto: aes_generic: fixed styling warnings

(please keep the cc's)

On Mon, 28 Mar 2022 at 00:46, Paul Lemmermann
<[email protected]> wrote:
>
> On Sun, Mar 27, 2022 at 01:41:19PM +0200, Ard Biesheuvel wrote:
> > On Sat, 26 Mar 2022 at 18:48, Paul Lemmermann
> > <[email protected]> wrote:
> > >
> > > Fixed all styling warnings from the checkpatch.pl script.
> > >
> > > Signed-off-by: Paul Lemmermann <[email protected]>
> >
> > Did you test this code after 'fixing' it?
> >
> No, I did not. Now that I scrutinized it a bit more, I realized the
> kernel coding conventions. Sorry about that, this is my first patch.

In that case, welcome!

This is not about coding conventions. This is about correctness.

For instance,

> > >
> > > -#define f_nround(bo, bi, k) do {\
> > > +#define f_nround(bo, bi, k) while (0) {\
> > > f_rn(bo, bi, 0, k); \
> > > f_rn(bo, bi, 1, k); \
> > > f_rn(bo, bi, 2, k); \
> > > f_rn(bo, bi, 3, k); \
> > > k += 4; \
> > > -} while (0)
> > > +}
> > >

Why are you making this change, and why do you think it produces the
same result?

> Can you remove everything in the patch past the section with line
> 1144, or do I have to resubit the patch?
>

checkpatch.pl is a useful tool for finding style issues, but please
use it with care. And changing decades old code just to fix issues
reported by checkpatch.pl is really just pointless churn.

So let's just drop this patch altogether, shall we? If you're
interested in helping out, please have a look at the staging/ tree -
there is a lot of code there that needs cleaning up.

Thanks,
Ard.


2022-03-28 22:08:35

by ThePaulODoom

[permalink] [raw]
Subject: Re: [PATCH] crypto: aes_generic: fixed styling warnings

On Mon, Mar 28, 2022 at 09:39:14AM +0200, Ard Biesheuvel wrote:
> (please keep the cc's)
>
> On Mon, 28 Mar 2022 at 00:46, Paul Lemmermann
> <[email protected]> wrote:
> >
> > On Sun, Mar 27, 2022 at 01:41:19PM +0200, Ard Biesheuvel wrote:
> > > On Sat, 26 Mar 2022 at 18:48, Paul Lemmermann
> > > <[email protected]> wrote:
> > > >
> > > > Fixed all styling warnings from the checkpatch.pl script.
> > > >
> > > > Signed-off-by: Paul Lemmermann <[email protected]>
> > >
> > > Did you test this code after 'fixing' it?
> > >
> > No, I did not. Now that I scrutinized it a bit more, I realized the
> > kernel coding conventions. Sorry about that, this is my first patch.
>
> In that case, welcome!
>
> This is not about coding conventions. This is about correctness.
>
> For instance,
>
> > > >
> > > > -#define f_nround(bo, bi, k) do {\
> > > > +#define f_nround(bo, bi, k) while (0) {\
> > > > f_rn(bo, bi, 0, k); \
> > > > f_rn(bo, bi, 1, k); \
> > > > f_rn(bo, bi, 2, k); \
> > > > f_rn(bo, bi, 3, k); \
> > > > k += 4; \
> > > > -} while (0)
> > > > +}
> > > >
>
> Why are you making this change, and why do you think it produces the
> same result?
>
> > Can you remove everything in the patch past the section with line
> > 1144, or do I have to resubit the patch?
> >
>
> checkpatch.pl is a useful tool for finding style issues, but please
> use it with care. And changing decades old code just to fix issues
> reported by checkpatch.pl is really just pointless churn.
>
> So let's just drop this patch altogether, shall we? If you're
> interested in helping out, please have a look at the staging/ tree -
> there is a lot of code there that needs cleaning up.
>
Yes, we can drop the patch. Thank you so much for your help and support.
Looking forward to contributing more to the Linux kernel.

Thanks,
Paul