Messages in this thread | | | Subject | Re: [PATCH v2 2/2] platform/x86: dell: Add new dell-wmi-ddv driver | From | Armin Wolf <> | Date | Fri, 21 Oct 2022 11:34:10 +0200 |
| |
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
| |