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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> ---
>  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
> 

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature


[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux