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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zwz-benEP4PHbRb2@smile.fi.intel.com>
Date: Mon, 14 Oct 2024 14:20:13 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Chen-Yu Tsai <wenst@...omium.org>
Cc: Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Wolfram Sang <wsa@...nel.org>, Benson Leung <bleung@...omium.org>,
	Tzung-Bi Shih <tzungbi@...nel.org>, chrome-platform@...ts.linux.dev,
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Douglas Anderson <dianders@...omium.org>,
	Johan Hovold <johan@...nel.org>, Jiri Kosina <jikos@...nel.org>,
	linux-i2c@...r.kernel.org
Subject: Re: [PATCH v8 6/8] i2c: of-prober: Add GPIO support to simple helpers

On Mon, Oct 14, 2024 at 12:06:16PM +0800, Chen-Yu Tsai wrote:
> On Thu, Oct 10, 2024 at 11:20 PM Andy Shevchenko
> <andriy.shevchenko@...ux.intel.com> wrote:
> > On Tue, Oct 08, 2024 at 03:34:25PM +0800, Chen-Yu Tsai wrote:
> > > Add GPIO support to the simple helpers for the I2C OF component prober.
> > > Components that the prober intends to probe likely require their
> > > regulator supplies be enabled, and GPIOs be toggled to enable them or
> > > bring them out of reset before they will respond to probe attempts.
> > > Regulator supplies were handled in the previous patch.
> > >
> > > The assumption is that the same class of components to be probed are
> > > always connected in the same fashion with the same regulator supply
> > > and GPIO. The names may vary due to binding differences, but the
> > > physical layout does not change.
> > >
> > > This supports at most one GPIO pin. The user must specify the GPIO name,
> > > the polarity, and the amount of time to wait after the GPIO is toggled.
> > > Devices with more than one GPIO pin likely require specific power
> > > sequencing beyond what generic code can easily support.

...

> > > +     /* An empty string signals an unnamed GPIO */
> > > +     if (!ctx->opts->gpio_name[0])
> > > +             con_id = NULL;
> > > +     else
> > > +             con_id = ctx->opts->gpio_name;
> >
> > Can it use positive conditional?
> >
> >         if (ctx->opts->gpio_name[0])
> >                 con_id = ctx->opts->gpio_name;
> >         else
> >                 con_id = NULL;
> 
> You suggested writing it this way in your reply to v7. Please pick one.

Oh, whatever you will finish with then, sorry for the noise.

...

> > > +static void i2c_of_probe_simple_disable_gpio(struct device *dev, struct i2c_of_probe_simple_ctx *ctx)
> > > +{
> > > +     if (!ctx->gpiod)
> > > +             return;
> >
> > Do you need this check for the future patches?
> 
> Not sure I follow. The check is needed because this function is called
> in i2c_of_probe_simple_cleanup(), but the GPIO could have been released
> earlier in i2c_of_probe_simple_cleanup_early(), and that makes this
> function a no-op.

Do you have a known race condition then? This is bad. You shouldn't rely on
the sequence of events here, or the serialisation has to be added.

> The helpers for the release side are quite short, but the ones on the
> request side wrap some conditional and error handling. I think it's
> better to keep it symmetric?

Yes, but why do you need the above check, I didn't still get...
I.o.w. you think that there is a gap in time that (if no check) the GPIO
descriptor might be changed? But then how does it affect anyway the possibility
that it becomes not NULL even with the current code.

> > > +     /* Ignore error if GPIO is not in output direction */
> > > +     gpiod_set_value(ctx->gpiod, !ctx->opts->gpio_assert_to_enable);
> > > +}

-- 
With Best Regards,
Andy Shevchenko



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