Hi,
On Tue, Jun 11, 2024 at 7:05 AM Tejas Vipin <tejasvipin76@xxxxxxxxx> wrote:
>
> mipi_dsi_msleep expects struct mipi_dsi_multi_context to be passed as a
> value and not as a reference.
>
> Fixes: a2ab7cb169da ("drm/panel: himax-hx83102: use wrapped MIPI DCS functions")
>
> Signed-off-by: Tejas Vipin <tejasvipin76@xxxxxxxxx>
Should be no blank line between "Fixes" and "Signed-off-by"
> ---
>
> Changes in v2:
> - Add Fixes tag
>
> v1: https://lore.kernel.org/all/d9f4546f-c2f9-456d-ba75-85cc195dd9b8@xxxxxxxxx/
>
> ---
> drivers/gpu/drm/panel/panel-himax-hx83102.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
I notice you didn't CC me, even though I authored the broken commit.
Presumably get_maintainer should have suggested you CC me?
> diff --git a/drivers/gpu/drm/panel/panel-himax-hx83102.c b/drivers/gpu/drm/panel/panel-himax-hx83102.c
> index 6009a3fe1b8f..ab00fd92cce0 100644
> --- a/drivers/gpu/drm/panel/panel-himax-hx83102.c
> +++ b/drivers/gpu/drm/panel/panel-himax-hx83102.c
> @@ -479,7 +479,7 @@ static int hx83102_disable(struct drm_panel *panel)
> mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
>
> - mipi_dsi_msleep(&dsi_ctx, 150);
> + mipi_dsi_msleep(dsi_ctx, 150);
So while your fix is correct, it's not really enough. I swore that I
compile tested my change and, sure enough, the bad code compile tests
fine. This is because the macro mipi_dsi_msleep() fell into a macro
trap. :( Specifically, we have:
#define mipi_dsi_msleep(ctx, delay) \
do { \
if (!ctx.accum_err) \
msleep(delay); \
} while (0)
Let's look at "if (!ctx.accum_err)". Before your patch, that translated to:
if (!&dsi_ctx.accum_err)
...adding extra parentheses for order of operations, that is:
if (!&(dsi_ctx.accum_err))
...in other words it's testing whether the address of the "accum_err"
is NULL. That's not a syntax error, but _really_ not what was meant.
We really need to fix the macro trap by changing it like this:
- if (!ctx.accum_err) \
+ if (!(ctx).accum_err) \
When you do that, though, you find that half the users of the macro
were using it wrong since every other "_multi_" function passes the
address. IMO while fixing the macro trap we should just change this to
pass the address and then fix up all the callers.
This is a serious enough problem (thanks for noticing it) that I'm
happy to send out patches, but also I'm fine if you want to tackle it.
If I don't see anything from you in a day or two I'll send out
patches.
Thanks!
-Doug
