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

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]

Date:   Fri, 21 Oct 2022 11:34:10 +0200
From:   Armin Wolf <W_Armin@....de>
To:     Hans de Goede <hdegoede@...hat.com>,
        Andy Shevchenko <andriy.shevchenko@...el.com>
Cc:     markgross@...nel.org, rafael@...nel.org, lenb@...nel.org,
        hmh@....eng.br, matan@...alib.org, corentin.chary@...il.com,
        jeremy@...tem76.com, productdev@...tem76.com,
        mario.limonciello@....com, pobrn@...tonmail.com,
        coproscefalo@...il.com, platform-driver-x86@...r.kernel.org,
        linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] platform/x86: dell: Add new dell-wmi-ddv driver

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