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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 9/28/22 12:47, Andy Shevchenko wrote:
> 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)

Then you can select N if you really want to.

> (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.)

This has been discussed during the review of v1 already.

There are quite a few dell modules and the choice has
been made to put these all behind a dell platform drivers
options and then default all the individual modules to 'm'.

> ...
> 
>> + * 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?
> 
>> +	}
> 
> ...
> 
>> +	kfree(obj);
> 
> I'm wondering what is the best to use in the drivers:
>  1) kfree()
>  2) acpi_os_free()
>  3) ACPI_FREE()
> 
> ?

Most ACPI driver code I know of just uses kfree() the other 2
are more ACPI-core / ACPICA internal helpers.


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

index is used in the registered sysfs attr show functions,
so if this fails then the sysfs attr should not be registered.

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

Because this is an extension to the ACPI battery driver, IOW
this adds extra attributes to the power-supply-class device
registered by the ACPI battery driver. Note that the device
in this case is managed by the power-supply-class code, so
there is no access to dev_groups even in the ACPI battery code.

> 
> ...
> 
>> +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().
> 
>> +}
> 
> ...
> 
>> +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.
> 
>> +	},
>> +	.id_table = dell_wmi_ddv_id_table,
>> +	.probe = dell_wmi_ddv_probe,
>> +};
> 


Regards,

Hans