Patchwork [1/2] acpi: bgrt: Fix the way the BGRT status field is used.

login
register
mail settings
Submitter Peter Jones
Date Jan. 29, 2019, 6:51 p.m.
Message ID <20190129185155.32386-1-pjones@redhat.com>
Download mbox | patch
Permalink /patch/712779/
State New
Headers show

Comments

Peter Jones - Jan. 29, 2019, 6:51 p.m.
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(-)
Môshe van der Sterre - Jan. 29, 2019, 8:10 p.m.
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
>
Peter Jones - Jan. 29, 2019, 9:43 p.m.
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 :)
Môshe van der Sterre - Jan. 29, 2019, 11:44 p.m.
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
Josh Triplett - Jan. 30, 2019, 12:56 a.m.
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

Patch

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