linux-kernel - Re: [PATCH v4 1/2] hwmon: add GPD devices sensor driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3e1db249-f89e-4cc4-9e92-1f00f2e262f9@gmail.com>
Date: Mon, 22 Jul 2024 15:59:52 +0800
From: Cryolitia PukNgae <cryolitia@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Jean Delvare <jdelvare@...e.com>, Jonathan Corbet <corbet@....net>,
 linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
 linux-doc@...r.kernel.org, Celeste Liu <CoelacanthusHex@...il.com>,
 Yao Zi <ziyao@...root.org>, Marcin Strągowski
 <marcin@...agowski.com>
Subject: Re: [PATCH v4 1/2] hwmon: add GPD devices sensor driver


On 2024/7/19 09:41, Guenter Roeck wrote:
> I am havng a hard time reviewing this driver. I am going to pint out a few
> issues, but this is far from a complete review.

I'm new to kernel development, so please forgive my mistakes and thank 
you for your patience.

I am modifying the source code of this driver according to your 
suggestions, and I would like to discuss some of your comments first.

>> +static const struct gpd_model_quirk gpd_win4_quirk = {
>> +	.model_name	= "win4",
>> +	.address	= {
>> +				.addr_port		= 0x2E,
>> +				.data_port		= 0x2F,
>> +				.manual_control_enable	= 0xC311,
>> +				.rpm_read		= 0xC880,
>> +				.pwm_write		= 0xC311,
>> +				.pwm_max		= 127,
>> +			},
>> +	.read_rpm	= gpd_win4_read_rpm,
>> +	// same as GPD Win Mini
>> +	.set_pwm_enable	= gpd_win_mini_set_pwm_enable,
>> +	.read_pwm	= gpd_read_pwm,
>> +	// same as GPD Win Mini
> I do not see te value in those comments.

It's the struct of win4, but it's part of functions are the same as 
win_mini's.

The comment is to remind that, it's by design to use win_mini's 
function, not by mistake.

>> +
>> +static int gpd_fan_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct gpd_driver_priv *data;
>> +	const struct resource *plat_res;
>> +	const struct device *dev_reg;
>> +	const struct resource *region_res;
>> +
>> +	data = dev_get_platdata(&pdev->dev);
>> +	if (IS_ERR(data))
>> +		return -ENODEV;
>> +
> With all the "const" spread through the driver, this one is really odd.
> I have never seen a driver there the _platform data_ is used to store
> instance-specific information. Normally _that_ information is considered
> constant and not modified by a driver.  I really have to say that it is
> extremely odd to have the init function
> declare values such as pwm enable and pwm value and use it in the driver.
>
> Please provide a rationale for this unusual approach.
I don't know how to pass which model the init function found. Is it a 
good idea the use a global pointer to point to the instance-specific 
information?
>> +	plat_res = platform_get_resource(pdev, IORESOURCE_IO, 0);
>> +	if (IS_ERR(plat_res))
>> +		return dev_err_probe(dev, PTR_ERR(plat_res),
>> +				     "Failed to get platform resource\n");
>> +
>> +	region_res = devm_request_region(dev, plat_res->start,
>> +					 resource_size(plat_res), DRIVER_NAME);
>> +	if (IS_ERR(region_res))
>> +		return dev_err_probe(dev, PTR_ERR(region_res),
>> +				     "Failed to request region\n");
>> +
>> +	dev_reg = devm_hwmon_device_register_with_info(
>> +		dev, DRIVER_NAME, data, &gpd_fan_chip_info, NULL);
> CHECK: Lines should not end with a '('
> #756: FILE: drivers/hwmon/gpd-fan.c:593:
> +	dev_reg = devm_hwmon_device_register_with_info(
>
> Plus on top of that multi-line code should be aligned with '('.

The source code has been formatted by clang-format with kernel's 
`.clang-format` file.

But I would be glad to manually adjust it's style if needed.

>> +static int gpd_fan_remove(struct platform_device *pdev)
>> +{
>> +	struct gpd_driver_priv *data = dev_get_platdata(&pdev->dev);
>> +
>> +	data->pwm_enable = AUTOMATIC;
>> +	data->quirk->set_pwm_enable(data, AUTOMATIC);
>> +
> This is even more unusual. Can you point me to other drivers in the kernel
> using that same approach for handling device specific private data ?

It's to set EC back to default status if user rmmod the driver, to 
prevent a hardware damage.

For example, they may use a userspace program to adjusting the fan 
curve, setting the EC to manually control mode. It happened that the 
device was in low power consumption and fan speed during rmmod, and the 
user remove the module and then performed some tasks that generated a 
lot of heat. Since the module was uninstalled and the EC was still in 
manual mode, there was nothing to protect the device.

I don't know how to implement this part elegantly

>> +
>> +	struct gpd_driver_priv data = {
>> +		.pwm_enable		= AUTOMATIC,
>> +		.pwm_value		= 255,
> This is unusual, to say it mildly. Since the pwm value is never read
> from the controller/chip, this is just a random value.

We cannot read pwm out on win_mini, only wm2 support it.

It's also to prevent the device from damaging.

Assuming the user switches to manual control mode immediately after 
loading the module, the fan will always run at full speed until the user 
specifies the fan speed.