[PATCH 0/2] fix handling of incorrect arguments by mipi_dsi_msleep

[PATCH 0/2] fix handling of incorrect arguments by mipi_dsi_msleep

[Tejas Vipin]

mipi_dsi_msleep is currently defined such that it treats ctx as an
argument passed by value. In the case of ctx being passed by
reference, it doesn't raise an error, but instead evaluates the
resulting expression in an undesired manner. Since the majority of the
usage of this function passes ctx by reference (similar to
other functions), mipi_dsi_msleep can be modified to treat ctx as a
pointer and do it correctly, and the other calls to this macro can be
adjusted accordingly.

Tejas Vipin (2):
  drm/panel : himax-hx83102: fix incorrect argument to mipi_dsi_msleep
  drm/mipi-dsi: fix handling of ctx in mipi_dsi_msleep

 drivers/gpu/drm/panel/panel-himax-hx83102.c | 6 +++---
 include/drm/drm_mipi_dsi.h                  | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.45.2

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

[Tejas Vipin]

mipi_dsi_msleep should be modified to accept ctx as a pointer and the
function call should be adjusted accordingly.

Fixes: a2ab7cb169da3 ("drm/panel: himax-hx83102: use wrapped MIPI DCS functions") 
Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
---
 drivers/gpu/drm/panel/panel-himax-hx83102.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-himax-hx83102.c b/drivers/gpu/drm/panel/panel-himax-hx83102.c
index 6009a3fe1b8f..6e4b7e4644ce 100644
--- a/drivers/gpu/drm/panel/panel-himax-hx83102.c
+++ b/drivers/gpu/drm/panel/panel-himax-hx83102.c
@@ -286,7 +286,7 @@ static int boe_nv110wum_init(struct hx83102 *ctx)
 	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, HX83102_SETBANK, 0x00);
 	hx83102_enable_extended_cmds(&dsi_ctx, false);
 
-	mipi_dsi_msleep(dsi_ctx, 50);
+	mipi_dsi_msleep(&dsi_ctx, 50);
 
 	return dsi_ctx.accum_err;
 };
@@ -391,7 +391,7 @@ static int ivo_t109nw41_init(struct hx83102 *ctx)
 	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, HX83102_SETBANK, 0x00);
 	hx83102_enable_extended_cmds(&dsi_ctx, false);
 
-	mipi_dsi_msleep(dsi_ctx, 60);
+	mipi_dsi_msleep(&dsi_ctx, 60);
 
 	return dsi_ctx.accum_err;
 };
@@ -538,7 +538,7 @@ static int hx83102_prepare(struct drm_panel *panel)
 	dsi_ctx.accum_err = ctx->desc->init(ctx);
 
 	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
-	mipi_dsi_msleep(dsi_ctx, 120);
+	mipi_dsi_msleep(&dsi_ctx, 120);
 	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
 	if (dsi_ctx.accum_err)
 		goto poweroff;
-- 
2.45.2

Re: [PATCH 1/2] drm/panel : himax-hx83102: fix incorrect argument to mipi_dsi_msleep

[Doug Anderson]

Hi,

On Wed, Jun 12, 2024 at 6:37 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
>
> mipi_dsi_msleep should be modified to accept ctx as a pointer and the
> function call should be adjusted accordingly.
>
> Fixes: a2ab7cb169da3 ("drm/panel: himax-hx83102: use wrapped MIPI DCS functions")
> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
> ---
>  drivers/gpu/drm/panel/panel-himax-hx83102.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Thanks!

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[PATCH 2/2] drm/mipi-dsi: fix handling of ctx in mipi_dsi_msleep

[Tejas Vipin]

ctx would be better off treated as a pointer to account for most of its
usage so far, and brackets should be added to account for operator 
precedence for correct evaluation.

Fixes: f79d6d28d8fe7 ("drm/mipi-dsi: wrap more functions for streamline handling") 
Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
---
 include/drm/drm_mipi_dsi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index bd5a0b6d0711..71d121aeef24 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -293,7 +293,7 @@ ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
 
 #define mipi_dsi_msleep(ctx, delay)	\
 	do {				\
-		if (!ctx.accum_err)	\
+		if (!(ctx)->accum_err)	\
 			msleep(delay);	\
 	} while (0)
 
-- 
2.45.2

Re: [PATCH 2/2] drm/mipi-dsi: fix handling of ctx in mipi_dsi_msleep

[Doug Anderson]

Hi,

On Wed, Jun 12, 2024 at 6:37 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
>
> ctx would be better off treated as a pointer to account for most of its
> usage so far, and brackets should be added to account for operator
> precedence for correct evaluation.
>
> Fixes: f79d6d28d8fe7 ("drm/mipi-dsi: wrap more functions for streamline handling")
> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
> ---
>  include/drm/drm_mipi_dsi.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Yeah. Looking closer at the history, it looks like it was always
intended to be a pointer since the first users all used it as a
pointer.

Suggested-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>

I've also compile-tested all the panels currently using mipi_dsi_msleep().

Neil: Given that this is a correctness thing, I'd rather see this land
sooner rather than later. If you agree, maybe you can land these two
patches whenever you're comfortable with them?


-Doug

Re: [PATCH 2/2] drm/mipi-dsi: fix handling of ctx in mipi_dsi_msleep

[neil.armstrong]

