Re: [PATCH] ACPI: don't show an error when we're not in charge of PCIe hotplug.

[Rafael J. Wysocki]

On Tue, Jun 21, 2016 at 8:01 PM,  <Mario_Limonciello@xxxxxxxx> wrote:
>> -----Original Message-----
>> From: Peter Jones [mailto:pjones@xxxxxxxxxx]
>> Sent: Tuesday, June 21, 2016 10:19 AM
>> To: Rafael J. Wysocki <rafael@xxxxxxxxxx>
>> Cc: ACPI Devel Maling List <linux-acpi@xxxxxxxxxxxxxxx>; Limonciello, Mario
>> <Mario_Limonciello@xxxxxxxx>; Linux Kernel Mailing List <linux-
>> kernel@xxxxxxxxxxxxxxx>; Len Brown <lenb@xxxxxxxxxx>; Rafael J . Wysocki
>> <rjw@xxxxxxxxxxxxx>; Andy Lutomirski <luto@xxxxxxxxxxxxxx>
>> Subject: Re: [PATCH] ACPI: don't show an error when we're not in charge of
>> PCIe hotplug.
>>
>> (Sorry for the slow response - it's deadline time over here.)
>>
>> On Thu, Jun 16, 2016 at 04:56:57PM +0200, Rafael J. Wysocki wrote:
>> > On Thu, Jun 16, 2016 at 2:12 AM, Rafael J. Wysocki <rafael@xxxxxxxxxx>
>> wrote:
>> > > On Thu, Jun 16, 2016 at 12:15 AM, Peter Jones <pjones@xxxxxxxxxx>
>> wrote:
>> > >> Right now when booting, on many laptops the firmware manages the
>> PCIe
>> > >> bus.  As a result, when we call the _OSC ACPI method, it returns an
>> > >> error code.  Unfortunately the errors are not very articulate.
>> > >
>> > > What exactly do you mean here?
>> > >
>> > >>  As a result, we show:
>> > >>
>> > >> ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-fe])
>> > >> acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM
>> Segments MSI]
>> > >> \_SB_.PCI0 (33DB4D5B-1FF7-401C-9657-7441C03DD766): _OSC invalid
>> UUID
>> > >> _OSC request data: 1 1f 0
>> > >
>> > > So _OSC told us that the UUID was invalid, didn't it?
>> >
>> > BTW, the above messages are KERN_DEBUG, so at least in theory they
>> > shouldn't be visible in production runs.
>> >
>> > Maybe the bug to fix is that they show up when they aren't supposed to?
>>
>> No - the workflow that I am really trying to remedy is this:
>>
>>  1) S3 resume sometimes isn't working on some laptop you've got.
>>  2) start looking at debug messages
>>  3) this shows an error, so it looks like it's probably the problem
>>  4) go fishing for red herring
>>  5) if you happen to know who maintains the DSDT for the platform in
>>     question, eventually work out that this is working as intended and
>>     the bug is someplace else.
>> 5b) if you don't know that person, eventually work out that it /might/
>>      be someplace else...
>>
>> So the idea was to make it look more like an indication of status, and
>> less like an error that's causing unrelated problems.
>>
>> When I talked to Mario at Dell (Cc'd), it wasn't clear to us that
>> there's a way to distinguish the between the UUID being
>> invalid/malformed, being merely unsupported, or being supported in some
>> configurations but not the current one.  In this particular DSDT, the
>> machine doesn't support the OS controlling any of this if USB-C /
>> thunderbolt are enabled.  The DSDT is clearly written with the belief
>> that you have to completely disable the handling for that UUID in this
>> case, and googling for this looks like it's not the only one written
>> with that belief.
>>
>> Reading the spec (v6.1, sections 6.2.11.3 and 6.2.11.4), it seems
>> plausible that you can express this instead by handling the UUID but
>> choosing each individual query/status bit in the way that accomplishes
>> the OS doing nothing with the response.  So it may well be that that's
>> just more code that vendors have thought wasn't necessary (or wasn't
>> correct for some reason.)
>>
>> Mario, want to jump in on your thinking here?
>>
>> --
>>   Peter
>
> After talking to the team, I was told this particular implementation to not let
> OS take control when acting on that specific UUID based upon a variable
> (NEXP in this case) came from Intel RC code.
>
> That's probably why this is all across a lot of platforms, including non-Dell.
>
> At least in the context of the laptop Peter noticed this on (Dell XPS 13 9350)
> NEXP is set in GNVS based upon Thunderbolt capability.
>
> As for why they return unrecognized UUID instead of just masking all the
> capabilities bits?  It's the same net functional result.  If the vendor provided
> RC code doesn't caused WCHK problems or functional problems it's hard to
> make a case for why it needs to be changed by the OEM.
>
> I think that Peter's patch is appropriate to message this is specifically
> what's going on.

No, it may hide real (ie. non-intentional) bugs in _OSC, so it is not
appropriate.

Debug-level messages really should not hurt anyone (and should never
show up in production anyway).

We can slightly tone down the "_OSC failed (%s); disabling ASPM\n"
message in negotiate_os_control() in drivers/acpi/pci_root.c if you
think it's too strong and that's it.

Thanks,
Rafael