| 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
| ||
|
Message-ID: <517da43f-6a94-43c2-8a1a-3fa2acffdec2@roeck-us.net>
Date: Sun, 12 Jan 2025 06:57:53 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Atharva Tiwari <evepolonium@...il.com>
Cc: Aun-Ali Zaidi <admin@...eit.net>, Henrik Rydberg <rydberg@...math.org>,
Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] hwmon: Add T2 Mac fan control support in applesmc
driver
On Sun, Jan 12, 2025 at 01:06:59AM +0530, Atharva Tiwari wrote:
> This patch adds support for fan control on T2 Macs in the applesmc driver.
> It introduces functions to handle floating-point fan speed values (which are required by t2 chips).
> The fan speed reading and writing are updated to support both integer and floating-point values.
> The fan manual control is also updated to handle T2 Mac-specific keys.
>
> and also Guenter Roeck asked me "Why use applesmc_write_key here?" (line: 917)
> The use of applesmc_write_key is to directly update specific SMC keys
> for controlling hardware parameters like fan speeds or sensor calibration
> Changes since v1:
> -- added spaces as Guenter Roeck asked me
> -- also removed the type casting for buffer
>
Change log should be after "---".
The subject line should start with "hwmon: (applesmc)".
Again, please run checkpatch on your patches. There are (still) two warnings.
> Signed-off-by: Aun-Ali Zaidi <admin@...eit.net>
> Co-developed-by: Aun-Ali Zaidi <admin@...eit.net>
>
> Signed-off-by: Atharva Tiwari <evepolonium@...il.com>
> ---
> drivers/hwmon/applesmc.c | 108 +++++++++++++++++++++++++++++++--------
> 1 file changed, 86 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index fc6d6a9053ce..1f60776fe56a 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -511,6 +511,31 @@ static int applesmc_read_s16(const char *key, s16 *value)
> return 0;
> }
>
> +static inline u32 applesmc_float_to_u32(u32 d)
> +{
> + u8 sign = (d >> 31) & 1;
> + s32 exp = ((d >> 23) & 0xff) - 0x7F;
> + u32 fr = d & ((1u << 23) - 1);
> +
> + if (sign || exp < 0)
> + return 0;
> +
> + return (1u << exp) + (fr >> (23 - exp));
Using BIT(), FIELD_GET(), and BIT_MASK() would make this much more readable.
> +}
> +
> +static inline u32 applesmc_u32_to_float(u32 d)
> +{
> + u32 dc = d, bc = 0, exp;
> +
> + if (!d)
> + return 0;
> + while (dc >>= 1)
> + ++bc;
> + exp = 0x7F + bc;
> + return (exp << 23) |
> + ((d << (23 - (exp - 0x7F))) & ((1u << 23) - 1));
> +}
> +
> /*
> * applesmc_device_init - initialize the accelerometer. Can sleep.
> */
> @@ -842,15 +867,24 @@ static ssize_t applesmc_show_fan_speed(struct device *dev,
> unsigned int speed = 0;
> char newkey[5];
> u8 buffer[2];
> + const struct applesmc_entry *entry;
>
> scnprintf(newkey, sizeof(newkey), fan_speed_fmt[to_option(attr)],
> to_index(attr));
>
> - ret = applesmc_read_key(newkey, buffer, 2);
> + entry = applesmc_get_entry_by_key(newkey);
> + if (IS_ERR(entry))
> + return PTR_ERR(entry);
> +
> + if (!strcmp(entry->type, "flt")) {
> + ret = applesmc_read_entry(entry, buffer, 4);
> + speed = applesmc_float_to_u32(*(u32 *)buffer);
Buffer size is 2 bytes.
Missing error handling will end up running calculations on uninitialized data.
> + } else {
> + ret = applesmc_read_entry(entry, buffer, 2);
> + speed = ((buffer[0] << 8 | buffer[1]) >> 2);
> + }
> if (ret)
> return ret;
> -
> - speed = ((buffer[0] << 8 | buffer[1]) >> 2);
> return sysfs_emit(sysfsbuf, "%u\n", speed);
> }
>
> @@ -861,7 +895,8 @@ static ssize_t applesmc_store_fan_speed(struct device *dev,
> int ret;
> unsigned long speed;
> char newkey[5];
> - u8 buffer[2];
> + u8 buffer[4];
> + const struct applesmc_entry *entry;
>
> if (kstrtoul(sysfsbuf, 10, &speed) < 0 || speed >= 0x4000)
> return -EINVAL; /* Bigger than a 14-bit value */
Does this limit still apply ?
> @@ -869,9 +904,18 @@ static ssize_t applesmc_store_fan_speed(struct device *dev,
> scnprintf(newkey, sizeof(newkey), fan_speed_fmt[to_option(attr)],
> to_index(attr));
>
> - buffer[0] = (speed >> 6) & 0xff;
> - buffer[1] = (speed << 2) & 0xff;
> - ret = applesmc_write_key(newkey, buffer, 2);
> + entry = applesmc_get_entry_by_key(newkey);
> + if (IS_ERR(entry))
> + return PTR_ERR(entry);
> +
> + if (!strcmp(entry->type, "flt")) {
> + *(u32 *)buffer = applesmc_u32_to_float(speed);
> + ret = applesmc_write_entry(entry, buffer, 4);
> + } else {
> + buffer[0] = (speed >> 6) & 0xff;
> + buffer[1] = (speed << 2) & 0xff;
> + ret = applesmc_write_key(newkey, buffer, 2);
Again, why not use applesmc_write_entry() here ?
Either case, the entire code repeatedly checks if the data type
is float. This seems excessive. Is there a reason to not check this
once and then use a flag such as has_accelerometer or has_accelerometer ?
> + }
>
> if (ret)
> return ret;
> @@ -885,12 +929,23 @@ static ssize_t applesmc_show_fan_manual(struct device *dev,
> int ret;
> u16 manual = 0;
> u8 buffer[2];
> + char newkey[5];
> + bool has_newkey = false;
> +
> + scnprintf(newkey, sizeof(newkey), "F%dMd", to_index(attr));
Other keys use a define. Please do the same.
> +
> + ret = applesmc_has_key(newkey, &has_newkey);
> + if (has_newkey) {
> + ret = applesmc_read_key(newkey, buffer, 1);
> + manual = buffer[0];
Is this guaranteed to be a single bit ? No mask needed ?
> + } else {
> + ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
> + manual = ((buffer[0] << 8 | buffer[1]) >> to_index(attr)) & 0x01;
> + }
Similar to float detection, it might be desirable to check once
if FANS_MANUAL or "F%dMd" is used and then use a flag instead of
checking each and every time if "F%dMd" exists.
>
> - ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
> if (ret)
> return ret;
>
> - manual = ((buffer[0] << 8 | buffer[1]) >> to_index(attr)) & 0x01;
> return sysfs_emit(sysfsbuf, "%d\n", manual);
> }
>
> @@ -900,28 +955,37 @@ static ssize_t applesmc_store_fan_manual(struct device *dev,
> {
> int ret;
> u8 buffer[2];
> + char newkey[5];
> + bool has_newkey = false;
> unsigned long input;
> u16 val;
>
> if (kstrtoul(sysfsbuf, 10, &input) < 0)
> return -EINVAL;
>
> - ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
> - if (ret)
> - goto out;
> -
> - val = (buffer[0] << 8 | buffer[1]);
> + scnprintf(newkey, sizeof(newkey), "F%dMd", to_index(attr));
>
> - if (input)
> - val = val | (0x01 << to_index(attr));
> - else
> - val = val & ~(0x01 << to_index(attr));
> -
> - buffer[0] = (val >> 8) & 0xFF;
> - buffer[1] = val & 0xFF;
> + ret = applesmc_has_key(newkey, &has_newkey);
> + if (ret)
> + return ret;
> + if (has_newkey) {
> + buffer[0] = input & 1;
This is a significant semantics change. Previously a value != 0
meant that the bit is to be set. With this, all odd values mean that
the bit is set, and even values mean that it is clear. That is
unacceptable, sorry.
> + ret = applesmc_write_key(newkey, buffer, 1);
> + } else {
> + ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
> + val = (buffer[0] << 8 | buffer[1]);
> + if (ret)
> + goto out;
Error check should precede value assignment.
Please be consistent. The gotos are completely unnecessary,
and you replaced it with a return above. Either always use the
(unnecessary) gotos, or always return immediately. Yes, I know,
that means to drop the label below, but that is unnecessary
to start with (as is else after return).
> + if (input)
> + val = val | (0x01 << to_index(attr));
> + else
> + val = val & ~(0x01 << to_index(attr));
>
> - ret = applesmc_write_key(FANS_MANUAL, buffer, 2);
> + buffer[0] = (val >> 8) & 0xFF;
> + buffer[1] = val & 0xFF;
>
> + ret = applesmc_write_key(FANS_MANUAL, buffer, 2);
> + }
> out:
> if (ret)
> return ret;
Powered by blists - more mailing lists