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

[Gary Hade <garyhade@us.ibm.com>]

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