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

[Armin Wolf]

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