[PATCH v2] drm/panel : himax-hx83102: fix incorrect argument to mipi_dsi_msleep

[Doug Anderson]

Hi,

On Tue, Jun 11, 2024 at 7:05 AM Tejas Vipin <tejasvipin76 at gmail.com> 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 at gmail.com>

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@gmail.com/
>
> ---
>  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