[PATCH 0/3] hwmon: lenovo-ec-sensors: Probe error handling fixes

[PATCH 0/3] hwmon: lenovo-ec-sensors: Probe error handling fixes

[Kean]

This series fixes several bugs in the probe() error handling path of
the lenovo-ec-sensors driver found during code review.

Patch 1 fixes a logic error in the EC signature check where && was
used instead of ||, causing the signature verification to be
effectively bypassed.

Patch 2 adds a missing NULL pointer check for dmi_first_match(),
which can return NULL on unsupported platforms.

Patch 3 converts manual request_region()/release_region() to the
devm-managed variant, fixing a double-release and a resource leak
in the probe error paths.

Kean (3):
  hwmon: lenovo-ec-sensors: Fix EC signature check logic in probe
  hwmon: lenovo-ec-sensors: Fix NULL pointer dereference when DMI match
    fails
  hwmon: lenovo-ec-sensors: Use devm_request_region for automatic
    cleanup

 drivers/hwmon/lenovo-ec-sensors.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

-- 
2.47.3

[PATCH 1/3] hwmon: lenovo-ec-sensors: Fix EC signature check logic in probe

[Kean]

The probe function reads a 4-byte signature ("MCHP") from the EC to
verify it is a Microchip EC before binding the driver. The condition
uses && (AND) to check if each byte differs from the expected value:

  if ((byte0 != 'M') && (byte1 != 'C') && (byte2 != 'H') && (byte3 != 'P'))

This rejects the device only if ALL four bytes are wrong simultaneously.
A partially matching signature (e.g. "MXXX") would pass the check and
cause the driver to bind to a non-Microchip EC, as long as at least one
byte matches the expected value.

Change && to || so the driver is rejected when ANY byte does not match
the expected "MCHP" signature.

Signed-off-by: Kean <rh_king@163.com>

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
 drivers/hwmon/lenovo-ec-sensors.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
index 8681bbf6665b..a32b1f2c6a3a 100644
--- a/drivers/hwmon/lenovo-ec-sensors.c
+++ b/drivers/hwmon/lenovo-ec-sensors.c
@@ -537,9 +537,9 @@ static int lenovo_ec_probe(struct platform_device *pdev)
 	outw_p(MCHP_SING_IDX, MCHP_EMI0_EC_ADDRESS);
 	mutex_unlock(&ec_data->mec_mutex);
 
