Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 7 Jun 2021 16:10:30 +0200
From:   Joerg Roedel <jroedel@suse.de>
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Joerg Roedel <joro@8bytes.org>,
        Bjorn Helgaas <bhelgaas@google.com>, rjw@rjwysocki.net,
        Len Brown <lenb@kernel.org>, linux-pci@vger.kernel.org,
        linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] PCI/APCI: Move acpi_pci_osc_support() check to
 negotiation phase
Message-ID: <YL4o1pJyIm74Lwz3@suse.de>
References: <20210603124814.19654-1-joro@8bytes.org>
 <20210603205047.GA2135380@bjorn-Precision-5520>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210603205047.GA2135380@bjorn-Precision-5520>
Precedence: bulk

Hi Bjorn,

On Thu, Jun 03, 2021 at 03:50:47PM -0500, Bjorn Helgaas wrote:
> On Thu, Jun 03, 2021 at 02:48:14PM +0200, Joerg Roedel wrote:

> If instead you mean that the OS has *not* been granted DPC control,
> but does _OSC(Query, SUPPORT=x, CONTROL=0), I think that means the OS
> is telling the platform what it supports but not requesting anything.
> That sounds legal to me, so if firmware complains about it, I would
> say it's a firmware problem.

I think it depends on how you look at it. The machine I was working with
has a BIOS setting where one can configure that DPC is controlled by the
OS. When it is configured that way, then the BIOS will issue an error
when an _OSC query is made with control set to 0. This is because it
indicates to the BIOS that the OS does not take control over DPC and
thus that the OS does not support it. The BIOS will issue a warning into
its log and when the Linux later takes control the warning is already
there.

> But please help me out if I'm misunderstanding something above.  I'm
> never confident that I really understand _OSC.

I am also not an _OSC expert, but you an Rafael already provided good
feedback on the necessity of at least one _OSC call, even when Linux
does not want to take control.

> Unrelated to *this* patch, but I don't understand the point of
> OSC_PCI_SUPPORT_MASKS and OSC_PCI_CONTROL_MASKS.  These are all
> internal static functions and it looks like pointless work to apply
> masks here and in acpi_pci_osc_control_set().

Okay, I will add a separate patch removing thos after this change.

> >  	status = acpi_pci_run_osc(root->device->handle, capbuf, &result);
> >  	if (ACPI_SUCCESS(status)) {
> 
> We can also drop the "if (control)" check inside the ACPI_SUCCESS()
> block, can't we?

Right, fixed that up.

Regards,

	Joerg