Message-ID: <7f311659-9f49-44dc-ad40-977d34066d98@linaro.org>
Date: Mon, 15 Jan 2024 17:13:21 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: 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>,
 Philipp Zabel <p.zabel@...gutronix.de>, 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 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.

> 
>> +       unsigned int lookup_flags;
>> +
>> +       /*
>> +        * Later we map GPIO flags between OF and Linux, however not all
>> +        * constants from include/dt-bindings/gpio/gpio.h and
>> +        * include/linux/gpio/machine.h match each other.
>> +        */
>> +       if (of_flags > GPIO_ACTIVE_LOW) {
>> +               pr_err("reset-gpio code does not support GPIO flags %u for GPIO %u\n",
>> +                       of_flags, gpio);
>> +               return -EINVAL;
>> +       }
>> +
>> +       gdev = gpio_device_find_by_fwnode(of_fwnode_handle(np));
> 
> ... 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?

> 
>> +       if (!gdev)
>> +               return -EPROBE_DEFER;
>> +
>> +       label = kstrdup(gpio_device_get_label(gdev), GFP_KERNEL);
>> +       if (!label)
>> +               return -EINVAL;
>> +
>> +       /* Size: one lookup entry plus sentinel */
>> +       lookup = kzalloc(struct_size(lookup, table, 2), GFP_KERNEL);
>> +       if (!lookup)
>> +               return -ENOMEM;
>> +
>> +       lookup->dev_id = kasprintf(GFP_KERNEL, "reset-gpio.%d", id);
>> +       if (!lookup->dev_id)
>> +               return -ENOMEM;
>> +
>> +       lookup_flags = GPIO_PERSISTENT;
>> +       lookup_flags |= of_flags & GPIO_ACTIVE_LOW;
>> +       lookup->table[0] = GPIO_LOOKUP(no_free_ptr(label), gpio, "reset",
>> +                                      lookup_flags);
>> +
>> +       gpiod_add_lookup_table(no_free_ptr(lookup));
> 
> You told me that this doesn't need to be removed or ever freed but a
> comment on that would be in order.

Sure, code further comments on this but I can make it explicit here as well.