-	if ((inb_p(MCHP_EMI0_EC_DATA_BYTE0) != 'M') &&
-	    (inb_p(MCHP_EMI0_EC_DATA_BYTE1) != 'C') &&
-	    (inb_p(MCHP_EMI0_EC_DATA_BYTE2) != 'H') &&
+	if ((inb_p(MCHP_EMI0_EC_DATA_BYTE0) != 'M') || 
+	    (inb_p(MCHP_EMI0_EC_DATA_BYTE1) != 'C') || 
+	    (inb_p(MCHP_EMI0_EC_DATA_BYTE2) != 'H') ||
 	    (inb_p(MCHP_EMI0_EC_DATA_BYTE3) != 'P')) {
 		release_region(IO_REGION_START, IO_REGION_LENGTH);
 		return -ENODEV;
-- 
2.47.3

[PATCH 2/3] hwmon: lenovo-ec-sensors: Fix NULL pointer dereference when DMI match fails

[Kean]

dmi_first_match() returns NULL if the running system does not match any
entry in thinkstation_dmi_table. Without a NULL check, the subsequent
dmi_id->driver_data access dereferences a NULL pointer, causing a kernel
oops or panic.

Add a NULL check and return -ENODEV to gracefully fail the probe when
the driver is loaded on an unsupported platform.

Signed-off-by: Kean <rh_king@163.com>

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
 drivers/hwmon/lenovo-ec-sensors.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
index a32b1f2c6a3a..b0f2a04ce679 100644
--- a/drivers/hwmon/lenovo-ec-sensors.c
+++ b/drivers/hwmon/lenovo-ec-sensors.c
@@ -546,6 +546,8 @@ static int lenovo_ec_probe(struct platform_device *pdev)
 	}
 
 	dmi_id = dmi_first_match(thinkstation_dmi_table);
+	if (!dmi_id)
+		return -ENODEV;
 
 	switch ((long)dmi_id->driver_data) {
 	case 0:
-- 
2.47.3

[PATCH 3/3] hwmon: lenovo-ec-sensors: Use devm_request_region for automatic cleanup

[Kean]

Replace manual request_region()/release_region() with
devm_request_region(). This lets the device-managed framework
handle I/O region lifetime automatically and fixes:

- A double release_region() when probe fails after acquiring the
  I/O region: the probe error path releases it, and then
  lenovo_ec_init() releases it again on the same error path.

- A release-after-free in lenovo_ec_exit() where release_region()
  was called after platform_device_unregister(), which has already
  released the I/O region via the platform device removal path.

- Missing release_region() in lenovo_ec_probe() on the DMI match
  failure path, which leaked the I/O region.

Remove all manual release_region() calls that are now handled
automatically by the devm framework.

Signed-off-by: Kean <rh_king@163.com>

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
 drivers/hwmon/lenovo-ec-sensors.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
index b0f2a04ce679..ea74bddbad5a 100644
--- a/drivers/hwmon/lenovo-ec-sensors.c
+++ b/drivers/hwmon/lenovo-ec-sensors.c
@@ -519,8 +519,8 @@ static int lenovo_ec_probe(struct platform_device *pdev)
 	if (!ec_data)
 		return -ENOMEM;
 
-	if (!request_region(IO_REGION_START, IO_REGION_LENGTH, "LNV-WKS")) {
-		pr_err(":request fail\n");
+	if (!devm_request_region(dev, IO_REGION_START, IO_REGION_LENGTH, "LNV-WKS")) {
+		dev_err(dev, "Failed to request I/O region.\n");
 		return -EIO;
 	}
 
@@ -541,7 +541,6 @@ static int lenovo_ec_probe(struct platform_device *pdev)
 	    (inb_p(MCHP_EMI0_EC_DATA_BYTE1) != 'C') || 
 	    (inb_p(MCHP_EMI0_EC_DATA_BYTE2) != 'H') ||
 	    (inb_p(MCHP_EMI0_EC_DATA_BYTE3) != 'P')) {
-		release_region(IO_REGION_START, IO_REGION_LENGTH);
 		return -ENODEV;
 	}
 
@@ -579,7 +578,8 @@ static int lenovo_ec_probe(struct platform_device *pdev)
 		lenovo_ec_chip_info.info = lenovo_ec_hwmon_info_p8;
 		break;
 	default:
-		release_region(IO_REGION_START, IO_REGION_LENGTH);
+		dev_err(dev, "Unsupported platform type %ld\n",
+			(long)dmi_id->driver_data);
 		return -ENODEV;
 	}
 
@@ -608,10 +608,8 @@ static int __init lenovo_ec_init(void)
 		platform_create_bundle(&lenovo_ec_sensors_platform_driver,
 				       lenovo_ec_probe, NULL, 0, NULL, 0);
 
-	if (IS_ERR(lenovo_ec_sensors_platform_device)) {
-		release_region(IO_REGION_START, IO_REGION_LENGTH);
+	if (IS_ERR(lenovo_ec_sensors_platform_device)) 
 		return PTR_ERR(lenovo_ec_sensors_platform_device);
-	}
 
 	return 0;
 }
@@ -619,7 +617,6 @@ module_init(lenovo_ec_init);
 
 static void __exit lenovo_ec_exit(void)
 {
-	release_region(IO_REGION_START, IO_REGION_LENGTH);
 	platform_device_unregister(lenovo_ec_sensors_platform_device);
 	platform_driver_unregister(&lenovo_ec_sensors_platform_driver);
 }
-- 
2.47.3

Re: [PATCH 2/3] hwmon: lenovo-ec-sensors: Fix NULL pointer dereference when DMI match fails

[Guenter Roeck]

On 5/13/26 18:14, Kean wrote:
> dmi_first_match() returns NULL if the running system does not match any
> entry in thinkstation_dmi_table. Without a NULL check, the subsequent
> dmi_id->driver_data access dereferences a NULL pointer, causing a kernel
> oops or panic.
> 
> Add a NULL check and return -ENODEV to gracefully fail the probe when
> the driver is loaded on an unsupported platform.
> 

How would that happen in practice ? The driver init code has

         if (!dmi_check_system(thinkstation_dmi_table))
                 return -ENODEV;

Please provide a reproducer.

Thanks,
Guenter

Re: [PATCH 3/3] hwmon: lenovo-ec-sensors: Use devm_request_region for automatic cleanup

[Guenter Roeck]

On 5/13/26 18:14, Kean wrote:
> Replace manual request_region()/release_region() with
> devm_request_region(). This lets the device-managed framework
> handle I/O region lifetime automatically and fixes:
> 
> - A double release_region() when probe fails after acquiring the
>    I/O region: the probe error path releases it, and then
>    lenovo_ec_init() releases it again on the same error path.
> 
> - A release-after-free in lenovo_ec_exit() where release_region()
>    was called after platform_device_unregister(), which has already
>    released the I/O region via the platform device removal path.
> 
> - Missing release_region() in lenovo_ec_probe() on the DMI match
>    failure path, which leaked the I/O region.
> 
> Remove all manual release_region() calls that are now handled
> automatically by the devm framework.
> 
> Signed-off-by: Kean <rh_king@163.com>
> 
Why this empty line ?

> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>

How do I know that this review really happened ?

> ---
>   drivers/hwmon/lenovo-ec-sensors.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
> index b0f2a04ce679..ea74bddbad5a 100644
> --- a/drivers/hwmon/lenovo-ec-sensors.c
> +++ b/drivers/hwmon/lenovo-ec-sensors.c
> @@ -519,8 +519,8 @@ static int lenovo_ec_probe(struct platform_device *pdev)
>   	if (!ec_data)
>   		return -ENOMEM;
>   
> -	if (!request_region(IO_REGION_START, IO_REGION_LENGTH, "LNV-WKS")) {
> -		pr_err(":request fail\n");
> +	if (!devm_request_region(dev, IO_REGION_START, IO_REGION_LENGTH, "LNV-WKS")) {
> +		dev_err(dev, "Failed to request I/O region.\n");
>   		return -EIO;
>   	}
>   
> @@ -541,7 +541,6 @@ static int lenovo_ec_probe(struct platform_device *pdev)
>   	    (inb_p(MCHP_EMI0_EC_DATA_BYTE1) != 'C') ||
>   	    (inb_p(MCHP_EMI0_EC_DATA_BYTE2) != 'H') ||
>   	    (inb_p(MCHP_EMI0_EC_DATA_BYTE3) != 'P')) {
> -		release_region(IO_REGION_START, IO_REGION_LENGTH);
>   		return -ENODEV;
>   	}
>   
> @@ -579,7 +578,8 @@ static int lenovo_ec_probe(struct platform_device *pdev)
>   		lenovo_ec_chip_info.info = lenovo_ec_hwmon_info_p8;
>   		break;
>   	default:
> -		release_region(IO_REGION_START, IO_REGION_LENGTH);
> +		dev_err(dev, "Unsupported platform type %ld\n",
> +			(long)dmi_id->driver_data);

This is not documented in the commit message and, on top of that, pointless.
It isn't even noise, it is just pointless (the default case can not be reached).

>   		return -ENODEV;
>   	}
>   
> @@ -608,10 +608,8 @@ static int __init lenovo_ec_init(void)
>   		platform_create_bundle(&lenovo_ec_sensors_platform_driver,
>   				       lenovo_ec_probe, NULL, 0, NULL, 0);
>   
> -	if (IS_ERR(lenovo_ec_sensors_platform_device)) {
> -		release_region(IO_REGION_START, IO_REGION_LENGTH);
> +	if (IS_ERR(lenovo_ec_sensors_platform_device))
>   		return PTR_ERR(lenovo_ec_sensors_platform_device);
> -	}
>   
>   	return 0;
>   }
> @@ -619,7 +617,6 @@ module_init(lenovo_ec_init);
>   
>   static void __exit lenovo_ec_exit(void)
>   {
> -	release_region(IO_REGION_START, IO_REGION_LENGTH);
>   	platform_device_unregister(lenovo_ec_sensors_platform_device);
>   	platform_driver_unregister(&lenovo_ec_sensors_platform_driver);
>   }

Re: [PATCH 1/3] hwmon: lenovo-ec-sensors: Fix EC signature check logic in probe

[Guenter Roeck]

On 5/13/26 18:14, Kean wrote:
> The probe function reads a 4-byte signature ("MCHP") from the EC to
> verify it is a Microchip EC before binding the driver. The condition
> uses && (AND) to check if each byte differs from the expected value:
> 
>    if ((byte0 != 'M') && (byte1 != 'C') && (byte2 != 'H') && (byte3 != 'P'))
> 
> This rejects the device only if ALL four bytes are wrong simultaneously.
> A partially matching signature (e.g. "MXXX") would pass the check and
> cause the driver to bind to a non-Microchip EC, as long as at least one
> byte matches the expected value.
> 
> Change && to || so the driver is rejected when ANY byte does not match
> the expected "MCHP" signature.
> 
> Signed-off-by: Kean <rh_king@163.com>
> 
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>

As with the other patches: How do I know that this review really happened ?

Guenter

