[PATCH] drm/bochs: add edid present check

[PATCH] drm/bochs: add edid present check

[Gerd Hoffmann]

Check first two header bytes before trying to read the edid blob,
to avoid the log being spammed in case qemu has no edid support (old
qemu or edid turned off).

Fixes: 01f23459cf drm/bochs: add edid support.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/bochs/bochs_hw.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/bochs/bochs_hw.c b/drivers/gpu/drm/bochs/bochs_hw.c
index c90a0d492f..f91e049625 100644
--- a/drivers/gpu/drm/bochs/bochs_hw.c
+++ b/drivers/gpu/drm/bochs/bochs_hw.c
@@ -89,6 +89,10 @@ int bochs_hw_load_edid(struct bochs_device *bochs)
 	if (!bochs->mmio)
 		return -1;
 
+	if (readb(bochs->mmio + 0) != 0x00 ||
+	    readb(bochs->mmio + 1) != 0xff)
+		return -1;
+
 	kfree(bochs->edid);
 	bochs->edid = drm_do_get_edid(&bochs->connector,
 				      bochs_get_edid_block, bochs);
-- 
2.9.3

Re: [PATCH] drm/bochs: add edid present check

[Oleksandr Andrushchenko]

Hello, Gerd!

On 12/18/18 9:33 AM, Gerd Hoffmann wrote:
> Check first two header bytes before trying to read the edid blob,
> to avoid the log being spammed in case qemu has no edid support (old
> qemu or edid turned off).
>
> Fixes: 01f23459cf drm/bochs: add edid support.
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   drivers/gpu/drm/bochs/bochs_hw.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/bochs/bochs_hw.c b/drivers/gpu/drm/bochs/bochs_hw.c
> index c90a0d492f..f91e049625 100644
> --- a/drivers/gpu/drm/bochs/bochs_hw.c
> +++ b/drivers/gpu/drm/bochs/bochs_hw.c
> @@ -89,6 +89,10 @@ int bochs_hw_load_edid(struct bochs_device *bochs)
>   	if (!bochs->mmio)
>   		return -1;
>   

You could probably have a comment here explaining the magic below

(just like in the commit message to ease the task of understanding

while reading the code why 2 of 8 bytes of the EDID header is checked

and why it is all needed). Of course one can use git blame... Up to you

> +	if (readb(bochs->mmio + 0) != 0x00 ||
> +	    readb(bochs->mmio + 1) != 0xff)

bochs->mmio is defined as "void __iomem   *mmio;". Can we please avoid

void pointer arithmetic here?

> +		return -1;
> +
>   	kfree(bochs->edid);
>   	bochs->edid = drm_do_get_edid(&bochs->connector,
>   				      bochs_get_edid_block, bochs);

With the above fixed:

Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Re: [PATCH] drm/bochs: add edid present check

[Gerd Hoffmann]

  Hi,

> You could probably have a comment here explaining the magic below
> (just like in the commit message to ease the task of understanding
> while reading the code why 2 of 8 bytes of the EDID header is checked
> and why it is all needed). Of course one can use git blame... Up to you

Makes sense.

> > +	if (readb(bochs->mmio + 0) != 0x00 ||
> > +	    readb(bochs->mmio + 1) != 0xff)
> 
> bochs->mmio is defined as "void __iomem   *mmio;". Can we please avoid
> void pointer arithmetic here?

Why is that a problem?  gcc uses bytes when doing pointer arithmetic
with void pointers (even though it is undefined in the C standard),
and as far I know the linux kernel depends on that behavior anyway.

Also the driver already does it everywhere.

cheers,
  Gerd

Re: [PATCH] drm/bochs: add edid present check

[Oleksandr Andrushchenko]

On 12/19/18 5:21 PM, Gerd Hoffmann wrote:
>    Hi,
>
>> You could probably have a comment here explaining the magic below
>> (just like in the commit message to ease the task of understanding
>> while reading the code why 2 of 8 bytes of the EDID header is checked
>> and why it is all needed). Of course one can use git blame... Up to you
> Makes sense.
>
>>> +	if (readb(bochs->mmio + 0) != 0x00 ||
>>> +	    readb(bochs->mmio + 1) != 0xff)
>> bochs->mmio is defined as "void __iomem   *mmio;". Can we please avoid
>> void pointer arithmetic here?
> Why is that a problem?  gcc uses bytes when doing pointer arithmetic
> with void pointers (even though it is undefined in the C standard),
> and as far I know the linux kernel depends on that behavior anyway.
>
> Also the driver already does it everywhere.
Ok then, just to be consistent with the rest of the driver.
> cheers,
>    Gerd
>