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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <89c6c7e8-0407-b6bb-7085-be11efce2524@roeck-us.net>
Date:   Mon, 17 Jul 2023 08:02:58 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Greg KH <gregkh@...uxfoundation.org>,
        Joaquín Ignacio Aramendía 
        <samsagax@...il.com>
Cc:     linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] hwmon: (oxp-sensors) Refactor init() and remove
 probe()

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