[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

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 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;

[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

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 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

[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

[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
> 
> 

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

[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

[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 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 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 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 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

[PATCH v2 0/2] hwmon: (lenovo-ec-sensors): Fix EC signature validation and I/O resource management

[Kean Ren]

Hi Guenter,

This is v2 of the lenovo-ec-sensors fix series.

The original v1 [1] included a combined patch for both the EC signature
fix and the I/O resource conversion, along with other driver fixes.
Based on your feedback, this v2 makes the following changes:

- Dropped the "Fix NULL pointer dereference" patch (previously patch 2).
- Keep the original case of default case, remove the dev_err.

Patch 1 converts manual request_region()/release_region() calls to
devm_request_region(), eliminating a double-release on the probe error
path and a release-after-use window on module exit.

Patch 2 fixes the EC "MCHP" signature check that used && instead of ||,
which caused the validation to accept a non-Microchip EC if any single
byte of the expected 4-byte signature happened to match.

Both patches apply cleanly against v7.1-rc3 and pass checkpatch with
0 errors, 0 warnings.

[1] https://sashiko.dev/#/patchset/20260514011411.4167069-1-rh_king@163.com

Kean (2):
  hwmon: (lenovo-ec-sensors): Convert to devm_request_region()
  hwmon: (lenovo-ec-sensors): Fix EC "MCHP" signature validation logic

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

-- 
2.53.0

[PATCH v2 1/2] hwmon: (lenovo-ec-sensors): Convert to devm_request_region()

[Kean Ren]

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-use window in lenovo_ec_exit() where
  release_region() was called before platform_device_unregister(),
  leaving the hwmon device active with a released I/O region.

- Missing release_region() in lenovo_ec_probe() if
  devm_hwmon_device_register_with_info() fails.

Remove all five manual release_region() calls that are now handled
automatically, and drop the unnecessary braces on the single-statement
blocks that previously contained them.

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Kean Ren <rh_king@163.com>
---
Changes in v2: Removed unnecessary error messages

 drivers/hwmon/lenovo-ec-sensors.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
index 8681bbf6665b..45db49e189d3 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;
 	}
 
@@ -577,7 +576,6 @@ 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);
 		return -ENODEV;
 	}
 
@@ -607,7 +605,6 @@ static int __init lenovo_ec_init(void)
 				       lenovo_ec_probe, NULL, 0, NULL, 0);
 
 	if (IS_ERR(lenovo_ec_sensors_platform_device)) {
-		release_region(IO_REGION_START, IO_REGION_LENGTH);
 		return PTR_ERR(lenovo_ec_sensors_platform_device);
 	}
 
@@ -617,7 +614,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.53.0

Re: [PATCH v2 1/2] hwmon: (lenovo-ec-sensors): Convert to devm_request_region()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The commit message inaccurately claims to remove five `release_region()` calls and drop unnecessary braces, but only four calls were removed and the braces were left intact.
--

commit e8c1e48be4d237988380d2577325024d4865618f
Author: Kean Ren <rh_king@163.com>

hwmon: (lenovo-ec-sensors): Convert to devm_request_region()

This commit replaces manual request_region() and release_region() calls
with devm_request_region() to allow the device-managed framework to
handle the I/O region lifetime automatically.

> Remove all five manual release_region() calls that are now handled
> automatically, and drop the unnecessary braces on the single-statement
> blocks that previously contained them.

[Severity: Medium]
Is this description accurate? The patch diff shows that only four
release_region() calls were removed.

> diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
> index 8681bbf6665b1..45db49e189d3d 100644
> --- a/drivers/hwmon/lenovo-ec-sensors.c
> +++ b/drivers/hwmon/lenovo-ec-sensors.c
> @@ -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;
>  	}

[Severity: Medium]
This isn't a bug, but could these curly braces be removed? The commit
message mentions dropping the unnecessary braces on the single-statement
blocks, but they appear to have been left intact here.

[ ... ]

> @@ -607,7 +605,6 @@ static int __init lenovo_ec_init(void)
>  				       lenovo_ec_probe, NULL, 0, NULL, 0);
>  
>  	if (IS_ERR(lenovo_ec_sensors_platform_device)) {
> -		release_region(IO_REGION_START, IO_REGION_LENGTH);
>  		return PTR_ERR(lenovo_ec_sensors_platform_device);
>  	}

[Severity: Medium]
Could the braces be removed from this block as well, to match the
description in the commit message?

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

Re: [PATCH v2 1/2] hwmon: (lenovo-ec-sensors): Convert to devm_request_region()

[Guenter Roeck]

