linux-kernel - Re: [PATCH v2] drm/panel : himax-hx83102: fix incorrect argument to mipi_dsi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAD=FV=VPQWUy4n75sPSxnzFi9RMTR2THmsL+VOd1PPG5paZN_w@mail.gmail.com>
Date: Tue, 11 Jun 2024 09:21:54 -0700
From: Doug Anderson <dianders@...omium.org>
To: Tejas Vipin <tejasvipin76@...il.com>
Cc: neil.armstrong@...aro.org, quic_jesszhan@...cinc.com, 
	maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de, 
	airlied@...il.com, daniel@...ll.ch, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org, Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Subject: Re: [PATCH v2] drm/panel : himax-hx83102: fix incorrect argument to mipi_dsi_msleep

Hi,

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