linux-kernel - Re: [PATCH 1/3] gpio: idio-16: Introduce the ACCES IDIO-16 GPIO library module

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]

Message-ID: <YyNIxWS0sneUKas0@fedora>
Date:   Thu, 15 Sep 2022 11:46:13 -0400
From:   William Breathitt Gray <william.gray@...aro.org>
To:     Andy Shevchenko <andriy.shevchenko@...el.com>
Cc:     brgl@...ev.pl, linus.walleij@...aro.org,
        linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH 1/3] gpio: idio-16: Introduce the ACCES IDIO-16 GPIO
 library module

On Tue, Sep 13, 2022 at 07:16:23PM +0300, Andy Shevchenko wrote:
> On Sun, Sep 11, 2022 at 04:34:38PM -0400, William Breathitt Gray wrote:
> > +	if (*mask & GENMASK(7, 0))
> > +		bitmap_set_value8(bits, ioread8(&reg->out0_7), 0);
> > +	if (*mask & GENMASK(15, 8))
> > +		bitmap_set_value8(bits, ioread8(&reg->out8_15), 8);
> > +	if (*mask & GENMASK(23, 16))
> > +		bitmap_set_value8(bits, ioread8(&reg->in0_7), 16);
> > +	if (*mask & GENMASK(31, 24))
> > +		bitmap_set_value8(bits, ioread8(&reg->in8_15), 24);
> 
> So, the addresses of the ports are not expected to be continuous?

No, unfortunately the IDIO-16 devices allocate the FET outputs to byte
offsets 0 and 4 while the isolated inputs are allocated to byte offsets
1 and 5. I don't know the design reason for the split but that's the
reason I'm reading these addresses by byte rather than by word.

> > +		return;
> > +
> > +	spin_lock_irqsave(&state->lock, flags);
> 
> > +	if (value)
> > +		set_bit(offset, state->out_state);
> > +	else
> > +		clear_bit(offset, state->out_state);
> 
> assign_bit()
> 
> But I'm wondering why do you need the atomic bitops under the lock?

I don't think atomic bitops are necessary in this case because of the
lock as you pointedly out, but I felt using these made the intention of
the code clearer. Is there a non-atomic version of assign_bit(), or do
you recommend I use bitwise operations directly here instead?

> > +static inline int idio_16_get_direction(const unsigned long offset)
> > +{
> > +	return (offset < IDIO_16_NOUT) ? 0 : 1;
> 
> 	return (offset >= IDIO_16_NOUT) ? 1 : 0;
> 
> ?

I have no particular preference in this case, so I can switch this to
the >= version for consistency with the rest of the code.

Thanks,

William Breathitt Gray

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