| Message ID | 20190129185155.32386-1-pjones@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/2] acpi: bgrt: Fix the way the BGRT status field is used. | expand |
Hi Peter, On 01/29/2019 07:51 PM, Peter Jones wrote: > The BGRT table's "status" field doesn't indicate "validity", but rather > if and how the image is being displayed. As such, we shouldn't decide > the table is invalid if status bits we don't understand are in use, and > it's better to expose the values we do understand directly. This goes against the conclusion of this discussion from 2015: https://patchwork.kernel.org/patch/6688521/ I wasn't involved with that discussion, so I have CC-ed the participants. You noted in another mail that the version field was not updated while the status field has been backwards-incompatibly changed. I honestly don't know what to think of this, because it shatters any meaning the words "must be zero" could have had. > This patch removes the validation of the status flag, and adds the files > "orientation" and "displayed" in sysfs. The "status" file in sysfs is > kept for compatibility with existing software. Greetings, Môshe > Signed-off-by: Peter Jones <pjones@redhat.com> > --- > drivers/acpi/bgrt.c | 15 +++++++++++++++ > drivers/firmware/efi/efi-bgrt.c | 5 ----- > Documentation/ABI/testing/sysfs-firmware-acpi | 7 ++++++- > 3 files changed, 21 insertions(+), 6 deletions(-) > > diff --git a/drivers/acpi/bgrt.c b/drivers/acpi/bgrt.c > index 048413e0689..63d17fac2cc 100644 > --- a/drivers/acpi/bgrt.c > +++ b/drivers/acpi/bgrt.c > @@ -32,6 +32,21 @@ static ssize_t show_status(struct device *dev, > } > static DEVICE_ATTR(status, S_IRUGO, show_status, NULL); > > +static ssize_t show_orientation(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + return snprintf(buf, PAGE_SIZE, "%d\n", > + ((bgrt_tab.status >> 1) & 0x3) * 90); > +} > +static DEVICE_ATTR(orientation, S_IRUGO, show_orientation, NULL); > + > +static ssize_t show_displayed(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.status & 0x1); > +} > +static DEVICE_ATTR(displayed, S_IRUGO, show_displayed, NULL); > + > static ssize_t show_type(struct device *dev, > struct device_attribute *attr, char *buf) > { > diff --git a/drivers/firmware/efi/efi-bgrt.c b/drivers/firmware/efi/efi-bgrt.c > index b2d98e16c7b..3a8797364b8 100644 > --- a/drivers/firmware/efi/efi-bgrt.c > +++ b/drivers/firmware/efi/efi-bgrt.c > @@ -120,11 +120,6 @@ void __init efi_bgrt_init(unsigned long rsdp_phys) > bgrt->version); > goto out; > } > - if (bgrt->status & 0xfe) { > - pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n", > - bgrt->status); > - goto out; > - } > if (bgrt->image_type != 0) { > pr_notice("Ignoring BGRT: invalid image type %u (expected 0)\n", > bgrt->image_type); > diff --git a/Documentation/ABI/testing/sysfs-firmware-acpi b/Documentation/ABI/testing/sysfs-firmware-acpi > index 613f42a9d5c..e4d3a1b5878 100644 > --- a/Documentation/ABI/testing/sysfs-firmware-acpi > +++ b/Documentation/ABI/testing/sysfs-firmware-acpi > @@ -10,13 +10,18 @@ Description: > transitions. > > image: The image bitmap. Currently a 32-bit BMP. > - status: 1 if the image is valid, 0 if firmware invalidated it. > type: 0 indicates image is in BMP format. > version: The version of the BGRT. Currently 1. > xoffset: The number of pixels between the left of the screen > and the left edge of the image. > yoffset: The number of pixels between the top of the screen > and the top edge of the image. > + status: The raw status flag of the image. The values are > + presented individually in the following files: > + displayed: 1 indicates the image was displayed, 0 indicates it > + wasn't. > + orientation: The orientation of the image in relation to its > + natural layout, in degrees rotated clockwise. > > What: /sys/firmware/acpi/hotplug/ > Date: February 2013 >
On Tue, Jan 29, 2019 at 09:10:08PM +0100, Môshe van der Sterre wrote: > Hi Peter, > > On 01/29/2019 07:51 PM, Peter Jones wrote: > > The BGRT table's "status" field doesn't indicate "validity", but rather > > if and how the image is being displayed. As such, we shouldn't decide > > the table is invalid if status bits we don't understand are in use, and > > it's better to expose the values we do understand directly. > > This goes against the conclusion of this discussion from 2015: > https://patchwork.kernel.org/patch/6688521/ > I wasn't involved with that discussion, so I have CC-ed the participants. > > You noted in another mail that the version field was not updated while > the status field has been backwards-incompatibly changed. I honestly > don't know what to think of this, because it shatters any meaning the > words "must be zero" could have had. Around 8 months *after* that discussion, ASWG (the keepers of the ACPI spec) had a discussion about an ECR that added both the orientation bits and this clarification: - 1-byte status field indicating current status. + 1-byte status field indicating current status of the image. But that change did not change the version field. This was considered an erratum, though I don't have enough context to know why - my guess is that someone thought adding bits would be a harmless forward-compatible change, without thinking about how our implementation might work with respect to the version field or the reserved bits. I share your dismay, but I think it's now clear that these bits aren't intended as the status of the BGRT, but rather they merely describe the image it refers to. With that in mind, it doesn't appear reasonable to go on dismissing the whole thing based on the reserved bits of that field - we can still use the image just fine. But that's just my opinion :)
On 01/29/2019 10:43 PM, Peter Jones wrote: > Around 8 months *after* that discussion, ASWG (the keepers of the ACPI > spec) had a discussion about an ECR that added both the orientation > bits and this clarification: > > - 1-byte status field indicating current status. > + 1-byte status field indicating current status of the image. > > But that change did not change the version field. This was considered > an erratum, though I don't have enough context to know why - my guess is > that someone thought adding bits would be a harmless forward-compatible > change, without thinking about how our implementation might work with > respect to the version field or the reserved bits. Thanks for this information. I have been CC-ed on changes to this file after my own patch to it in 2016, and one thing that I noticed is that the error handling code has caused multiple issues for people; and this has happened both ways (too strict and too lax). The approach that is useful on the most hardware seems to me the best one. But that is probably a hard question to answer. > I share your dismay, but I think it's now clear that these bits aren't > intended as the status of the BGRT, but rather they merely describe the > image it refers to. With that in mind, it doesn't appear reasonable to > go on dismissing the whole thing based on the reserved bits of that > field - we can still use the image just fine. But that's just my > opinion :) I imagine the counter argument goes like this: A firmware that cannot correctly follow "must be zero", shouldn't be trusted to get the size of a potentially large memory copy during boot correct either. However, as long as the pro's and con's have been considered, I see no reason to personally object to your change. My view on what real world firmwares are actually doing is too limited to make a reasonable argument either way. Greetings, Môshe
On Tue, Jan 29, 2019 at 09:10:08PM +0100, Môshe van der Sterre wrote: > Hi Peter, > > On 01/29/2019 07:51 PM, Peter Jones wrote: > > The BGRT table's "status" field doesn't indicate "validity", but rather > > if and how the image is being displayed. As such, we shouldn't decide > > the table is invalid if status bits we don't understand are in use, and > > it's better to expose the values we do understand directly. > > This goes against the conclusion of this discussion from 2015: > https://patchwork.kernel.org/patch/6688521/ > I wasn't involved with that discussion, so I have CC-ed the participants. Reading this patch: please don't remove the check entirely, please just check for flags set that the implementation doesn't understand (which now would be & 0xf8 instead of & 0xfe). We have no way of knowing whether another field added in the reserved bits could change the interpretation of the image. - Josh Triplett
diff --git a/drivers/acpi/bgrt.c b/drivers/acpi/bgrt.c index 048413e0689..63d17fac2cc 100644 --- a/drivers/acpi/bgrt.c +++ b/drivers/acpi/bgrt.c @@ -32,6 +32,21 @@ static ssize_t show_status(struct device *dev, } static DEVICE_ATTR(status, S_IRUGO, show_status, NULL); +static ssize_t show_orientation(struct device *dev, + struct device_attribute *attr, char *buf) +{ + return snprintf(buf, PAGE_SIZE, "%d\n", + ((bgrt_tab.status >> 1) & 0x3) * 90); +} +static DEVICE_ATTR(orientation, S_IRUGO, show_orientation, NULL); + +static ssize_t show_displayed(struct device *dev, + struct device_attribute *attr, char *buf) +{ + return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.status & 0x1); +} +static DEVICE_ATTR(displayed, S_IRUGO, show_displayed, NULL); + static ssize_t show_type(struct device *dev, struct device_attribute *attr, char *buf) { diff --git a/drivers/firmware/efi/efi-bgrt.c b/drivers/firmware/efi/efi-bgrt.c index b2d98e16c7b..3a8797364b8 100644 --- a/drivers/firmware/efi/efi-bgrt.c +++ b/drivers/firmware/efi/efi-bgrt.c @@ -120,11 +120,6 @@ void __init efi_bgrt_init(unsigned long rsdp_phys) bgrt->version); goto out; } - if (bgrt->status & 0xfe) { - pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n", - bgrt->status); - goto out; - } if (bgrt->image_type != 0) { pr_notice("Ignoring BGRT: invalid image type %u (expected 0)\n", bgrt->image_type); diff --git a/Documentation/ABI/testing/sysfs-firmware-acpi b/Documentation/ABI/testing/sysfs-firmware-acpi index 613f42a9d5c..e4d3a1b5878 100644 --- a/Documentation/ABI/testing/sysfs-firmware-acpi +++ b/Documentation/ABI/testing/sysfs-firmware-acpi @@ -10,13 +10,18 @@ Description: transitions. image: The image bitmap. Currently a 32-bit BMP. - status: 1 if the image is valid, 0 if firmware invalidated it. type: 0 indicates image is in BMP format. version: The version of the BGRT. Currently 1. xoffset: The number of pixels between the left of the screen and the left edge of the image. yoffset: The number of pixels between the top of the screen and the top edge of the image. + status: The raw status flag of the image. The values are + presented individually in the following files: + displayed: 1 indicates the image was displayed, 0 indicates it + wasn't. + orientation: The orientation of the image in relation to its + natural layout, in degrees rotated clockwise. What: /sys/firmware/acpi/hotplug/ Date: February 2013
The BGRT table's "status" field doesn't indicate "validity", but rather if and how the image is being displayed. As such, we shouldn't decide the table is invalid if status bits we don't understand are in use, and it's better to expose the values we do understand directly. This patch removes the validation of the status flag, and adds the files "orientation" and "displayed" in sysfs. The "status" file in sysfs is kept for compatibility with existing software. Signed-off-by: Peter Jones <pjones@redhat.com> --- drivers/acpi/bgrt.c | 15 +++++++++++++++ drivers/firmware/efi/efi-bgrt.c | 5 ----- Documentation/ABI/testing/sysfs-firmware-acpi | 7 ++++++- 3 files changed, 21 insertions(+), 6 deletions(-)