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

[Armin Wolf]

Am 28.09.22 um 12:47 schrieb Andy Shevchenko:

> On Tue, Sep 27, 2022 at 10:45:21PM +0200, Armin Wolf wrote:
> > The dell-wmi-ddv driver adds support for reading
> > the current temperature and ePPID of ACPI batteries
> > on supported Dell machines.
> >
> > Since the WMI interface used by this driver does not
> > do any input validation and thus cannot be used for probing,
> > the driver depends on the ACPI battery extension machanism
> > to discover batteries.
> >
> > The driver also supports a debugfs interface for retrieving
> > buffers containing fan and thermal sensor information.
> > Since the meaing of the content of those buffers is currently
> > unknown, the interface is meant for reverse-engineering and
> > will likely be replaced with an hwmon interface once the
> > meaning has been understood.
> >
> > The driver was tested on a Dell Inspiron 3505.
> ...
>
> > +config DELL_WMI_DDV
> > +	tristate "Dell WMI sensors Support"
> > +	default m
> Why? (Imagine I have Dell, but old machine)
>
> (And yes, I see that other Kconfig options are using it, but we shall avoid
>   cargo cult and each default choice like this has to be explained at least.)
>
> ...
>
> > + * dell-wmi-ddv.c -- Linux driver for WMI sensor information on Dell notebooks.
> Please, remove file name from the file. This will be an additional burden in
> the future in case it will be renamed.
>
> ...
>
> > +#include <acpi/battery.h>
> Is it required to be the first? Otherwise it seems ACPI specific to me and the
> general rule is to put inclusions from generic towards custom. I.o.w. can you
> move it after linux/wmi.h with a blank line in between?
>
> > +#include <linux/acpi.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/kstrtox.h>
> > +#include <linux/math.h>
> > +#include <linux/module.h>
> > +#include <linux/limits.h>
> > +#include <linux/power_supply.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/wmi.h>
> ...
>
> > +struct dell_wmi_ddv_data {
> > +	struct acpi_battery_hook hook;
> > +	struct device_attribute temp_attr, eppid_attr;
> It's hard to read and easy to miss that the data type has two members here.
> Please, put one member per one line.
>
> > +	struct wmi_device *wdev;
> > +};
> ...
>
> > +	if (obj->type != type) {
> > +		kfree(obj);
> > +		return -EIO;
> EINVAL?

In my opinion, EINVAL should be returned if the parameters are invalid.
In this case however, the error comes from the wmi device returning invalid
data, which would be represented better with EIO.

> > +	}
> ...
>
> > +	kfree(obj);
> I'm wondering what is the best to use in the drivers:
>   1) kfree()
>   2) acpi_os_free()
>   3) ACPI_FREE()
>
> ?
>
> ...
>
> > +static int dell_wmi_ddv_battery_index(struct acpi_device *acpi_dev, u32 *index)
> > +{
> > +	const char *uid_str = acpi_device_uid(acpi_dev);
> > +
> > +	if (!uid_str)
> > +		return -ENODEV;
> It will be better for maintaining to have
>
> 	const char *uid_str...;
>
> 	uid_str = ...
> 	if (!uid_str)
> 		...
>
> > +	return kstrtou32(uid_str, 10, index);
> > +}
> ...
>
> > +	/* Return 0 instead of error to avoid being unloaded */
> > +	ret = dell_wmi_ddv_battery_index(to_acpi_device(battery->dev.parent), &index);
> > +	if (ret < 0)
> > +		return 0;
> How index is used?
>
> ...
>
> > +	ret = device_create_file(&battery->dev, &data->temp_attr);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = device_create_file(&battery->dev, &data->eppid_attr);
> > +	if (ret < 0) {
> > +		device_remove_file(&battery->dev, &data->temp_attr);
> > +
> > +		return ret;
> > +	}
> Why dev_groups member can't be utilized?
>
> ...
>
> > +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.

> > +}
> ...
>
> > +static struct wmi_driver dell_wmi_ddv_driver = {
> > +	.driver = {
> > +		.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.

Armin Wolf

> > +	},
> > +	.id_table = dell_wmi_ddv_id_table,
> > +	.probe = dell_wmi_ddv_probe,
> > +};