[PATCH v2] x86: i8237: Register based on FADT legacy boot flag

[PATCH v2] x86: i8237: Register based on FADT legacy boot flag

[Rajneesh Bhardwaj]

>From Skylake onwards, the platform controller hub (Sunrisepoint PCH) does
not support legacy DMA operations to IO ports 81h-83h, 87h, 89h-8Bh, 8Fh.
Currently this driver registers as syscore ops and its resume function is
called on every resume from S3. On Skylake and Kabylake, this causes a
resume delay of around 100ms due to port IO operations, which is a problem.

This change allows to load the driver only when the platform bios
explicitly supports such devices or has a cut-off date earlier than 2017.

Please refer to chapter 21 of 6th Generation Intel® Core™ Processor
Platform Controller Hub Family: BIOS Specification.

https://www.intel.in/content/www/in/en/embedded/products/skylake/u-mobile/software-and-drivers.html

Cc: Alan Cox <alan@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
---
Changes in v2:
 * changed to dma_inb()

This depends on recently introduced dmi_get_bios_year() helper.
https://patchwork.kernel.org/patch/10252151/
 arch/x86/kernel/i8237.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/x86/kernel/i8237.c b/arch/x86/kernel/i8237.c
index 8eeaa81de066..bf5d8104e17b 100644
--- a/arch/x86/kernel/i8237.c
+++ b/arch/x86/kernel/i8237.c
@@ -9,6 +9,7 @@
  * your option) any later version.
  */
 
+#include <linux/dmi.h>
 #include <linux/init.h>
 #include <linux/syscore_ops.h>
 
@@ -49,6 +50,30 @@ static struct syscore_ops i8237_syscore_ops = {
 
 static int __init i8237A_init_ops(void)
 {
+	/*
+	 * From SKL PCH onwards, the port 0x61 bit 4 would stop toggle and
+	 * the legacy DMA device is removed in which the I/O ports (81h-83h,
+	 * 87h, 89h-8Bh, 8Fh)  related to it are removed as well. All
+	 * removed ports must return 0xff for a inb() request.
+	 *
+	 * Note: DMA_PAGE_2 (port 0x81) should not be checked for detecting
+	 * the presence of DMA device since it may be used by BIOS to decode
+	 * LPC traffic for POST codes. Original LPC only decodes one byte of
+	 * port 0x80 but some BIOS may choose to enhance PCH LPC port 0x8x
+	 * decoding.
+	 */
+	if (dma_inb(DMA_PAGE_0) == 0xFF)
+		return -ENODEV;
+
+	/*
+	 * It should be OK to not load this driver as newer SoC may not
+	 * support 8237 DMA or bus mastering from LPC. Platform firmware
+	 * must announce the support for such legacy devices via
+	 * ACPI_FADT_LEGACY_DEVICES field in FADT table.
+	 */
+	if (!x86_platform.legacy.devices.pnpbios && dmi_get_bios_year() >= 2017)
+		return -ENODEV;
+
 	register_syscore_ops(&i8237_syscore_ops);
 	return 0;
 }
-- 
2.7.4

Re: [PATCH v2] x86: i8237: Register based on FADT legacy boot flag

[Andy Shevchenko]

On Wed, 2018-03-14 at 10:47 +0530, Rajneesh Bhardwaj wrote:
> From Skylake onwards, the platform controller hub (Sunrisepoint PCH)
> does
> not support legacy DMA operations to IO ports 81h-83h, 87h, 89h-8Bh,
> 8Fh.
> Currently this driver registers as syscore ops and its resume function
> is
> called on every resume from S3. On Skylake and Kabylake, this causes a
> resume delay of around 100ms due to port IO operations, which is a
> problem.
> 
> This change allows to load the driver only when the platform bios
> explicitly supports such devices or has a cut-off date earlier than
> 2017.
> 
> Please refer to chapter 21 of 6th Generation Intel® Core™ Processor
> Platform Controller Hub Family: BIOS Specification.
> 
> https://www.intel.in/content/www/in/en/embedded/products/skylake/u-mob
> ile/software-and-drivers.html
> 
> Cc: Alan Cox <alan@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> ---
> Changes in v2:
>  * changed to dma_inb()
> 
> This depends on recently introduced dmi_get_bios_year() helper.
> https://patchwork.kernel.org/patch/10252151/
>  arch/x86/kernel/i8237.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/arch/x86/kernel/i8237.c b/arch/x86/kernel/i8237.c
> index 8eeaa81de066..bf5d8104e17b 100644
> --- a/arch/x86/kernel/i8237.c
> +++ b/arch/x86/kernel/i8237.c
> @@ -9,6 +9,7 @@
>   * your option) any later version.
>   */
>  
> +#include <linux/dmi.h>
>  #include <linux/init.h>
>  #include <linux/syscore_ops.h>

As kbuild bot rightfully noticed, you need to add
x86_init.h inclusion to the driver.

