Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230621151652.79579-1-andriy.shevchenko@linux.intel.com>
In-Reply-To: <20230621151652.79579-1-andriy.shevchenko@linux.intel.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Thu, 22 Jun 2023 17:53:13 +0200
Message-ID: <CAJZ5v0jt8XCzUxQaBXLz0zXezih1Urq=dt-K9PWVY1JpN=Go6Q@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] ACPI: platform: Ignore SMB0001 only when it has resources
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-i2c@vger.kernel.org, acpica-devel@lists.linuxfoundation.org,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Len Brown <lenb@kernel.org>,
        Andi Shyti <andi.shyti@kernel.org>,
        Robert Moore <robert.moore@intel.com>,
        Michael Brunner <michael.brunner@kontron.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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