PCI/ACPI: Disable AER when _OSC control bit is clear.

Commit Message

Yazen Ghannam Jan. 11, 2018, 3:03 p.m. UTC

From: Yazen Ghannam <[e-mail]>

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 <[e-mail]>
---
 drivers/acpi/pci_root.c       | 3 +++
 drivers/pci/pcie/aer/aerdrv.c | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

Comments

Rafael J. Wysocki Jan. 11, 2018, 5:38 p.m. UTC | #1

On Thu, Jan 11, 2018 at 4:03 PM, Yazen Ghannam <[e-mail]> wrote:
> From: Yazen Ghannam <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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);
>  }
> --
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Yazen Ghannam Jan. 11, 2018, 5:48 p.m. UTC | #2

> -----Original Message-----

> From: [e-mail] [mailto:[e-mail]] On Behalf Of

> Rafael J. Wysocki

> Sent: Thursday, January 11, 2018 12:39 PM

> To: Ghannam, Yazen <[e-mail]>

> Cc: ACPI Devel Maling List <[e-mail]>; Linux Kernel Mailing

> List <[e-mail]>; Linux PCI <[e-mail]>;

> Rafael J. Wysocki <[e-mail]>; Len Brown <[e-mail]>; Bjorn

> Helgaas <[e-mail]>; Borislav Petkov <[e-mail]>

> Subject: Re: [PATCH] PCI/ACPI: Disable AER when _OSC control bit is clear.

> 

> On Thu, Jan 11, 2018 at 4:03 PM, Yazen Ghannam

> <[e-mail]> wrote:

> > From: Yazen Ghannam <[e-mail]>

> >

> > 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 <[e-mail]>

> > ---

> >  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

Rafael J. Wysocki Jan. 11, 2018, 6:03 p.m. UTC | #3

On Thu, Jan 11, 2018 at 6:48 PM, Ghannam, Yazen <[e-mail]> wrote:
>> -----Original Message-----
>> From: [e-mail] [mailto:[e-mail]] On Behalf Of
>> Rafael J. Wysocki
>> Sent: Thursday, January 11, 2018 12:39 PM
>> To: Ghannam, Yazen <[e-mail]>
>> Cc: ACPI Devel Maling List <[e-mail]>; Linux Kernel Mailing
>> List <[e-mail]>; Linux PCI <[e-mail]>;
>> Rafael J. Wysocki <[e-mail]>; Len Brown <[e-mail]>; Bjorn
>> Helgaas <[e-mail]>; Borislav Petkov <[e-mail]>
>> Subject: Re: [PATCH] PCI/ACPI: Disable AER when _OSC control bit is clear.
>>
>> On Thu, Jan 11, 2018 at 4:03 PM, Yazen Ghannam
>> <[e-mail]> wrote:
>> > From: Yazen Ghannam <[e-mail]>
>> >
>> > 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 <[e-mail]>
>> > ---
>> >  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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas Jan. 11, 2018, 6:09 p.m. UTC | #4

On Thu, Jan 11, 2018 at 09:03:16AM -0600, Yazen Ghannam wrote:
> From: Yazen Ghannam <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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 [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Borislav Petkov Jan. 11, 2018, 7:04 p.m. UTC | #5

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.

Rafael J. Wysocki Jan. 12, 2018, 12:28 a.m. UTC | #6

On Thu, Jan 11, 2018 at 8:04 PM, Borislav Petkov <[e-mail]> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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);
 }