[PATCH 1/4] platform/x86: int3472: Check for adev == NULL

[PATCH 1/4] platform/x86: int3472: Check for adev == NULL

[Hans de Goede]

Not all devices have an ACPI companion fwnode, so adev might be NULL. This
can e.g. (theoretically) happen when a user manually binds one of
the int3472 drivers to another i2c/platform device through sysfs.

Add a check for adev not being set and return -ENODEV in that case.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel/int3472/discrete.c | 3 +++
 drivers/platform/x86/intel/int3472/tps68470.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 3de463c3d13b..15678508ee50 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -336,6 +336,9 @@ static int skl_int3472_discrete_probe(struct platform_device *pdev)
 	struct int3472_cldb cldb;
 	int ret;
 
+	if (!adev)
+		return -ENODEV;
+
 	ret = skl_int3472_fill_cldb(adev, &cldb);
 	if (ret) {
 		dev_err(&pdev->dev, "Couldn't fill CLDB structure\n");
diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
index 1e107fd49f82..81ac4c691963 100644
--- a/drivers/platform/x86/intel/int3472/tps68470.c
+++ b/drivers/platform/x86/intel/int3472/tps68470.c
@@ -152,6 +152,9 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
 	int ret;
 	int i;
 
+	if (!adev)
+		return -ENODEV;
+
 	n_consumers = skl_int3472_fill_clk_pdata(&client->dev, &clk_pdata);
 	if (n_consumers < 0)
 		return n_consumers;
-- 
2.47.0

[PATCH 2/4] platform/x86: int3472: Drop "pin number mismatch" messages

[Hans de Goede]

It seems that Windows is only using the ACPI GPIO resources and never
looks at the part of the _DSM return value which encodes the pin number.

For example on a Terra Pad 1262 v2 the following messages are printend:

int3472-discrete INT3472:01: reset \_SB.GPI0 pin number mismatch _DSM 103 resource 359
int3472-discrete INT3472:01: powerdown \_SB.GPI0 pin number mismatch _DSM 207 resource 335
int3472-discrete INT3472:02: reset \_SB.GPI0 pin number mismatch _DSM 101 resource 357

Notice for the 2 reset pins that the _DSM value is off by 256, this is
caused by there only being 8 bits reserved in the _DSM return value for
the pin-number.

As for the powerdown pin, testing has shown that the pin-number 335 from
the ACPI GPIO resource is correct and the _DSM value is bogus.

Drop the warning about these mismatches since Windows clearly is just
ignoring the _DSM pin-number so invalid values are too be expected there.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel/int3472/discrete.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 15678508ee50..01da18b426ae 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -189,9 +189,9 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 {
 	struct int3472_discrete_device *int3472 = data;
 	struct acpi_resource_gpio *agpio;
-	u8 active_value, pin, type;
 	union acpi_object *obj;
 	struct gpio_desc *gpio;
+	u8 active_value, type;
 	const char *err_msg;
 	const char *func;
 	u32 polarity;
@@ -219,12 +219,6 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 	int3472_get_func_and_polarity(type, &func, &polarity);
 
-	pin = FIELD_GET(INT3472_GPIO_DSM_PIN, obj->integer.value);
-	if (pin != agpio->pin_table[0])
-		dev_warn(int3472->dev, "%s %s pin number mismatch _DSM %d resource %d\n",
-			 func, agpio->resource_source.string_ptr, pin,
-			 agpio->pin_table[0]);
-
 	active_value = FIELD_GET(INT3472_GPIO_DSM_SENSOR_ON_VAL, obj->integer.value);
 	if (!active_value)
 		polarity ^= GPIO_ACTIVE_LOW;
-- 
2.47.0

Re: [PATCH 2/4] platform/x86: int3472: Drop "pin number mismatch" messages

[Andy Shevchenko]

On Thu, Nov 28, 2024 at 04:42:10PM +0100, Hans de Goede wrote:
> It seems that Windows is only using the ACPI GPIO resources and never
> looks at the part of the _DSM return value which encodes the pin number.
> 
> For example on a Terra Pad 1262 v2 the following messages are printend:
> 
> int3472-discrete INT3472:01: reset \_SB.GPI0 pin number mismatch _DSM 103 resource 359
> int3472-discrete INT3472:01: powerdown \_SB.GPI0 pin number mismatch _DSM 207 resource 335
> int3472-discrete INT3472:02: reset \_SB.GPI0 pin number mismatch _DSM 101 resource 357
> 
> Notice for the 2 reset pins that the _DSM value is off by 256, this is
> caused by there only being 8 bits reserved in the _DSM return value for
> the pin-number.
> 
> As for the powerdown pin, testing has shown that the pin-number 335 from
> the ACPI GPIO resource is correct and the _DSM value is bogus.
> 
> Drop the warning about these mismatches since Windows clearly is just
> ignoring the _DSM pin-number so invalid values are too be expected there.

...

> -	pin = FIELD_GET(INT3472_GPIO_DSM_PIN, obj->integer.value);
> -	if (pin != agpio->pin_table[0])
> -		dev_warn(int3472->dev, "%s %s pin number mismatch _DSM %d resource %d\n",
> -			 func, agpio->resource_source.string_ptr, pin,
> -			 agpio->pin_table[0]);
> -

Hmm... Perhaps move it to dev_dbg(FW_BUG) ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] platform/x86: int3472: Drop "pin number mismatch" messages

[Hans de Goede]

Hi,

On 28-Nov-24 4:52 PM, Andy Shevchenko wrote:
> On Thu, Nov 28, 2024 at 04:42:10PM +0100, Hans de Goede wrote:
>> It seems that Windows is only using the ACPI GPIO resources and never
>> looks at the part of the _DSM return value which encodes the pin number.
>>
>> For example on a Terra Pad 1262 v2 the following messages are printend:
>>
>> int3472-discrete INT3472:01: reset \_SB.GPI0 pin number mismatch _DSM 103 resource 359
>> int3472-discrete INT3472:01: powerdown \_SB.GPI0 pin number mismatch _DSM 207 resource 335
>> int3472-discrete INT3472:02: reset \_SB.GPI0 pin number mismatch _DSM 101 resource 357
>>
>> Notice for the 2 reset pins that the _DSM value is off by 256, this is
>> caused by there only being 8 bits reserved in the _DSM return value for
>> the pin-number.
>>
>> As for the powerdown pin, testing has shown that the pin-number 335 from
>> the ACPI GPIO resource is correct and the _DSM value is bogus.
>>
>> Drop the warning about these mismatches since Windows clearly is just
>> ignoring the _DSM pin-number so invalid values are too be expected there.
> 
> ...
> 
>> -	pin = FIELD_GET(INT3472_GPIO_DSM_PIN, obj->integer.value);
>> -	if (pin != agpio->pin_table[0])
>> -		dev_warn(int3472->dev, "%s %s pin number mismatch _DSM %d resource %d\n",
>> -			 func, agpio->resource_source.string_ptr, pin,
>> -			 agpio->pin_table[0]);
>> -
> 
> Hmm... Perhaps move it to dev_dbg(FW_BUG) ?

I'm not sure there is much value in keeping this. If we do go for dev_dbg()
then the check should be changed to:

	if (pin != (agpio->pin_table[0] % 256))

to avoid false positives and the need for that IMHO already shows that
there is little use in keeping the check.

But lowering it to dev_dbg() + adding the % 256 also works for me,
please let me know how you want to proceed.

Regards,

Hans

Re: [PATCH 2/4] platform/x86: int3472: Drop "pin number mismatch" messages

[Andy Shevchenko]

On Wed, Dec 4, 2024 at 7:36 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 28-Nov-24 4:52 PM, Andy Shevchenko wrote:
> > On Thu, Nov 28, 2024 at 04:42:10PM +0100, Hans de Goede wrote:

...

> >> -    if (pin != agpio->pin_table[0])
> >> -            dev_warn(int3472->dev, "%s %s pin number mismatch _DSM %d resource %d\n",
> >> -                     func, agpio->resource_source.string_ptr, pin,
> >> -                     agpio->pin_table[0]);
> >> -
> >
> > Hmm... Perhaps move it to dev_dbg(FW_BUG) ?
>
> I'm not sure there is much value in keeping this. If we do go for dev_dbg()
> then the check should be changed to:
>
>         if (pin != (agpio->pin_table[0] % 256))
>
> to avoid false positives and the need for that IMHO already shows that
> there is little use in keeping the check.
>
> But lowering it to dev_dbg() + adding the % 256 also works for me,
> please let me know how you want to proceed.

I think dev_dbg() might still make (a little, though) sense. But also
add a FW_BUG prefix to the message.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/4] platform/x86: int3472: Fix skl_int3472_handle_gpio_resources() return value

