Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH v2 2/2] platform/x86: dell: Add new dell-wmi-ddv driver
To:     Hans de Goede <hdegoede@redhat.com>,
        Andy Shevchenko <andriy.shevchenko@intel.com>
Cc:     markgross@kernel.org, rafael@kernel.org, lenb@kernel.org,
        hmh@hmh.eng.br, matan@svgalib.org, corentin.chary@gmail.com,
        jeremy@system76.com, productdev@system76.com,
        mario.limonciello@amd.com, pobrn@protonmail.com,
        coproscefalo@gmail.com, platform-driver-x86@vger.kernel.org,
        linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220927204521.601887-1-W_Armin@gmx.de>
 <20220927204521.601887-3-W_Armin@gmx.de>
 <YzQmQw0hEwzXV/iz@smile.fi.intel.com>
 <34774c9d-1210-0015-f78e-97fdf717480c@gmx.de>
 <YzVqbSBHm3OrjIaQ@smile.fi.intel.com>
 <bf84f13d-f76a-c0ac-8a1f-f4e1b55e6f51@redhat.com>
From:   Armin Wolf <W_Armin@gmx.de>
Message-ID: <11964cd1-94b5-dc6a-a6c9-7fd5fe335ed4@gmx.de>
Date:   Fri, 21 Oct 2022 11:34:10 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <bf84f13d-f76a-c0ac-8a1f-f4e1b55e6f51@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: quoted-printable
Content-Language: en-US
Precedence: bulk

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 =3D 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_informatio=
n", 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_debugf=
s_init() would be ignored
>>> when called, which means that returning an error would serve no purpos=
e.
>>> Additionally, devm_add_action_or_reset() automatically executes the cl=
eanup 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 e=
xisting
>> (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 =3D 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 debu=
gfs it
>> mustn't change the driver name, because the latter is ABI, while the fo=
rmer 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 a=
n 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