Re: [RFC PATCH] usb: optimize acpi companion search for usb port devices

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 01.06.2017 02:01, Rafael J. Wysocki wrote:
> On 5/25/2017 5:24 PM, Mathias Nyman wrote:
>> On 24.05.2017 17:44, Greg KH wrote:
>>> On Wed, May 24, 2017 at 04:11:12PM +0300, Mathias Nyman wrote:
>>>> This optimization significantly reduces xhci driver load time.
>>>>
>>>> In ACPI tables the acpi companion port devices are children of
>>>> the hub device. The port devices are identified by their port number
>>>> returned by the ACPI _ADR method.
>>>> _ADR 0 is reserved for the root hub device.
>>>>
>>>> The current implementation to find a acpi companion port device
>>>> loops through all acpi port devices under that parent hub, calling
>>>> their _ADR method each time a new port device is added.
>>>>
>>>> for a xHC controller with 25 ports under its roothub it
>>>> will end up invoking ACPI bytecode 625 times before all ports
>>>> are ready, making it really slow.
>>>>
>>>> The _ADR values are already read and cached earler. So instead of
>>>> running the bytecode again we can check the cached _ADR value first,
>>>> and then fall back to the old way.
>>>>
>>>> As one of the more significant changes, the xhci load time on
>>>> Intel kabylake reduced by 70%, (28ms) from
>>>> initcall xhci_pci_init+0x0/0x49 returned 0 after 39537 usecs
>>>> to
>>>> initcall xhci_pci_init+0x0/0x49 returned 0 after 11270 usecs
>>>>
>>>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>>>> ---
>>>>   drivers/usb/core/usb-acpi.c | 26 +++++++++++++++++++++++---
>>>>   1 file changed, 23 insertions(+), 3 deletions(-)
>>>
>>> Why is this RFC?  What's wrong with it as-is?
>>>
>>
>> Last minute doubt, nothing should be wrong, but I started to wonder if there is
>> any particular reason the ACPI part was done the way it was.
>>
>> Or if maybe other drivers could benefit from checking cached _ADR value first as
>> well, and this whole thing should be a part of drivers/acpi/glue.c instead?
>>
>
> That or we should just evaluate _ADR if present during the very initialization and store the value in a filed under struct acpi_device.
>

Yes, that is pretty much done, acpi_init_device_object() will end up evaluating _ADR
and store it in acpi_device.pnp.bus_address

Just considering if there should be something like acpi_get_adr(acpi_device *adev) that
would first check if adev->pnp.bus_address exists, if not, then it would evaluate _ADR.

But really just a thought, I think I'll send this forward as is.

Thanks
Mathias