linux-kernel - Re: [PATCH v3 4/4] gpio: xilinx: Utilize for_each_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 30 Apr 2020 19:32:45 -0400
From:   William Breathitt Gray <vilhelm.gray@...il.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Syed Nayyar Waris <syednwaris@...il.com>,
        akpm@...ux-foundation.org, linus.walleij@...aro.org,
        bgolaszewski@...libre.com, michal.simek@...inx.com,
        linux-gpio@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 4/4] gpio: xilinx: Utilize for_each_set_clump macro

On Thu, Apr 30, 2020 at 07:38:55PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 30, 2020 at 09:45:14PM +0530, Syed Nayyar Waris wrote:
> > On Wed, Apr 29, 2020 at 01:21:14PM +0300, Andy Shevchenko wrote:
> > > On Wed, Apr 29, 2020 at 04:39:47AM +0530, Syed Nayyar Waris wrote:
> 
> ...
> 
> > > > +	const unsigned long state_size = BITS_PER_TYPE(*state);
> > > 
> > > This '*state' is unneeded complication, use BITS_PER_U32.
> > > 
> > > > +#define TOTAL_BITS BITS_PER_TYPE(chip->gpio_state)
> > > 
> > > This macro makes code uglier, besides the fact of absence of #undef.
> > > And also see above.
> > 
> > Thank you for your review comments. Just want to clarify, you want
> >  a new macro to be created - 'BITS_PER_U32' ?
> 
> It's already there (read bits.h).

I'm having trouble finding the BITS_PER_U32 macro; are you thinking of
BITS_PER_LONG? I don't think there are any cases where u32 is not 32
bits wide, so perhaps it'll be better to just hardcode 32 directly in
the code here to make it easier to read.

William Breathitt Gray

> 
> > Also, don't you think that with BITS_PER_TYPE(), in case later the type
> > of 'state' changes, it will be reflected in this code without any code
> > change?
> 
> If it changes the bits per type will be least issues there. The rationale
> behind is to have code readable. In proposed change it is not.
> 
> > Let me know if I have misunderstood something.
> > 
> > > 
> > > > +	DECLARE_BITMAP(old, TOTAL_BITS);
> > > > +	DECLARE_BITMAP(new, TOTAL_BITS);
> > > > +	DECLARE_BITMAP(changed, TOTAL_BITS);
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)