[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