>  
> @@ -49,6 +50,30 @@ static struct syscore_ops i8237_syscore_ops = {
>  
>  static int __init i8237A_init_ops(void)
>  {
> +	/*
> +	 * From SKL PCH onwards, the port 0x61 bit 4 would stop
> toggle and
> +	 * the legacy DMA device is removed in which the I/O ports
> (81h-83h,
> +	 * 87h, 89h-8Bh, 8Fh)  related to it are removed as well. All
> +	 * removed ports must return 0xff for a inb() request.
> +	 *
> +	 * Note: DMA_PAGE_2 (port 0x81) should not be checked for
> detecting
> +	 * the presence of DMA device since it may be used by BIOS to
> decode
> +	 * LPC traffic for POST codes. Original LPC only decodes one
> byte of
> +	 * port 0x80 but some BIOS may choose to enhance PCH LPC port
> 0x8x
> +	 * decoding.
> +	 */
> +	if (dma_inb(DMA_PAGE_0) == 0xFF)
> +		return -ENODEV;
> +
> +	/*
> +	 * It should be OK to not load this driver as newer SoC may
> not
> +	 * support 8237 DMA or bus mastering from LPC. Platform
> firmware
> +	 * must announce the support for such legacy devices via
> +	 * ACPI_FADT_LEGACY_DEVICES field in FADT table.
> +	 */
> +	if (!x86_platform.legacy.devices.pnpbios &&
> dmi_get_bios_year() >= 2017)
> +		return -ENODEV;
> +
>  	register_syscore_ops(&i8237_syscore_ops);
>  	return 0;
>  }

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2] x86: i8237: Register based on FADT legacy boot flag

[Thomas Gleixner]

On Wed, 14 Mar 2018, Rajneesh Bhardwaj wrote:
>  static int __init i8237A_init_ops(void)
>  {
> +	/*
> +	 * From SKL PCH onwards, the port 0x61 bit 4 would stop toggle and

s/would stop toggle/stops toggling/

You are describing a fact, right?

> +	 * the legacy DMA device is removed in which the I/O ports (81h-83h,
> +	 * 87h, 89h-8Bh, 8Fh)  related to it are removed as well. All
> +	 * removed ports must return 0xff for a inb() request.
> +	 *
> +	 * Note: DMA_PAGE_2 (port 0x81) should not be checked for detecting
> +	 * the presence of DMA device since it may be used by BIOS to decode
> +	 * LPC traffic for POST codes. Original LPC only decodes one byte of
> +	 * port 0x80 but some BIOS may choose to enhance PCH LPC port 0x8x
> +	 * decoding.
> +	 */
> +	if (dma_inb(DMA_PAGE_0) == 0xFF)
> +		return -ENODEV;
> +
> +	/*
> +	 * It should be OK to not load this driver as newer SoC may not

Should? Is this based on facts or hope?

> +	 * support 8237 DMA or bus mastering from LPC. Platform firmware
> +	 * must announce the support for such legacy devices via
> +	 * ACPI_FADT_LEGACY_DEVICES field in FADT table.
> +	 */
> +	if (!x86_platform.legacy.devices.pnpbios && dmi_get_bios_year() >= 2017)

Please use arch_pnpbios_disabled() and explain why you need that year
check. If there is no pnpbios then why is the year interesting and why
would anyone trust something which comes from BIOS?

Thanks,

	tglx

Re: [PATCH v2] x86: i8237: Register based on FADT legacy boot flag

[Thomas Gleixner]

On Mon, 19 Mar 2018, Anshuman Gupta wrote:
> On Wed, Mar 14, 2018 at 04:07:43PM +0100, Thomas Gleixner wrote:
> > On Wed, 14 Mar 2018, Rajneesh Bhardwaj wrote:
> > > +	 * support 8237 DMA or bus mastering from LPC. Platform firmware
> > > +	 * must announce the support for such legacy devices via
> > > +	 * ACPI_FADT_LEGACY_DEVICES field in FADT table.
> > > +	 */
> > > +	if (!x86_platform.legacy.devices.pnpbios && dmi_get_bios_year() >= 2017)
> > 
> > Please use arch_pnpbios_disabled() and explain why you need that year
>   arch_pnpbios_disabled is defined only when CONFIG_PNPBIOS is set, which may 
>   not be true for all configurations.

The please create a helper function x86_pnpbios_disabled() and use that one
in arch_pnp_bios_disabled() and in your new code.

> > check. If there is no pnpbios then why is the year interesting and why
> > would anyone trust something which comes from BIOS?
>
>   Modern bios fsp have started using to use ACPI_FADT_LEGACY_DEVICES field in
>   FADT table, that is the reason we require a year check for BIOS.
>   https://review.coreboot.org/#/c/coreboot/+/23833/
>   https://review.coreboot.org/#/c/coreboot/+/23835/ 

Sorry, this gerrit stuff is unreadable. Please explain it in your own
words. You have to do that anyway as that information wants to be in a
comment or at least in the change log.

Thanks,

	tglx

[PATCH v3] x86: i8237: Register based on FADT legacy boot flag

[Anshuman Gupta]

From: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>

>From Skylake onwards, the platform controller hub (Sunrisepoint PCH) does
not support legacy DMA operations to IO ports 81h-83h, 87h, 89h-8Bh, 8Fh.
Currently this driver registers as syscore ops and its resume function is
called on every resume from S3. On Skylake and Kabylake, this causes a
resume delay of around 100ms due to port IO operations, which is a problem.

This change allows to load the driver only when the platform bios
explicitly supports such devices or has a cut-off date earlier than 2017.
For example open source system firmware like coreboot started unsetting
ACPI_FADT_LEGACY_DEVICES field in FADT table very recently.
https://github.com/coreboot/coreboot/blob/05132707ca1e13a11cd13c77326bc65011b09feb/src/soc/intel/skylake/acpi.c#L271

Please refer to chapter 21 of 6th Generation Intel® Core™ Processor
Platform Controller Hub Family: BIOS Specification.

https://www.intel.in/content/www/in/en/embedded/products/skylake/u-mobile/software-and-drivers.html

Cc: Alan Cox <alan@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
---
Changes in v3:
 * Added x86_pnpbios_disabled and using it instead of pnpbios.
 * Modified the commit message.

Changes in v2:
 * changed to dma_inb()
---
 arch/x86/include/asm/x86_init.h   |  1 +
 arch/x86/kernel/i8237.c           | 25 +++++++++++++++++++++++++
 arch/x86/kernel/platform-quirks.c |  7 ++++++-
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index fc2f082..b6ceac0 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -286,6 +286,7 @@ extern struct x86_msi_ops x86_msi;
 extern struct x86_io_apic_ops x86_io_apic_ops;
 
 extern void x86_early_init_platform_quirks(void);
+extern bool x86_pnpbios_disabled(void);
 extern void x86_init_noop(void);
 extern void x86_init_uint_noop(unsigned int unused);
 
diff --git a/arch/x86/kernel/i8237.c b/arch/x86/kernel/i8237.c
index 8eeaa81..0a3e70f 100644
--- a/arch/x86/kernel/i8237.c
+++ b/arch/x86/kernel/i8237.c
@@ -9,10 +9,12 @@
  * your option) any later version.
  */
 
