linux-kernel - Re: [PATCH] platform/x86: Add Steam Deck 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] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 6 Feb 2022 09:51:16 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Andrey Smirnov <andrew.smirnov@...il.com>,
        platform-driver-x86@...r.kernel.org
Cc:     Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <markgross@...nel.org>,
        Jean Delvare <jdelvare@...e.com>, linux-kernel@...r.kernel.org,
        linux-hwmon@...r.kernel.org
Subject: Re: [PATCH] platform/x86: Add Steam Deck driver

On 2/5/22 18:20, Andrey Smirnov wrote:
> Add a driver exposing various bits and pieces of functionality
> provided by Steam Deck specific VLV0100 device presented by EC
> firmware. This includes but not limited to:
> 
>      - CPU/device's fan control
>      - Read-only access to DDIC registers
>      - Battery tempreature measurements
>      - Various display related control knobs
>      - USB Type-C connector event notification
> 
> Cc: Hans de Goede <hdegoede@...hat.com>
> Cc: Mark Gross <markgross@...nel.org>
> Cc: Jean Delvare <jdelvare@...e.com>
> Cc: Guenter Roeck <linux@...ck-us.net>
> Cc: linux-kernel@...r.kernel.org (open list)
> Cc: platform-driver-x86@...r.kernel.org
> Cc: linux-hwmon@...r.kernel.org
> Signed-off-by: Andrey Smirnov <andrew.smirnov@...il.com>
> ---
> 
...

 > +config STEAMDECK
 > +       tristate "Valve Steam Deck platform driver"
 > +       depends on X86_64
 > +       help
 > +         Driver exposing various bits and pieces of functionality
 > +	 provided by Steam Deck specific VLV0100 device presented by
 > +	 EC firmware. This includes but not limited to:

There seems to be some indentation issue.

 > +	     - CPU/device's fan control
 > +	     - Read-only access to DDIC registers
 > +	     - Battery tempreature measurements
 > +	     - Various display related control knobs
 > +	     - USB Type-C connector event notification
 > +
 > +	 Say N unless you are running on a Steam Deck.
 > +

This doesn't depend on hwmon, yet it fails if devm_hwmon_device_register_with_info()
returns an eror. That has a couple of problems: if HWMON=n, it won't compile,
and if STEAMDECK=y and HWMON=m it won't compile either. You'll have to provide
some dependency against HWMON to make this work.

...

> +
> +static int steamdeck_read_fan_speed(struct steamdeck *jup, long *speed)
> +{
> +	unsigned long long val;
> +
> +	if (ACPI_FAILURE(acpi_evaluate_integer(jup->adev->handle,
> +					       "FANR", NULL, &val)))
> +		return -EIO;
> +
> +	*speed = val;
> +	return 0;
> +}
> +
> +static int
> +steamdeck_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +		     u32 attr, int channel, long *out)
> +{
> +	struct steamdeck *sd = dev_get_drvdata(dev);
> +	unsigned long long val;
> +
> +	switch (type) {
> +	case hwmon_temp:
> +		if (attr != hwmon_temp_input)
> +			return -EOPNOTSUPP;
> +
> +		if (ACPI_FAILURE(acpi_evaluate_integer(sd->adev->handle,
> +						       "BATT", NULL, &val)))
> +			return -EIO;
> +		/*
> +		 * Assuming BATT returns deg C we need to mutiply it
> +		 * by 1000 to convert to mC
> +		 */
> +		*out = val * 1000;
> +		break;
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_input:
> +			return steamdeck_read_fan_speed(sd, out);

There is a bit of inconsistency here: All other atributes are handled directly,
except for this one, yet there isn't really a difference in the actual operation.
Maybe I am missing something. What is the reason for using a function here
but not for the other attributes ?

> +		case hwmon_fan_target:
> +			*out = sd->fan_target;
> +			break;
> +		case hwmon_fan_fault:
> +			if (ACPI_FAILURE(acpi_evaluate_integer(
> +						 sd->adev->handle,
> +						 "FANC", NULL, &val)))
> +				return -EIO;
> +			/*
> +			 * FANC (Fan check):
> +			 * 0: Abnormal
> +			 * 1: Normal
> +			 */
> +			*out = !val;
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +steamdeck_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
> +			    u32 attr, int channel, const char **str)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		*str = "Battery Temp";
> +		break;
> +	case hwmon_fan:
> +		*str = "System Fan";
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +steamdeck_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> +		      u32 attr, int channel, long val)
> +{
> +	struct steamdeck *sd = dev_get_drvdata(dev);
> +
> +	if (type != hwmon_fan ||
> +	    attr != hwmon_fan_target)
> +		return -EOPNOTSUPP;
> +
> +	if (val > U16_MAX)
> +		return -EINVAL;

This accepts negative values, and it expects the user to find
valid ranges. I suggest to use clamp_val() instead.

Guenter