Date:   Wed, 28 Sep 2022 22:57:16 +0200
From:   Armin Wolf <W_Armin@....de>
To:     Andy Shevchenko <andriy.shevchenko@...el.com>
Cc:     hdegoede@...hat.com, markgross@...nel.org, rafael@...nel.org,
        lenb@...nel.org, hmh@....eng.br, matan@...alib.org,
        corentin.chary@...il.com, jeremy@...tem76.com,
        productdev@...tem76.com, mario.limonciello@....com,
        pobrn@...tonmail.com, coproscefalo@...il.com,
        platform-driver-x86@...r.kernel.org, linux-acpi@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] platform/x86: dell: Add new dell-wmi-ddv driver

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,
>> +};

Powered by blists - more mailing lists