> ---
>   drivers/hwmon/lenovo-ec-sensors.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
> index 8681bbf6665b..a32b1f2c6a3a 100644
> --- a/drivers/hwmon/lenovo-ec-sensors.c
> +++ b/drivers/hwmon/lenovo-ec-sensors.c
> @@ -537,9 +537,9 @@ static int lenovo_ec_probe(struct platform_device *pdev)
>   	outw_p(MCHP_SING_IDX, MCHP_EMI0_EC_ADDRESS);
>   	mutex_unlock(&ec_data->mec_mutex);
>   
> -	if ((inb_p(MCHP_EMI0_EC_DATA_BYTE0) != 'M') &&
> -	    (inb_p(MCHP_EMI0_EC_DATA_BYTE1) != 'C') &&
> -	    (inb_p(MCHP_EMI0_EC_DATA_BYTE2) != 'H') &&
> +	if ((inb_p(MCHP_EMI0_EC_DATA_BYTE0) != 'M') ||
> +	    (inb_p(MCHP_EMI0_EC_DATA_BYTE1) != 'C') ||
> +	    (inb_p(MCHP_EMI0_EC_DATA_BYTE2) != 'H') ||
>   	    (inb_p(MCHP_EMI0_EC_DATA_BYTE3) != 'P')) {
>   		release_region(IO_REGION_START, IO_REGION_LENGTH);
>   		return -ENODEV;

Re: [PATCH 3/3] hwmon: lenovo-ec-sensors: Use devm_request_region for automatic cleanup

[Mark Pearson]

Hi Guenter,

On Wed, May 13, 2026, at 9:36 PM, Guenter Roeck wrote:
> On 5/13/26 18:14, Kean wrote:
>> Replace manual request_region()/release_region() with
>> devm_request_region(). This lets the device-managed framework
>> handle I/O region lifetime automatically and fixes:
>> 
>> - A double release_region() when probe fails after acquiring the
>>    I/O region: the probe error path releases it, and then
>>    lenovo_ec_init() releases it again on the same error path.
>> 
>> - A release-after-free in lenovo_ec_exit() where release_region()
>>    was called after platform_device_unregister(), which has already
>>    released the I/O region via the platform device removal path.
>> 
>> - Missing release_region() in lenovo_ec_probe() on the DMI match
>>    failure path, which leaked the I/O region.
>> 
>> Remove all manual release_region() calls that are now handled
>> automatically by the devm framework.
>> 
>> Signed-off-by: Kean <rh_king@163.com>
>> 
> Why this empty line ?
>
>> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>
> How do I know that this review really happened ?

The reviewed by's are correct - I can confirm I did review these patches before Kean pushed them upstream. We did some internal review first to discuss the issues he identified.

We sometimes take this approach with the platform/x86 patches. We can do it separately next time if preferred here.

Mark

>
>> ---
>>   drivers/hwmon/lenovo-ec-sensors.c | 13 +++++--------
>>   1 file changed, 5 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
>> index b0f2a04ce679..ea74bddbad5a 100644
>> --- a/drivers/hwmon/lenovo-ec-sensors.c
>> +++ b/drivers/hwmon/lenovo-ec-sensors.c
>> @@ -519,8 +519,8 @@ static int lenovo_ec_probe(struct platform_device *pdev)
>>   	if (!ec_data)
>>   		return -ENOMEM;
>>   
>> -	if (!request_region(IO_REGION_START, IO_REGION_LENGTH, "LNV-WKS")) {
>> -		pr_err(":request fail\n");
>> +	if (!devm_request_region(dev, IO_REGION_START, IO_REGION_LENGTH, "LNV-WKS")) {
>> +		dev_err(dev, "Failed to request I/O region.\n");
>>   		return -EIO;
>>   	}
>>   
>> @@ -541,7 +541,6 @@ static int lenovo_ec_probe(struct platform_device *pdev)
>>   	    (inb_p(MCHP_EMI0_EC_DATA_BYTE1) != 'C') ||
>>   	    (inb_p(MCHP_EMI0_EC_DATA_BYTE2) != 'H') ||
>>   	    (inb_p(MCHP_EMI0_EC_DATA_BYTE3) != 'P')) {
>> -		release_region(IO_REGION_START, IO_REGION_LENGTH);
>>   		return -ENODEV;
>>   	}
>>   
>> @@ -579,7 +578,8 @@ static int lenovo_ec_probe(struct platform_device *pdev)
>>   		lenovo_ec_chip_info.info = lenovo_ec_hwmon_info_p8;
>>   		break;
>>   	default:
>> -		release_region(IO_REGION_START, IO_REGION_LENGTH);
>> +		dev_err(dev, "Unsupported platform type %ld\n",
>> +			(long)dmi_id->driver_data);
>
> This is not documented in the commit message and, on top of that, pointless.
> It isn't even noise, it is just pointless (the default case can not be reached).
>
>>   		return -ENODEV;
>>   	}
>>   
>> @@ -608,10 +608,8 @@ static int __init lenovo_ec_init(void)
>>   		platform_create_bundle(&lenovo_ec_sensors_platform_driver,
>>   				       lenovo_ec_probe, NULL, 0, NULL, 0);
>>   
>> -	if (IS_ERR(lenovo_ec_sensors_platform_device)) {
>> -		release_region(IO_REGION_START, IO_REGION_LENGTH);
>> +	if (IS_ERR(lenovo_ec_sensors_platform_device))
>>   		return PTR_ERR(lenovo_ec_sensors_platform_device);
>> -	}
>>   
>>   	return 0;
>>   }
>> @@ -619,7 +617,6 @@ module_init(lenovo_ec_init);
>>   
>>   static void __exit lenovo_ec_exit(void)
>>   {
>> -	release_region(IO_REGION_START, IO_REGION_LENGTH);
>>   	platform_device_unregister(lenovo_ec_sensors_platform_device);
>>   	platform_driver_unregister(&lenovo_ec_sensors_platform_driver);
>>   }