[Hans de Goede]

The INT3472 code never wants a copy of the ACPI resource to be added
to the list-head passed to acpi_dev_get_resources().

Make skl_int3472_handle_gpio_resources() always return -errno or 1
and drop the now no longer acpi_dev_free_resource_list() call.

Also update the inaccurate comment about the return value.
skl_int3472_handle_gpio_resources() was already returning 1 in the case
of not a GPIO resource or invalid _DSM return and not -EINVAL / -ENODEV
as the comment claimed.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Despite the "Fix" in the subject this is not really a bugfix, the old code
works fine too, so no Fixes tag
---
 drivers/platform/x86/intel/int3472/discrete.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 01da18b426ae..05e442078f8f 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -178,11 +178,11 @@ static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polar
  * to create clocks and regulators via the usual frameworks.
  *
  * Return:
- * * 1		- To continue the loop
- * * 0		- When all resources found are handled properly.
- * * -EINVAL	- If the resource is not a GPIO IO resource
- * * -ENODEV	- If the resource has no corresponding _DSM entry
- * * -Other	- Errors propagated from one of the sub-functions.
+ * * 1		- Continue the loop without adding a copy of the resource to
+ * *		  the list passed to acpi_dev_get_resources()
+ * * 0		- Continue the loop after adding a copy of the resource to
+ * *		  the list passed to acpi_dev_get_resources()
+ * * -errno	- Error, break loop
  */
 static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 					     void *data)
