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