Re: [PATCH 1/3] hwmon: lenovo-ec-sensors: Fix EC signature check logic in probe

[Mark Pearson]

Hi Guenter,

On Wed, May 13, 2026, at 9:37 PM, Guenter Roeck wrote:
> On 5/13/26 18:14, Kean wrote:
>> The probe function reads a 4-byte signature ("MCHP") from the EC to
>> verify it is a Microchip EC before binding the driver. The condition
>> uses && (AND) to check if each byte differs from the expected value:
>> 
>>    if ((byte0 != 'M') && (byte1 != 'C') && (byte2 != 'H') && (byte3 != 'P'))
>> 
>> This rejects the device only if ALL four bytes are wrong simultaneously.
>> A partially matching signature (e.g. "MXXX") would pass the check and
>> cause the driver to bind to a non-Microchip EC, as long as at least one
>> byte matches the expected value.
>> 
>> Change && to || so the driver is rejected when ANY byte does not match
>> the expected "MCHP" signature.
>> 
>> Signed-off-by: Kean <rh_king@163.com>
>> 
>> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>
> As with the other patches: How do I know that this review really happened ?

I confirm I did it.
For the series:
Reviewed-by: Mark Pearson <mpearson@lenovo@squebb.ca>

Mark

>
> Guenter
>
>> ---
>>   drivers/hwmon/lenovo-ec-sensors.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
>> index 8681bbf6665b..a32b1f2c6a3a 100644
>> --- a/drivers/hwmon/lenovo-ec-sensors.c
>> +++ b/drivers/hwmon/lenovo-ec-sensors.c
>> @@ -537,9 +537,9 @@ static int lenovo_ec_probe(struct platform_device *pdev)
>>   	outw_p(MCHP_SING_IDX, MCHP_EMI0_EC_ADDRESS);
>>   	mutex_unlock(&ec_data->mec_mutex);
>>   
>> -	if ((inb_p(MCHP_EMI0_EC_DATA_BYTE0) != 'M') &&
>> -	    (inb_p(MCHP_EMI0_EC_DATA_BYTE1) != 'C') &&
>> -	    (inb_p(MCHP_EMI0_EC_DATA_BYTE2) != 'H') &&
>> +	if ((inb_p(MCHP_EMI0_EC_DATA_BYTE0) != 'M') ||
>> +	    (inb_p(MCHP_EMI0_EC_DATA_BYTE1) != 'C') ||
>> +	    (inb_p(MCHP_EMI0_EC_DATA_BYTE2) != 'H') ||
>>   	    (inb_p(MCHP_EMI0_EC_DATA_BYTE3) != 'P')) {
>>   		release_region(IO_REGION_START, IO_REGION_LENGTH);
>>   		return -ENODEV;

Re: [PATCH 3/3] hwmon: lenovo-ec-sensors: Use devm_request_region for automatic cleanup

[Guenter Roeck]

On 5/13/26 19:39, Mark Pearson wrote:
> Hi Guenter,
> 
> On Wed, May 13, 2026, at 9:36 PM, Guenter Roeck wrote:
>> On 5/13/26 18:14, Kean wrote:
>>> Replace manual request_region()/release_region() with
>>> devm_request_region(). This lets the device-managed framework
>>> handle I/O region lifetime automatically and fixes:
>>>
>>> - A double release_region() when probe fails after acquiring the
>>>     I/O region: the probe error path releases it, and then
>>>     lenovo_ec_init() releases it again on the same error path.
>>>
>>> - A release-after-free in lenovo_ec_exit() where release_region()
>>>     was called after platform_device_unregister(), which has already
>>>     released the I/O region via the platform device removal path.
>>>
>>> - Missing release_region() in lenovo_ec_probe() on the DMI match
>>>     failure path, which leaked the I/O region.
>>>
>>> Remove all manual release_region() calls that are now handled
>>> automatically by the devm framework.
>>>
>>> Signed-off-by: Kean <rh_king@163.com>
>>>
>> Why this empty line ?
>>
>>> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>>
>> How do I know that this review really happened ?
> 
> The reviewed by's are correct - I can confirm I did review these patches before Kean pushed them upstream. We did some internal review first to discuss the issues he identified.
> 
> We sometimes take this approach with the platform/x86 patches. We can do it separately next time if preferred here.
> 

Kean <rh_king@163.com> is not a full name, has no patches in the upstream
kernel or in linux-next, and otherwise seems to be a complete unknown.
How do you expect me to know that this isn't all made up by some AI ?

Anyway, when you do that, please at least ask people to run checkpatch.
I really don't want to have to deal with trivial issues such as

ERROR: trailing whitespace
#135: FILE: drivers/hwmon/lenovo-ec-sensors.c:611:
+^Iif (IS_ERR(lenovo_ec_sensors_platform_device)) $

in this patch and the first patch of the series.

Guenter

> Mark
> 
>>
>>> ---
>>>    drivers/hwmon/lenovo-ec-sensors.c | 13 +++++--------
>>>    1 file changed, 5 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
>>> index b0f2a04ce679..ea74bddbad5a 100644
>>> --- a/drivers/hwmon/lenovo-ec-sensors.c
>>> +++ b/drivers/hwmon/lenovo-ec-sensors.c
>>> @@ -519,8 +519,8 @@ static int lenovo_ec_probe(struct platform_device *pdev)
>>>    	if (!ec_data)
>>>    		return -ENOMEM;
>>>    
>>> -	if (!request_region(IO_REGION_START, IO_REGION_LENGTH, "LNV-WKS")) {
>>> -		pr_err(":request fail\n");
>>> +	if (!devm_request_region(dev, IO_REGION_START, IO_REGION_LENGTH, "LNV-WKS")) {
>>> +		dev_err(dev, "Failed to request I/O region.\n");
>>>    		return -EIO;
>>>    	}
>>>    
>>> @@ -541,7 +541,6 @@ static int lenovo_ec_probe(struct platform_device *pdev)
>>>    	    (inb_p(MCHP_EMI0_EC_DATA_BYTE1) != 'C') ||
>>>    	    (inb_p(MCHP_EMI0_EC_DATA_BYTE2) != 'H') ||
>>>    	    (inb_p(MCHP_EMI0_EC_DATA_BYTE3) != 'P')) {
>>> -		release_region(IO_REGION_START, IO_REGION_LENGTH);
>>>    		return -ENODEV;
>>>    	}
>>>    
>>> @@ -579,7 +578,8 @@ static int lenovo_ec_probe(struct platform_device *pdev)
>>>    		lenovo_ec_chip_info.info = lenovo_ec_hwmon_info_p8;
>>>    		break;
>>>    	default:
>>> -		release_region(IO_REGION_START, IO_REGION_LENGTH);
>>> +		dev_err(dev, "Unsupported platform type %ld\n",
>>> +			(long)dmi_id->driver_data);
>>
>> This is not documented in the commit message and, on top of that, pointless.
>> It isn't even noise, it is just pointless (the default case can not be reached).
>>
>>>    		return -ENODEV;
>>>    	}
>>>    
>>> @@ -608,10 +608,8 @@ static int __init lenovo_ec_init(void)
>>>    		platform_create_bundle(&lenovo_ec_sensors_platform_driver,
>>>    				       lenovo_ec_probe, NULL, 0, NULL, 0);
>>>    
>>> -	if (IS_ERR(lenovo_ec_sensors_platform_device)) {
>>> -		release_region(IO_REGION_START, IO_REGION_LENGTH);
>>> +	if (IS_ERR(lenovo_ec_sensors_platform_device))
>>>    		return PTR_ERR(lenovo_ec_sensors_platform_device);
>>> -	}
>>>    
>>>    	return 0;
>>>    }
>>> @@ -619,7 +617,6 @@ module_init(lenovo_ec_init);
>>>    
>>>    static void __exit lenovo_ec_exit(void)
>>>    {
>>> -	release_region(IO_REGION_START, IO_REGION_LENGTH);
>>>    	platform_device_unregister(lenovo_ec_sensors_platform_device);
>>>    	platform_driver_unregister(&lenovo_ec_sensors_platform_driver);
>>>    }
> 

