| 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: <8248d422-c764-4b2d-ba82-3a68cff21256@roeck-us.net>
Date: Tue, 27 Jan 2026 09:22:20 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Marius Cristea <marius.cristea@...rochip.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Jonathan Corbet <corbet@....net>, linux-hwmon@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org
Subject: Re: [PATCH v4 2/2] hwmon: temperature: add support for EMC1812
Hi,
On Tue, Jan 27, 2026 at 05:05:25PM +0200, Marius Cristea wrote:
> This is the hwmon driver for Microchip EMC1812/13/14/15/33
> Multichannel Low-Voltage Remote Diode Sensor Family.
>
> EMC1812 has one external remote temperature monitoring channel.
> EMC1813 has two external remote temperature monitoring channels.
> EMC1814 has three external remote temperature monitoring channels and
> channels 2 and 3 supports anti parallel diode.
> EMC1815 has four external remote temperature monitoring channels and
> channels 1/2 and 3/4 supports anti parallel diode.
> EMC1833 has two external remote temperature monitoring channels and
> channels 1 and 2 supports anti parallel diode.
>
> Signed-off-by: Marius Cristea <marius.cristea@...rochip.com>
> ---
> Documentation/hwmon/emc1812.rst | 68 +++
> Documentation/hwmon/index.rst | 1 +
> MAINTAINERS | 2 +
> drivers/hwmon/Kconfig | 11 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/emc1812.c | 963 ++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 1046 insertions(+)
>
> diff --git a/Documentation/hwmon/emc1812.rst b/Documentation/hwmon/emc1812.rst
> new file mode 100644
> index 0000000000000000000000000000000000000000..799111a89541c57a839a121bb3dfc12f42604bc2
> --- /dev/null
> +++ b/Documentation/hwmon/emc1812.rst
> @@ -0,0 +1,68 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver emc1802
> +=====================
> +
> +Supported chips:
> +
> + * Microchip EMC1812, EMC1813, EMC1814, EMC1815, EMC1833
> +
> + Addresses scanned: I2C 0x1c, 0x3c, 0x4c, 0x4d, 0x5c, 0x6c, 0x7c
> +
> + Prefix: 'emc1812'
> +
> + Datasheets:
> +
> + - https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/EMC1812-3-4-5-33-Data-Sheet-DS20005751.pdf
> +
> +Author:
> + Marius Cristea <marius.cristea@...rochip.com
> +
> +
> +Description
> +-----------
> +
> +The Microchip EMC181x/33 chips contain up to 4 remote temperature sensors
> +and one internal.
> +- The EMC1812 is a single channel remote temperature sensor.
> +- The EMC1813 and EMC1833 is a dual channel remote temperature sensor. The
> +remote channels for this selection of devices can support substrate diodes,
> +discrete diode-connected transistors or CPU/GPU thermal diodes.
> +- The EMC1814 is a three channel remote temperature sensor that supports
> +Anti-Parallel Diode (APD) only on one channel. For the channel that does not
> +support APD functionality, substrate diodes, discrete diode-connected
> +transistors or CPU/GPU thermal diodes are supported. For the channel that
> +supports APD, only discrete diode-connected transistors may be implemented.
> +However, if APD is disabled on the EMC1814, then the channel that supports
> +APD will be functional with substrate diodes, discrete diode-connected
> +transistors and CPU/GPU thermal diodes.
> +- The EMC1815 is a four channel remote temperature sensor. The EMC1815 and
> +EMC1833 support APD on all channels. When APD is enabled, the channels support
> +only diode-connected transistors. If APD is disabled, then the channels will
> +support substrate transistors, discrete diode-connected transistors and
> +CPU/GPU thermal diodes.
> +
> +Note: Disabling APD functionality to implement substrate diodes on devices
> +that support APD eliminates the benefit of APD (two diodes on one channel).
> +
> +The chips implement three limits for each sensor: low (tempX_min), high
> +(tempX_max) and critical (tempX_crit). The chips also implement an
> +hysteresis mechanism which applies to all limits. The relative difference
> +is stored in a single register on the chip, which means that the relative
> +difference between the limit and its hysteresis is always the same for
> +all three limits.
> +
> +This implementation detail implies the following:
> +
> +* When setting a limit, its hysteresis will automatically follow, the
> + difference staying unchanged. For example, if the old critical limit was
> + 80 degrees C, and the hysteresis was 75 degrees C, and you change the
> + critical limit to 90 degrees C, then the hysteresis will automatically
> + change to 85 degrees C.
> +* The hysteresis values can't be set independently. We decided to make
> + only tempX_crit_hyst writable, while all other hysteresis attributes
> + are read-only. Setting tempX_crit_hyst writes the difference between
> + tempX_crit_hyst and tempX_crit into the chip, and the same relative
> + hysteresis applies automatically to all other limits.
> +* The limits should be set before the hysteresis. At power up the device
> + starts with a 10 degree written into hysteresis register.
...
> --- /dev/null
> +++ b/drivers/hwmon/emc1812.c
...
> +
> +static int emc1812_chip_identify(struct emc1812_data *data, struct i2c_client *client)
> +{
> + const struct emc1812_features *chip;
> + struct device *dev = &client->dev;
> + int ret, tmp;
> +
> + ret = regmap_read(data->regmap, EMC1812_PRODUCT_ID_ADDR, &tmp);
> + if (ret)
> + return ret;
> +
> + chip = device_get_match_data(&client->dev);
> +
> + switch (tmp) {
> + case EMC1812_PID:
> + data->chip = &emc1812_chip_config;
> + break;
> + case EMC1813_PID:
> + data->chip = &emc1813_chip_config;
> + break;
> + case EMC1814_PID:
> + data->chip = &emc1814_chip_config;
> + break;
> + case EMC1815_PID:
> + data->chip = &emc1815_chip_config;
> + break;
> + case EMC1833_PID:
> + data->chip = &emc1833_chip_config;
> + break;
> + default:
> + /*
> + * If failed to identify the hardware based on internal registers,
> + * try using fallback compatible in device tree to deal with some
> + * newer part number.
> + */
> + dev_info(dev, "Unknown hardware id: %x\n", tmp);
dev_warn() might be more appropriate. Alternatively, use dev_err() and bail out
with -ENODEV, as suggested below.
> +
> + data->chip = chip;
> +
AI feedback:
Potential NULL pointer dereference. `device_get_match_data()` returns NULL
when the driver is instantiated via I2C ID table (e.g. via sysfs
`new_device`) instead of via Device Tree or ACPI. If the hardware ID
is also unknown (triggering the `default` case), `data->chip` becomes NULL.
The function returns 0 (success), and `emc1812_probe` proceeds to call
`emc1812_parse_fw_config`, which immediately dereferences `data->chip`:
Side note (not from AI): It might be appropriate to notify the user
if the devicetree data does not match the real hardware.
> +static int emc1812_parse_fw_config(struct emc1812_data *data, struct device *dev)
> +{
> + /* to be able to load the driver in case we don't have device tree */
> + if (!dev_fwnode(dev)) {
> + data->active_ch_mask = BIT(data->chip->phys_channels) - 1;
> + return 0;
> + }
This will crash.
The driver should likely use `i2c_get_match_data(client)` instead of
`device_get_match_data()` to support both ID table and FW node matching,
or return an error in the default case if `chip` is NULL.
> + return 0;
> + }
> +
> + return 0;
> +}
> +
Powered by blists - more mailing lists