lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 22 Jun 2023 17:53:13 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-i2c@...r.kernel.org, acpica-devel@...ts.linuxfoundation.org,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Len Brown <lenb@...nel.org>,
        Andi Shyti <andi.shyti@...nel.org>,
        Robert Moore <robert.moore@...el.com>,
        Michael Brunner <michael.brunner@...tron.com>
Subject: Re: [PATCH v2 1/2] ACPI: platform: Ignore SMB0001 only when it has resources

On Wed, Jun 21, 2023 at 5:16 PM Andy Shevchenko
<andriy.shevchenko@...ux.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@...tron.com>
> Tested-by: Michael Brunner <michael.brunner@...tron.com>
> Reviewed-by: Andi Shyti <andi.shyti@...nel.org>
> Link: https://lore.kernel.org/r/20230620163534.1042-1-andriy.shevchenko@linux.intel.com
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.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;
> --

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