[PATCH v2 1/2] ACPI: platform: Ignore SMB0001 only when it has resources

[PATCH v2 1/2] ACPI: platform: Ignore SMB0001 only when it has resources

[Andy Shevchenko]

After switching i2c-scmi driver to be a plaform one, it stopped
being enumerated on number of Kontron platforms, because it's
listed in the forbidden_id_list.

To resolve the situation, split the list to generic one and
another that holds devices that has to be skipped if and only
if they have bogus resources attached (_CRS method returns some).

Fixes: 03d4287add6e ("i2c: scmi: Convert to be a platform driver")
Closes: https://lore.kernel.org/r/60c1756765b9a3f1eab0dcbd84f59f00fe1caf48.camel@kontron.com
Reported-by: Michael Brunner <michael.brunner@kontron.com>
Tested-by: Michael Brunner <michael.brunner@kontron.com>
Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Link: https://lore.kernel.org/r/20230620163534.1042-1-andriy.shevchenko@linux.intel.com
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: added tags (Andi, Michael), fixed spelling (Andi)
 drivers/acpi/acpi_platform.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
index fe00a5783f53..089a98bd18bf 100644
--- a/drivers/acpi/acpi_platform.c
+++ b/drivers/acpi/acpi_platform.c
@@ -19,13 +19,17 @@
 
 #include "internal.h"
 
+static const struct acpi_device_id forbidden_id_with_resourses[] = {
+	{"SMB0001",  0},	/* ACPI SMBUS virtual device */
+	{ }
+};
+
 static const struct acpi_device_id forbidden_id_list[] = {
 	{"ACPI0009", 0},	/* IOxAPIC */
 	{"ACPI000A", 0},	/* IOAPIC */
 	{"PNP0000",  0},	/* PIC */
 	{"PNP0100",  0},	/* Timer */
 	{"PNP0200",  0},	/* AT DMA Controller */
-	{"SMB0001",  0},	/* ACPI SMBUS virtual device */
 	{ }
 };
 
@@ -83,6 +87,15 @@ static void acpi_platform_fill_resource(struct acpi_device *adev,
 		dest->parent = pci_find_resource(to_pci_dev(parent), dest);
 }
 
+static int acpi_platform_resource_count(struct acpi_resource *ares, void *data)
+{
+	int *count = data;
+
+	*count = *count + 1;
+
+	return 1;
+}
+
 /**
  * acpi_create_platform_device - Create platform device for ACPI device node
  * @adev: ACPI device node to create a platform device for.
@@ -103,7 +116,8 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
 	struct resource_entry *rentry;
 	struct list_head resource_list;
 	struct resource *resources = NULL;
-	int count;
+	int count = 0;
+	int ret;
 
 	/* If the ACPI node already has a physical device attached, skip it. */
 	if (adev->physical_node_count)
@@ -113,6 +127,15 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
 		return ERR_PTR(-EINVAL);
 
 	INIT_LIST_HEAD(&resource_list);
+	ret = acpi_dev_get_resources(adev, &resource_list, acpi_platform_resource_count, &count);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	acpi_dev_free_resource_list(&resource_list);
+
+	if (count > 0 && !acpi_match_device_ids(adev, forbidden_id_with_resourses))
+		return ERR_PTR(-EINVAL);
+
 	count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
 	if (count < 0)
 		return NULL;
-- 
2.40.0.1.gaa8946217a0b

[PATCH v2 2/2] ACPI: platform: Move SMB0001 HID to the header and reuse

[Andy Shevchenko]

There are at least two places in the kernel that are using
the SMB0001 HID. Make it to be available via acpi_drivers.h
header file. While at it, replace hard coded one with a
definition.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Link: https://lore.kernel.org/r/20230620163534.1042-2-andriy.shevchenko@linux.intel.com
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: added tag (Andi)
 drivers/acpi/acpi_platform.c  | 2 +-
 drivers/i2c/busses/i2c-scmi.c | 3 ---
 include/acpi/acpi_drivers.h   | 2 ++
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
index 089a98bd18bf..e86f76ee3473 100644
--- a/drivers/acpi/acpi_platform.c
+++ b/drivers/acpi/acpi_platform.c
@@ -20,7 +20,7 @@
 #include "internal.h"
 
 static const struct acpi_device_id forbidden_id_with_resourses[] = {
-	{"SMB0001",  0},	/* ACPI SMBUS virtual device */
+	{ACPI_SMBUS_MS_HID,  0},	/* ACPI SMBUS virtual device */
 	{ }
 };
 
