| Message ID | 20230621151652.79579-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] ACPI: platform: Ignore SMB0001 only when it has resources | expand |
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; > --
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!
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.
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!
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)++;
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.
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?
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;
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(-)