+#include <linux/dmi.h>
 #include <linux/init.h>
 #include <linux/syscore_ops.h>
 
 #include <asm/dma.h>
+#include <asm/x86_init.h>
 
 /*
  * This module just handles suspend/resume issues with the
@@ -49,6 +51,29 @@ static struct syscore_ops i8237_syscore_ops = {
 
 static int __init i8237A_init_ops(void)
 {
+	/*
+	 * From SKL PCH onwards, the legacy DMA device is removed in which the
+	 * I/O ports (81h-83h, 87h, 89h-8Bh, 8Fh) related to it are removed
+	 * as well. All removed ports must return 0xff for a inb() request.
+	 *
+	 * Note: DMA_PAGE_2 (port 0x81) should not be checked for detecting
+	 * the presence of DMA device since it may be used by BIOS to decode
+	 * LPC traffic for POST codes. Original LPC only decodes one byte of
+	 * port 0x80 but some BIOS may choose to enhance PCH LPC port 0x8x
+	 * decoding.
+	 */
+	if (dma_inb(DMA_PAGE_0) == 0xFF)
+		return -ENODEV;
+
+	/*
+	 * It is not required to load this driver as newer SoC may not
+	 * support 8237 DMA or bus mastering from LPC. Platform firmware
+	 * must announce the support for such legacy devices via
+	 * ACPI_FADT_LEGACY_DEVICES field in FADT table.
+	 */
+	if (x86_pnpbios_disabled() && dmi_get_bios_year() >= 2017)
+		return -ENODEV;
+
 	register_syscore_ops(&i8237_syscore_ops);
 	return 0;
 }
diff --git a/arch/x86/kernel/platform-quirks.c b/arch/x86/kernel/platform-quirks.c
index 235fe60..b348a67 100644
--- a/arch/x86/kernel/platform-quirks.c
+++ b/arch/x86/kernel/platform-quirks.c
@@ -33,9 +33,14 @@ void __init x86_early_init_platform_quirks(void)
 		x86_platform.set_legacy_features();
 }
 
+bool __init x86_pnpbios_disabled(void)
+{
+	return x86_platform.legacy.devices.pnpbios == 0;
+}
+
 #if defined(CONFIG_PNPBIOS)
 bool __init arch_pnpbios_disabled(void)
 {
-	return x86_platform.legacy.devices.pnpbios == 0;
+	return x86_pnpbios_disabled();
 }
 #endif
-- 
2.7.4

Re: [PATCH v3] x86: i8237: Register based on FADT legacy boot flag

[Rajneesh Bhardwaj]

On Thu, Mar 22, 2018 at 03:51:58PM +0530, Anshuman Gupta wrote:

Adding Thomas.