> 
>> +
>> +       return 0;
>> +}
>> +
>> +/*
>> + * @reset_args:        phandle to the GPIO provider with all the args like GPIO number
>> + */
>> +static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
>> +{
>> +       struct reset_gpio_lookup *rgpio_dev;
>> +       struct platform_device *pdev;
>> +       int id, ret;
>> +
>> +       /*
>> +        * Registering reset-gpio device might cause immediate
>> +        * bind, resulting in its probe() registering new reset controller thus
>> +        * taking reset_list_mutex lock via reset_controller_register().
>> +        */
>> +       lockdep_assert_not_held(&reset_list_mutex);
> 
> So how does dumping the stack help here exactly?

This is self-documenting code. dumping stack does not matter, the point
is that future developers should see this lockdep before they start
playing with new locks.

> 
>> +
>> +       mutex_lock(&reset_gpio_lookup_mutex);
>> +
>> +       list_for_each_entry(rgpio_dev, &reset_gpio_lookup_list, list) {
>> +               if (args->np == rgpio_dev->of_args.np) {
>> +                       if (__reset_gpios_args_match(args, &rgpio_dev->of_args))
>> +                               goto out; /* Already on the list, done */
>> +               }
>> +       }
>> +
>> +       id = ida_alloc(&reset_gpio_ida, GFP_KERNEL);
>> +       if (id < 0) {
>> +               ret = id;
>> +               goto err_unlock;
>> +       }
>> +
>> +       /*
>> +        * Not freed in normal path, persisent subsystem data (which is assumed
>> +        * also in the reset-gpio driver).
>> +        */
>> +       rgpio_dev = kzalloc(sizeof(*rgpio_dev), GFP_KERNEL);
>> +       if (!rgpio_dev) {
>> +               ret = -ENOMEM;
>> +               goto err_ida_free;
>> +       }
>> +
>> +       ret = __reset_add_reset_gpio_lookup(id, args->np, args->args[0],
>> +                                           args->args[1]);
>> +       if (ret < 0)
>> +               goto err_kfree;
>> +
>> +       rgpio_dev->of_args = *args;
>> +       /*
>> +        * We keep the device_node reference, but of_args.np is put at the end
>> +        * of __of_reset_control_get(), so get it one more time.
>> +        * Hold reference as long as rgpio_dev memory is valid.
>> +        */
>> +       of_node_get(rgpio_dev->of_args.np);
>> +       pdev = platform_device_register_data(NULL, "reset-gpio", id,
>> +                                            &rgpio_dev->of_args,
>> +                                            sizeof(rgpio_dev->of_args));
>> +       ret = PTR_ERR_OR_ZERO(pdev);
>> +       if (ret)
>> +               goto err_put;
>> +
>> +       list_add(&rgpio_dev->list, &reset_gpio_lookup_list);
>> +
>> +out:
>> +       mutex_unlock(&reset_gpio_lookup_mutex);
>> +
>> +       return 0;
>> +
>> +err_put:
>> +       of_node_put(rgpio_dev->of_args.np);
>> +err_kfree:
>> +       kfree(rgpio_dev);
>> +err_ida_free:
>> +       ida_free(&reset_gpio_ida, id);
>> +err_unlock:
>> +       mutex_unlock(&reset_gpio_lookup_mutex);
>> +
> 
> You're already using cleanup helpers above, why not here too? Would
> make this function much more readable and allow you to drop all but

Not sure how much it would be cleaner considering that these are not
free on success.

> the ida_free() here. Possibly you'd need to define the __free()
> callback for of_node_put() though.
> 
>> +       return ret;
>> +}
>> +
>> +static struct reset_controller_dev *__reset_find_rcdev(const struct of_phandle_args *args,
>> +                                                      bool gpio_fallback)
>> +{
>> +       struct reset_controller_dev *r, *rcdev;
>> +
>> +       lockdep_assert_held(&reset_list_mutex);
>> +
>> +       rcdev = NULL;
>> +       list_for_each_entry(r, &reset_controller_list, list) {
>> +               if (args->np == r->of_node) {
>> +                       if (gpio_fallback) {
>> +                               if (__reset_gpios_args_match(args, r->of_args)) {
>> +                                       rcdev = r;
>> +                                       break;
>> +                               }
>> +                       } else {
>> +                               rcdev = r;
>> +                               break;
>> +                       }
>> +               }
>> +       }
>> +
>> +       return rcdev;
>> +}
>> +
>>  struct reset_control *
>>  __of_reset_control_get(struct device_node *node, const char *id, int index,
>>                        bool shared, bool optional, bool acquired)
>>  {
>> +       struct of_phandle_args args = {0};
>> +       bool gpio_fallback = false;
>>         struct reset_control *rstc;
>> -       struct reset_controller_dev *r, *rcdev;
>> -       struct of_phandle_args args;
>> +       struct reset_controller_dev *rcdev;
>>         int rstc_id;
>>         int ret;
>>
>> @@ -839,39 +1028,49 @@ __of_reset_control_get(struct device_node *node, const char *id, int index,
>>                                          index, &args);
>>         if (ret == -EINVAL)
>>                 return ERR_PTR(ret);
>> -       if (ret)
>> -               return optional ? NULL : ERR_PTR(ret);
>> +       if (ret) {
>> +               /*
>> +                * There can be only one reset-gpio for regular devices, so
>> +                * don't bother with GPIO index.
>> +                */
>> +               ret = of_parse_phandle_with_args(node, "reset-gpios", "#gpio-cells",
>> +                                                0, &args);
>> +               if (ret)
>> +                       return optional ? NULL : ERR_PTR(ret);
>>
>> -       mutex_lock(&reset_list_mutex);
>> -       rcdev = NULL;
>> -       list_for_each_entry(r, &reset_controller_list, list) {
>> -               if (args.np == r->of_node) {
>> -                       rcdev = r;
>> -                       break;
>> +               gpio_fallback = true;
>> +
>> +               ret = __reset_add_reset_gpio_device(&args);
>> +               if (ret) {
>> +                       rstc = ERR_PTR(ret);
>> +                       goto out_put;
>>                 }
>>         }
>>
>> +       mutex_lock(&reset_list_mutex);
>> +       rcdev = __reset_find_rcdev(&args, gpio_fallback);
>>         if (!rcdev) {
>>                 rstc = ERR_PTR(-EPROBE_DEFER);
>> -               goto out;
>> +               goto out_unlock;
>>         }
>>
>>         if (WARN_ON(args.args_count != rcdev->of_reset_n_cells)) {
>>                 rstc = ERR_PTR(-EINVAL);
>> -               goto out;
>> +               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.

Best regards,
Krzysztof

Powered by blists - more mailing lists