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;
Date:   Thu, 29 Sep 2022 12:50:37 +0300
From:   Andy Shevchenko <andriy.shevchenko@intel.com>
To:     Armin Wolf <W_Armin@gmx.de>
Cc:     hdegoede@redhat.com, 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
Subject: Re: [PATCH v2 2/2] platform/x86: dell: Add new dell-wmi-ddv driver
Message-ID: <YzVqbSBHm3OrjIaQ@smile.fi.intel.com>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <34774c9d-1210-0015-f78e-97fdf717480c@gmx.de>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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];
> > > +
> > > +	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.

> > > +}

That said, you must check error code of devm_ call above. This is a potential
leak of resources right now in the code.

...

> > > +		.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.

I think now you got my point.

-- 
With Best Regards,
Andy Shevchenko