> From: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> 
> From Skylake onwards, the platform controller hub (Sunrisepoint PCH) does
> not support legacy DMA operations to IO ports 81h-83h, 87h, 89h-8Bh, 8Fh.
> Currently this driver registers as syscore ops and its resume function is
> called on every resume from S3. On Skylake and Kabylake, this causes a
> resume delay of around 100ms due to port IO operations, which is a problem.
> 
> This change allows to load the driver only when the platform bios
> explicitly supports such devices or has a cut-off date earlier than 2017.
> For example open source system firmware like coreboot started unsetting
> ACPI_FADT_LEGACY_DEVICES field in FADT table very recently.
> https://github.com/coreboot/coreboot/blob/05132707ca1e13a11cd13c77326bc65011b09feb/src/soc/intel/skylake/acpi.c#L271
> 
> Please refer to chapter 21 of 6th Generation Intel® Core™ Processor
> Platform Controller Hub Family: BIOS Specification.
> 
> https://www.intel.in/content/www/in/en/embedded/products/skylake/u-mobile/software-and-drivers.html
> 
> Cc: Alan Cox <alan@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> ---
> Changes in v3:
>  * Added x86_pnpbios_disabled and using it instead of pnpbios.
>  * Modified the commit message.
> 
> Changes in v2:
>  * changed to dma_inb()
> ---
>  arch/x86/include/asm/x86_init.h   |  1 +
>  arch/x86/kernel/i8237.c           | 25 +++++++++++++++++++++++++
>  arch/x86/kernel/platform-quirks.c |  7 ++++++-
>  3 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> index fc2f082..b6ceac0 100644
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -286,6 +286,7 @@ extern struct x86_msi_ops x86_msi;
>  extern struct x86_io_apic_ops x86_io_apic_ops;
>  
>  extern void x86_early_init_platform_quirks(void);
> +extern bool x86_pnpbios_disabled(void);
>  extern void x86_init_noop(void);
>  extern void x86_init_uint_noop(unsigned int unused);
>  
> diff --git a/arch/x86/kernel/i8237.c b/arch/x86/kernel/i8237.c
> index 8eeaa81..0a3e70f 100644
> --- a/arch/x86/kernel/i8237.c
> +++ b/arch/x86/kernel/i8237.c
> @@ -9,10 +9,12 @@
>   * your option) any later version.
>   */
>  
> +#include <linux/dmi.h>
>  #include <linux/init.h>
>  #include <linux/syscore_ops.h>
>  
>  #include <asm/dma.h>
> +#include <asm/x86_init.h>
>  
>  /*
>   * This module just handles suspend/resume issues with the
> @@ -49,6 +51,29 @@ static struct syscore_ops i8237_syscore_ops = {
>  
>  static int __init i8237A_init_ops(void)
>  {
> +	/*
> +	 * From SKL PCH onwards, the legacy DMA device is removed in which the
> +	 * I/O ports (81h-83h, 87h, 89h-8Bh, 8Fh) related to it are removed
> +	 * as well. All removed ports must return 0xff for a inb() request.
> +	 *
> +	 * Note: DMA_PAGE_2 (port 0x81) should not be checked for detecting
> +	 * the presence of DMA device since it may be used by BIOS to decode
> +	 * LPC traffic for POST codes. Original LPC only decodes one byte of
> +	 * port 0x80 but some BIOS may choose to enhance PCH LPC port 0x8x
> +	 * decoding.
> +	 */
> +	if (dma_inb(DMA_PAGE_0) == 0xFF)
> +		return -ENODEV;
> +
> +	/*
> +	 * It is not required to load this driver as newer SoC may not
> +	 * support 8237 DMA or bus mastering from LPC. Platform firmware
> +	 * must announce the support for such legacy devices via
> +	 * ACPI_FADT_LEGACY_DEVICES field in FADT table.
> +	 */
> +	if (x86_pnpbios_disabled() && dmi_get_bios_year() >= 2017)
> +		return -ENODEV;
> +
>  	register_syscore_ops(&i8237_syscore_ops);
>  	return 0;
>  }
> diff --git a/arch/x86/kernel/platform-quirks.c b/arch/x86/kernel/platform-quirks.c
> index 235fe60..b348a67 100644
> --- a/arch/x86/kernel/platform-quirks.c
> +++ b/arch/x86/kernel/platform-quirks.c
> @@ -33,9 +33,14 @@ void __init x86_early_init_platform_quirks(void)
>  		x86_platform.set_legacy_features();
>  }
>  
> +bool __init x86_pnpbios_disabled(void)
> +{
> +	return x86_platform.legacy.devices.pnpbios == 0;
> +}
> +
>  #if defined(CONFIG_PNPBIOS)
>  bool __init arch_pnpbios_disabled(void)
>  {
> -	return x86_platform.legacy.devices.pnpbios == 0;
> +	return x86_pnpbios_disabled();
>  }
>  #endif
> -- 
> 2.7.4
> 

-- 
Best Regards,
Rajneesh

Re: [PATCH v3] x86: i8237: Register based on FADT legacy boot flag

[Thomas Gleixner]

On Thu, 22 Mar 2018, Rajneesh Bhardwaj wrote:

> On Thu, Mar 22, 2018 at 03:51:58PM +0530, Anshuman Gupta wrote:
> 
> Adding Thomas.

I'm already there via x86@kernel.org :)

But thanks for noticing nevertheless.

    tglx

Re: [PATCH v3] x86: i8237: Register based on FADT legacy boot flag

[Thomas Gleixner]

On Thu, 22 Mar 2018, Anshuman Gupta wrote:

> From: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> 
> >From Skylake onwards, the platform controller hub (Sunrisepoint PCH) does
> not support legacy DMA operations to IO ports 81h-83h, 87h, 89h-8Bh, 8Fh.
> Currently this driver registers as syscore ops and its resume function is
> called on every resume from S3. On Skylake and Kabylake, this causes a
> resume delay of around 100ms due to port IO operations, which is a problem.
> 
> This change allows to load the driver only when the platform bios
> explicitly supports such devices or has a cut-off date earlier than 2017.

Please explain WHY 2017 is the cut-off date. I still have no clue how that
is decided aside of being a random number.

Thanks,

	tglx

Re: [PATCH v3] x86: i8237: Register based on FADT legacy boot flag

[Rajneesh Bhardwaj]

On Sun, Mar 25, 2018 at 01:50:40PM +0200, Thomas Gleixner wrote:
> On Thu, 22 Mar 2018, Anshuman Gupta wrote:
> 
> > From: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> > 
> > >From Skylake onwards, the platform controller hub (Sunrisepoint PCH) does
> > not support legacy DMA operations to IO ports 81h-83h, 87h, 89h-8Bh, 8Fh.
> > Currently this driver registers as syscore ops and its resume function is
> > called on every resume from S3. On Skylake and Kabylake, this causes a
> > resume delay of around 100ms due to port IO operations, which is a problem.
> > 
> > This change allows to load the driver only when the platform bios
> > explicitly supports such devices or has a cut-off date earlier than 2017.
> 
> Please explain WHY 2017 is the cut-off date. I still have no clue how that
> is decided aside of being a random number.

Hello Thomas,

We tested on few Intel platforms such as Skylake, Kabylake, Geminilake etc
and realized that the BIOS always sets the FADT flag to be true though the
device may not be physically present on the SoC. This is a BIOS bug. To keep
the impact minimum, we decided to add a cut-off date since we are not aware
of any BIOS (other than the coreboot link provided in the commit msg) that
properly sets this field. SoCs released after Skylake will not have this DMA
device on the PCH. So, because of these two reasons, we decided to add a
cut-off date as 2017.

Please let us know if you feel strongly about it and we can change it or
remove it if you feel so.

Ideally, we didnt want to add this BIOS check at all and only wanted to use
inb() approch but unfortunately, that too was broken for port 0x81.

@Rafael / Alan / Andy - Please add more or correct me in case of anything
missed  or not communicated fully.

> 
> Thanks,
> 
> 	tglx

-- 
Best Regards,
Rajneesh

Re: [PATCH v3] x86: i8237: Register based on FADT legacy boot flag

[Thomas Gleixner]

On Mon, 26 Mar 2018, Rajneesh Bhardwaj wrote:

> On Sun, Mar 25, 2018 at 01:50:40PM +0200, Thomas Gleixner wrote:
> > On Thu, 22 Mar 2018, Anshuman Gupta wrote:
> > 
> > > From: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> > > 
> > > >From Skylake onwards, the platform controller hub (Sunrisepoint PCH) does
> > > not support legacy DMA operations to IO ports 81h-83h, 87h, 89h-8Bh, 8Fh.
> > > Currently this driver registers as syscore ops and its resume function is
> > > called on every resume from S3. On Skylake and Kabylake, this causes a
> > > resume delay of around 100ms due to port IO operations, which is a problem.
> > > 
> > > This change allows to load the driver only when the platform bios
> > > explicitly supports such devices or has a cut-off date earlier than 2017.
> > 
> > Please explain WHY 2017 is the cut-off date. I still have no clue how that
> > is decided aside of being a random number.
> 
> Hello Thomas,
> 
> We tested on few Intel platforms such as Skylake, Kabylake, Geminilake etc
> and realized that the BIOS always sets the FADT flag to be true though the
> device may not be physically present on the SoC. This is a BIOS bug. To keep
> the impact minimum, we decided to add a cut-off date since we are not aware
> of any BIOS (other than the coreboot link provided in the commit msg) that
> properly sets this field. SoCs released after Skylake will not have this DMA
> device on the PCH. So, because of these two reasons, we decided to add a
> cut-off date as 2017.
> 
> Please let us know if you feel strongly about it and we can change it or
> remove it if you feel so.

I don't feel strongly about the cut off itself, but I want a reasonable
explanation in the changelog or code comment because half a year from now
nobody remembers ....

Thanks,

	tglx

Re: [PATCH v3] x86: i8237: Register based on FADT legacy boot flag

[hpa]