Re: [PATCH 2/3] hwmon: lenovo-ec-sensors: Fix NULL pointer dereference when DMI match fails

[Guenter Roeck]

On Thu, May 14, 2026 at 09:14:10AM +0800, Kean wrote:
> dmi_first_match() returns NULL if the running system does not match any
> entry in thinkstation_dmi_table. Without a NULL check, the subsequent
> dmi_id->driver_data access dereferences a NULL pointer, causing a kernel
> oops or panic.
> 
> Add a NULL check and return -ENODEV to gracefully fail the probe when
> the driver is loaded on an unsupported platform.
> 
> Signed-off-by: Kean <rh_king@163.com>
> 
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>

ERROR: trailing whitespace
#104: FILE: drivers/hwmon/lenovo-ec-sensors.c:540:
+^Iif ((inb_p(MCHP_EMI0_EC_DATA_BYTE0) != 'M') || $

ERROR: trailing whitespace
#105: FILE: drivers/hwmon/lenovo-ec-sensors.c:541:
+^I    (inb_p(MCHP_EMI0_EC_DATA_BYTE1) != 'C') || $

total: 2 errors, 0 warnings, 0 checks, 12 lines checked

> ---
>  drivers/hwmon/lenovo-ec-sensors.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
> index a32b1f2c6a3a..b0f2a04ce679 100644
> --- a/drivers/hwmon/lenovo-ec-sensors.c
> +++ b/drivers/hwmon/lenovo-ec-sensors.c
> @@ -546,6 +546,8 @@ static int lenovo_ec_probe(struct platform_device *pdev)
>  	}
>  
>  	dmi_id = dmi_first_match(thinkstation_dmi_table);
> +	if (!dmi_id)
> +		return -ENODEV;
>  
>  	switch ((long)dmi_id->driver_data) {
>  	case 0:

Re: [PATCH 2/3] hwmon: lenovo-ec-sensors: Fix NULL pointer dereference when DMI match fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly added error path for DMI match failure introduces an I/O resource leak.
--

commit 6d1770785fa851d42148760f539b79225f6680d7
Author: Kean <rh_king@163.com>

hwmon: lenovo-ec-sensors: Fix NULL pointer dereference when DMI match fails

This commit adds a check to ensure dmi_first_match() does not return NULL
before accessing its driver_data. If no match is found, the probe function
now gracefully returns -ENODEV to prevent a kernel oops.

> diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
> index 6cd83c378f091..10fc0376cb7f7 100644
> --- a/drivers/hwmon/lenovo-ec-sensors.c
> +++ b/drivers/hwmon/lenovo-ec-sensors.c

[ ... ]

> @@ -546,6 +546,8 @@ static int lenovo_ec_probe(struct platform_device *pdev)
>  	}
>  
>  	dmi_id = dmi_first_match(thinkstation_dmi_table);
> +	if (!dmi_id)
> +		return -ENODEV;

Does this new error path leak the I/O region requested earlier in the probe
function?

Looking earlier in lenovo_ec_probe(), request_region() is called to reserve
the I/O ports:

drivers/hwmon/lenovo-ec-sensors.c:lenovo_ec_probe() {
        ...
        if (!request_region(IO_REGION_START, IO_REGION_LENGTH, "LNV-WKS")) {
                pr_err(":request fail\n");
                return -EIO;
        }
        ...
}

Other error paths in the function explicitly call release_region() before
returning, but this newly added return does not.

I note that this issue is resolved later in the series by the commit
"hwmon: lenovo-ec-sensors: Use devm_request_region for automatic cleanup".

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514011411.4167069-1-rh_king@163.com?part=2

Re: [PATCH 3/3] hwmon: lenovo-ec-sensors: Use devm_request_region for automatic cleanup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Commit message inaccurately claims to fix non-existent 'release-after-free' and 'leak' bugs.
--

commit 0f5c675a52c38754efb1c95225e331f855159802
Author: Kean <rh_king@163.com>

hwmon: lenovo-ec-sensors: Use devm_request_region for automatic cleanup

This patch replaces manual request_region and release_region calls with
devm_request_region. It delegates the I/O region lifetime management to
the device-managed framework.

> - A release-after-free in lenovo_ec_exit() where release_region()
>   was called after platform_device_unregister(), which has already
>   released the I/O region via the platform device removal path.

Is this description accurate? Looking at the original lenovo_ec_exit(),
release_region() was actually called before platform_device_unregister():

static void __exit lenovo_ec_exit(void)
{
    release_region(IO_REGION_START, IO_REGION_LENGTH);
    platform_device_unregister(lenovo_ec_sensors_platform_device);
    platform_driver_unregister(&lenovo_ec_sensors_platform_driver);
}

