Re: [PATCH v4 09/20] ACPI: platform_profile: Create class for ACPI platform profile

[Armin Wolf <W_Armin@gmx.de>]

Am 05.11.24 um 16:33 schrieb Mario Limonciello:

> When registering a platform profile handler create a class device
> that will allow changing a single platform profile handler.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/acpi/platform_profile.c  | 42 +++++++++++++++++++++++++++++---
>   include/linux/platform_profile.h |  2 ++
>   2 files changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index fd1c4e9dccf0a..f8e9dc9a10d54 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -5,6 +5,7 @@
>   #include <linux/acpi.h>
>   #include <linux/bits.h>
>   #include <linux/init.h>
> +#include <linux/kdev_t.h>
>   #include <linux/mutex.h>
>   #include <linux/platform_profile.h>
>   #include <linux/sysfs.h>
> @@ -22,6 +23,12 @@ static const char * const profile_names[] = {
>   };
>   static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
>
> +static DEFINE_IDR(platform_profile_minor_idr);

Please use a IDA instead, you can take a look at the hwmon subsystem for an example.

> +
> +static const struct class platform_profile_class = {
> +	.name = "platform-profile",
> +};
> +
>   static ssize_t platform_profile_choices_show(struct device *dev,
>   					struct device_attribute *attr,
>   					char *buf)
> @@ -113,6 +120,8 @@ void platform_profile_notify(void)
>   {
>   	if (!cur_profile)
>   		return;
> +	if (!class_is_registered(&platform_profile_class))
> +		return;
>   	sysfs_notify(acpi_kobj, NULL, "platform_profile");
>   }
>   EXPORT_SYMBOL_GPL(platform_profile_notify);
> @@ -123,6 +132,9 @@ int platform_profile_cycle(void)
>   	enum platform_profile_option next;
>   	int err;
>
> +	if (!class_is_registered(&platform_profile_class))
> +		return -ENODEV;
> +
>   	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>   		if (!cur_profile)
>   			return -ENODEV;
> @@ -167,12 +179,33 @@ int platform_profile_register(struct platform_profile_handler *pprof)
>   	if (cur_profile)
>   		return -EEXIST;
>
> -	err = sysfs_create_group(acpi_kobj, &platform_profile_group);
> -	if (err)
> -		return err;
> +	if (!class_is_registered(&platform_profile_class)) {
> +		/* class for individual handlers */
> +		err = class_register(&platform_profile_class);
> +		if (err)
> +			return err;

I think it would be better to register the class during module initialization. This way userspace
applications can depend on the class directory being present if the driver has been loaded (even if
it might be empty).

> +		/* legacy sysfs files */
> +		err = sysfs_create_group(acpi_kobj, &platform_profile_group);
> +		if (err)
> +			goto cleanup_class;
> +	}
> +
> +	/* create class interface for individual handler */
> +	pprof->minor = idr_alloc(&platform_profile_minor_idr, pprof, 0, 0, GFP_KERNEL);
> +	pprof->class_dev = device_create(&platform_profile_class, pprof->dev,
> +					 MKDEV(0, pprof->minor), NULL, "platform-profile-%d",
> +					 pprof->minor);

Please use MKDEV(0, 0), as we have no associated character device.

> +	if (IS_ERR(pprof->class_dev))
> +		return PTR_ERR(pprof->class_dev);

You still need to free "minor" in case of an error.

> +	dev_set_drvdata(pprof->class_dev, pprof);

Please use device_create() for setting the drvdata of the class device.

>
>   	cur_profile = pprof;
>   	return 0;
> +
> +cleanup_class:
> +	class_unregister(&platform_profile_class);
> +
> +	return err;
>   }
>   EXPORT_SYMBOL_GPL(platform_profile_register);
>
> @@ -181,6 +214,9 @@ int platform_profile_remove(struct platform_profile_handler *pprof)
>   	guard(mutex)(&profile_lock);
>
>   	sysfs_remove_group(acpi_kobj, &platform_profile_group);
> +
> +	device_destroy(&platform_profile_class, MKDEV(0, pprof->minor));

Using device_unregister() would be more suitable here. Also keep in mind that "minor" still
needs to be freed. Again drivers/hwmon/hwmon.c could be useful in this regard.

Thanks,
Armin Wolf

> +
>   	cur_profile = NULL;
>   	return 0;
>   }
> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> index bcaf3aa39160f..f81c3afcb7575 100644
> --- a/include/linux/platform_profile.h
> +++ b/include/linux/platform_profile.h
> @@ -29,6 +29,8 @@ enum platform_profile_option {
>   struct platform_profile_handler {
>   	const char *name;
>   	struct device *dev;
> +	struct device *class_dev;
> +	int minor;
>   	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>   	int (*profile_get)(struct platform_profile_handler *pprof,
>   				enum platform_profile_option *profile);