On Sat, 2024-03-16 at 21:48 +0200, Andy Shevchenko wrote:
> Thu, Feb 29, 2024 at 04:10:28PM +0100, Nuno Sa kirjoitti:
> > Use the new cleanup magic for handling mutexes in IIO. This allows us to
> > greatly simplify some code paths.
> >
> > While at it, also use __free(kfree) where allocations are done and drop
> > obvious comment in iio_channel_read_min().
>
> ...
>
> > int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
> > {
> > - int i = 0, ret = 0;
> > + int i = 0, ret;
> > struct iio_map_internal *mapi;
>
> Why not making it reversed xmas tree order at the same time?
>
> > if (!maps)
> > return 0;
>
> ...
>
> > -error_ret:
> > - if (ret)
> > - iio_map_array_unregister_locked(indio_dev);
> > - mutex_unlock(&iio_map_list_lock);
> >
> > + return 0;
> > +error_ret:
> > + iio_map_array_unregister_locked(indio_dev);
> > return ret;
> > }
>
> Do we really need to split this? (I'm fine with a new code, but seems to me as
> unneeded churn.)
>
> ...
>
> > + struct iio_channel *channel __free(kfree) =
> > kzalloc(sizeof(*channel),
> > + GFP_KERNEL);
>
> I would indent a bit differently:
>
> struct iio_channel *channel __free(kfree) =
> kzalloc(sizeof(*channel),
> GFP_KERNEL);
>
> (maybe less TABs, but you got the idea)
>
> > if (!channel)
> > return ERR_PTR(-ENOMEM);
>
> ...
>
> > + struct iio_channel *chans __free(kfree) = kcalloc(nummaps + 1,
> > + sizeof(*chans),
> > + GFP_KERNEL);
>
> Ditto.
>
> > if (!chans)
> > return ERR_PTR(-ENOMEM);
>
> ...
>
> > /* 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))
>
> I would kill these ' != 0' pieces, but I see they are in the original code.
>
> > + continue;
> > + c = c_i;
> > + iio_device_get(c->indio_dev);
> > + break;
> > + }
> > }
>
> ...
>
> > - channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> > + struct iio_channel *channel __free(kfree) =
> > kzalloc(sizeof(*channel),
> > + GFP_KERNEL);
>
> Indentation?
>
> ...
>
> > -error_no_chan:
> > - kfree(channel);
> > error_no_mem:
> > iio_device_put(c->indio_dev);
> > return ERR_PTR(err);
>
> Effectively you move kfree after device put, would it be a problem?
>
This one got my attention... But I think we're fine. But yeah, subtle ordering
change that I did not unnoticed.
- Nuno Sá
