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@xxxxxxx> > > > Cc: Gary Hade <garyhade@xxxxxxxxxx> > > > Cc: Len Brown <len.brown@xxxxxxxxx> > > > Cc: stable@xxxxxxxxxxxxxxx [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@xxxxxxxxxx> Gary -- Gary Hade System x Enablement IBM Linux Technology Center 503-578-4503 IBM T/L: 775-4503 garyhade@xxxxxxxxxx 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
