From: Jonathan Cameron <jic23@kernel.org>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Nuno Sa <nuno.sa@analog.com>,
linux-iio@vger.kernel.org, Lars-Peter Clausen <lars@metafoo.de>
Subject: Re: [PATCH v3 4/4] iio: inkern: move to the cleanup.h magic
Date: Sat, 23 Mar 2024 18:09:51 +0000 [thread overview]
Message-ID: <20240323180951.5e990e11@jic23-huawei> (raw)
In-Reply-To: <ZfX3gnbwYcZlGpBq@surfacebook.localdomain>
On Sat, 16 Mar 2024 21:48:18 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> 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?
>
I tweaked this. Went a bit further as mixing declarations that
set values and ones that don't is a bad pattern for readability.
struct iio_map_internal *mapi;
int i = 0;
int ret;
> > 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.)
I much prefer not to have the
if (ret) // error case
do something.
//back to both good and bad paths.
return ret;
pattern - so I've very keen to have this spit as I disliked the original
code and there is even less reason to combine the paths now we don't need
the mutex_unlock.
>
> ...
>
> > + 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)
Given I was rebasing anyway, tidied this up (in 4 places) as well (fewer tabs ;)
>
> > 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.
Don't mind a change doing that, but not in this patch.
>
> > + 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?
It's not immediately obvious what that put pairs with so we should probably
address that a bit more clearly anyway - but the change should be safe.
>
> ...
>
> > 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?
Trivial, but meh, I'm tweaking anyway so done.
>
> > 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);
>
prev parent reply other threads:[~2024-03-23 18:10 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-29 15:10 [PATCH v3 0/4] iio: move IIO to the cleanup.h magic Nuno Sa
2024-02-29 15:10 ` [PATCH v3 1/4] iio: core: move to " Nuno Sa
2024-02-29 15:10 ` [PATCH v3 2/4] iio: trigger: move to the " Nuno Sa
2024-03-16 19:32 ` Andy Shevchenko
2024-03-18 12:33 ` Jonathan Cameron
2024-03-18 13:12 ` Andy Shevchenko
2024-03-18 14:15 ` Jonathan Cameron
2024-03-16 19:39 ` Andy Shevchenko
2024-03-18 9:22 ` Nuno Sá
2024-02-29 15:10 ` [PATCH v3 3/4] iio: buffer: iio: core: " Nuno Sa
2024-03-16 19:38 ` Andy Shevchenko
2024-03-18 9:23 ` Nuno Sá
2024-03-18 12:35 ` Jonathan Cameron
2024-03-16 19:49 ` Andy Shevchenko
2024-02-29 15:10 ` [PATCH v3 4/4] iio: inkern: " Nuno Sa
2024-03-03 14:24 ` Jonathan Cameron
2024-03-04 8:04 ` Nuno Sá
2024-03-09 17:41 ` Jonathan Cameron
2024-03-16 13:26 ` Jonathan Cameron
2024-03-16 19:48 ` Andy Shevchenko
2024-03-18 9:20 ` Nuno Sá
2024-03-23 18:09 ` Jonathan Cameron [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240323180951.5e990e11@jic23-huawei \
--to=jic23@kernel.org \
--cc=andy.shevchenko@gmail.com \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=nuno.sa@analog.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).