linux-kernel - Re: [RFC v2 2/2] i2c: Pass i2c_device_id to probe func when using DT ids through ACPI

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] [day] [month] [year] [list]

Message-ID: <20160613092655.GF1791@lahna.fi.intel.com>
Date:	Mon, 13 Jun 2016 12:26:55 +0300
From:	Mika Westerberg <mika.westerberg@...ux.intel.com>
To:	Crestez Dan Leonard <leonard.crestez@...el.com>
Cc:	linux-acpi@...r.kernel.org,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Len Brown <lenb@...nel.org>, linux-i2c@...r.kernel.org,
	Wolfram Sang <wsa@...-dreams.de>, linux-kernel@...r.kernel.org,
	Irina Tirdea <irina.tirdea@...el.com>,
	Octavian Purdila <octavian.purdila@...el.com>,
	Daniel Baluta <daniel.baluta@...el.com>
Subject: Re: [RFC v2 2/2] i2c: Pass i2c_device_id to probe func when using DT
 ids through ACPI

On Fri, Jun 10, 2016 at 06:57:36PM +0300, Crestez Dan Leonard wrote:
> On 06/10/2016 09:32 AM, Mika Westerberg wrote:
> > On Thu, Jun 09, 2016 at 04:06:03PM +0300, Crestez Dan Leonard wrote:
> >> When devices are instatiated through devicetree the i2c_client->name is
> >> set to the compatible string with company name stripped out. This is
> >> then matched to the i2c_device_id table to pass the device_id to the
> >> probe function. This id parameter is used by some device drivers to
> >> differentiate between model numbers.
> >>
> >> When using ACPI this id parameter is NULL and the driver usually needs
> >> to do ACPI-specific differentiation.
> >>
> >> This patch attempts to find a valid i2c_device_id when using ACPI with
> >> DT-like compatible strings.
> > 
> > So I don't really understand why it would be good idea to pass
> > i2c_device_id for devices which are matched against their ACPI/DT
> > tables. Apparently DT is already doing that so maybe there is some
> > reason.
> > 
> > Anyway, why not fill in the device name when it is first enumerated
> > if it uses DT compatible property? Just like DT does.
> > 
> This automatic matching of i2c_device_id works for devicetree because
> of_i2c_register_device sets i2c_board_info.type to the compatible string
> with the vendor prefix removed. For I2C devices described via ACPI the
> i2c_board_info.type string is set to the ACPI device name. This ends up
> something like "PRP0001:00".
> 
> This could be changed in acpi_i2c_get_info to use the of_compatible
> string from DSD if present. Is that what you mean? That would work and
> it would be cleaner than my patch. Something like this:
> 
> diff --git drivers/i2c/i2c-core.c drivers/i2c/i2c-core.c
> index 1e0ef9b..ba2fe7f 100644
> --- drivers/i2c/i2c-core.c
> +++ drivers/i2c/i2c-core.c
> @@ -181,7 +181,24 @@ static int acpi_i2c_get_info(struct acpi_device *adev,
> 
>         acpi_dev_free_resource_list(&resource_list);
> 
> -       strlcpy(info->type, dev_name(&adev->dev), sizeof(info->type));
> +       /*
> +        * If we have a DT id set info.type to the first compatible
> string with
> +        * the vendor prefix stripped. This is similar to of_modalias_node
> +        */
> +       if (adev->data.of_compatible) {
> +               const union acpi_object *obj;
> +               const char *str, *chr;
> +
> +               obj = adev->data.of_compatible;
> +               if (obj->type == ACPI_TYPE_PACKAGE)
> +                       obj = obj->package.elements;
> +               str = obj->string.pointer;
> +               chr = strchr(str, ',');
> +               if (chr)
> +                       str = chr + 1;
> +               strlcpy(info->type, str, sizeof(info->type));
> +       } else
> +               strlcpy(info->type, dev_name(&adev->dev),
> sizeof(info->type));
> 
>         return 0;
>  }

Yes, that's what I mean.

> The biggest concern is that this would change the i2c device name
> between kernel versions. Is that acceptable?

I don't think that is a problem since I still have not seen a single
system using ACPI _DSD so I would not expect anything to break.

However, I'm still not convinced it is good idea to return i2c_device_id
from a completely different table if we match using ACPI/DT table.