[PATCH] ACPI, APEI: Fixup common access width firmware bug

[PATCH] ACPI, APEI: Fixup common access width firmware bug

[Jean Delvare]

Many firmwares have a common register definition bug where 8-bit
access width is specified for a 32-bit register. Ideally this should
be fixed in the BIOS, but earlier versions of the kernel did not
complain, so fix that up silently.

This closes kernel bug #43282:
https://bugzilla.kernel.org/show_bug.cgi?id=43282

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Gary Hade <garyhade@us.ibm.com>
Cc: Len Brown <len.brown@intel.com>
Cc: stable@vger.kernel.org [3.4+]
---
 drivers/acpi/apei/apei-base.c |    5 +++++
 1 file changed, 5 insertions(+)

--- linux-3.4.orig/drivers/acpi/apei/apei-base.c	2012-06-08 10:02:06.000000000 +0200
+++ linux-3.4/drivers/acpi/apei/apei-base.c	2012-06-08 10:04:16.503779775 +0200
@@ -586,6 +586,11 @@ static int apei_check_gar(struct acpi_ge
 	}
 	*access_bit_width = 1UL << (access_size_code + 2);
 
+	/* Fixup common BIOS bug */
+	if (bit_width == 32 && bit_offset == 0 && (*paddr & 0x03) == 0 &&
+	    *access_bit_width < 32)
+		*access_bit_width = 32;
+
 	if ((bit_width + bit_offset) > *access_bit_width) {
 		pr_warning(FW_BUG APEI_PFX
 			   "Invalid bit width + offset in GAR [0x%llx/%u/%u/%u/%u]\n",

-- 
Jean Delvare
Suse L3

Re: [PATCH] ACPI, APEI: Fixup common access width firmware bug

[Huang Ying]

On Tue, 2012-06-12 at 10:43 +0200, Jean Delvare wrote:
> Many firmwares have a common register definition bug where 8-bit
> access width is specified for a 32-bit register. Ideally this should
> be fixed in the BIOS, but earlier versions of the kernel did not
> complain, so fix that up silently.
> 
> This closes kernel bug #43282:
> https://bugzilla.kernel.org/show_bug.cgi?id=43282
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Gary Hade <garyhade@us.ibm.com>
> Cc: Len Brown <len.brown@intel.com>
> Cc: stable@vger.kernel.org [3.4+]
Acked-by: Huang Ying <ying.huang@intel.com>

Best Regards,
Huang Ying

Re: [PATCH] ACPI, APEI: Fixup common access width firmware bug

[Gary Hade]

On Tue, Jun 12, 2012 at 10:43:28AM +0200, Jean Delvare wrote:
> Many firmwares have a common register definition bug where 8-bit
> access width is specified for a 32-bit register. Ideally this should
> be fixed in the BIOS, but earlier versions of the kernel did not
> complain, so fix that up silently.
> 
> This closes kernel bug #43282:
> https://bugzilla.kernel.org/show_bug.cgi?id=43282
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Gary Hade <garyhade@us.ibm.com>
> Cc: Len Brown <len.brown@intel.com>
> Cc: stable@vger.kernel.org [3.4+]
> ---
>  drivers/acpi/apei/apei-base.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> --- linux-3.4.orig/drivers/acpi/apei/apei-base.c	2012-06-08 10:02:06.000000000 +0200
> +++ linux-3.4/drivers/acpi/apei/apei-base.c	2012-06-08 10:04:16.503779775 +0200
> @@ -586,6 +586,11 @@ static int apei_check_gar(struct acpi_ge
>  	}
>  	*access_bit_width = 1UL << (access_size_code + 2);
> 
> +	/* Fixup common BIOS bug */
> +	if (bit_width == 32 && bit_offset == 0 && (*paddr & 0x03) == 0 &&
> +	    *access_bit_width < 32)
> +		*access_bit_width = 32;
> +
>  	if ((bit_width + bit_offset) > *access_bit_width) {
>  		pr_warning(FW_BUG APEI_PFX
>  			   "Invalid bit width + offset in GAR [0x%llx/%u/%u/%u/%u]\n",

This seems reasonable but since the "Access Size < Register Bit Width"
condition has apparently been seen on a number of systems I have been
wondering if this might simply be a BIOS writer's way of telling us
not to touch some of the high bits in the register.  Reducing the
width of the register seems like a better way, but maybe not the
only way?

Jean, What do you (and others that are listening who know more
about this than I do) think about a change to sanity check only
the bit offset and not complain (at least for now) about cases
where an access using the specified access size will not address
every bit in the register?  

Perhaps something like:
--- linux-3.5-rc2/drivers/acpi/apei/apei-base.c.ORIG	2012-06-08 18:40:09.000000000 -0700
+++ linux-3.5-rc2/drivers/acpi/apei/apei-base.c	2012-06-13 16:32:49.000000000 -0700
@@ -586,9 +586,9 @@ static int apei_check_gar(struct acpi_ge
 	}
 	*access_bit_width = 1UL << (access_size_code + 2);
 
-	if ((bit_width + bit_offset) > *access_bit_width) {
+	if (bit_offset >= *access_bit_width) {
 		pr_warning(FW_BUG APEI_PFX
-			   "Invalid bit width + offset in GAR [0x%llx/%u/%u/%u/%u]\n",
+			   "Invalid bit offset in GAR [0x%llx/%u/%u/%u/%u]\n",
 			   *paddr, bit_width, bit_offset, access_size_code,
 			   space_id);
 		return -EINVAL;

Gary

-- 
Gary Hade
System x Enablement
IBM Linux Technology Center
503-578-4503  IBM T/L: 775-4503
garyhade@us.ibm.com
http://www.ibm.com/linux/ltc

Re: [PATCH] ACPI, APEI: Fixup common access width firmware bug

[Jean Delvare]

Hi Gary,

Le mercredi 13 juin 2012 à 18:09 -0700, Gary Hade a écrit :
> On Tue, Jun 12, 2012 at 10:43:28AM +0200, Jean Delvare wrote:
> > Many firmwares have a common register definition bug where 8-bit
> > access width is specified for a 32-bit register. Ideally this should
> > be fixed in the BIOS, but earlier versions of the kernel did not
> > complain, so fix that up silently.
> > 
> > This closes kernel bug #43282:
> > https://bugzilla.kernel.org/show_bug.cgi?id=43282
> > 
> > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > Cc: Gary Hade <garyhade@us.ibm.com>
> > Cc: Len Brown <len.brown@intel.com>
> > Cc: stable@vger.kernel.org [3.4+]
> > ---
> >  drivers/acpi/apei/apei-base.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > --- linux-3.4.orig/drivers/acpi/apei/apei-base.c	2012-06-08 10:02:06.000000000 +0200
> > +++ linux-3.4/drivers/acpi/apei/apei-base.c	2012-06-08 10:04:16.503779775 +0200
> > @@ -586,6 +586,11 @@ static int apei_check_gar(struct acpi_ge
> >  	}
> >  	*access_bit_width = 1UL << (access_size_code + 2);
> > 
> > +	/* Fixup common BIOS bug */
> > +	if (bit_width == 32 && bit_offset == 0 && (*paddr & 0x03) == 0 &&
> > +	    *access_bit_width < 32)
> > +		*access_bit_width = 32;
> > +
> >  	if ((bit_width + bit_offset) > *access_bit_width) {
> >  		pr_warning(FW_BUG APEI_PFX
> >  			   "Invalid bit width + offset in GAR [0x%llx/%u/%u/%u/%u]\n",
> 
> This seems reasonable but since the "Access Size < Register Bit Width"
> condition has apparently been seen on a number of systems I have been
> wondering if this might simply be a BIOS writer's way of telling us
> not to touch some of the high bits in the register.  Reducing the
> width of the register seems like a better way, but maybe not the
> only way?

I see it as a firmware bug, nothing else (although I am still waiting
for Asus to reply on this.) And if it really isn't a bug, I can only
read it as "we have a 32-bit register that can only be read 8-bit at a
time so please issue 4 8-bit reads and build up a 32-bit value out of
these." And certainly not as "don't touch the upper 24-bits."

As I mentioned in another discussion thread, the 1-bit granularity of
bit_width and bit_offset clearly means that these are the fields the
vendor must use to tell us which part of a register we should touch -
they are the "what". "Access Size" is there for the hardware constraints
- it is the "how".

> Jean, What do you (and others that are listening who know more
> about this than I do) think about a change to sanity check only
> the bit offset and not complain (at least for now) about cases
> where an access using the specified access size will not address
> every bit in the register?  
> 
> Perhaps something like:
> --- linux-3.5-rc2/drivers/acpi/apei/apei-base.c.ORIG	2012-06-08 18:40:09.000000000 -0700
> +++ linux-3.5-rc2/drivers/acpi/apei/apei-base.c	2012-06-13 16:32:49.000000000 -0700
> @@ -586,9 +586,9 @@ static int apei_check_gar(struct acpi_ge
>  	}
>  	*access_bit_width = 1UL << (access_size_code + 2);
>  
> -	if ((bit_width + bit_offset) > *access_bit_width) {
> +	if (bit_offset >= *access_bit_width) {
>  		pr_warning(FW_BUG APEI_PFX
> -			   "Invalid bit width + offset in GAR [0x%llx/%u/%u/%u/%u]\n",
> +			   "Invalid bit offset in GAR [0x%llx/%u/%u/%u/%u]\n",
>  			   *paddr, bit_width, bit_offset, access_size_code,
>  			   space_id);
>  		return -EINVAL;

Doesn't make much sense to me, as I don't think it will catch much.
Also, your original patch (ACPI, APEI: Fix incorrect APEI register bit
width check and usage) was not only reporting new errors, it also
changed the width parameter passed to ACPI read and write functions. It
was the register width and it is now (correctly, I believe) the access
width. So if the access width is wrong, and we don't fix it up (as my
proposed patch does) we might have a regression on some systems. Even if
the firmware is at fault, regressions are always bad.

(But again I don't know much about ACPI so I might be completely
wrong...)

-- 
Jean Delvare
Suse L3

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI, APEI: Fixup common access width firmware bug

[Gary Hade]

On Thu, Jun 14, 2012 at 10:35:11AM +0200, Jean Delvare wrote:
> Hi Gary,
> 
> Le mercredi 13 juin 2012 à 18:09 -0700, Gary Hade a écrit :
> > On Tue, Jun 12, 2012 at 10:43:28AM +0200, Jean Delvare wrote:
> > > Many firmwares have a common register definition bug where 8-bit
> > > access width is specified for a 32-bit register. Ideally this should
> > > be fixed in the BIOS, but earlier versions of the kernel did not
> > > complain, so fix that up silently.
> > > 
> > > This closes kernel bug #43282:
> > > https://bugzilla.kernel.org/show_bug.cgi?id=43282
> > > 
> > > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > > Cc: Gary Hade <garyhade@us.ibm.com>
> > > Cc: Len Brown <len.brown@intel.com>
> > > Cc: stable@vger.kernel.org [3.4+]
> > > ---
> > >  drivers/acpi/apei/apei-base.c |    5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > --- linux-3.4.orig/drivers/acpi/apei/apei-base.c	2012-06-08 10:02:06.000000000 +0200
> > > +++ linux-3.4/drivers/acpi/apei/apei-base.c	2012-06-08 10:04:16.503779775 +0200
> > > @@ -586,6 +586,11 @@ static int apei_check_gar(struct acpi_ge
> > >  	}
> > >  	*access_bit_width = 1UL << (access_size_code + 2);
> > > 
> > > +	/* Fixup common BIOS bug */
> > > +	if (bit_width == 32 && bit_offset == 0 && (*paddr & 0x03) == 0 &&
> > > +	    *access_bit_width < 32)
> > > +		*access_bit_width = 32;
> > > +
> > >  	if ((bit_width + bit_offset) > *access_bit_width) {
> > >  		pr_warning(FW_BUG APEI_PFX
> > >  			   "Invalid bit width + offset in GAR [0x%llx/%u/%u/%u/%u]\n",
> > 
> > This seems reasonable but since the "Access Size < Register Bit Width"
> > condition has apparently been seen on a number of systems I have been
> > wondering if this might simply be a BIOS writer's way of telling us
> > not to touch some of the high bits in the register.  Reducing the
> > width of the register seems like a better way, but maybe not the
> > only way?
> 
> I see it as a firmware bug, nothing else

That was my strong feeling but since the number of sightings
seems to be on the increase, I thought it might be good to talk
about whether our interpretation is actually correct.

> (although I am still waiting
> for Asus to reply on this.) And if it really isn't a bug, I can only
> read it as "we have a 32-bit register that can only be read 8-bit at a
> time so please issue 4 8-bit reads and build up a 32-bit value out of
> these." And certainly not as "don't touch the upper 24-bits."

It will definitely be interesting to see what you hear from Asus.

Multiple reads or writes to access all the bits in a register
does _not_ sound like a reasonable interpretation of the ACPI
spec to me.

> 
> As I mentioned in another discussion thread, the 1-bit granularity of
> bit_width and bit_offset clearly means that these are the fields the
> vendor must use to tell us which part of a register we should touch -
> they are the "what". "Access Size" is there for the hardware constraints
> - it is the "how".
> 
> > Jean, What do you (and others that are listening who know more
> > about this than I do) think about a change to sanity check only
> > the bit offset and not complain (at least for now) about cases
> > where an access using the specified access size will not address
> > every bit in the register?  
> > 
> > Perhaps something like:
> > --- linux-3.5-rc2/drivers/acpi/apei/apei-base.c.ORIG	2012-06-08 18:40:09.000000000 -0700
> > +++ linux-3.5-rc2/drivers/acpi/apei/apei-base.c	2012-06-13 16:32:49.000000000 -0700
> > @@ -586,9 +586,9 @@ static int apei_check_gar(struct acpi_ge
> >  	}
> >  	*access_bit_width = 1UL << (access_size_code + 2);
> >  
> > -	if ((bit_width + bit_offset) > *access_bit_width) {
> > +	if (bit_offset >= *access_bit_width) {
> >  		pr_warning(FW_BUG APEI_PFX
> > -			   "Invalid bit width + offset in GAR [0x%llx/%u/%u/%u/%u]\n",
> > +			   "Invalid bit offset in GAR [0x%llx/%u/%u/%u/%u]\n",
> >  			   *paddr, bit_width, bit_offset, access_size_code,
> >  			   space_id);
> >  		return -EINVAL;
> 
> Doesn't make much sense to me, as I don't think it will catch much.
> Also, your original patch (ACPI, APEI: Fix incorrect APEI register bit
> width check and usage) was not only reporting new errors, it also
> changed the width parameter passed to ACPI read and write functions. It
> was the register width and it is now (correctly, I believe) the access
> width. So if the access width is wrong, and we don't fix it up (as my
> proposed patch does) we might have a regression on some systems. Even if
> the firmware is at fault, regressions are always bad.

Agreed.  Your patch looks fine to me.
Acked-by: Gary Hade <garyhade@us.ibm.com>

Gary

-- 
Gary Hade
System x Enablement
IBM Linux Technology Center
503-578-4503  IBM T/L: 775-4503
garyhade@us.ibm.com
http://www.ibm.com/linux/ltc

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI, APEI: Fixup common access width firmware bug

[Jean Delvare]

Hi Gary,

Le jeudi 14 juin 2012 à 10:07 -0700, Gary Hade a écrit :
> On Thu, Jun 14, 2012 at 10:35:11AM +0200, Jean Delvare wrote:
> > I see it as a firmware bug, nothing else
> 
> That was my strong feeling but since the number of sightings
> seems to be on the increase, I thought it might be good to talk
> about whether our interpretation is actually correct.

It's always good to ensure we got things right, sure. WRT to the number
of faulty systems, I wouldn't jump to any conclusion. It is sufficient
to have a bug in the reference BIOS code in order to see the same bug
spread across boards and vendors all around.

> > (although I am still waiting
> > for Asus to reply on this.) And if it really isn't a bug, I can only
> > read it as "we have a 32-bit register that can only be read 8-bit at a
> > time so please issue 4 8-bit reads and build up a 32-bit value out of
> > these." And certainly not as "don't touch the upper 24-bits."
> 
> It will definitely be interesting to see what you hear from Asus.

If I ever hear something... No news yet :(

> Multiple reads or writes to access all the bits in a register
> does _not_ sound like a reasonable interpretation of the ACPI
> spec to me.

Well, I can imagine that some hardware has (or has had) such
limitations, and the ACPI spec authors may have wanted to play it safe
and make it possible to describe this case. I didn't mean that I expect
it to be the case for my Asus board - I still believe it is a simple
BIOS bug here.


-- 
Jean Delvare
Suse L3

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI, APEI: Fixup common access width firmware bug

[Len Brown]

applied for 3.6

thanks,
Len Brown, Intel Open Source Technology Center