linux-kernel - Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend

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: Wed, 24 Apr 2024 20:00:53 -0400
From: Aren <aren@...cevolution.org>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Jonathan Cameron <jic23@...nel.org>, 
	Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>, 
	Jernej Skrabec <jernej.skrabec@...il.com>, Samuel Holland <samuel@...lland.org>, 
	Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, Ondrej Jirman <megi@....cz>, 
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>, linux-iio@...r.kernel.org, phone-devel@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev, 
	Willow Barraco <contact@...lowbarraco.fr>
Subject: Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and
 power it off during suspend

On Wed, Apr 24, 2024 at 02:16:06AM +0300, Andy Shevchenko wrote:
> On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan <aren@...cevolution.org> wrote:
> >
> > From: Ondrej Jirman <megi@....cz>
> >
> > VDD power input can be used to completely power off the chip during
> > system suspend. Do so if available.
> 
> ...
> 
> >         ret = stk3310_init(indio_dev);
> >         if (ret < 0)
> > -               return ret;
> > +               goto err_vdd_disable;
> 
> This is wrong. You will have the regulator being disabled _before_
> IRQ. Note, that the original code likely has a bug which sets states
> before disabling IRQ and removing a handler.

Oh! now I see the issue you were talking about last time around. I
expect that means the irq shouldn't be managed with devres, so it can be
the first thing freed in the remove function (I haven't checked the docs
to see if there's an easier way yet).

I'll add a patch to fix the order of the handling of the irq (both this and
the issue Ondřej brought up).

> Side note, you may make the driver neater with help of
> 
>   struct device *dev = &client->dev;
> 
> defined in this patch.

Good point, it's minor, but it should be a net improvement.

> ...
> 
> >  static int stk3310_suspend(struct device *dev)
> >  {
> >         struct stk3310_data *data;
> 
> >         data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
> 
> Side note: This may be updated (in a separate change) to use
> dev_get_drvdata() directly.
> 
> Jonathan, do we have something like iio_priv_from_drvdata(struct
> device *dev)? Seems many drivers may utilise it.
> 

At this rate I'm going to need to split off a separate style / code
cleanup series so I don't keep introducing dumb bugs while rebasing this
one.

Thank you for your time
 - Aren