On March 26, 2018 2:11:51 AM PDT, Thomas Gleixner <tglx@linutronix.de> wrote:
>On Mon, 26 Mar 2018, Rajneesh Bhardwaj wrote:
>
>> On Sun, Mar 25, 2018 at 01:50:40PM +0200, Thomas Gleixner wrote:
>> > On Thu, 22 Mar 2018, Anshuman Gupta wrote:
>> > 
>> > > From: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
>> > > 
>> > > >From Skylake onwards, the platform controller hub (Sunrisepoint
>PCH) does
>> > > not support legacy DMA operations to IO ports 81h-83h, 87h,
>89h-8Bh, 8Fh.
>> > > Currently this driver registers as syscore ops and its resume
>function is
>> > > called on every resume from S3. On Skylake and Kabylake, this
>causes a
>> > > resume delay of around 100ms due to port IO operations, which is
>a problem.
>> > > 
>> > > This change allows to load the driver only when the platform bios
>> > > explicitly supports such devices or has a cut-off date earlier
>than 2017.
>> > 
>> > Please explain WHY 2017 is the cut-off date. I still have no clue
>how that
>> > is decided aside of being a random number.
>> 
>> Hello Thomas,
>> 
>> We tested on few Intel platforms such as Skylake, Kabylake,
>Geminilake etc
>> and realized that the BIOS always sets the FADT flag to be true
>though the
>> device may not be physically present on the SoC. This is a BIOS bug.
>To keep
>> the impact minimum, we decided to add a cut-off date since we are not
>aware
>> of any BIOS (other than the coreboot link provided in the commit msg)
>that
>> properly sets this field. SoCs released after Skylake will not have
>this DMA
>> device on the PCH. So, because of these two reasons, we decided to
>add a
>> cut-off date as 2017.
>> 
>> Please let us know if you feel strongly about it and we can change it
>or
>> remove it if you feel so.
>
>I don't feel strongly about the cut off itself, but I want a reasonable
>explanation in the changelog or code comment because half a year from
>now
>nobody remembers ....
>
>Thanks,
>
>	tglx

Can we probe safely for this device?
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v3] x86: i8237: Register based on FADT legacy boot flag

[Rajneesh Bhardwaj]

On Mon, Mar 26, 2018 at 03:34:44AM -0700, hpa@zytor.com wrote:
> On March 26, 2018 2:11:51 AM PDT, Thomas Gleixner <tglx@linutronix.de> wrote:
> >On Mon, 26 Mar 2018, Rajneesh Bhardwaj wrote:
> >
> >> On Sun, Mar 25, 2018 at 01:50:40PM +0200, Thomas Gleixner wrote:
> >> > On Thu, 22 Mar 2018, Anshuman Gupta wrote:
> >> > 
> >> > > From: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> >> > > 
> >> > > >From Skylake onwards, the platform controller hub (Sunrisepoint
> >PCH) does
> >> > > not support legacy DMA operations to IO ports 81h-83h, 87h,
> >89h-8Bh, 8Fh.
> >> > > Currently this driver registers as syscore ops and its resume
> >function is
> >> > > called on every resume from S3. On Skylake and Kabylake, this
> >causes a
> >> > > resume delay of around 100ms due to port IO operations, which is
> >a problem.
> >> > > 
> >> > > This change allows to load the driver only when the platform bios
> >> > > explicitly supports such devices or has a cut-off date earlier
> >than 2017.
> >> > 
> >> > Please explain WHY 2017 is the cut-off date. I still have no clue
> >how that
> >> > is decided aside of being a random number.
> >> 
> >> Hello Thomas,
> >> 
> >> We tested on few Intel platforms such as Skylake, Kabylake,
> >Geminilake etc
> >> and realized that the BIOS always sets the FADT flag to be true
> >though the
> >> device may not be physically present on the SoC. This is a BIOS bug.
> >To keep
> >> the impact minimum, we decided to add a cut-off date since we are not
> >aware
> >> of any BIOS (other than the coreboot link provided in the commit msg)
> >that
> >> properly sets this field. SoCs released after Skylake will not have
> >this DMA
> >> device on the PCH. So, because of these two reasons, we decided to
> >add a
> >> cut-off date as 2017.
> >> 
> >> Please let us know if you feel strongly about it and we can change it
> >or
> >> remove it if you feel so.
> >
> >I don't feel strongly about the cut off itself, but I want a reasonable
> >explanation in the changelog or code comment because half a year from
> >now
> >nobody remembers ....
> >
> >Thanks,
> >
> >	tglx
> 
> Can we probe safely for this device?

Hi Peter - Apparently No, that's why we are trying all these indirect means to
determine the presence of the DMA device. As you might have noticed, this
driver registers as syscore ops and not on the basis of a ACPI object or
HWID or anything else.

> -- 
> Sent from my Android device with K-9 Mail. Please excuse my brevity.

-- 
Best Regards,
Rajneesh

Re: [PATCH v3] x86: i8237: Register based on FADT legacy boot flag

[Alan Cox]

> Please explain WHY 2017 is the cut-off date. I still have no clue how
> that
> is decided aside of being a random number.

Because if it's prior to 2017 then it'll almost certainly have those
registers even if the firmware says otherwise. After that point we
think the firmware is going to give valid answers (although the 0xFF
check is actually the important one in reality).

Alan

Re: [PATCH v3] x86: i8237: Register based on FADT legacy boot flag

[Alan Cox]

> Can we probe safely for this device?