diff --git a/drivers/i2c/busses/i2c-scmi.c b/drivers/i2c/busses/i2c-scmi.c
index 104570292241..421735acfa14 100644
--- a/drivers/i2c/busses/i2c-scmi.c
+++ b/drivers/i2c/busses/i2c-scmi.c
@@ -13,9 +13,6 @@
 #include <linux/i2c.h>
 #include <linux/acpi.h>
 
-/* SMBUS HID definition as supported by Microsoft Windows */
-#define ACPI_SMBUS_MS_HID		"SMB0001"
-
 struct smbus_methods_t {
 	char *mt_info;
 	char *mt_sbr;
diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
index 8372b0e7fd15..b14d165632e7 100644
--- a/include/acpi/acpi_drivers.h
+++ b/include/acpi/acpi_drivers.h
@@ -27,6 +27,8 @@
 #define ACPI_BAY_HID			"LNXIOBAY"
 #define ACPI_DOCK_HID			"LNXDOCK"
 #define ACPI_ECDT_HID			"LNXEC"
+/* SMBUS HID definition as supported by Microsoft Windows */
+#define ACPI_SMBUS_MS_HID		"SMB0001"
 /* Quirk for broken IBM BIOSes */
 #define ACPI_SMBUS_IBM_HID		"SMBUSIBM"
 
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v2 1/2] ACPI: platform: Ignore SMB0001 only when it has resources

[Rafael J. Wysocki]

On Wed, Jun 21, 2023 at 5:16 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> After switching i2c-scmi driver to be a plaform one, it stopped

"platform"

> being enumerated on number of Kontron platforms, because it's
> listed in the forbidden_id_list.
>
> To resolve the situation, split the list to generic one and
> another that holds devices that has to be skipped if and only

"have"

> if they have bogus resources attached (_CRS method returns some).
>
> Fixes: 03d4287add6e ("i2c: scmi: Convert to be a platform driver")
> Closes: https://lore.kernel.org/r/60c1756765b9a3f1eab0dcbd84f59f00fe1caf48.camel@kontron.com
> Reported-by: Michael Brunner <michael.brunner@kontron.com>
> Tested-by: Michael Brunner <michael.brunner@kontron.com>
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
> Link: https://lore.kernel.org/r/20230620163534.1042-1-andriy.shevchenko@linux.intel.com
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: added tags (Andi, Michael), fixed spelling (Andi)
>  drivers/acpi/acpi_platform.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> index fe00a5783f53..089a98bd18bf 100644
> --- a/drivers/acpi/acpi_platform.c
> +++ b/drivers/acpi/acpi_platform.c
> @@ -19,13 +19,17 @@
>
>  #include "internal.h"
>
> +static const struct acpi_device_id forbidden_id_with_resourses[] = {

I don't quite like this name and the driver_data field could be used
to indicate the need to check the resources.

> +       {"SMB0001",  0},        /* ACPI SMBUS virtual device */
> +       { }
> +};
> +
>  static const struct acpi_device_id forbidden_id_list[] = {
>         {"ACPI0009", 0},        /* IOxAPIC */
>         {"ACPI000A", 0},        /* IOAPIC */
>         {"PNP0000",  0},        /* PIC */
>         {"PNP0100",  0},        /* Timer */
>         {"PNP0200",  0},        /* AT DMA Controller */
> -       {"SMB0001",  0},        /* ACPI SMBUS virtual device */
>         { }
>  };
>
> @@ -83,6 +87,15 @@ static void acpi_platform_fill_resource(struct acpi_device *adev,
>                 dest->parent = pci_find_resource(to_pci_dev(parent), dest);
>  }
>
> +static int acpi_platform_resource_count(struct acpi_resource *ares, void *data)
> +{
> +       int *count = data;
> +
> +       *count = *count + 1;

Why not (*count)++?

> +
> +       return 1;
> +}
> +
>  /**
>   * acpi_create_platform_device - Create platform device for ACPI device node
>   * @adev: ACPI device node to create a platform device for.
> @@ -103,7 +116,8 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
>         struct resource_entry *rentry;
>         struct list_head resource_list;
>         struct resource *resources = NULL;
> -       int count;
> +       int count = 0;
> +       int ret;
>
>         /* If the ACPI node already has a physical device attached, skip it. */
>         if (adev->physical_node_count)
> @@ -113,6 +127,15 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
>                 return ERR_PTR(-EINVAL);
>
>         INIT_LIST_HEAD(&resource_list);
> +       ret = acpi_dev_get_resources(adev, &resource_list, acpi_platform_resource_count, &count);
> +       if (ret < 0)
> +               return ERR_PTR(ret);

Why not use acpi_walk_resources() directly here?

Also, this extra resources walk is only needed if the resources check
is needed to decide whether or not to skip the device, so what's the
benefit of doing it for every device that's not skipped?

