Re: [PATCH] devres: Move remaining devm_alloc_percpu and devm_device_add_group to devres.h

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, May 22, 2025 at 05:01:26PM +0800, Pei Xiao wrote:
> Since commit f5e5631df596("devres: Move devm_*_action*() APIs to
> devres.h"), But devm_alloc_percpu() and devm_device_add_group didn't be
> moved.
> 
> so move it.The changes improve header organization by keeping all
> resource-managed device APIs in the dedicated devres.h header,
> reducing cross-header dependencies and making the interfaces
> easier to locate.

Thanks for the patch, my comments below.

...

> -/**
> - * devm_alloc_percpu - Resource-managed alloc_percpu
> - * @dev: Device to allocate per-cpu memory for
> - * @type: Type to allocate per-cpu memory for
> - *
> - * Managed alloc_percpu. Per-cpu memory allocated with this function is
> - * automatically freed on driver detach.
> - *
> - * RETURNS:
> - * Pointer to allocated memory on success, NULL on failure.
> - */
> -#define devm_alloc_percpu(dev, type)      \
> -	((typeof(type) __percpu *)__devm_alloc_percpu((dev), sizeof(type), \
> -						      __alignof__(type)))
> -
> -void __percpu *__devm_alloc_percpu(struct device *dev, size_t size,
> -				   size_t align);
> -void devm_free_percpu(struct device *dev, void __percpu *pdata);

Don't you need to cleanup the header inclusions as well?

...

> -int __must_check devm_device_add_group(struct device *dev,
> -				       const struct attribute_group *grp);

I'm not sure about this. The percpu seems standalone piece, but this has
relations with the other group related definitions just above.

...

> +#include <linux/sysfs.h>

> +#include <asm/percpu.h>

What for are these new inclusions, please?

...

> +/**
> + * devm_alloc_percpu - Resource-managed alloc_percpu
> + * @dev: Device to allocate per-cpu memory for
> + * @type: Type to allocate per-cpu memory for
> + *
> + * Managed alloc_percpu. Per-cpu memory allocated with this function is
> + * automatically freed on driver detach.
> + *
> + * RETURNS:

Please, check that all of the kernel-doc are in align (using same style).

> + * Pointer to allocated memory on success, NULL on failure.
> + */
> +#define devm_alloc_percpu(dev, type)      \
> +	((typeof(type) __percpu *)__devm_alloc_percpu((dev), sizeof(type), \
> +						      __alignof__(type)))

Just make it one line. it will be less than 100.

> +void __percpu *__devm_alloc_percpu(struct device *dev, size_t size,
> +				   size_t align);

Ditto.

> +void devm_free_percpu(struct device *dev, void __percpu *pdata);

Please, take your time to understand what is behind the __percpu and
why the asm/percpu.h is redundant here.

-- 
With Best Regards,
Andy Shevchenko