On 12/06/2024 16:21, Doug Anderson wrote:
> Hi,
> 
> On Wed, Jun 12, 2024 at 6:37 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
>>
>> ctx would be better off treated as a pointer to account for most of its
>> usage so far, and brackets should be added to account for operator
>> precedence for correct evaluation.
>>
>> Fixes: f79d6d28d8fe7 ("drm/mipi-dsi: wrap more functions for streamline handling")
>> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
>> ---
>>   include/drm/drm_mipi_dsi.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Yeah. Looking closer at the history, it looks like it was always
> intended to be a pointer since the first users all used it as a
> pointer.
> 
> Suggested-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> 
> I've also compile-tested all the panels currently using mipi_dsi_msleep().
> 
> Neil: Given that this is a correctness thing, I'd rather see this land
> sooner rather than later. If you agree, maybe you can land these two
> patches whenever you're comfortable with them?

Applying them, but inverting them, fix should go first.

Neil

> 
> 
> -Doug

Re: [PATCH 2/2] drm/mipi-dsi: fix handling of ctx in mipi_dsi_msleep

[Doug Anderson]

Hi,

On Wed, Jun 12, 2024 at 7:34 AM <neil.armstrong@linaro.org> wrote:
>
> On 12/06/2024 16:21, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Jun 12, 2024 at 6:37 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
> >>
> >> ctx would be better off treated as a pointer to account for most of its
> >> usage so far, and brackets should be added to account for operator
> >> precedence for correct evaluation.
> >>
> >> Fixes: f79d6d28d8fe7 ("drm/mipi-dsi: wrap more functions for streamline handling")
> >> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
> >> ---
> >>   include/drm/drm_mipi_dsi.h | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > Yeah. Looking closer at the history, it looks like it was always
> > intended to be a pointer since the first users all used it as a
> > pointer.
> >
> > Suggested-by: Douglas Anderson <dianders@chromium.org>
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> >
> > I've also compile-tested all the panels currently using mipi_dsi_msleep().
> >
> > Neil: Given that this is a correctness thing, I'd rather see this land
> > sooner rather than later. If you agree, maybe you can land these two
> > patches whenever you're comfortable with them?
>
> Applying them, but inverting them, fix should go first.

Well, they're both fixes, and inverting them means that you get a
compile failure across several panels if you happen to be bisecting
and land on the first commit, but it doesn't really matter. I guess
the compile failure is maybe a benefit given that they were not doing
their delays properly... ;-)

-Doug

Re: [PATCH 2/2] drm/mipi-dsi: fix handling of ctx in mipi_dsi_msleep

[neil.armstrong]

On 12/06/2024 16:52, Doug Anderson wrote:
> Hi,
> 
> On Wed, Jun 12, 2024 at 7:34 AM <neil.armstrong@linaro.org> wrote:
>>
>> On 12/06/2024 16:21, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Wed, Jun 12, 2024 at 6:37 AM Tejas Vipin <tejasvipin76@gmail.com> wrote:
>>>>
>>>> ctx would be better off treated as a pointer to account for most of its
>>>> usage so far, and brackets should be added to account for operator
>>>> precedence for correct evaluation.
>>>>
>>>> Fixes: f79d6d28d8fe7 ("drm/mipi-dsi: wrap more functions for streamline handling")
>>>> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
>>>> ---
>>>>    include/drm/drm_mipi_dsi.h | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> Yeah. Looking closer at the history, it looks like it was always
>>> intended to be a pointer since the first users all used it as a
>>> pointer.
>>>
>>> Suggested-by: Douglas Anderson <dianders@chromium.org>
>>> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>>>
>>> I've also compile-tested all the panels currently using mipi_dsi_msleep().
>>>
>>> Neil: Given that this is a correctness thing, I'd rather see this land
>>> sooner rather than later. If you agree, maybe you can land these two
>>> patches whenever you're comfortable with them?
>>
>> Applying them, but inverting them, fix should go first.
> 
> Well, they're both fixes, and inverting them means that you get a
> compile failure across several panels if you happen to be bisecting
> and land on the first commit, but it doesn't really matter. I guess
> the compile failure is maybe a benefit given that they were not doing
> their delays properly... ;-)

Yes, and thanksfully there's a fix for the build failure!

> 
> -Doug

Re: [PATCH 0/2] fix handling of incorrect arguments by mipi_dsi_msleep

[Neil Armstrong]

Hi,

On Wed, 12 Jun 2024 19:05:41 +0530, Tejas Vipin wrote:
> mipi_dsi_msleep is currently defined such that it treats ctx as an
> argument passed by value. In the case of ctx being passed by
> reference, it doesn't raise an error, but instead evaluates the
> resulting expression in an undesired manner. Since the majority of the
> usage of this function passes ctx by reference (similar to
> other functions), mipi_dsi_msleep can be modified to treat ctx as a
> pointer and do it correctly, and the other calls to this macro can be
> adjusted accordingly.
> 
> [...]

Thanks, Applied to https://gitlab.freedesktop.org/drm/misc/kernel.git (drm-misc-next)

[1/2] drm/panel : himax-hx83102: fix incorrect argument to mipi_dsi_msleep
      https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/a13aaf157467e694a3824d81304106b58d4c20d6
[2/2] drm/mipi-dsi: fix handling of ctx in mipi_dsi_msleep
      https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/66055636a146c435cd226fb5a334176304652f3c

-- 
Neil