> +
> +       acpi_dev_free_resource_list(&resource_list);
> +
> +       if (count > 0 && !acpi_match_device_ids(adev, forbidden_id_with_resourses))
> +               return ERR_PTR(-EINVAL);
> +
>         count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
>         if (count < 0)
>                 return NULL;
> --

Re: [PATCH v2 1/2] ACPI: platform: Ignore SMB0001 only when it has resources

[Andy Shevchenko]

On Thu, Jun 22, 2023 at 05:53:13PM +0200, Rafael J. Wysocki wrote:
> On Wed, Jun 21, 2023 at 5:16 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > After switching i2c-scmi driver to be a plaform one, it stopped
> 
> "platform"
> 
> > being enumerated on number of Kontron platforms, because it's
> > listed in the forbidden_id_list.
> >
> > To resolve the situation, split the list to generic one and
> > another that holds devices that has to be skipped if and only
> 
> "have"
> 
> > if they have bogus resources attached (_CRS method returns some).

Thanks for the typo fixes!

...

> > +static const struct acpi_device_id forbidden_id_with_resourses[] = {
> 
> I don't quite like this name and the driver_data field could be used
> to indicate the need to check the resources.

Okay, something like

/* Check if the device has resources provided by _CRS method */
#define ACPI_PLATFORM_CHECK_RES		BIT(0)

?

> > +       {"SMB0001",  0},        /* ACPI SMBUS virtual device */
> > +       { }
> > +};

...

> > +static int acpi_platform_resource_count(struct acpi_resource *ares, void *data)
> > +{
> > +       int *count = data;
> > +
> > +       *count = *count + 1;
> 
> Why not (*count)++?

Can be that way, I just copied'n'pasted from the existing code.

> > +       return 1;
> > +}

...

> >         INIT_LIST_HEAD(&resource_list);
> > +       ret = acpi_dev_get_resources(adev, &resource_list, acpi_platform_resource_count, &count);
> > +       if (ret < 0)
> > +               return ERR_PTR(ret);
> 
> Why not use acpi_walk_resources() directly here?

Can be done that way. Again, I just used a template (existing code in kernel)
for similar functionality.

> Also, this extra resources walk is only needed if the resources check
> is needed to decide whether or not to skip the device, so what's the
> benefit of doing it for every device that's not skipped?

Ah, indeed. Makes sense to have done it conditionally.

> > +       acpi_dev_free_resource_list(&resource_list);

...

Thank you for the review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] ACPI: platform: Move SMB0001 HID to the header and reuse

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 536 bytes --]

On Wed, Jun 21, 2023 at 06:16:52PM +0300, Andy Shevchenko wrote:
> There are at least two places in the kernel that are using
> the SMB0001 HID. Make it to be available via acpi_drivers.h
> header file. While at it, replace hard coded one with a
> definition.
> 
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
> Link: https://lore.kernel.org/r/20230620163534.1042-2-andriy.shevchenko@linux.intel.com
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Wolfram Sang <wsa@kernel.org> # for I2C


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] ACPI: platform: Ignore SMB0001 only when it has resources

[Rafael J. Wysocki]

On Thu, Jun 22, 2023 at 8:19 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Jun 22, 2023 at 05:53:13PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Jun 21, 2023 at 5:16 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > After switching i2c-scmi driver to be a plaform one, it stopped
> >
> > "platform"
> >
> > > being enumerated on number of Kontron platforms, because it's
> > > listed in the forbidden_id_list.
> > >
> > > To resolve the situation, split the list to generic one and
> > > another that holds devices that has to be skipped if and only
> >
> > "have"
> >
> > > if they have bogus resources attached (_CRS method returns some).
>
> Thanks for the typo fixes!
>
> ...
>
> > > +static const struct acpi_device_id forbidden_id_with_resourses[] = {
> >
> > I don't quite like this name and the driver_data field could be used
> > to indicate the need to check the resources.
>
> Okay, something like
>
> /* Check if the device has resources provided by _CRS method */
> #define ACPI_PLATFORM_CHECK_RES         BIT(0)
>
> ?

Could be, but this is specific to forbidden_ids_list[].  Maybe
ACPI_ALLOW_WO_RESOURCES or similar.

> > > +       {"SMB0001",  0},        /* ACPI SMBUS virtual device */
> > > +       { }
> > > +};
>
> ...
>
> > > +static int acpi_platform_resource_count(struct acpi_resource *ares, void *data)
> > > +{
> > > +       int *count = data;
> > > +
> > > +       *count = *count + 1;
> >
> > Why not (*count)++?
>
> Can be that way, I just copied'n'pasted from the existing code.
>
> > > +       return 1;
> > > +}

BTW, this doesn't need to increment the count even.  It could just
terminate the walk on the first valid resource found and tell the
caller to return true in that case.

