Re: [PATCH v1 1/1] devres: Pass unique name of the resource to devm_add_action()

[Mirsad Todorovac]

Hi, Andy,

On 2/24/23 21:07, Andy Shevchenko wrote:
> Pass the unique name of the resource to devm_add_action(),
> so it will be easier to debug managed resources.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
>   drivers/base/devres.c  | 11 ++++++-----
>   include/linux/device.h |  5 ++++-
>   2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index c0e100074aa3..5c998cfac335 100644
> --- a/drivers/base/devres.c
> +++ b/drivers/base/devres.c
> @@ -722,20 +722,21 @@ static void devm_action_release(struct device *dev, void *res)
>   }
>   
>   /**
> - * devm_add_action() - add a custom action to list of managed resources
> + * __devm_add_action() - add a custom action to list of managed resources
>    * @dev: Device that owns the action
>    * @action: Function that should be called
>    * @data: Pointer to data passed to @action implementation
> + * @name: Name of the resource (for debugging purposes)
>    *
>    * This adds a custom action to the list of managed resources so that
>    * it gets executed as part of standard resource unwinding.
>    */
> -int devm_add_action(struct device *dev, void (*action)(void *), void *data)
> +int __devm_add_action(struct device *dev, void (*action)(void *), void *data, const char *name)
>   {
>   	struct action_devres *devres;
>   
> -	devres = devres_alloc(devm_action_release,
> -			      sizeof(struct action_devres), GFP_KERNEL);
> +	devres = __devres_alloc_node(devm_action_release, sizeof(struct action_devres),
> +				     GFP_KERNEL, NUMA_NO_NODE, name);
>   	if (!devres)
>   		return -ENOMEM;
>   
> @@ -745,7 +746,7 @@ int devm_add_action(struct device *dev, void (*action)(void *), void *data)
>   	devres_add(dev, devres);
>   	return 0;
>   }
> -EXPORT_SYMBOL_GPL(devm_add_action);
> +EXPORT_SYMBOL_GPL(__devm_add_action);
>   
>   /**
>    * devm_remove_action() - removes previously added custom action
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 1508e637bb26..5b9f3cb22f78 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -243,10 +243,13 @@ void __iomem *devm_of_iomap(struct device *dev,
>   			    resource_size_t *size);
>   
>   /* allows to add/remove a custom action to devres stack */
> -int devm_add_action(struct device *dev, void (*action)(void *), void *data);
>   void devm_remove_action(struct device *dev, void (*action)(void *), void *data);
>   void devm_release_action(struct device *dev, void (*action)(void *), void *data);
>   
> +int __devm_add_action(struct device *dev, void (*action)(void *), void *data, const char *name);
> +#define devm_add_action(release, action, data) \
> +	__devm_add_action(release, action, data, #action)
> +
>   static inline int devm_add_action_or_reset(struct device *dev,
>   					   void (*action)(void *), void *data)
>   {

I have built last couple of kernels including 6.3-rc1+ w your patch.

(I'm late two weeks w testing, but those were rather busy two weeks.)

I see what it is meant to do, but I am unsure of how to test whether it works.

Being the unfaithful Thomas, I always prefer to test rather to just assume it
is OK.

Is this OK output you expected to see in syslog?

Mar  8 22:20:36 pc-mtodorov kernel: gpio-sim gpio-sim.0: DEVRES ADD 00000000ea28d384 action (16 bytes)
Mar  8 22:20:36 pc-mtodorov kernel: gpio-sim gpio-sim.0: DEVRES ADD 000000000cb0e6b1 devm_kzalloc_release (16 bytes)
Mar  8 22:20:36 pc-mtodorov kernel: gpio-sim gpio-sim.0: DEVRES ADD 0000000056043cc0 devm_kzalloc_release (40 bytes)
Mar  8 22:20:36 pc-mtodorov kernel: gpio-sim gpio-sim.0: DEVRES ADD 00000000c904ae95 devm_kzalloc_release (24 bytes)
Mar  8 22:20:36 pc-mtodorov kernel: gpio-sim gpio-sim.0: DEVRES ADD 00000000cd39c068 devm_kzalloc_release (40 bytes)
Mar  8 22:20:36 pc-mtodorov kernel: gpio-sim gpio-sim.0: DEVRES ADD 00000000918a0de4 devm_kzalloc_release (40 bytes)
Mar  8 22:20:36 pc-mtodorov kernel: gpio-sim gpio-sim.0: DEVRES ADD 000000008192a378 devm_kzalloc_release (10 bytes)
Mar  8 22:20:36 pc-mtodorov kernel: gpio-sim gpio-sim.0: DEVRES ADD 000000004090f288 action (16 bytes)
Mar  8 22:20:36 pc-mtodorov kernel: gpio-sim gpio-sim.0: DEVRES REL 000000004090f288 action (16 bytes)
Mar  8 22:20:36 pc-mtodorov kernel: gpio-sim gpio-sim.0: DEVRES REL 000000008192a378 devm_kzalloc_release (10 bytes)
Mar  8 22:20:36 pc-mtodorov kernel: gpio-sim gpio-sim.0: DEVRES REL 00000000918a0de4 devm_kzalloc_release (40 bytes)
Mar  8 22:20:36 pc-mtodorov kernel: gpio-sim gpio-sim.0: DEVRES REL 00000000cd39c068 devm_kzalloc_release (40 bytes)
Mar  8 22:20:36 pc-mtodorov kernel: gpio-sim gpio-sim.0: DEVRES REL 00000000c904ae95 devm_kzalloc_release (24 bytes)
Mar  8 22:20:36 pc-mtodorov kernel: gpio-sim gpio-sim.0: DEVRES REL 0000000056043cc0 devm_kzalloc_release (40 bytes)
Mar  8 22:20:36 pc-mtodorov kernel: gpio-sim gpio-sim.0: DEVRES REL 000000000cb0e6b1 devm_kzalloc_release (16 bytes)
Mar  8 22:20:36 pc-mtodorov kernel: gpio-sim gpio-sim.0: DEVRES REL 00000000ea28d384 action (16 bytes)

NOTE: Maybe I should emphasise that this is not seen in either dmesg or kernel console.

I have just checked, and DEVRES lines are only in /var/log/messages (on AlmaLinux 8.7, CentOS fork).

As you must have guessed yourself already, this will frustrate debugging past the lifetime of rsyslog process.

Also, there is no way known to me to access dmesg log from the previous kernel run.

Thanks,
Mirsad


--
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu

System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia