[PATCH 0/3] hwmon: (oxp-sensors) Refactor probe() and init() and remove devm_add_groups()

[PATCH 0/3] hwmon: (oxp-sensors) Refactor probe() and init() and remove devm_add_groups()

[Joaquín Ignacio Aramendía]

Remove the use of devm_add_groups() in favour of dev_groups in platform
driver structure. This will allow for removal of the function as it was
intended in Greg's email[1].

Also since the driver is not hotpluggable move al the initialization and
detection logic to init() instead of probe() so we don't instantiate the
driver if the detection fails.

[1] Link: https://lore.kernel.org/linux-hwmon/ZKW7WuP0T9QdCR+G@google.com/

Joaquín Ignacio Aramendía (3):
  hwmon: (oxp-sensors) Move tt_toggle attribute to dev_groups
  hwmon: (oxp-sensors) Move board detection to the init function
  hwmon: (oxp-sensors) Refactor init() and remove probe()

 drivers/hwmon/oxp-sensors.c | 81 +++++++++++++++++++++----------------
 1 file changed, 46 insertions(+), 35 deletions(-)

-- 
2.41.0

[PATCH 1/3] hwmon: (oxp-sensors) Move tt_toggle attribute to dev_groups

[Joaquín Ignacio Aramendía]

A driver should not be manually adding groups in its probe function (it will
race with userspace), so replace the call to devm_device_add_groups() to use
the platform dev_groups callback instead.

This will allow for removal of the devm_device_add_groups() function.
---
 drivers/hwmon/oxp-sensors.c | 38 +++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
index e1a907cae820..1e1cc67bcdea 100644
--- a/drivers/hwmon/oxp-sensors.c
+++ b/drivers/hwmon/oxp-sensors.c
@@ -220,6 +220,20 @@ static int tt_toggle_disable(void)
 }
 
 /* Callbacks for turbo toggle attribute */
+static umode_t tt_toggle_is_visible(struct kobject *kobj,
+				    struct attribute *attr, int n)
+{
+	switch (board) {
+	case aok_zoe_a1:
+	case oxp_mini_amd_a07:
+	case oxp_mini_amd_pro:
+		return attr->mode;
+	default:
+		break;
+	}
+	return 0;
+}
+
 static ssize_t tt_toggle_store(struct device *dev,
 			       struct device_attribute *attr, const char *buf,
 			       size_t count)
@@ -396,7 +410,15 @@ static struct attribute *oxp_ec_attrs[] = {
 	NULL
 };
 
-ATTRIBUTE_GROUPS(oxp_ec);
+static struct attribute_group oxp_ec_attribute_group = {
+	.is_visible = tt_toggle_is_visible,
+	.attrs = oxp_ec_attrs,
+};
+
+static const struct attribute_group *oxp_ec_groups[] = {
+	&oxp_ec_attribute_group,
+	NULL
+};
 
 static const struct hwmon_ops oxp_ec_hwmon_ops = {
 	.is_visible = oxp_ec_hwmon_is_visible,
@@ -415,7 +437,6 @@ static int oxp_platform_probe(struct platform_device *pdev)
 	const struct dmi_system_id *dmi_entry;
 	struct device *dev = &pdev->dev;
 	struct device *hwdev;
-	int ret;
 
 	/*
 	 * Have to check for AMD processor here because DMI strings are the
@@ -430,18 +451,6 @@ static int oxp_platform_probe(struct platform_device *pdev)
 
 	board = (enum oxp_board)(unsigned long)dmi_entry->driver_data;
 
-	switch (board) {
-	case aok_zoe_a1:
-	case oxp_mini_amd_a07:
-	case oxp_mini_amd_pro:
-		ret = devm_device_add_groups(dev, oxp_ec_groups);
-		if (ret)
-			return ret;
-		break;
-	default:
-		break;
-	}
-
 	hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
 						     &oxp_ec_chip_info, NULL);
 
@@ -451,6 +460,7 @@ static int oxp_platform_probe(struct platform_device *pdev)
 static struct platform_driver oxp_platform_driver = {
 	.driver = {
 		.name = "oxp-platform",
+		.dev_groups = oxp_ec_groups,
 	},
 	.probe = oxp_platform_probe,
 };
-- 
2.41.0

Re: [PATCH 1/3] hwmon: (oxp-sensors) Move tt_toggle attribute to dev_groups

[Greg KH]

On Mon, Jul 17, 2023 at 09:40:04AM -0300, Joaquín Ignacio Aramendía wrote:
> A driver should not be manually adding groups in its probe function (it will
> race with userspace), so replace the call to devm_device_add_groups() to use
> the platform dev_groups callback instead.
> 
> This will allow for removal of the devm_device_add_groups() function.
> ---
>  drivers/hwmon/oxp-sensors.c | 38 +++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)

Nice, but you forgot a signed-off-by: line :(

Re: [PATCH 1/3] hwmon: (oxp-sensors) Move tt_toggle attribute to dev_groups

[Joaquin Aramendia]

> Nice, but you forgot a signed-off-by: line :(

Note to self: don't submit without morning coffee

Will add it and submit. Thanks for your review.
-- 
Joaquín I. Aramendía

[PATCH 2/3] hwmon: (oxp-sensors) Move board detection to the init function

[Joaquín Ignacio Aramendía]

This driver is not hotpluggable by nature so it makes more sense to
detect the board on init() instead of on probe().

Move detection logic to the start of init() function so we won't
instantiate the driver if the board is not compatible.
---
 drivers/hwmon/oxp-sensors.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
index 1e1cc67bcdea..c70d9355eeba 100644
--- a/drivers/hwmon/oxp-sensors.c
+++ b/drivers/hwmon/oxp-sensors.c
@@ -438,19 +438,6 @@ static int oxp_platform_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device *hwdev;
 
-	/*
-	 * Have to check for AMD processor here because DMI strings are the
-	 * same between Intel and AMD boards, the only way to tell them apart
-	 * is the CPU.
-	 * Intel boards seem to have different EC registers and values to
-	 * read/write.
-	 */
-	dmi_entry = dmi_first_match(dmi_table);
-	if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
-		return -ENODEV;
-
-	board = (enum oxp_board)(unsigned long)dmi_entry->driver_data;
-
 	hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
 						     &oxp_ec_chip_info, NULL);
 
