Re: [PATCH] x86/efi-bgrt: Don't ignore the BGRT if the 'valid' bit is 0

[Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org>]

On Fri, Jan 08, 2016 at 11:30:47AM +0000, Matt Fleming wrote:
> On Tue, 05 Jan, at 05:38:54PM, Môshe van der Sterre wrote:
> > On 01/05/2016 04:46 PM, Matt Fleming wrote:
> > >Shouldn't this be pr_debug() instead of pr_err()?
> > 
> > It is an actual error for the image to be anything else. I did look at your
> > commit "x86/efi-bgrt: Switch pr_err() to pr_debug() for invalid BGRT", and
> > actually intentionally chose pr_err() instead of pr_debug() because of it.
> > 
> > From that commit message:
> >     However, Josh points that out it still makes sense to test the
> >     validity of the upper 7 bits of the 'status' field, since
> >     they're marked as "reserved" in the spec and must be zero. If
> >     firmware violates this it really *is* an error.
> > 
> > From the ACPI spec:
> >     If the value is 0, the Image Type is Bitmap. The format for a Bitmap is
> > defined at the reference located in the ACPI Link Document under the heading
> > "Types of Bitmaps". All other values not defined in the table are reserved
> > for future use.
> > and also:
> >     Implementations must present the image in a 24 bit bitmap with pixel
> > format 0xRRGGBB, or a 32-bit bitmap with the pixel format 0xrrRRGGBB, where
> > ‘rr’ is reserved.
> > Following the link to MSDN, the header id is definitely also required to be
> > "BM" for those types,
> > That said, I'm not 100% sure if pr_err() is right for any of the errors in
> 
> OK, I'll queue this up. Josh, does this look OK to you?

Yes, checking the bitmap file header seems like a good additional
cross-check.  And since the spec says that the image must be a bitmap
(given the one possible value for the format field), and since we can't
reliably know *how much* data to read without reading the bitmap header,
we should indeed error out if the data doesn't look like a bitmap.

Reviewed-by: Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org>