linux-kernel - RE: [PATCH v20 1/4] usb: Add support for Intel LJCA device

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]

Message-ID: <DM6PR11MB43169A9ADDA7681DB7D9347C8DD7A@DM6PR11MB4316.namprd11.prod.outlook.com>
Date:   Mon, 16 Oct 2023 05:52:28 +0000
From:   "Wu, Wentong" <wentong.wu@...el.com>
To:     Hans de Goede <hdegoede@...hat.com>,
        "Shevchenko, Andriy" <andriy.shevchenko@...el.com>
CC:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "oneukum@...e.com" <oneukum@...e.com>,
        "wsa@...nel.org" <wsa@...nel.org>,
        "andi.shyti@...ux.intel.com" <andi.shyti@...ux.intel.com>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "bartosz.golaszewski@...aro.org" <bartosz.golaszewski@...aro.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
        "sakari.ailus@...ux.intel.com" <sakari.ailus@...ux.intel.com>,
        "Wang, Zhifeng" <zhifeng.wang@...el.com>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v20 1/4] usb: Add support for Intel LJCA device

> From: Hans de Goede <hdegoede>
> 
> Hi Andy,
> 
> On 10/13/23 22:05, Shevchenko, Andriy wrote:
> > On Thu, Oct 12, 2023 at 01:14:23PM +0200, Hans de Goede wrote:
> 
> <snip>
> 
> >> Ah ok, I see. So the code:
> >>
> >> 1. First tries to find the matching child acpi_device for the auxdev
> >> by ADR
> >>
> >> 2. If 1. fails then falls back to HID + UID matching
> >>
> >> And there are DSDTs which use either:
> >>
> >> 1. Only use _ADR to identify which child device is which, like the example
> >>    DSDT snippet from the commit msg.
> >>
> >> 2. Only use _HID + _UID like the 2 example DSDT snippets from me
> >> email
> >>
> >> But there never is a case where both _ADR and _HID are used at the
> >> same time (which would be an ACPI spec violation as Andy said).
> >>
> >> So AFAICT there is no issue here since  _ADR and _HID are never user
> >> at the same time and the commit message correctly describes scenario
> >> 1. from above, so the commit message is fine too.
> >>
> >> So I believe that we can continue with this patch series in its
> >> current v20 form, which has already been staged for going into -next
> >> by Greg.
> >>
> >> Andy can you confirm that moving ahead with the current version is ok
> >> ?
> >
> > Yes as we have a few weeks to fix corner cases.
> >
> > What I'm worrying is that opening door for _ADR that seems never used
> > is kinda an overkill here (resolving non-existing problem).
> 
> I assume that there actually some DSDTs using the _ADR approach and that this
> support is not there just for fun.

right, it's not for fun, we use _ADR here is to reduce the maintain effort because
currently it defines _HID for every new platform and the drivers have to be updated
accordingly, while _ADR doesn't have that problem.

> Wentong, can you confirm that the _ADR using codepaths are actually used on
> some hardware / with some DSDTs out there ?

what I can share is that we will see.

> > Looking at the design of the
> > driver I'm not sure why ACPI HIDs are collected somewhere else than in
> > the respective drivers.

AFAIK, auxiliary bus doesn't support parsing fwnodes currently. Probably we can 
support it for auxiliary bus in another patch. 

> > And looking at the ID lists themselves I am
> > not sure why the firmware of the respective hardware platforms are not using
> _CID.

I think firmware can select _CID as well, but the shipped hw doesn't use _CID,
the driver has to make sure the shipped hw working as well. And switching to _CID
for the shipped hw is not easy, and it has to change windows driver as well.

> 
> This is a USB device which has 4 functions:
> 
> 1. GPIO controller
> 2. I2C controller 1
> 3. I2C controller 2
> 4. SPI controller
> 
> The driver for the main USB interface uses the new auxbus to create 4 child
> devices. The _ADR or if that fails _HID + _UID matching is done to find the
> correct acpi_device child of the acpi_device which is the ACPI-companion of the
> main USB device.
> 
> After looking up the correct acpi_device child this is then set as the fwnode /
> ACPI-companion of the auxbus device created for that function.
> 
> Having the correct fwnode is important because other parts of the DSDT
> reference this fwnode to specify GPIO / I2C / SPI resources and if the fwnode of
> the aux-device is not set correctly then the resources for other devices
> referencing it (typically a camera
> sensor) can not be found.
> 
> As for why the driver for the auxbus devices / children do not use HID matching,
> AFAIK the auxbus has no support for using ACPI (or DT) matching for aux-devices
> and these drivers need to be auxiliary_driver's and bind to the auxbus device and
> not to a platform_device instantiated for the acpi_device since they need the
> auxbus device to access the USB device.

Yes, total agree. Thanks

Thanks
Wentong
> 
> Regards,
> 
> Hans
>