Re: [PATCH v2 1/2] ACPI: platform: Ignore SMB0001 only when it has resources

[Andy Shevchenko]

On Fri, Jun 23, 2023 at 04:43:55PM +0200, Rafael J. Wysocki wrote:
> On Thu, Jun 22, 2023 at 8:19 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Jun 22, 2023 at 05:53:13PM +0200, Rafael J. Wysocki wrote:
> > > On Wed, Jun 21, 2023 at 5:16 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > /* Check if the device has resources provided by _CRS method */
> > #define ACPI_PLATFORM_CHECK_RES         BIT(0)
> >
> > ?
> 
> Could be, but this is specific to forbidden_ids_list[].  Maybe
> ACPI_ALLOW_WO_RESOURCES or similar.

Got it, will do this way.

...

> BTW, this doesn't need to increment the count even.  It could just
> terminate the walk on the first valid resource found and tell the
> caller to return true in that case.

Indeed, thank you for the hint!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] ACPI: platform: Ignore SMB0001 only when it has resources

[Andy Shevchenko]

On Mon, Jun 26, 2023 at 11:15:19AM +0300, Andy Shevchenko wrote:
> On Fri, Jun 23, 2023 at 04:43:55PM +0200, Rafael J. Wysocki wrote:
> > On Thu, Jun 22, 2023 at 8:19 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Jun 22, 2023 at 05:53:13PM +0200, Rafael J. Wysocki wrote:
> > > > On Wed, Jun 21, 2023 at 5:16 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > BTW, this doesn't need to increment the count even.  It could just
> > terminate the walk on the first valid resource found and tell the
> > caller to return true in that case.
> 
> Indeed, thank you for the hint!

Actually it's doesn't matter if we count them or not, we still must use the
context of the call to set up a flag or whatever. With the current code in mind
I prefer to count resources and compare that to be non-zero. This will help to
read and understand code better.

That said, I will go with (*counter)++;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] ACPI: platform: Ignore SMB0001 only when it has resources

[Rafael J. Wysocki]

On Mon, Jun 26, 2023 at 12:44 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Jun 26, 2023 at 11:15:19AM +0300, Andy Shevchenko wrote:
> > On Fri, Jun 23, 2023 at 04:43:55PM +0200, Rafael J. Wysocki wrote:
> > > On Thu, Jun 22, 2023 at 8:19 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Thu, Jun 22, 2023 at 05:53:13PM +0200, Rafael J. Wysocki wrote:
> > > > > On Wed, Jun 21, 2023 at 5:16 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
>
> ...
>
> > > BTW, this doesn't need to increment the count even.  It could just
> > > terminate the walk on the first valid resource found and tell the
> > > caller to return true in that case.
> >
> > Indeed, thank you for the hint!
>
> Actually it's doesn't matter if we count them or not, we still must use the
> context of the call to set up a flag or whatever.

No, it is sufficient to pass a pointer to a bool variable.

> With the current code in mind I prefer to count resources and compare that
> to be non-zero. This will help to read and understand code better.

I'm not sure.  The condition is "if there is at least one valid
resource, skip the device".  Counting them all will make casual
readers wonder why IMO.

Re: [PATCH v2 1/2] ACPI: platform: Ignore SMB0001 only when it has resources

[Andy Shevchenko]

On Mon, Jun 26, 2023 at 02:11:10PM +0200, Rafael J. Wysocki wrote:
> On Mon, Jun 26, 2023 at 12:44 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Jun 26, 2023 at 11:15:19AM +0300, Andy Shevchenko wrote:
> > > On Fri, Jun 23, 2023 at 04:43:55PM +0200, Rafael J. Wysocki wrote:
> > > > On Thu, Jun 22, 2023 at 8:19 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Thu, Jun 22, 2023 at 05:53:13PM +0200, Rafael J. Wysocki wrote:
> > > > > > On Wed, Jun 21, 2023 at 5:16 PM Andy Shevchenko
> > > > > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > > > BTW, this doesn't need to increment the count even.  It could just
> > > > terminate the walk on the first valid resource found and tell the
> > > > caller to return true in that case.
> > >
> > > Indeed, thank you for the hint!
> >
> > Actually it's doesn't matter if we count them or not, we still must use the
> > context of the call to set up a flag or whatever.
> 
> No, it is sufficient to pass a pointer to a bool variable.
> 
> > With the current code in mind I prefer to count resources and compare that
> > to be non-zero. This will help to read and understand code better.
> 
> I'm not sure.  The condition is "if there is at least one valid
> resource, skip the device".  Counting them all will make casual
> readers wonder why IMO.

Okay, Can you check v3 for the rest and I can send a v4 if nothing else there?

-- 
With Best Regards,
Andy Shevchenko