| 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
| ||
|
Message-ID: <c478f7a0f3b91585618ec8e5ee57c5c4efd59f32.camel@pengutronix.de>
Date: Tue, 25 Nov 2025 15:33:17 +0100
From: Philipp Zabel <p.zabel@...gutronix.de>
To: Bartosz Golaszewski <brgl@...ev.pl>, Krzysztof Kozlowski
<krzk@...nel.org>, Val Packett <val@...kett.cool>
Cc: linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org, Bartosz
Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH] reset: gpio: add a devlink between reset-gpio and its
consumer
Hi Bartosz,
On Di, 2025-11-25 at 13:55 +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
>
> The device that requests the reset control managed by the reset-gpio
> device is effectively its consumer but the devlink is only established
> between it and the GPIO controller exposing the reset pin. Add a devlink
> between the consumer of the reset control and its supplier. This will
> allow us to simplify the GPIOLIB code managing shared GPIOs when
> handling the corner case of reset-gpio and gpiolib-shared interacting.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> ---
> This change will allow us to simplify the code in gpiolib-shared.c in
> the next cycle, so it would be awesome if it could make v6.19.
>
> Val: I'm Cc'ing you as you're already playing with audio on hamoa. If v2
> of the reset-gpios fix works for you, could you please also test this
> and make sure it doesn't break anything with soundwire?
>
> Thanks in advance,
> Bart
> ---
> drivers/reset/core.c | 73 +++++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 58 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 0135dd0ae20498396fdb5a682f913b6048cb5750..15b8da9ebcf196f7d5ce7921e4f8aba0ea6b0de4 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -20,6 +20,7 @@
> #include <linux/kref.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/of_platform.h>
> #include <linux/reset.h>
> #include <linux/reset-controller.h>
> #include <linux/slab.h>
> @@ -82,6 +83,7 @@ struct reset_gpio_lookup {
> struct of_phandle_args of_args;
> struct fwnode_handle *swnode;
> struct list_head list;
> + struct auxiliary_device *adev;
> };
>
> static const char *rcdev_name(struct reset_controller_dev *rcdev)
> @@ -829,16 +831,16 @@ static void reset_gpio_aux_device_release(struct device *dev)
> kfree(adev);
> }
>
> -static int reset_add_gpio_aux_device(struct device *parent,
> - struct fwnode_handle *swnode,
> - int id, void *pdata)
> +static struct auxiliary_device *
> +reset_add_gpio_aux_device(struct device *parent, struct fwnode_handle *swnode,
> + int id, void *pdata)
This function grows ever more similar to auxiliary_device_create().
s/add/create/?
> {
> struct auxiliary_device *adev;
> int ret;
>
> adev = kzalloc(sizeof(*adev), GFP_KERNEL);
> if (!adev)
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
>
> adev->id = id;
> adev->name = "gpio";
> @@ -850,23 +852,55 @@ static int reset_add_gpio_aux_device(struct device *parent,
> ret = auxiliary_device_init(adev);
> if (ret) {
> kfree(adev);
> - return ret;
> + return ERR_PTR(ret);
> }
>
> ret = __auxiliary_device_add(adev, "reset");
> if (ret) {
> auxiliary_device_uninit(adev);
> kfree(adev);
> - return ret;
> + return ERR_PTR(ret);
> }
>
> - return ret;
> + return adev;
> +}
> +
> +static void reset_gpio_add_devlink(struct device_node *np,
> + struct reset_gpio_lookup *rgpio_dev)
> +{
> + struct device *consumer;
> +
> + /*
> + * We must use get_dev_from_fwnode() and not of_find_device_by_node()
> + * because the latter only considers the platform bus while we want to
> + * get consumers of any kind that can be associated with firmware
> + * nodes: auxiliary, soundwire, etc.
> + */
> + consumer = get_dev_from_fwnode(of_fwnode_handle(np));
If called via __reset_control_get(), this just reconstructs the device
from dev->of_node. I think it would be better to move the linking into
__reset_control_get() and use the passed in consumer device directly.
> + if (consumer) {
> + if (!device_link_add(consumer, &rgpio_dev->adev->dev, DL_FLAG_STATELESS))
Who removes this link when the consumer puts the reset control, or if
we error out later?
> + pr_warn("Failed to create a device link between reset-gpio and its consumer");
> +
> + put_device(consumer);
> + }
> + /*
> + * else { }
> + *
> + * TODO: If ever there's a case where we need to support shared
> + * reset-gpios retrieved from a device node for which there's no
> + * device present yet, this is where we'd set up a notifier waiting
> + * for the device to appear in the system. This would be a lot of code
> + * that would go unused for now so let's cross that bridge when and if
> + * we get there.
> + */
> }
>
> /*
> - * @args: phandle to the GPIO provider with all the args like GPIO number
> + * @np: OF-node associated with the consumer
> + * @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)
> +static int __reset_add_reset_gpio_device(struct device_node *np,
> + const struct of_phandle_args *args)
> {
> struct property_entry properties[2] = { };
> unsigned int offset, of_flags, lflags;
> @@ -916,8 +950,14 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
>
> list_for_each_entry(rgpio_dev, &reset_gpio_lookup_list, list) {
> if (args->np == rgpio_dev->of_args.np) {
> - if (of_phandle_args_equal(args, &rgpio_dev->of_args))
> - return 0; /* Already on the list, done */
> + if (of_phandle_args_equal(args, &rgpio_dev->of_args)) {
> + /*
> + * Already on the list, create the device link
> + * and stop here.
> + */
> + reset_gpio_add_devlink(np, rgpio_dev);
I think instead of linking here ...
> + return 0;
> + }
> }
> }
>
> @@ -950,11 +990,14 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
> goto err_put_of_node;
> }
>
> - ret = reset_add_gpio_aux_device(parent, rgpio_dev->swnode, id,
> - &rgpio_dev->of_args);
> - if (ret)
> + rgpio_dev->adev = reset_add_gpio_aux_device(parent, rgpio_dev->swnode,
> + id, &rgpio_dev->of_args);
> + if (IS_ERR(rgpio_dev->adev)) {
> + ret = PTR_ERR(rgpio_dev->adev);
> goto err_del_swnode;
> + }
>
> + reset_gpio_add_devlink(np, rgpio_dev);
... and here, we could just create a device link between the passed in
consumer dev and rstc->rcdev->dev in __reset_control_get().
regards
Philipp
Powered by blists - more mailing lists