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?
...
> struct iio_channel *iio_channel_get_all(struct device *dev)
> {
> const char *name;
> - struct iio_channel *chans;
> + struct iio_channel *fw_chans;
> struct iio_map_internal *c = NULL;
Move it here for better ordering?
> int nummaps = 0;
> int mapind = 0;
...
> - chans = fwnode_iio_channel_get_all(dev);
> + fw_chans = fwnode_iio_channel_get_all(dev);
I would move it before conditional...
> /*
> * We only want to carry on if the error is -ENODEV. Anything else
> * should be reported up the stack.
> */
> - if (!IS_ERR(chans) || PTR_ERR(chans) != -ENODEV)
> - return chans;
...here.
> + if (!IS_ERR(fw_chans) || PTR_ERR(fw_chans) != -ENODEV)
> + return fw_chans;
...
> + struct iio_channel *chans __free(kfree) = kcalloc(nummaps + 1,
> + sizeof(*chans),
> + GFP_KERNEL);
Indentation?
> + if (!chans)
> + return ERR_PTR(-ENOMEM);
--
With Best Regards,
Andy Shevchenko
