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
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 <[email protected]>
---
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
Hi,
On Wed, Jun 12, 2024 at 6:37 AM Tejas Vipin <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
Reviewed-by: Douglas Anderson <[email protected]>
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
On 12/06/2024 16:21, Doug Anderson wrote:
> Hi,
>
> On Wed, Jun 12, 2024 at 6:37 AM Tejas Vipin <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
> Reviewed-by: Douglas Anderson <[email protected]>
>
> 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
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
Hi,
On Wed, Jun 12, 2024 at 7:34 AM <[email protected]> wrote:
>
> On 12/06/2024 16:21, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Jun 12, 2024 at 6:37 AM Tejas Vipin <[email protected]> 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 <[email protected]>
> >> ---
> >> 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 <[email protected]>
> > Reviewed-by: Douglas Anderson <[email protected]>
> >
> > 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
On 12/06/2024 16:52, Doug Anderson wrote:
> Hi,
>
> On Wed, Jun 12, 2024 at 7:34 AM <[email protected]> wrote:
>>
>> On 12/06/2024 16:21, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Wed, Jun 12, 2024 at 6:37 AM Tejas Vipin <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
>>> Reviewed-by: Douglas Anderson <[email protected]>
>>>
>>> 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