On Wed, Jun 13, 2012 at 10:46:51AM +0200, Jean Delvare wrote:
> Hi Xiao,
>
> On Wed, 13 Jun 2012 15:39:44 +0800, Xiao, Hui wrote:
> > Fix the incorrect bit width + offset check condition in apei_check_gar()
> > function introduced by commit v3.3-5-g15afae6.
> >
> > The bug caused regression on EINJ error injection with errors:
> >
> > [Firmware Bug]: APEI: Invalid bit width + offset in GAR [0x1121a5000/64/0/3/0]
> >
> > on a valid address region of:
> > - Register bit width: 64 bits
> > - Register bit offset: 0
> > - Access Size: 03 [DWord Access: 32]
>
> I don't see how this is valid, sorry. If you have a 64-bit register,
> you want 64-bit access, don't you?
>
> If the access code is supposed to be able to read large registers in
> smaller chunks and assemble them transparently to a larger value, then
> there is no point in having any check at all, everything is valid. If
> not, then the above is just as invalid as the firmware issue discussed
> in bug #43282.
>
> >
> > Signed-off-by: Xiao, Hui <hui.xiao@xxxxxxxxxxxxxxx>
> > Signed-off-by: Chen Gong <gong.chen@xxxxxxxxxxxxxxx>
> > ---
> > drivers/acpi/apei/apei-base.c | 7 +++++--
> > 1 files changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
> > index 5577762..95e07b2 100644
> > --- a/drivers/acpi/apei/apei-base.c
> > +++ b/drivers/acpi/apei/apei-base.c
> > @@ -586,9 +586,12 @@ static int apei_check_gar(struct acpi_generic_address *reg, u64 *paddr,
> > }
> > *access_bit_width = 1UL << (access_size_code + 2);
> >
> > - if ((bit_width + bit_offset) > *access_bit_width) {
> > + /* bit_width and bit_offset must be zero when addressing a data
> > + * structure. So just check for non-zero case here */
> > + if ((bit_width != 0 && *access_bit_width > bit_width) ||
> > + bit_offset > *access_bit_width) {
>
> I can't make any sense of this test, sorry. And it will trigger on
> valid cases, starting with the most frequent case where
> *access_bit_width == bit_width. So, nack.
I agree that the change will trigger firmware bug messages for
valid cases. Here is a good example of a valid case from one
of our systems that confirms this.
[110h 0272 1] Action : 06 [Check Busy Status]
[111h 0273 1] Instruction : 01 [Read Register Value]
[112h 0274 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[113h 0275 1] Reserved : 00
[114h 0276 12] Register Region : [Generic Address Structure]
[114h 0276 1] Space ID : 00 [SystemMemory]
[115h 0277 1] Bit Width : 01
[116h 0278 1] Bit Offset : 1F
[117h 0279 1] Encoded Access Width : 03 [DWord Access:32]
[118h 0280 8] Address : 000000007F2D7038
[120h 0288 8] Value : 0000000000000001
[128h 0296 8] Mask : 0000000000000001
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