99% of the time yes the inb gives us a straight answer. However (and
we've hit this with port 0x81 for real) there are concerns that some
systems will trap those addresses into SMM and do weirdness that makes
the 0xFF check fail.

Alan

[PATCH v4] x86: i8237: Register based on FADT legacy boot flag

[Anshuman Gupta]

From: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>

>From Skylake onwards, the platform controller hub (Sunrisepoint PCH) does
not support legacy DMA operations to IO ports 81h-83h, 87h, 89h-8Bh, 8Fh.
Currently this driver registers as syscore ops and its resume function is
called on every resume from S3. On Skylake and Kabylake, this causes a
resume delay of around 100ms due to port IO operations, which is a problem.

This change allows to load the driver only when the platform bios
explicitly supports such devices or has a cut-off date earlier than 2017
due to the following reasons:

   - The platforms released before year 2017 have support for the 8237.
     (except Sunrisepoint PCH e.g. Skylake)

   - Some of the BIOS that were released for platforms (Skylake, Kabylake)
     during 2016-17 are buggy. These BIOS do not set/unset the
     ACPI_FADT_LEGACY_DEVICES field in FADT table properly based on the
     presence or absence of the DMA device.

Very recently, open source system firmware like coreboot started unsetting
ACPI_FADT_LEGACY_DEVICES field in FADT table if the 8237 DMA device is not
present on the PCH.
https://github.com/coreboot/coreboot/blob/05132707ca1e13a11cd13c77326bc65011b09feb/src/soc/intel/skylake/acpi.c#L271

Please refer to chapter 21 of 6th Generation Intel® Core™ Processor
Platform Controller Hub Family: BIOS Specification.

https://www.intel.in/content/www/in/en/embedded/products/skylake/u-mobile/software-and-drivers.html

Cc: Alan Cox <alan@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
---
    Changes in v4:
     * Modified the commit message about cut-off date year 2017.
    
    Changes in v3:
     * Added x86_pnpbios_disabled and using it instead of pnpbios.
     * Modified the commit message.
    
    Changes in v2:
     * changed to dma_inb()
---
 arch/x86/include/asm/x86_init.h   |  1 +
 arch/x86/kernel/i8237.c           | 25 +++++++++++++++++++++++++
 arch/x86/kernel/platform-quirks.c |  7 ++++++-
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index fc2f082..b6ceac0 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -286,6 +286,7 @@ extern struct x86_msi_ops x86_msi;
 extern struct x86_io_apic_ops x86_io_apic_ops;
 
 extern void x86_early_init_platform_quirks(void);
+extern bool x86_pnpbios_disabled(void);
 extern void x86_init_noop(void);
 extern void x86_init_uint_noop(unsigned int unused);
 
diff --git a/arch/x86/kernel/i8237.c b/arch/x86/kernel/i8237.c
index 8eeaa81..0a3e70f 100644
--- a/arch/x86/kernel/i8237.c
+++ b/arch/x86/kernel/i8237.c
@@ -9,10 +9,12 @@
  * your option) any later version.
  */
 
+#include <linux/dmi.h>
 #include <linux/init.h>
 #include <linux/syscore_ops.h>
 
 #include <asm/dma.h>
+#include <asm/x86_init.h>
 
 /*
  * This module just handles suspend/resume issues with the
@@ -49,6 +51,29 @@ static struct syscore_ops i8237_syscore_ops = {
 
 static int __init i8237A_init_ops(void)
 {
+	/*
+	 * From SKL PCH onwards, the legacy DMA device is removed in which the
+	 * I/O ports (81h-83h, 87h, 89h-8Bh, 8Fh) related to it are removed
+	 * as well. All removed ports must return 0xff for a inb() request.
+	 *
+	 * Note: DMA_PAGE_2 (port 0x81) should not be checked for detecting
+	 * the presence of DMA device since it may be used by BIOS to decode
+	 * LPC traffic for POST codes. Original LPC only decodes one byte of
+	 * port 0x80 but some BIOS may choose to enhance PCH LPC port 0x8x
+	 * decoding.
+	 */
+	if (dma_inb(DMA_PAGE_0) == 0xFF)
+		return -ENODEV;
+
+	/*
+	 * It is not required to load this driver as newer SoC may not
+	 * support 8237 DMA or bus mastering from LPC. Platform firmware
+	 * must announce the support for such legacy devices via
+	 * ACPI_FADT_LEGACY_DEVICES field in FADT table.
+	 */
+	if (x86_pnpbios_disabled() && dmi_get_bios_year() >= 2017)
+		return -ENODEV;
+
 	register_syscore_ops(&i8237_syscore_ops);
 	return 0;
 }
diff --git a/arch/x86/kernel/platform-quirks.c b/arch/x86/kernel/platform-quirks.c
index 235fe60..b348a67 100644
--- a/arch/x86/kernel/platform-quirks.c
+++ b/arch/x86/kernel/platform-quirks.c
@@ -33,9 +33,14 @@ void __init x86_early_init_platform_quirks(void)
 		x86_platform.set_legacy_features();
 }
 
+bool __init x86_pnpbios_disabled(void)
+{
+	return x86_platform.legacy.devices.pnpbios == 0;
+}
+
 #if defined(CONFIG_PNPBIOS)
 bool __init arch_pnpbios_disabled(void)
 {
-	return x86_platform.legacy.devices.pnpbios == 0;
+	return x86_pnpbios_disabled();
 }
 #endif
-- 
2.7.4

[tip:x86/dma] x86/i8237: Register device based on FADT legacy boot flag

[tip-bot for Rajneesh Bhardwaj]

