On Tue, Jan 22, 2019 at 07:07:30PM +0100, Ard Biesheuvel wrote:
> On Tue, 22 Jan 2019 at 18:32, Lorenzo Pieralisi <[email protected]>
> wrote:
> > Side note (not related to this patch):
> >
> > I do not understand this check:
> >
> > if (bgrt->status & 0xfe) {
> > pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n",
> > bgrt->status);
> > goto out;
> > }
> >
> > Should not we check against 0xf8 (reserved bits are [7:3]) ? And what
> > about status[0] (valid), should we check that as well ?
>
> I will let Peter take that question ...
I think this was originally coded from a draft spec, but also in ACPI
6.1A bits [2:1] got defined without updating the "version" field :/, so
if we're going to check them, checking against 0xf8 would be correct -
but it's probably saner just to not check them? Nothing that's been
defined so far is actually a reason not to expose the data, and it's not
clear that anything in the future would either. Also, the "valid" bit
is meant only to reflect if the image is on the screen or not. This was
clarified in one of the ACPI 5.0A spec - the field has been renamed to
"displayed", so as not to confuse people. Here's what the current ACPI
spec says:
Name Size Offset Description
Status 1 38 1-byte status field indicating current status
about the table.
Bits [7:3] = Reserved (must be zero)
Bits [2:1] = Orientation Offset. These bits
describe the clockwise degree
offset from the image’s default
orientation.
[00] = 0, no offset
[01] = 90
[10] = 180
[11] = 270
And I think 6.1 or 6.1A has accidentally dropped the text:
Bit [0] = Displayed. A one indicates the boot
image graphic is displayed.
Further down it says:
Bit 0 - Displayed
The status field contains information about the current status
of the table. The Valid bit is bit 0 of the lowest byte. It
should be set to 1 while the image resource is displayed on the
screen, and set to 0 while it is not displayed.
Clearly there has been some editing error going on here, and I'll send
in an ECR to fix that up. It might be worth exposing "orientation" and
"displayed" directly instead of just exposing "status".
Aside from that an Lorenzo's other observations, the patch looks
reasonable to me.
--
Peter