On 5/19/26 19:51, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The commit message inaccurately claims to remove five `release_region()` calls and drop unnecessary braces, but only four calls were removed and the braces were left intact.
> --
> 
> commit e8c1e48be4d237988380d2577325024d4865618f
> Author: Kean Ren <rh_king@163.com>
> 
> hwmon: (lenovo-ec-sensors): Convert to devm_request_region()
> 
> This commit replaces manual request_region() and release_region() calls
> with devm_request_region() to allow the device-managed framework to
> handle the I/O region lifetime automatically.
> 
>> Remove all five manual release_region() calls that are now handled
>> automatically, and drop the unnecessary braces on the single-statement
>> blocks that previously contained them.
> 
> [Severity: Medium]
> Is this description accurate? The patch diff shows that only four
> release_region() calls were removed.
> 

As far as I can see there are only four calls in the code. Description
problem ?

>> diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
>> index 8681bbf6665b1..45db49e189d3d 100644
>> --- a/drivers/hwmon/lenovo-ec-sensors.c
>> +++ b/drivers/hwmon/lenovo-ec-sensors.c
>> @@ -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;
>>   	}
> 
> [Severity: Medium]
> This isn't a bug, but could these curly braces be removed? The commit
> message mentions dropping the unnecessary braces on the single-statement
> blocks, but they appear to have been left intact here.
> 

Hmm, yes, the description does not match the code changes. Please drop
the now unnecessary {}.

Thanks,
Guenter

> [ ... ]
> 
>> @@ -607,7 +605,6 @@ static int __init lenovo_ec_init(void)
>>   				       lenovo_ec_probe, NULL, 0, NULL, 0);
>>   
>>   	if (IS_ERR(lenovo_ec_sensors_platform_device)) {
>> -		release_region(IO_REGION_START, IO_REGION_LENGTH);
>>   		return PTR_ERR(lenovo_ec_sensors_platform_device);
>>   	}
> 
> [Severity: Medium]
> Could the braces be removed from this block as well, to match the
> description in the commit message?
> 

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

[Kean Ren]

Hi Guenter,

Thank you for the review!

>> Remove all five manual release_region() calls that are now handled
>> automatically, and drop the unnecessary braces on the single-statement
>> blocks that previously contained them.
>
> [Severity: Medium]
> Is this description accurate? The patch diff shows that only four
> release_region() calls were removed.
>
>>As far as I can see there are only four calls in the code. Description
>>problem ?
Yes, you are right, thanks for your point out, I will update it.

>> @@ -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;
>>   	}
>
> [Severity: Medium]
> This isn't a bug, but could these curly braces be removed? The commit
> message mentions dropping the unnecessary braces on the single-statement
> blocks, but they appear to have been left intact here.
>
>>Hmm, yes, the description does not match the code changes. Please drop
>>the now unnecessary {}.
Thanks, you are right, it is unnecessary {}, I have removed it in the V2 2/2,
Please review it. Anyway, I will update the next version V3 that will 
include all the modify which kindly pointed out from your review.

Thanks,
Kean

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

[Guenter Roeck]

On 5/19/26 22:52, Kean Ren wrote:
> Hi Guenter,
> 
> Thank you for the review!
> 
>>> Remove all five manual release_region() calls that are now handled
>>> automatically, and drop the unnecessary braces on the single-statement
>>> blocks that previously contained them.
>>
>> [Severity: Medium]
>> Is this description accurate? The patch diff shows that only four
>> release_region() calls were removed.
>>
>>> As far as I can see there are only four calls in the code. Description
>>> problem ?
> Yes, you are right, thanks for your point out, I will update it.
> 
>>> @@ -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;
>>>    	}
>>
>> [Severity: Medium]
>> This isn't a bug, but could these curly braces be removed? The commit
>> message mentions dropping the unnecessary braces on the single-statement
>> blocks, but they appear to have been left intact here.
>>
>>> Hmm, yes, the description does not match the code changes. Please drop
>>> the now unnecessary {}.
> Thanks, you are right, it is unnecessary {}, I have removed it in the V2 2/2,

I have seen that, but that patch only removes one of the now unnecessary {},
and they should be removed when they were made unnecessary (i.e., in this
patch).

Thanks,
Guenter

[PATCH v2 2/2] hwmon: (lenovo-ec-sensors): Fix EC "MCHP" signature validation logic

[Kean Ren]

The EC signature check uses && instead of || between the four
byte comparisons.  With &&, the condition is true only when ALL
four bytes fail to match simultaneously, meaning the driver
accepts a device as a valid Microchip EC if ANY single byte of
the 4-byte "MCHP" signature happens to match.

