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

[Samuel Abraham]

On Fri, Apr 4, 2025 at 10:06 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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@xxxxxxxxx> wrote:
> > >
> > > When a value has been tested for NULL in an expression, a
> > > second NULL test on the same value in another expression
> > > is unnecessary when the value has not been assigned NULL.
> > >
> > > Remove unnecessary duplicate NULL tests on the same value that
> > > has previously been NULL tested.
> > >
> > > Found by Coccinelle.
> >
> > ...
> >
> > > +                       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.
>
> 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.
>
> regards,
> dan carpenter

Hello Dan,
Thank you for your review.
Please can you provide some guidance on what the next steps should be for me
as regards the patch?
Thank you.

Adekunle