@@ -283,7 +283,8 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	if (ret < 0)
 		return dev_err_probe(int3472->dev, ret, err_msg);
 
-	return ret;
+	/* Tell acpi_dev_get_resources() to not make a copy of the resource */
+	return 1;
 }
 
 static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
@@ -299,8 +300,6 @@ static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
 	if (ret < 0)
 		return ret;
 
-	acpi_dev_free_resource_list(&resource_list);
-
 	/* Register _DSM based clock (no-op if a GPIO clock was already registered) */
 	ret = skl_int3472_register_dsm_clock(int3472);
 	if (ret < 0)
-- 
2.47.0

Re: [PATCH 3/4] platform/x86: int3472: Fix skl_int3472_handle_gpio_resources() return value

[Andy Shevchenko]

On Thu, Nov 28, 2024 at 04:42:11PM +0100, Hans de Goede wrote:
> The INT3472 code never wants a copy of the ACPI resource to be added
> to the list-head passed to acpi_dev_get_resources().
> 
> Make skl_int3472_handle_gpio_resources() always return -errno or 1
> and drop the now no longer acpi_dev_free_resource_list() call.
> 
> Also update the inaccurate comment about the return value.
> skl_int3472_handle_gpio_resources() was already returning 1 in the case
> of not a GPIO resource or invalid _DSM return and not -EINVAL / -ENODEV
> as the comment claimed.

...

> -	acpi_dev_free_resource_list(&resource_list);

Even though it's better to have this (no-op) call. As people may use the driver
as an example and then make the real leakage somewhere else.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] platform/x86: int3472: Fix skl_int3472_handle_gpio_resources() return value

[Hans de Goede]

Hi,