@@ -469,6 +456,21 @@ static struct platform_device *oxp_platform_device;
 
 static int __init oxp_platform_init(void)
 {
+	const struct dmi_system_id *dmi_entry;
+
+	/*
+	 * Have to check for AMD processor here because DMI strings are the
+	 * same between Intel and AMD boards, the only way to tell them apart
+	 * is the CPU.
+	 * Intel boards seem to have different EC registers and values to
+	 * read/write.
+	 */
+	dmi_entry = dmi_first_match(dmi_table);
+	if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
+		return -ENODEV;
+
+	board = (enum oxp_board)(unsigned long)dmi_entry->driver_data;
+
 	oxp_platform_device =
 		platform_create_bundle(&oxp_platform_driver,
 				       oxp_platform_probe, NULL, 0, NULL, 0);
-- 
2.41.0

[PATCH 3/3] hwmon: (oxp-sensors) Refactor init() and remove probe()

[Joaquín Ignacio Aramendía]

Since the driver is not hotpluggable the probe() funtion is not used
more than once.

Move all attribute registration logic to the init() function.
---
 drivers/hwmon/oxp-sensors.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
index c70d9355eeba..39de49c8a392 100644
--- a/drivers/hwmon/oxp-sensors.c
+++ b/drivers/hwmon/oxp-sensors.c
@@ -431,32 +431,20 @@ static const struct hwmon_chip_info oxp_ec_chip_info = {
 	.info = oxp_platform_sensors,
 };
 
-/* Initialization logic */
-static int oxp_platform_probe(struct platform_device *pdev)
-{
-	const struct dmi_system_id *dmi_entry;
-	struct device *dev = &pdev->dev;
-	struct device *hwdev;
-
-	hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
-						     &oxp_ec_chip_info, NULL);
-
-	return PTR_ERR_OR_ZERO(hwdev);
-}
-
 static struct platform_driver oxp_platform_driver = {
 	.driver = {
 		.name = "oxp-platform",
 		.dev_groups = oxp_ec_groups,
 	},
-	.probe = oxp_platform_probe,
 };
 
 static struct platform_device *oxp_platform_device;
 
