linux-kernel - Re: [PATCH v3 2/5] reset: Instantiate reset GPIO controller for shared reset-gpios

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 15 Jan 2024 18:32:07 +0100
From: Philipp Zabel <p.zabel@...gutronix.de>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, Bartosz
 Golaszewski <brgl@...ev.pl>
Cc: Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio
 <konrad.dybcio@...aro.org>, Srinivas Kandagatla
 <srinivas.kandagatla@...aro.org>,  Banajit Goswami <bgoswami@...cinc.com>,
 Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,  Rob
 Herring <robh+dt@...nel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
 Peter Rosin <peda@...ntia.se>, Jaroslav Kysela <perex@...ex.cz>, Takashi
 Iwai <tiwai@...e.com>, linux-arm-msm@...r.kernel.org, 
 alsa-devel@...a-project.org, linux-sound@...r.kernel.org, 
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
 linux-i2c@...r.kernel.org, Chris Packham
 <chris.packham@...iedtelesis.co.nz>,  Sean Anderson <sean.anderson@...o.com>
Subject: Re: [PATCH v3 2/5] reset: Instantiate reset GPIO controller for
 shared reset-gpios

On Mo, 2024-01-15 at 17:13 +0100, Krzysztof Kozlowski wrote:
> On 15/01/2024 17:06, Bartosz Golaszewski wrote:
> > > +
> > > +static int __reset_add_reset_gpio_lookup(int id, struct device_node *np,
> > > +                                        unsigned int gpio,
> > > +                                        unsigned int of_flags)
> > > +{
> > > +       struct gpiod_lookup_table *lookup __free(kfree) = NULL;
> > > +       struct gpio_device *gdev __free(gpio_device_put) = NULL;
> > > +       char *label __free(kfree) = NULL;
> > 
> > I got yelled at by Linus Torvalds personally for doing it like this. I
> > know this is a common pattern in code using GLib but Linus wants auto
> > variables to be initialized where they're declared...
> 
> Declaration is here. Initialization is here. Therefore this is
> initialized where it is declared. What's more it is initialized to a
> valid value, because __free() accepts NULLs.
[...]
> > ... so this should become:
> > 
> >   struct gpio_device *gdev __free(gpio_device_put) = gpio_device_find(...)
> > 
> > and same for the rest.
> > 
> > Don't get me wrong, I love cleanup.h but there's a (unofficial for
> > now) coding style.
> 
> So you just want to declare it not in top-part of the function but just
> before first use?

IIUC, Linus wants exactly this:

https://lore.kernel.org/all/CAHk-=wgRHiV5VSxtfXA4S6aLUmcQYEuB67u3BJPJPtuESs1JyA@mail.gmail.com/

[...]
> > 
> > > +               goto out_unlock;
> > >         }
> > > 
> > >         rstc_id = rcdev->of_xlate(rcdev, &args);
> > >         if (rstc_id < 0) {
> > >                 rstc = ERR_PTR(rstc_id);
> > > -               goto out;
> > > +               goto out_unlock;
> > >         }
> > > 
> > >         /* reset_list_mutex also protects the rcdev's reset_control list */
> > >         rstc = __reset_control_get_internal(rcdev, rstc_id, shared, acquired);
> > > 
> > > -out:
> > > +out_unlock:
> > >         mutex_unlock(&reset_list_mutex);
> > > +out_put:
> > >         of_node_put(args.np);
> > 
> > I suggest reworking this to use cleanup.h as well.
> 
> It's independent task. This is an existing code and any refactoring to
> cleanup or not is independent thing.

Seconded. Separate cleanup very welcome, but this series is about
adding functionality.

regards
Philipp