On 28-Nov-24 4:53 PM, Andy Shevchenko wrote:
> On Thu, Nov 28, 2024 at 04:42:11PM +0100, Hans de Goede wrote:
>> The INT3472 code never wants a copy of the ACPI resource to be added
>> to the list-head passed to acpi_dev_get_resources().
>>
>> Make skl_int3472_handle_gpio_resources() always return -errno or 1
>> and drop the now no longer acpi_dev_free_resource_list() call.
>>
>> Also update the inaccurate comment about the return value.
>> skl_int3472_handle_gpio_resources() was already returning 1 in the case
>> of not a GPIO resource or invalid _DSM return and not -EINVAL / -ENODEV
>> as the comment claimed.
> 
> ...
> 
>> -	acpi_dev_free_resource_list(&resource_list);
> 
> Even though it's better to have this (no-op) call. As people may use the driver
> as an example and then make the real leakage somewhere else.

Ok, I'll keep the call for v2 and adjust the commit msg to match.

Regards,

Hans

[PATCH 4/4] platform/x86: int3472: Debug log the sensor name

[Hans de Goede]

Debug log the sensor name to make it easier to figure out which INT3472
device is associated with which sensor.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel/int3472/common.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/platform/x86/intel/int3472/common.c b/drivers/platform/x86/intel/int3472/common.c
index b3a2578e06c1..1638be8fa71e 100644
--- a/drivers/platform/x86/intel/int3472/common.c
+++ b/drivers/platform/x86/intel/int3472/common.c
@@ -70,6 +70,8 @@ int skl_int3472_get_sensor_adev_and_name(struct device *dev,
 		return -ENODEV;
 	}
 
+	dev_dbg(dev, "Sensor name %s\n", acpi_dev_name(sensor));
+
 	*name_ret = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT,
 				   acpi_dev_name(sensor));
 	if (!*name_ret)
-- 
2.47.0

Re: [PATCH 1/4] platform/x86: int3472: Check for adev == NULL

[Andy Shevchenko]

On Thu, Nov 28, 2024 at 04:42:09PM +0100, Hans de Goede wrote:
> Not all devices have an ACPI companion fwnode, so adev might be NULL. This
> can e.g. (theoretically) happen when a user manually binds one of
> the int3472 drivers to another i2c/platform device through sysfs.
> 
> Add a check for adev not being set and return -ENODEV in that case.

But what kind of "bad thing" can happen in such cases?
If none, why this change?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] platform/x86: int3472: Check for adev == NULL

[Hans de Goede]

Hi,

On 28-Nov-24 4:51 PM, Andy Shevchenko wrote:
> On Thu, Nov 28, 2024 at 04:42:09PM +0100, Hans de Goede wrote:
>> Not all devices have an ACPI companion fwnode, so adev might be NULL. This
>> can e.g. (theoretically) happen when a user manually binds one of
>> the int3472 drivers to another i2c/platform device through sysfs.
>>
>> Add a check for adev not being set and return -ENODEV in that case.
> 
> But what kind of "bad thing" can happen in such cases?

NULL pointer deref oops in skl_int3472_get_acpi_buffer() during
probe() when it tries to get adev->handle.

I guess for v2 you want me to reword the second paragraph of the commit
message to e.g. :

Add a check for adev not being set and return -ENODEV in that case to
avoid a possible NULL pointer deref in skl_int3472_get_acpi_buffer().

?

Regards,

Hans

Re: [PATCH 1/4] platform/x86: int3472: Check for adev == NULL

[Andy Shevchenko]

On Wed, Dec 4, 2024 at 7:40 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 28-Nov-24 4:51 PM, Andy Shevchenko wrote:
> > On Thu, Nov 28, 2024 at 04:42:09PM +0100, Hans de Goede wrote:
> >> Not all devices have an ACPI companion fwnode, so adev might be NULL. This
> >> can e.g. (theoretically) happen when a user manually binds one of
> >> the int3472 drivers to another i2c/platform device through sysfs.
> >>
> >> Add a check for adev not being set and return -ENODEV in that case.
> >
> > But what kind of "bad thing" can happen in such cases?
>
> NULL pointer deref oops in skl_int3472_get_acpi_buffer() during
> probe() when it tries to get adev->handle.
>
> I guess for v2 you want me to reword the second paragraph of the commit
> message to e.g. :
>
> Add a check for adev not being set and return -ENODEV in that case to
> avoid a possible NULL pointer deref in skl_int3472_get_acpi_buffer().
>
> ?

I don't remember, but it sounds good to me.

-- 
With Best Regards,
Andy Shevchenko