Commit-ID:  f79b1c573cb4dc551919f81ed5797419f6fc1f3a
Gitweb:     https://git.kernel.org/tip/f79b1c573cb4dc551919f81ed5797419f6fc1f3a
Author:     Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
AuthorDate: Thu, 29 Mar 2018 20:36:55 +0530
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Fri, 27 Apr 2018 16:44:29 +0200

x86/i8237: Register device based on FADT legacy boot flag

>From Skylake onwards, the platform controller hub (Sunrisepoint PCH) does
not support legacy DMA operations to IO ports 81h-83h, 87h, 89h-8Bh, 8Fh.
Currently this driver registers as syscore ops and its resume function is
called on every resume from S3. On Skylake and Kabylake, this causes a
resume delay of around 100ms due to port IO operations, which is a problem.

This change allows to load the driver only when the platform bios
explicitly supports such devices or has a cut-off date earlier than 2017
due to the following reasons:

   - The platforms released before year 2017 have support for the 8237.
     (except Sunrisepoint PCH e.g. Skylake)

   - Some of the BIOS that were released for platforms (Skylake, Kabylake)
     during 2016-17 are buggy. These BIOS do not set/unset the
     ACPI_FADT_LEGACY_DEVICES field in FADT table properly based on the
     presence or absence of the DMA device.

Very recently, open source system firmware like coreboot started unsetting
ACPI_FADT_LEGACY_DEVICES field in FADT table if the 8237 DMA device is not
present on the PCH.

Please refer to chapter 21 of 6th Generation Intel® Core™ Processor
Platform Controller Hub Family: BIOS Specification.

Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: rjw@rjwysocki.net
Cc: hpa@zytor.com
Cc: Alan Cox <alan@linux.intel.com>
Link: https://lkml.kernel.org/r/1522336015-22994-1-git-send-email-anshuman.gupta@intel.com
---
 arch/x86/include/asm/x86_init.h   |  1 +
 arch/x86/kernel/i8237.c           | 25 +++++++++++++++++++++++++
 arch/x86/kernel/platform-quirks.c |  7 ++++++-
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index ce8b4da07e35..db98e3ab3295 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -301,5 +301,6 @@ extern struct x86_apic_ops x86_apic_ops;
 extern void x86_early_init_platform_quirks(void);
 extern void x86_init_noop(void);
 extern void x86_init_uint_noop(unsigned int unused);
+extern bool x86_pnpbios_disabled(void);
 
 #endif
diff --git a/arch/x86/kernel/i8237.c b/arch/x86/kernel/i8237.c
index 8eeaa81de066..0a3e70fd00d6 100644
--- a/arch/x86/kernel/i8237.c
+++ b/arch/x86/kernel/i8237.c
@@ -9,10 +9,12 @@
  * your option) any later version.
  */
 
+#include <linux/dmi.h>
 #include <linux/init.h>
 #include <linux/syscore_ops.h>
 
 #include <asm/dma.h>
+#include <asm/x86_init.h>
 
 /*
  * This module just handles suspend/resume issues with the
@@ -49,6 +51,29 @@ static struct syscore_ops i8237_syscore_ops = {
 
 static int __init i8237A_init_ops(void)
 {
+	/*
+	 * From SKL PCH onwards, the legacy DMA device is removed in which the
+	 * I/O ports (81h-83h, 87h, 89h-8Bh, 8Fh) related to it are removed
+	 * as well. All removed ports must return 0xff for a inb() request.
+	 *
+	 * Note: DMA_PAGE_2 (port 0x81) should not be checked for detecting
+	 * the presence of DMA device since it may be used by BIOS to decode
+	 * LPC traffic for POST codes. Original LPC only decodes one byte of
+	 * port 0x80 but some BIOS may choose to enhance PCH LPC port 0x8x
+	 * decoding.
+	 */
+	if (dma_inb(DMA_PAGE_0) == 0xFF)
+		return -ENODEV;
+
+	/*
+	 * It is not required to load this driver as newer SoC may not
+	 * support 8237 DMA or bus mastering from LPC. Platform firmware
+	 * must announce the support for such legacy devices via
+	 * ACPI_FADT_LEGACY_DEVICES field in FADT table.
+	 */
+	if (x86_pnpbios_disabled() && dmi_get_bios_year() >= 2017)
+		return -ENODEV;
+
 	register_syscore_ops(&i8237_syscore_ops);
 	return 0;
 }
diff --git a/arch/x86/kernel/platform-quirks.c b/arch/x86/kernel/platform-quirks.c
index 235fe6008ac8..b348a672f71d 100644
--- a/arch/x86/kernel/platform-quirks.c
+++ b/arch/x86/kernel/platform-quirks.c
@@ -33,9 +33,14 @@ void __init x86_early_init_platform_quirks(void)
 		x86_platform.set_legacy_features();
 }
 
+bool __init x86_pnpbios_disabled(void)
+{
+	return x86_platform.legacy.devices.pnpbios == 0;
+}
+
 #if defined(CONFIG_PNPBIOS)
 bool __init arch_pnpbios_disabled(void)
 {
-	return x86_platform.legacy.devices.pnpbios == 0;
+	return x86_pnpbios_disabled();
 }
 #endif