From: Yazen Ghannam <[email protected]>
Currently, aer_service_init() checks if AER is available and that
Firmware First handling is not enabled. The _OSC request for AER is
not taken into account when deciding to enable AER in Linux.
We should check that the _OSC control for AER is set. If it's not
then AER should be disabled.
The _OSC control for AER is not requested when APEI Firmware First is
used, so the same condition applies.
Mark AER as disabled if the _OSC request was not made or accepted.
Remove redunant check for aer_acpi_firmware_first() when calling
aer_service_init(), since this is check is already included when
checking the _OSC control.
Signed-off-by: Yazen Ghannam <[email protected]>
---
drivers/acpi/pci_root.c | 3 +++
drivers/pci/pcie/aer/aerdrv.c | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 6fc204a52493..19a625ed8de9 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -512,6 +512,9 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
*/
*no_aspm = 1;
}
+
+ if (!(requested & control & OSC_PCI_EXPRESS_AER_CONTROL))
+ pci_no_aer();
}
static int acpi_pci_root_add(struct acpi_device *device,
diff --git a/drivers/pci/pcie/aer/aerdrv.c b/drivers/pci/pcie/aer/aerdrv.c
index 6ff5f5b4f5e6..39bb059777d0 100644
--- a/drivers/pci/pcie/aer/aerdrv.c
+++ b/drivers/pci/pcie/aer/aerdrv.c
@@ -374,7 +374,7 @@ static void aer_error_resume(struct pci_dev *dev)
*/
static int __init aer_service_init(void)
{
- if (!pci_aer_available() || aer_acpi_firmware_first())
+ if (!pci_aer_available())
return -ENXIO;
return pcie_port_service_register(&aerdriver);
}
--
2.14.1
On Thu, Jan 11, 2018 at 4:03 PM, Yazen Ghannam <[email protected]> wrote:
> From: Yazen Ghannam <[email protected]>
>
> Currently, aer_service_init() checks if AER is available and that
> Firmware First handling is not enabled. The _OSC request for AER is
> not taken into account when deciding to enable AER in Linux.
>
> We should check that the _OSC control for AER is set. If it's not
> then AER should be disabled.
>
> The _OSC control for AER is not requested when APEI Firmware First is
> used, so the same condition applies.
>
> Mark AER as disabled if the _OSC request was not made or accepted.
>
> Remove redunant check for aer_acpi_firmware_first() when calling
> aer_service_init(), since this is check is already included when
> checking the _OSC control.
>
> Signed-off-by: Yazen Ghannam <[email protected]>
> ---
> drivers/acpi/pci_root.c | 3 +++
> drivers/pci/pcie/aer/aerdrv.c | 2 +-
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 6fc204a52493..19a625ed8de9 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -512,6 +512,9 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
> */
> *no_aspm = 1;
> }
> +
> + if (!(requested & control & OSC_PCI_EXPRESS_AER_CONTROL))
One of the operators above needs to be a && I suppose?
> + pci_no_aer();
> }
>
> static int acpi_pci_root_add(struct acpi_device *device,
> diff --git a/drivers/pci/pcie/aer/aerdrv.c b/drivers/pci/pcie/aer/aerdrv.c
> index 6ff5f5b4f5e6..39bb059777d0 100644
> --- a/drivers/pci/pcie/aer/aerdrv.c
> +++ b/drivers/pci/pcie/aer/aerdrv.c
> @@ -374,7 +374,7 @@ static void aer_error_resume(struct pci_dev *dev)
> */
> static int __init aer_service_init(void)
> {
> - if (!pci_aer_available() || aer_acpi_firmware_first())
> + if (!pci_aer_available())
> return -ENXIO;
> return pcie_port_service_register(&aerdriver);
> }
> --
> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On Behalf Of
> Rafael J. Wysocki
> Sent: Thursday, January 11, 2018 12:39 PM
> To: Ghannam, Yazen <[email protected]>
> Cc: ACPI Devel Maling List <[email protected]>; Linux Kernel Mailing
> List <[email protected]>; Linux PCI <[email protected]>;
> Rafael J. Wysocki <[email protected]>; Len Brown <[email protected]>; Bjorn
> Helgaas <[email protected]>; Borislav Petkov <[email protected]>
> Subject: Re: [PATCH] PCI/ACPI: Disable AER when _OSC control bit is clear.
>
> On Thu, Jan 11, 2018 at 4:03 PM, Yazen Ghannam
> <[email protected]> wrote:
> > From: Yazen Ghannam <[email protected]>
> >
> > Currently, aer_service_init() checks if AER is available and that
> > Firmware First handling is not enabled. The _OSC request for AER is
> > not taken into account when deciding to enable AER in Linux.
> >
> > We should check that the _OSC control for AER is set. If it's not
> > then AER should be disabled.
> >
> > The _OSC control for AER is not requested when APEI Firmware First is
> > used, so the same condition applies.
> >
> > Mark AER as disabled if the _OSC request was not made or accepted.
> >
> > Remove redunant check for aer_acpi_firmware_first() when calling
> > aer_service_init(), since this is check is already included when
> > checking the _OSC control.
> >
> > Signed-off-by: Yazen Ghannam <[email protected]>
> > ---
> > drivers/acpi/pci_root.c | 3 +++
> > drivers/pci/pcie/aer/aerdrv.c | 2 +-
> > 2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > index 6fc204a52493..19a625ed8de9 100644
> > --- a/drivers/acpi/pci_root.c
> > +++ b/drivers/acpi/pci_root.c
> > @@ -512,6 +512,9 @@ static void negotiate_os_control(struct
> acpi_pci_root *root, int *no_aspm)
> > */
> > *no_aspm = 1;
> > }
> > +
> > + if (!(requested & control & OSC_PCI_EXPRESS_AER_CONTROL))
>
> One of the operators above needs to be a && I suppose?
>
It's a 3-way bitwise AND to check that OSC_PCI_EXPRESS_AER_CONTROL is
set in both "requested" and "control".
IOW, we check if AER was requested by the OS and that the platform
granted the request.
Thanks,
Yazen
On Thu, Jan 11, 2018 at 6:48 PM, Ghannam, Yazen <[email protected]> wrote:
>> -----Original Message-----
>> From: [email protected] [mailto:[email protected]] On Behalf Of
>> Rafael J. Wysocki
>> Sent: Thursday, January 11, 2018 12:39 PM
>> To: Ghannam, Yazen <[email protected]>
>> Cc: ACPI Devel Maling List <[email protected]>; Linux Kernel Mailing
>> List <[email protected]>; Linux PCI <[email protected]>;
>> Rafael J. Wysocki <[email protected]>; Len Brown <[email protected]>; Bjorn
>> Helgaas <[email protected]>; Borislav Petkov <[email protected]>
>> Subject: Re: [PATCH] PCI/ACPI: Disable AER when _OSC control bit is clear.
>>
>> On Thu, Jan 11, 2018 at 4:03 PM, Yazen Ghannam
>> <[email protected]> wrote:
>> > From: Yazen Ghannam <[email protected]>
>> >
>> > Currently, aer_service_init() checks if AER is available and that
>> > Firmware First handling is not enabled. The _OSC request for AER is
>> > not taken into account when deciding to enable AER in Linux.
>> >
>> > We should check that the _OSC control for AER is set. If it's not
>> > then AER should be disabled.
>> >
>> > The _OSC control for AER is not requested when APEI Firmware First is
>> > used, so the same condition applies.
>> >
>> > Mark AER as disabled if the _OSC request was not made or accepted.
>> >
>> > Remove redunant check for aer_acpi_firmware_first() when calling
>> > aer_service_init(), since this is check is already included when
>> > checking the _OSC control.
>> >
>> > Signed-off-by: Yazen Ghannam <[email protected]>
>> > ---
>> > drivers/acpi/pci_root.c | 3 +++
>> > drivers/pci/pcie/aer/aerdrv.c | 2 +-
>> > 2 files changed, 4 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> > index 6fc204a52493..19a625ed8de9 100644
>> > --- a/drivers/acpi/pci_root.c
>> > +++ b/drivers/acpi/pci_root.c
>> > @@ -512,6 +512,9 @@ static void negotiate_os_control(struct
>> acpi_pci_root *root, int *no_aspm)
>> > */
>> > *no_aspm = 1;
>> > }
>> > +
>> > + if (!(requested & control & OSC_PCI_EXPRESS_AER_CONTROL))
>>
>> One of the operators above needs to be a && I suppose?
>>
>
> It's a 3-way bitwise AND to check that OSC_PCI_EXPRESS_AER_CONTROL is
> set in both "requested" and "control".
>
> IOW, we check if AER was requested by the OS and that the platform
> granted the request.
OK
I'll queue this up if Bjorn doesn't object, unless Bjorn wants to
apply it himself.
Thanks,
Rafael
On Thu, Jan 11, 2018 at 09:03:16AM -0600, Yazen Ghannam wrote:
> From: Yazen Ghannam <[email protected]>
>
> Currently, aer_service_init() checks if AER is available and that
> Firmware First handling is not enabled. The _OSC request for AER is
> not taken into account when deciding to enable AER in Linux.
>
> We should check that the _OSC control for AER is set. If it's not
> then AER should be disabled.
>
> The _OSC control for AER is not requested when APEI Firmware First is
> used, so the same condition applies.
>
> Mark AER as disabled if the _OSC request was not made or accepted.
>
> Remove redunant check for aer_acpi_firmware_first() when calling
> aer_service_init(), since this is check is already included when
> checking the _OSC control.
s/redunant/redundant/
The concept seems right to me. Please add a citation to the relevant spec
section. I think this is somewhere in the PCI Firmware spec.
> Signed-off-by: Yazen Ghannam <[email protected]>
> ---
> drivers/acpi/pci_root.c | 3 +++
> drivers/pci/pcie/aer/aerdrv.c | 2 +-
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 6fc204a52493..19a625ed8de9 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -512,6 +512,9 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
> */
> *no_aspm = 1;
> }
> +
> + if (!(requested & control & OSC_PCI_EXPRESS_AER_CONTROL))
> + pci_no_aer();
> }
>
> static int acpi_pci_root_add(struct acpi_device *device,
> diff --git a/drivers/pci/pcie/aer/aerdrv.c b/drivers/pci/pcie/aer/aerdrv.c
> index 6ff5f5b4f5e6..39bb059777d0 100644
> --- a/drivers/pci/pcie/aer/aerdrv.c
> +++ b/drivers/pci/pcie/aer/aerdrv.c
> @@ -374,7 +374,7 @@ static void aer_error_resume(struct pci_dev *dev)
> */
> static int __init aer_service_init(void)
> {
> - if (!pci_aer_available() || aer_acpi_firmware_first())
> + if (!pci_aer_available())
> return -ENXIO;
> return pcie_port_service_register(&aerdriver);
> }
> --
> 2.14.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 11, 2018 at 07:03:23PM +0100, Rafael J. Wysocki wrote:
> >> > + if (!(requested & control & OSC_PCI_EXPRESS_AER_CONTROL))
> >>
> >> One of the operators above needs to be a && I suppose?
> >>
> >
> > It's a 3-way bitwise AND to check that OSC_PCI_EXPRESS_AER_CONTROL is
> > set in both "requested" and "control".
> >
> > IOW, we check if AER was requested by the OS and that the platform
> > granted the request.
>
> OK
This definitely needs a comment - people will keep tripping over this.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
On Thu, Jan 11, 2018 at 8:04 PM, Borislav Petkov <[email protected]> wrote:
> On Thu, Jan 11, 2018 at 07:03:23PM +0100, Rafael J. Wysocki wrote:
>> >> > + if (!(requested & control & OSC_PCI_EXPRESS_AER_CONTROL))
>> >>
>> >> One of the operators above needs to be a && I suppose?
>> >>
>> >
>> > It's a 3-way bitwise AND to check that OSC_PCI_EXPRESS_AER_CONTROL is
>> > set in both "requested" and "control".
>> >
>> > IOW, we check if AER was requested by the OS and that the platform
>> > granted the request.
>>
>> OK
>
> This definitely needs a comment - people will keep tripping over this.
Fair enough.