On Sun, 2024-02-25 at 13:12 +0000, Jonathan Cameron wrote:
> On Fri, 23 Feb 2024 13:43:48 +0100
> Nuno Sa <nuno.sa@xxxxxxxxxx> wrote:
>
> > Use the new cleanup magic for handling mutexes in IIO. This allows us to
> > greatly simplify some code paths.
> >
> > Signed-off-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> > ---
> > drivers/iio/inkern.c | 224 ++++++++++++++++-----------------------------------
> > 1 file changed, 71 insertions(+), 153 deletions(-)
> >
> > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > index 7a1f6713318a..6a1d6ff8eb97 100644
> > --- a/drivers/iio/inkern.c
> > +++ b/drivers/iio/inkern.c
> > @@ -3,6 +3,7 @@
> > *
> > * Copyright (c) 2011 Jonathan Cameron
> > */
> > +#include <linux/cleanup.h>
> > #include <linux/err.h>
> > #include <linux/export.h>
> > #include <linux/minmax.h>
> > @@ -43,30 +44,26 @@ static int iio_map_array_unregister_locked(struct iio_dev
> > *indio_dev)
> >
> > int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
> > {
> > - int i = 0, ret = 0;
> > + int i = 0;
> > struct iio_map_internal *mapi;
> >
> > if (!maps)
> > return 0;
> >
> > - mutex_lock(&iio_map_list_lock);
> > + guard(mutex)(&iio_map_list_lock);
> > while (maps[i].consumer_dev_name) {
> > mapi = kzalloc(sizeof(*mapi), GFP_KERNEL);
> > if (!mapi) {
> > - ret = -ENOMEM;
> > - goto error_ret;
> > + iio_map_array_unregister_locked(indio_dev);
> > + return -ENOMEM;
>
> break this out to a separate error path via a goto.
> The cleanup is not totally obvious so I'd like it to stand out more
> than being burried here. This wasn't good in original code either
> as that should just have duplicated the mutex_unlock.
>
>
> > }
> > mapi->map = &maps[i];
> > mapi->indio_dev = indio_dev;
> > list_add_tail(&mapi->l, &iio_map_list);
> > i++;
> > }
> > -error_ret:
> > - if (ret)
> > - iio_map_array_unregister_locked(indio_dev);
> > - mutex_unlock(&iio_map_list_lock);
> >
> > - return ret;
> > + return 0;
> > }
> > EXPORT_SYMBOL_GPL(iio_map_array_register);
>
> ...
>
> > EXPORT_SYMBOL_GPL(iio_map_array_unregister);
> >
> > @@ -337,17 +329,17 @@ static struct iio_channel *iio_channel_get_sys(const char
> > *name,
> > return ERR_PTR(-ENODEV);
> >
> > /* first find matching entry the channel map */
> > - mutex_lock(&iio_map_list_lock);
> > - list_for_each_entry(c_i, &iio_map_list, l) {
> > - if ((name && strcmp(name, c_i->map->consumer_dev_name) != 0) ||
> > - (channel_name &&
> > - strcmp(channel_name, c_i->map->consumer_channel) != 0))
> > - continue;
> > - c = c_i;
> > - iio_device_get(c->indio_dev);
> > - break;
> > + scoped_guard(mutex, &iio_map_list_lock) {
> > + list_for_each_entry(c_i, &iio_map_list, l) {
> > + if ((name && strcmp(name, c_i->map->consumer_dev_name)
> > != 0) ||
> > + (channel_name &&
> > + strcmp(channel_name, c_i->map->consumer_channel) !=
> > 0))
> > + continue;
> > + c = c_i;
> > + iio_device_get(c->indio_dev);
> > + break;
> This mix of continue and break is odd. But not something to cleanup in this patch.
> It's based on assumption we either have name or channel_name which is checked
> above.
> if ((name && strcmp(name, c_i->map->consumer_dev_name) ==
> 0) ||
> (!name && stcmp(channel_name, c_i->map-
> >consumer_channel == 0)) {
> c = c_i;
> iio_device_get(c->indio_dev);
> break;
> }
> is I think equivalent. Still too complex for this patch I think.
>
> > + }
> > }
> > - mutex_unlock(&iio_map_list_lock);
> > if (!c)
> > return ERR_PTR(-ENODEV);
> >
> > @@ -469,7 +461,7 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
> >
> > name = dev_name(dev);
> >
> > - mutex_lock(&iio_map_list_lock);
> > + guard(mutex)(&iio_map_list_lock);
> > /* first count the matching maps */
> > list_for_each_entry(c, &iio_map_list, l)
> > if (name && strcmp(name, c->map->consumer_dev_name) != 0)
> > @@ -477,17 +469,13 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
> > else
> > nummaps++;
> >
> > - if (nummaps == 0) {
> > - ret = -ENODEV;
> > - goto error_ret;
> > - }
> > + if (nummaps == 0)
> > + return ERR_PTR(-ENODEV);
> >
> > /* NULL terminated array to save passing size */
> > chans = kcalloc(nummaps + 1, sizeof(*chans), GFP_KERNEL);
>
> as below, consider dragging the instantiation down here and use __free(kfree) for
> this
> plus make sure to return_ptr() at the good exit path.
>
> > - if (!chans) {
> > - ret = -ENOMEM;
> > - goto error_ret;
> > - }
> > + if (!chans)
> > + return ERR_PTR(-ENOMEM);
> >
> > /* for each map fill in the chans element */
> > list_for_each_entry(c, &iio_map_list, l) {
> > @@ -509,7 +497,6 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
> > ret = -ENODEV;
> > goto error_free_chans;
> > }
> > - mutex_unlock(&iio_map_list_lock);
> >
> > return chans;
> >
> > @@ -517,9 +504,6 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
> > for (i = 0; i < nummaps; i++)
> > iio_device_put(chans[i].indio_dev);
> > kfree(chans);
>
> Could use __free(kfree) and return_ptr(chans); Not a huge gain though
> so maybe not worth it.
>
I'll see how it looks like. Even though not a huge gain, I guess it makes the code
more consistent as we move to the cleanup "idiom"...
- Nuno Sá
>
