* [PATCH] drm/bridge: anx7625: Fix edid_read break case in sp_tx_edid_read()
@ 2021-11-12 11:24 Hsin-Yi Wang
2021-11-18 14:53 ` Robert Foss
2021-11-18 16:52 ` Guenter Roeck
0 siblings, 2 replies; 4+ messages in thread
From: Hsin-Yi Wang @ 2021-11-12 11:24 UTC (permalink / raw)
To: Robert Foss, Sam Ravnborg
Cc: Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, David Airlie, Daniel Vetter, Pi-Hsun Shih,
Xin Ji, Tzung-Bi Shih, Hsin-Yi Wang, Nicolas Boichat, dri-devel,
linux-kernel
edid_read() was assumed to return 0 on success. After
7f16d0f3b8e2("drm/bridge: anx7625: Propagate errors from sp_tx_rst_aux()"),
the function can return >= 0 for successful case. Fix the g_edid_break
condition in sp_tx_edid_read().
Fixes: 7f16d0f3b8e2("drm/bridge: anx7625: Propagate errors from sp_tx_rst_aux()")
Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
drivers/gpu/drm/bridge/analogix/anx7625.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index 1a871f6b6822ee..392203b576042b 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -826,7 +826,7 @@ static int sp_tx_edid_read(struct anx7625_data *ctx,
g_edid_break = edid_read(ctx, offset,
pblock_buf);
- if (g_edid_break)
+ if (g_edid_break < 0)
break;
memcpy(&pedid_blocks_buf[offset],
--
2.34.0.rc1.387.gb447b232ab-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/bridge: anx7625: Fix edid_read break case in sp_tx_edid_read()
2021-11-12 11:24 [PATCH] drm/bridge: anx7625: Fix edid_read break case in sp_tx_edid_read() Hsin-Yi Wang
@ 2021-11-18 14:53 ` Robert Foss
2021-11-18 16:52 ` Guenter Roeck
1 sibling, 0 replies; 4+ messages in thread
From: Robert Foss @ 2021-11-18 14:53 UTC (permalink / raw)
To: Hsin-Yi Wang
Cc: Sam Ravnborg, Andrzej Hajda, Neil Armstrong, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter,
Pi-Hsun Shih, Xin Ji, Tzung-Bi Shih, Nicolas Boichat, dri-devel,
linux-kernel
Hey Hsin-Yi,
Thanks for catching this.
checkpatch --strict is throwing an error for this patch. With that,
feel free to add my r-b.
Reviewed-by: Robert Foss <robert.foss@linaro.org>
On Fri, 12 Nov 2021 at 12:24, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> edid_read() was assumed to return 0 on success. After
> 7f16d0f3b8e2("drm/bridge: anx7625: Propagate errors from sp_tx_rst_aux()"),
> the function can return >= 0 for successful case. Fix the g_edid_break
> condition in sp_tx_edid_read().
>
> Fixes: 7f16d0f3b8e2("drm/bridge: anx7625: Propagate errors from sp_tx_rst_aux()")
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
> drivers/gpu/drm/bridge/analogix/anx7625.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index 1a871f6b6822ee..392203b576042b 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -826,7 +826,7 @@ static int sp_tx_edid_read(struct anx7625_data *ctx,
> g_edid_break = edid_read(ctx, offset,
> pblock_buf);
>
> - if (g_edid_break)
> + if (g_edid_break < 0)
> break;
>
> memcpy(&pedid_blocks_buf[offset],
> --
> 2.34.0.rc1.387.gb447b232ab-goog
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/bridge: anx7625: Fix edid_read break case in sp_tx_edid_read()
2021-11-12 11:24 [PATCH] drm/bridge: anx7625: Fix edid_read break case in sp_tx_edid_read() Hsin-Yi Wang
2021-11-18 14:53 ` Robert Foss
@ 2021-11-18 16:52 ` Guenter Roeck
2021-11-18 19:08 ` Hsin-Yi Wang
1 sibling, 1 reply; 4+ messages in thread
From: Guenter Roeck @ 2021-11-18 16:52 UTC (permalink / raw)
To: Hsin-Yi Wang
Cc: Robert Foss, Sam Ravnborg, Nicolas Boichat, Pi-Hsun Shih,
Jonas Karlman, David Airlie, dri-devel, Neil Armstrong,
linux-kernel, Jernej Skrabec, Andrzej Hajda, Laurent Pinchart,
Tzung-Bi Shih, Xin Ji
On Fri, Nov 12, 2021 at 07:24:33PM +0800, Hsin-Yi Wang wrote:
> edid_read() was assumed to return 0 on success. After
> 7f16d0f3b8e2("drm/bridge: anx7625: Propagate errors from sp_tx_rst_aux()"),
> the function can return >= 0 for successful case. Fix the g_edid_break
> condition in sp_tx_edid_read().
>
> Fixes: 7f16d0f3b8e2("drm/bridge: anx7625: Propagate errors from sp_tx_rst_aux()")
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> Reviewed-by: Robert Foss <robert.foss@linaro.org>
> ---
> drivers/gpu/drm/bridge/analogix/anx7625.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index 1a871f6b6822ee..392203b576042b 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -826,7 +826,7 @@ static int sp_tx_edid_read(struct anx7625_data *ctx,
> g_edid_break = edid_read(ctx, offset,
> pblock_buf);
>
> - if (g_edid_break)
> + if (g_edid_break < 0)
> break;
g_edid_break is declared u8 and thus never < 0. The break; statement
doesn't indicate an error but that a break was encountered, and the
value of g_edid_break == 1 is used elsewhere in the function to indicate
that condition. It also doesn't break out of the outer loop, but only
out of the switch statement.
With this patch in place, g_edid_break will have a more or less random
value after an error, and the behavior of this function will be undefined.
Guenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/bridge: anx7625: Fix edid_read break case in sp_tx_edid_read()
2021-11-18 16:52 ` Guenter Roeck
@ 2021-11-18 19:08 ` Hsin-Yi Wang
0 siblings, 0 replies; 4+ messages in thread
From: Hsin-Yi Wang @ 2021-11-18 19:08 UTC (permalink / raw)
To: Guenter Roeck
Cc: Robert Foss, Sam Ravnborg, Nicolas Boichat, Pi-Hsun Shih,
Jonas Karlman, David Airlie, dri-devel, Neil Armstrong,
linux-kernel, Jernej Skrabec, Andrzej Hajda, Laurent Pinchart,
Tzung-Bi Shih, Xin Ji
On Fri, Nov 19, 2021 at 12:52 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Fri, Nov 12, 2021 at 07:24:33PM +0800, Hsin-Yi Wang wrote:
> > edid_read() was assumed to return 0 on success. After
> > 7f16d0f3b8e2("drm/bridge: anx7625: Propagate errors from sp_tx_rst_aux()"),
> > the function can return >= 0 for successful case. Fix the g_edid_break
> > condition in sp_tx_edid_read().
> >
> > Fixes: 7f16d0f3b8e2("drm/bridge: anx7625: Propagate errors from sp_tx_rst_aux()")
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > Reviewed-by: Robert Foss <robert.foss@linaro.org>
> > ---
> > drivers/gpu/drm/bridge/analogix/anx7625.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index 1a871f6b6822ee..392203b576042b 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -826,7 +826,7 @@ static int sp_tx_edid_read(struct anx7625_data *ctx,
> > g_edid_break = edid_read(ctx, offset,
> > pblock_buf);
> >
> > - if (g_edid_break)
> > + if (g_edid_break < 0)
> > break;
>
> g_edid_break is declared u8 and thus never < 0. The break; statement
> doesn't indicate an error but that a break was encountered, and the
> value of g_edid_break == 1 is used elsewhere in the function to indicate
> that condition. It also doesn't break out of the outer loop, but only
> out of the switch statement.
>
> With this patch in place, g_edid_break will have a more or less random
> value after an error, and the behavior of this function will be undefined.
>
Before 7f16d0f3b8e2("drm/bridge: anx7625: Propagate errors from
sp_tx_rst_aux()"), edid_read() only returns 0 (i2c read bytes > 0) or
-EIO, so g_edid_break also couldn't be 1. After checking the code
more, I guess what it wants to do is:
- For the 1st and 2nd block (case 0 and 1) in sp_tx_edid_read(), copy
the buffer on success.
- For other blocks (case 2 and 3), process the buffer when the last
g_edid_break updated in previous blocks is not 1. These blocks
probably don't need to be updated if previous blocks have failed. The
whole edid will be checked if valid.
I will update the patch.
> Guenter
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-11-18 19:08 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-12 11:24 [PATCH] drm/bridge: anx7625: Fix edid_read break case in sp_tx_edid_read() Hsin-Yi Wang
2021-11-18 14:53 ` Robert Foss
2021-11-18 16:52 ` Guenter Roeck
2021-11-18 19:08 ` Hsin-Yi Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).