Additionally, platform_device_unregister() does not automatically free
manually requested I/O regions. Could the commit message be updated to
reflect the historical behavior?

> - Missing release_region() in lenovo_ec_probe() on the DMI match
>   failure path, which leaked the I/O region.

Did this path actually leak the I/O region? If the probe fails due to a
DMI match failure, the error propagates back up to lenovo_ec_init().

Looking at the original lenovo_ec_init() error handling:

if (IS_ERR(lenovo_ec_sensors_platform_device)) {
    release_region(IO_REGION_START, IO_REGION_LENGTH);
    return PTR_ERR(lenovo_ec_sensors_platform_device);
}

Since lenovo_ec_init() caught the ERR_PTR and correctly called
release_region(), it appears no leak occurred on this path. Is there
another path being referenced here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514011411.4167069-1-rh_king@163.com?part=3

Re: [PATCH 2/3] hwmon: lenovo-ec-sensors: Fix NULL pointer dereference when DMI match fails

[Kean]

Hi Guenter,
Thank you for the review and for pointing this out!

You're absolutely right. I realize now that my patch was overly
cautious — in normal operation dmi_first_match() can never return
NULL here because lenovo_ec_init() already guards the probe behind:

	static int __init lenovo_ec_init(void)
	{
		if (!dmi_check_system(thinkstation_dmi_table))
			return -ENODEV;
		...
	}

That said, I tend to follow a defensive programming style — checking
for errors and returning early whenever something looks even slightly
unexpected. This is exactly what lenovo_ec_init() itself does with
dmi_check_system(), and it's also why we often put a return (or break)
in the default branch of a switch statement. So I added the NULL check
for dmi_first_match() as an extra sanity guard, even though logically
it should never trigger.

I should have made this clearer in the commit message. The patch was
meant as a defensive sanity check, but my description made it sound
like an actual reachable bug, which it isn't. That's my mistake.

I'm happy to drop this patch from the series if you'd prefer. Please
let me know how you'd like me to proceed.

For other parts and the format issues is my mistake that missed the 
--strict to check the patches file, I will send the V2 version, hope
 get your review, any problem you can tell me, I will feedback and 
tested as your requested.

Thanks,
Kean

Re: [PATCH 2/3] hwmon: lenovo-ec-sensors: Fix NULL pointer dereference when DMI match fails

[Kean]

Hi Guenter,

Just to follow up — if we drop patch 2, patches 1 and 3 remain
independent and should apply cleanly. They don't depend on the
NULL check in any way.

If you have any other concerns or requests for those two patches,
please let me know. I'll address them and send a v2 with just
those two for your review.

>>>    	default:
>>> -		release_region(IO_REGION_START, IO_REGION_LENGTH);
>>> +		dev_err(dev, "Unsupported platform type %ld\n",
>>> +			(long)dmi_id->driver_data);

This part I will remove as your comments is clear and will keep 
the orignal but 
release_region(IO_REGION_START, IO_REGION_LENGTH); still will
be remove for it works with other 2 patches.

Thanks,
Kean

Re: [PATCH 2/3] hwmon: lenovo-ec-sensors: Fix NULL pointer dereference when DMI match fails

[Guenter Roeck]

On 5/15/26 01:10, Kean wrote:
> Hi Guenter,
> Thank you for the review and for pointing this out!
> 
> You're absolutely right. I realize now that my patch was overly
> cautious — in normal operation dmi_first_match() can never return
> NULL here because lenovo_ec_init() already guards the probe behind:
> 
> 	static int __init lenovo_ec_init(void)
> 	{
> 		if (!dmi_check_system(thinkstation_dmi_table))
> 			return -ENODEV;
> 		...
> 	}
> 
> That said, I tend to follow a defensive programming style — checking
> for errors and returning early whenever something looks even slightly
> unexpected. This is exactly what lenovo_ec_init() itself does with
> dmi_check_system(), and it's also why we often put a return (or break)
> in the default branch of a switch statement. So I added the NULL check
> for dmi_first_match() as an extra sanity guard, even though logically
> it should never trigger.
> 

Maybe other subsystems accept that nowadays. Historically it was considered
waste. I still consider it waste, and I won't accept it.

Guenter

> I should have made this clearer in the commit message. The patch was
> meant as a defensive sanity check, but my description made it sound
> like an actual reachable bug, which it isn't. That's my mistake.
> 
> I'm happy to drop this patch from the series if you'd prefer. Please
> let me know how you'd like me to proceed.
> 
> For other parts and the format issues is my mistake that missed the
> --strict to check the patches file, I will send the V2 version, hope
>   get your review, any problem you can tell me, I will feedback and
> tested as your requested.
> 
> Thanks,
> Kean
> 
>