[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250628164054.54ae2e7a@jic23-huawei>
Date: Sat, 28 Jun 2025 16:40:54 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: David Lechner <dlechner@...libre.com>, jean-baptiste.maneyrol@....com,
Lars-Peter Clausen <lars@...afoo.de>, Nuno Sá
<nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>,
linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, Julia Lawall
<Julia.Lawall@...ia.fr>
Subject: Re: [PATCH v5 2/3] iio: imu: inv_icm42600: add WoM support
On Thu, 26 Jun 2025 14:48:10 -0500
Dan Carpenter <dan.carpenter@...aro.org> wrote:
> On Thu, Jun 26, 2025 at 07:53:23PM +0100, Jonathan Cameron wrote:
> > > > +static int inv_icm42600_accel_disable_wom(struct iio_dev *indio_dev)
> > > > +{
> > > > + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> > > > + struct device *pdev = regmap_get_device(st->map);
> > > > + struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
> > > > + unsigned int sleep_ms = 0;
> > > > + int ret;
> > > > +
> > > > + scoped_guard(mutex, &st->lock) {
> > > > + /*
> > > > + * Consider that turning off WoM is always working to avoid
> > > > + * blocking the chip in on mode and prevent going back to sleep.
> > > > + * If there is an error, the chip will anyway go back to sleep
> > > > + * and the feature will not work anymore.
> > > > + */
> > > > + st->apex.wom.enable = false;
> > > > + st->apex.on--;
> > > > + ret = inv_icm42600_disable_wom(st);
> > > > + if (ret)
> > > > + break;
> > >
> > > The fact that scoped_guard() uses a for loop is an implementation
> > > detail so using break here makes this look like improper C code. I
> > > think this would be better to split out the protected section to a
> > > separate function and just use the regular guard() macro.
> >
> > Good catch. This feels like something we should have some static analysis
> > around as we definitely don't want code assuming that implementation.
> >
> > +CC Dan / Julia to see if they agree.
> >
>
> I feel like the scoped_guard() macro is so complicated because they
> wanted break statements to work as expected... (As opposed to how I write
> half my loop macros using nested for loops so that when I break it only
> breaks from the inner loop and corrupts memory).
Was a while back but don't remember that coming up as a reason.
I thought the for loop construct was just a way to define the scope in
a place where the following or preceding code couldn't influence what was
instantiated.
Anyhow I think breaks in a scoped_guard() is a horrible pattern based on hidden
implementation details so I'm keen to avoid it at least in IIO.
Maybe this will become common enough that I'll revisit that view in a year
or two. Factoring out the code as a function seems the right answer in this
case.
Never mind on checking for it generally if we think it might be something
that was intended as a feature not a bug.
Thanks
Jonathan
>
> regards,
> dan carpenter
>
Powered by blists - more mailing lists