Re: [v3 1/1] staging: rtl8723bs: Prevent duplicate NULL tests on a value

Andy Shevchenko
2025-04-04T10:53:54Z

On Fri, Apr 4, 2025 at 12:06 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> On Fri, Apr 04, 2025 at 10:53:22AM +0300, Andy Shevchenko wrote:
> > On Fri, Apr 4, 2025 at 3:03 AM Abraham Samuel Adekunle
> > <abrahamadekunle50@gmail.com> wrote:

...

> > > + psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
> >
> > > + psta->BA_starting_seqctrl[pattrib->priority & 0x0f] =
> > > + (tx_seq + 1) & 0xfff;
> >
> > > + psta->BA_starting_seqctrl[pattrib->priority & 0x0f] =
> > > + (pattrib->seqnum + 1) % 4096;
> >
> > Logically it's obvious that you need to align all cases to have
> > consistent approach.
> > Besides that the commit message should mention this change. Something like this
> > "While at it, convert '& 0xfff' cases to use modulo operator and
> > decimal number to make the upper limit visible and clear what the
> > semantic of it is."
>
> No, I'm sorry but that's really against the rules in drivers/staging.
> Don't mix unrelated changes into a patch. It needs to be done as a
> separate patch if we're going to do that.
>
> To be honest, I don't even want people fixing line length issues or
> adding spaces. I would have accepted small white space changes but I
> prefered the v2 version of this patch. Once you start changing
> "& 0xfff" to "% 4096" that's not white space and it must be done
> in a separate patch. I use a script to review white space patches
> because I'm always nervous someone will slip something malicious
> into 100+ lines of reformated code. It's really fast to review
> patches with my script but once people start mixing things in then
> it's a headache for me.

Separate patch is even better, indeed.

> Also if the change accidentally introduces a bug, I want it to be a
> one liner change and not something hidden inside a giant reformat.

The noisy {] have no point to be left. Now I'm curious, what do you
propose here?

--
With Best Regards,
Andy Shevchenko