Due to short-circuit evaluation, if the first byte reads back as
'M' (0x4D, a very common register value), the remaining three
comparisons are skipped entirely and the device is accepted.

Change && to || so the check rejects devices that do not fully
match the expected EC signature, as originally intended.

Also remove the now-unnecessary braces around the single-statement
if body.

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Kean Ren <rh_king@163.com>
---
Changes in v2: No change, just correct the format.

 drivers/hwmon/lenovo-ec-sensors.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
index 45db49e189d3..01138eed45e5 100644
--- a/drivers/hwmon/lenovo-ec-sensors.c
+++ b/drivers/hwmon/lenovo-ec-sensors.c
@@ -537,12 +537,11 @@ 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') &&
-	    (inb_p(MCHP_EMI0_EC_DATA_BYTE3) != 'P')) {
+	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'))
 		return -ENODEV;
-	}
 
 	dmi_id = dmi_first_match(thinkstation_dmi_table);
 
-- 
2.53.0

Re: [PATCH v2 2/2] hwmon: (lenovo-ec-sensors): Fix EC "MCHP" signature validation logic

[sashiko-bot]

> The EC signature check uses && instead of || between the four
> byte comparisons.  With &&, the condition is true only when ALL
> four bytes fail to match simultaneously, meaning the driver
> accepts a device as a valid Microchip EC if ANY single byte of
> the 4-byte "MCHP" signature happens to match.
> 
> Due to short-circuit evaluation, if the first byte reads back as
> 'M' (0x4D, a very common register value), the remaining three
> comparisons are skipped entirely and the device is accepted.
> 
> Change && to || so the check rejects devices that do not fully
> match the expected EC signature, as originally intended.
> 
> Also remove the now-unnecessary braces around the single-statement
> if body.
> 
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Kean Ren <rh_king@163.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

[PATCH v3 0/2] hwmon: (lenovo-ec-sensors): Fix EC signature validation and I/O resource management

[Kean Ren]

Hi Guenter,

Thank you for your patient review.
This is v3 of the lenovo-ec-sensors fix series.

Changes from v2:

- Corrected bug description and Remove unnecessary {}.
- Reproduce patch 0002 based 0001 changed.

Patch 1 converts manual request_region()/release_region() calls to
devm_request_region(), eliminating a double-release on the probe error
path and a release-after-use window on module exit.

Patch 2 fixes the EC "MCHP" signature check that used && instead of ||,
which caused the validation to accept a non-Microchip EC if any single
byte of the expected 4-byte signature happened to match.

Both patches apply cleanly against v7.1-rc3 and pass checkpatch with
0 errors, 0 warnings.

Kean Ren (2):
  hwmon: (lenovo-ec-sensors): Convert to  devm_request_region()
  hwmon: (lenovo-ec-sensors): Fix EC "MCHP" signature  validation logic

 drivers/hwmon/lenovo-ec-sensors.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

-- 
2.53.0

[PATCH v3 1/2] hwmon: (lenovo-ec-sensors): Convert to devm_request_region()

[Kean Ren]

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-use window in lenovo_ec_exit() where
  release_region() was called before platform_device_unregister(),
  leaving the hwmon device active with a released I/O region.

- Missing release_region() in lenovo_ec_probe() if
  devm_hwmon_device_register_with_info() fails.

Remove all four manual release_region() calls that are now handled
automatically and replace request_region with
devm_request_region, use dev_err replace pr_err.

Also remove the now-unnecessary braces around the single-statement
if body.

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Kean Ren <rh_king@163.com>
---
Changes in v3: Corrected bug description.

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

diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
index 8681bbf6665b..a16cc5e4053a 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;
 	}
 
@@ -540,10 +540,8 @@ static int lenovo_ec_probe(struct platform_device *pdev)
 	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);
+	    (inb_p(MCHP_EMI0_EC_DATA_BYTE3) != 'P'))
 		return -ENODEV;
-	}
 
 	dmi_id = dmi_first_match(thinkstation_dmi_table);
 
@@ -577,7 +575,6 @@ 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);
 		return -ENODEV;
 	}
 
@@ -606,10 +603,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;
 }
@@ -617,7 +612,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.53.0

Re: [PATCH v3 1/2] hwmon: (lenovo-ec-sensors): Convert to devm_request_region()

[sashiko-bot]

> 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-use window in lenovo_ec_exit() where
>   release_region() was called before platform_device_unregister(),
>   leaving the hwmon device active with a released I/O region.
> 
> - Missing release_region() in lenovo_ec_probe() if
>   devm_hwmon_device_register_with_info() fails.
> 
> Remove all four manual release_region() calls that are now handled
> automatically and replace request_region with
> devm_request_region, use dev_err replace pr_err.
> 
> Also remove the now-unnecessary braces around the single-statement
> if body.
> 
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Kean Ren <rh_king@163.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH v3 1/2] hwmon: (lenovo-ec-sensors): Convert to devm_request_region()

