Re: [PATCH v2 2/2] platform/x86: dell: Add new dell-wmi-ddv driver

[Armin Wolf <W_Armin@gmx.de>]

Am 29.09.22 um 15:12 schrieb Hans de Goede:

> Hi,
>
> On 9/29/22 11:50, Andy Shevchenko wrote:
>> On Wed, Sep 28, 2022 at 10:57:16PM +0200, Armin Wolf wrote:
>>> Am 28.09.22 um 12:47 schrieb Andy Shevchenko:
>>>> On Tue, Sep 27, 2022 at 10:45:21PM +0200, Armin Wolf wrote:
>> ...
>>
>>>>> +static void dell_wmi_ddv_debugfs_init(struct wmi_device *wdev)
>>>> Strictly speaking this should return int (see below).
>>>>
>>>>> +{
>>>>> +	struct dentry *entry;
>>>>> +	char name[64];
>>>>> +Fujitsu Academy
>>>>>
>>>>> +	scnprintf(name, ARRAY_SIZE(name), "%s-%s", DRIVER_NAME, dev_name(&wdev->dev));
>>>>> +	entry = debugfs_create_dir(name, NULL);
>>>>> +
>>>>> +	debugfs_create_devm_seqfile(&wdev->dev, "fan_sensor_information", entry,
>>>>> +				    dell_wmi_ddv_fan_read);
>>>>> +	debugfs_create_devm_seqfile(&wdev->dev, "thermal_sensor_information", entry,
>>>>> +				    dell_wmi_ddv_temp_read);
>>>>> +
>>>>> +	devm_add_action_or_reset(&wdev->dev, dell_wmi_ddv_debugfs_remove, entry);
>>>> return devm...
>>>>
>>>> This is not related to debugfs and there is no rule to avoid checking error
>>>> codes from devm_add_action_or_reset().
>>>>
>>> According to the documentation of debugfs_create_dir(), drivers should work fine if debugfs
>>> initialization fails. Thus the the return value of dell_wmi_ddv_debugfs_init() would be ignored
>>> when called, which means that returning an error would serve no purpose.
>>> Additionally, devm_add_action_or_reset() automatically executes the cleanup function if devres
>>> registration fails, so we do not have to care about that.
>> The problem with your code that if devm_ call fails and you ain't stop probing
>> the remove-insert module (or unbind-bind) cycle will fail, because of existing
>> (leaked) debugfs dentries.
> No it won't if the devm_ call fails then it will directly call
> the passed in handler so in this case we can simply continue
> without debugfs entries (which will have been removed by the
> handler). The directly calling of the action handler on
> failure is the whole difference between devm_add_action()
> and devm_add_action_or_reset()
>
> So using it this way in the case of a debugfs init function
> is fine.
>
>>>>> +		.name = DRIVER_NAME,
>>>> I would use explicit literal since this is a (semi-) ABI, and having it as
>>>> a define feels not fully right.
>>> The driver name is used in two places (init and debugfs), so having a define for it
>>> avoids problems in case someone forgets to change both.
>> Which is exactly what we must prevent developer to do. If changing debugfs it
>> mustn't change the driver name, because the latter is ABI, while the former is
>> not.
> Arguably both are not really ABI. Drivers have been renamed in the past
> without issues for userspace.
>
> Regards,
>
> Hans
>
What is the current status of this patch set? If necessary, i can submit an v3 patch set which includes the
patch regarding the minor style fixes. I also tested the driver on my Dell Insprion 3505 for some time, so
i can proof it works.

Armin Wolf