+/* Initialization logic */
 static int __init oxp_platform_init(void)
 {
 	const struct dmi_system_id *dmi_entry;
+	struct device *hwdev;
 
 	/*
 	 * Have to check for AMD processor here because DMI strings are the
@@ -472,10 +460,21 @@ static int __init oxp_platform_init(void)
 	board = (enum oxp_board)(unsigned long)dmi_entry->driver_data;
 
 	oxp_platform_device =
-		platform_create_bundle(&oxp_platform_driver,
-				       oxp_platform_probe, NULL, 0, NULL, 0);
+		platform_create_bundle(&oxp_platform_driver, NULL, NULL, 0,
+				       NULL, 0);
+	if (IS_ERR(oxp_platform_device))
+		return PTR_ERR(oxp_platform_device);
 
-	return PTR_ERR_OR_ZERO(oxp_platform_device);
+	hwdev = devm_hwmon_device_register_with_info(&oxp_platform_device->dev,
+						     "oxpec", NULL,
+						     &oxp_ec_chip_info, NULL);
+	if (IS_ERR(hwdev)) {
+		platform_device_unregister(oxp_platform_device);
+		platform_driver_unregister(&oxp_platform_driver);
+		return PTR_ERR(hwdev);
+	}
+
+	return 0;
 }
 
 static void __exit oxp_platform_exit(void)
-- 
2.41.0

Re: [PATCH 3/3] hwmon: (oxp-sensors) Refactor init() and remove probe()

[Greg KH]

On Mon, Jul 17, 2023 at 09:40:06AM -0300, Joaquín Ignacio Aramendía wrote:
> Since the driver is not hotpluggable the probe() funtion is not used
> more than once.
> 
> Move all attribute registration logic to the init() function.

Again, as in patch 2/3, you forgot a signed-off-by line.

But this change isn't correct, just because a device is not
hotpluggable, does not mean it should not be using probe/release, in
fact just the opposite, it should be using that and NOT init.

But I understand why you changed the init call in patch 2/3, that is ok,
this isn't because:

> ---
>  drivers/hwmon/oxp-sensors.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
> index c70d9355eeba..39de49c8a392 100644
> --- a/drivers/hwmon/oxp-sensors.c
> +++ b/drivers/hwmon/oxp-sensors.c
> @@ -431,32 +431,20 @@ static const struct hwmon_chip_info oxp_ec_chip_info = {
>  	.info = oxp_platform_sensors,
>  };
>  
> -/* Initialization logic */
> -static int oxp_platform_probe(struct platform_device *pdev)
> -{
> -	const struct dmi_system_id *dmi_entry;
> -	struct device *dev = &pdev->dev;
> -	struct device *hwdev;
> -
> -	hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
> -						     &oxp_ec_chip_info, NULL);
> -
> -	return PTR_ERR_OR_ZERO(hwdev);
> -}
> -
>  static struct platform_driver oxp_platform_driver = {
>  	.driver = {
>  		.name = "oxp-platform",
>  		.dev_groups = oxp_ec_groups,
>  	},
> -	.probe = oxp_platform_probe,
>  };
>  
>  static struct platform_device *oxp_platform_device;
>  
> +/* Initialization logic */
>  static int __init oxp_platform_init(void)
>  {
>  	const struct dmi_system_id *dmi_entry;
> +	struct device *hwdev;
>  
>  	/*
>  	 * Have to check for AMD processor here because DMI strings are the
> @@ -472,10 +460,21 @@ static int __init oxp_platform_init(void)
>  	board = (enum oxp_board)(unsigned long)dmi_entry->driver_data;
>  
>  	oxp_platform_device =
> -		platform_create_bundle(&oxp_platform_driver,
> -				       oxp_platform_probe, NULL, 0, NULL, 0);
> +		platform_create_bundle(&oxp_platform_driver, NULL, NULL, 0,
> +				       NULL, 0);
> +	if (IS_ERR(oxp_platform_device))
> +		return PTR_ERR(oxp_platform_device);
>  
> -	return PTR_ERR_OR_ZERO(oxp_platform_device);
> +	hwdev = devm_hwmon_device_register_with_info(&oxp_platform_device->dev,
> +						     "oxpec", NULL,
> +						     &oxp_ec_chip_info, NULL);

You are creating a fake platform device out of no where here, which is
tied to nothing, which isn't ok.  Keep it in the proper device tree and
have it be passed to you by the driver core in the probe() function.

I think you will see that this changed where in /sys/devices/ your
device is now, right?


> +	if (IS_ERR(hwdev)) {
> +		platform_device_unregister(oxp_platform_device);

Making fake platform devices is generally never a good idea, please
don't do that.

thanks,

greg k-h

Re: [PATCH 3/3] hwmon: (oxp-sensors) Refactor init() and remove probe()

[Guenter Roeck]

On 7/17/23 06:45, Greg KH wrote:
> On Mon, Jul 17, 2023 at 09:40:06AM -0300, Joaquín Ignacio Aramendía wrote:
>> Since the driver is not hotpluggable the probe() funtion is not used
>> more than once.
>>
>> Move all attribute registration logic to the init() function.
> 
> Again, as in patch 2/3, you forgot a signed-off-by line.
> 
> But this change isn't correct, just because a device is not
> hotpluggable, does not mean it should not be using probe/release, in
> fact just the opposite, it should be using that and NOT init.
> 
> But I understand why you changed the init call in patch 2/3, that is ok,
> this isn't because:
> 
>> ---
>>   drivers/hwmon/oxp-sensors.c | 33 ++++++++++++++++-----------------
>>   1 file changed, 16 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
>> index c70d9355eeba..39de49c8a392 100644
>> --- a/drivers/hwmon/oxp-sensors.c
>> +++ b/drivers/hwmon/oxp-sensors.c
>> @@ -431,32 +431,20 @@ static const struct hwmon_chip_info oxp_ec_chip_info = {
>>   	.info = oxp_platform_sensors,
>>   };
>>   
>> -/* Initialization logic */
>> -static int oxp_platform_probe(struct platform_device *pdev)
>> -{
>> -	const struct dmi_system_id *dmi_entry;
>> -	struct device *dev = &pdev->dev;
>> -	struct device *hwdev;
>> -
>> -	hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
>> -						     &oxp_ec_chip_info, NULL);
>> -
>> -	return PTR_ERR_OR_ZERO(hwdev);
>> -}
>> -
>>   static struct platform_driver oxp_platform_driver = {
>>   	.driver = {
>>   		.name = "oxp-platform",
>>   		.dev_groups = oxp_ec_groups,
>>   	},
>> -	.probe = oxp_platform_probe,
>>   };
>>   
>>   static struct platform_device *oxp_platform_device;
>>   
>> +/* Initialization logic */
>>   static int __init oxp_platform_init(void)
>>   {
>>   	const struct dmi_system_id *dmi_entry;
>> +	struct device *hwdev;
>>   
>>   	/*
>>   	 * Have to check for AMD processor here because DMI strings are the
>> @@ -472,10 +460,21 @@ static int __init oxp_platform_init(void)
>>   	board = (enum oxp_board)(unsigned long)dmi_entry->driver_data;
>>   
>>   	oxp_platform_device =
>> -		platform_create_bundle(&oxp_platform_driver,
>> -				       oxp_platform_probe, NULL, 0, NULL, 0);
>> +		platform_create_bundle(&oxp_platform_driver, NULL, NULL, 0,
>> +				       NULL, 0);
>> +	if (IS_ERR(oxp_platform_device))
>> +		return PTR_ERR(oxp_platform_device);
>>   
>> -	return PTR_ERR_OR_ZERO(oxp_platform_device);
>> +	hwdev = devm_hwmon_device_register_with_info(&oxp_platform_device->dev,
>> +						     "oxpec", NULL,
>> +						     &oxp_ec_chip_info, NULL);
> 
> You are creating a fake platform device out of no where here, which is
> tied to nothing, which isn't ok.  Keep it in the proper device tree and
> have it be passed to you by the driver core in the probe() function.
> 

This is a system with dmi data, so it won't support devicetree. Other
than that, you are correct, this patch is definitely not a good idea
and needs to be dropped.

Thanks,
Guenter

> I think you will see that this changed where in /sys/devices/ your
> device is now, right?
> 
> 
>> +	if (IS_ERR(hwdev)) {
>> +		platform_device_unregister(oxp_platform_device);
> 
> Making fake platform devices is generally never a good idea, please
> don't do that.
> 
> thanks,
> 
> greg k-h

Re: [PATCH 3/3] hwmon: (oxp-sensors) Refactor init() and remove probe()

[Joaquin Aramendia]

Hello Guenter and Greg:

> > Again, as in patch 2/3, you forgot a signed-off-by line.

Will resubmit with proper Sign-off

> > You are creating a fake platform device out of no where here, which is
> > tied to nothing, which isn't ok.  Keep it in the proper device tree and
> > have it be passed to you by the driver core in the probe() function.
> >
>
> This is a system with dmi data, so it won't support devicetree. Other
> than that, you are correct, this patch is definitely not a good idea
> and needs to be dropped.
>
> Thanks,
> Guenter
>
> > I think you will see that this changed where in /sys/devices/ your
> > device is now, right?

The attribute is created in the same place as before this patch. And
works the same as before this patch.

I can drop this patch and only resubmit 1 and 2. Thanks for the review
to both of you.

-- 
Joaquín I. Aramendía