[Guenter Roeck]

On Thu, May 21, 2026 at 11:52:27AM +0800, Kean Ren 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-use window in lenovo_ec_exit() where
>   release_region() was called before platform_device_unregister(),
>   leaving the hwmon device active with a released I/O region.
> 
> - Missing release_region() in lenovo_ec_probe() if
>   devm_hwmon_device_register_with_info() fails.
> 
> Remove all four manual release_region() calls that are now handled
> automatically and replace request_region with
> devm_request_region, use dev_err replace pr_err.
> 
> Also remove the now-unnecessary braces around the single-statement
> if body.
> 
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Kean Ren <rh_king@163.com>

Applied.

Thanks,
Guenter

[PATCH v3 2/2] hwmon: (lenovo-ec-sensors): Fix EC "MCHP" signature validation logic

[Kean Ren]

The EC signature check uses && instead of || between the four
byte comparisons.  With &&, the condition is true only when ALL
four bytes fail to match simultaneously, meaning the driver
accepts a device as a valid Microchip EC if ANY single byte of
the 4-byte "MCHP" signature happens to match.

Due to short-circuit evaluation, if the first byte reads back as
'M' (0x4D, a very common register value), the remaining three
comparisons are skipped entirely and the device is accepted.

Change && to || so the check rejects devices that do not fully
match the expected EC signature, as originally intended.

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Kean Ren <rh_king@163.com>
---
Changes in v3: Corrected bug description and regenerated.

 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 a16cc5e4053a..24a182abf9a3 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'))
 		return -ENODEV;
 
-- 
2.53.0

Re: [PATCH v3 2/2] hwmon: (lenovo-ec-sensors): Fix EC "MCHP" signature validation logic

[sashiko-bot]

> The EC signature check uses && instead of || between the four
> byte comparisons.  With &&, the condition is true only when ALL
> four bytes fail to match simultaneously, meaning the driver
> accepts a device as a valid Microchip EC if ANY single byte of
> the 4-byte "MCHP" signature happens to match.
> 
> Due to short-circuit evaluation, if the first byte reads back as
> 'M' (0x4D, a very common register value), the remaining three
> comparisons are skipped entirely and the device is accepted.
> 
> Change && to || so the check rejects devices that do not fully
> match the expected EC signature, as originally intended.
> 
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Kean Ren <rh_king@163.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH v3 2/2] hwmon: (lenovo-ec-sensors): Fix EC "MCHP" signature validation logic

[Guenter Roeck]

On Thu, May 21, 2026 at 11:52:28AM +0800, Kean Ren wrote:
> The EC signature check uses && instead of || between the four
> byte comparisons.  With &&, the condition is true only when ALL
> four bytes fail to match simultaneously, meaning the driver
> accepts a device as a valid Microchip EC if ANY single byte of
> the 4-byte "MCHP" signature happens to match.
> 
> Due to short-circuit evaluation, if the first byte reads back as
> 'M' (0x4D, a very common register value), the remaining three
> comparisons are skipped entirely and the device is accepted.
> 
> Change && to || so the check rejects devices that do not fully
> match the expected EC signature, as originally intended.
> 
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Kean Ren <rh_king@163.com>

Applied.

Thanks,
Guenter

Re:Re: [PATCH v3 2/2] hwmon: (lenovo-ec-sensors): Fix EC "MCHP" signature validation logic

[任海]

Hi Guenter,
Thanks for your patient review and approval.
Kean













At 2026-05-21 21:46:00, "Guenter Roeck" <linux@roeck-us.net> wrote:
>On Thu, May 21, 2026 at 11:52:28AM +0800, Kean Ren wrote:
>> The EC signature check uses && instead of || between the four
>> byte comparisons.  With &&, the condition is true only when ALL
>> four bytes fail to match simultaneously, meaning the driver
>> accepts a device as a valid Microchip EC if ANY single byte of
>> the 4-byte "MCHP" signature happens to match.
>> 
>> Due to short-circuit evaluation, if the first byte reads back as
>> 'M' (0x4D, a very common register value), the remaining three
>> comparisons are skipped entirely and the device is accepted.
>> 
>> Change && to || so the check rejects devices that do not fully
>> match the expected EC signature, as originally intended.
>> 
>> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> Signed-off-by: Kean Ren <rh_king@163.com>
>
>Applied.
>
>Thanks,
>Guenter