* [PATCH 1/2] drm: gm12u320: Do not take a mutex from a wait_event condition
@ 2019-08-11 14:37 Hans de Goede
2019-08-11 14:37 ` [PATCH 2/2] drm: gm12u320: Add -ENODEV to list of errors to ignore Hans de Goede
2019-08-11 16:45 ` [PATCH 1/2] drm: gm12u320: Do not take a mutex from a wait_event condition Sam Ravnborg
0 siblings, 2 replies; 3+ messages in thread
From: Hans de Goede @ 2019-08-11 14:37 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Sean Paul, Daniel Vetter,
David Airlie
Cc: Hans de Goede, Sam Ravnborg, dri-devel
I made the condition of the wait_event_timeout call in
gm12u320_fb_update_work a helper which takes a mutex to make sure
that any writes to fb_update.run or fb_update.fb from other CPU cores
are seen before the check is done.
This is not necessary as the wait_event helpers contain the necessary
barriers for this themselves.
More over it is harmfull since by the time the check is done the task
is no longer in the TASK_RUNNING state and calling mutex_lock while not
in task-running is not allowed, leading to this warning when the kernel
is build with some extra locking checks enabled:
[11947.450011] do not call blocking ops when !TASK_RUNNING; state=2 set at
[<00000000e4306de6>] prepare_to_wait_event+0x61/0x190
This commit fixes this by dropping the helper and simply directly
checking the condition (without unnecessary locking) in the
wait_event_timeout call.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/gpu/drm/tiny/gm12u320.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
index 4c47aa4de09b..4d66765b1125 100644
--- a/drivers/gpu/drm/tiny/gm12u320.c
+++ b/drivers/gpu/drm/tiny/gm12u320.c
@@ -342,17 +342,6 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320)
mutex_unlock(&gm12u320->fb_update.lock);
}
-static int gm12u320_fb_update_ready(struct gm12u320_device *gm12u320)
-{
- int ret;
-
- mutex_lock(&gm12u320->fb_update.lock);
- ret = !gm12u320->fb_update.run || gm12u320->fb_update.fb != NULL;
- mutex_unlock(&gm12u320->fb_update.lock);
-
- return ret;
-}
-
static void gm12u320_fb_update_work(struct work_struct *work)
{
struct gm12u320_device *gm12u320 =
@@ -426,7 +415,8 @@ static void gm12u320_fb_update_work(struct work_struct *work)
* switches back to showing its logo.
*/
wait_event_timeout(gm12u320->fb_update.waitq,
- gm12u320_fb_update_ready(gm12u320),
+ !gm12u320->fb_update.run ||
+ gm12u320->fb_update.fb != NULL,
IDLE_TIMEOUT);
}
return;
--
2.22.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH 2/2] drm: gm12u320: Add -ENODEV to list of errors to ignore
2019-08-11 14:37 [PATCH 1/2] drm: gm12u320: Do not take a mutex from a wait_event condition Hans de Goede
@ 2019-08-11 14:37 ` Hans de Goede
2019-08-11 16:45 ` [PATCH 1/2] drm: gm12u320: Do not take a mutex from a wait_event condition Sam Ravnborg
1 sibling, 0 replies; 3+ messages in thread
From: Hans de Goede @ 2019-08-11 14:37 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Sean Paul, Daniel Vetter,
David Airlie
Cc: Hans de Goede, Sam Ravnborg, dri-devel
Add -ENODEV to the list of usb-transfer errors which we ignore to
avoid logging Frame update errors when the device gets unplugged.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/gpu/drm/tiny/gm12u320.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
index 4d66765b1125..08a52a67ec9e 100644
--- a/drivers/gpu/drm/tiny/gm12u320.c
+++ b/drivers/gpu/drm/tiny/gm12u320.c
@@ -422,7 +422,7 @@ static void gm12u320_fb_update_work(struct work_struct *work)
return;
err:
/* Do not log errors caused by module unload or device unplug */
- if (ret != -ECONNRESET && ret != -ESHUTDOWN)
+ if (ret != -ENODEV && ret != -ECONNRESET && ret != -ESHUTDOWN)
GM12U320_ERR("Frame update error: %d\n", ret);
}
--
2.22.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH 1/2] drm: gm12u320: Do not take a mutex from a wait_event condition
2019-08-11 14:37 [PATCH 1/2] drm: gm12u320: Do not take a mutex from a wait_event condition Hans de Goede
2019-08-11 14:37 ` [PATCH 2/2] drm: gm12u320: Add -ENODEV to list of errors to ignore Hans de Goede
@ 2019-08-11 16:45 ` Sam Ravnborg
1 sibling, 0 replies; 3+ messages in thread
From: Sam Ravnborg @ 2019-08-11 16:45 UTC (permalink / raw)
To: Hans de Goede; +Cc: Maxime Ripard, dri-devel, David Airlie, Sean Paul
Hi Hans.
On Sun, Aug 11, 2019 at 04:37:24PM +0200, Hans de Goede wrote:
> I made the condition of the wait_event_timeout call in
> gm12u320_fb_update_work a helper which takes a mutex to make sure
> that any writes to fb_update.run or fb_update.fb from other CPU cores
> are seen before the check is done.
>
> This is not necessary as the wait_event helpers contain the necessary
> barriers for this themselves.
>
> More over it is harmfull since by the time the check is done the task
> is no longer in the TASK_RUNNING state and calling mutex_lock while not
> in task-running is not allowed, leading to this warning when the kernel
> is build with some extra locking checks enabled:
>
> [11947.450011] do not call blocking ops when !TASK_RUNNING; state=2 set at
> [<00000000e4306de6>] prepare_to_wait_event+0x61/0x190
>
> This commit fixes this by dropping the helper and simply directly
> checking the condition (without unnecessary locking) in the
> wait_event_timeout call.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Both patches are:
Acked-by: Sam Ravnborg <sam@ravnborg.org>
Code looks sane and matches your explanations.
But with limited clue on locking or USB this is not a proper review.
Sam
> ---
> drivers/gpu/drm/tiny/gm12u320.c | 14 ++------------
> 1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
> index 4c47aa4de09b..4d66765b1125 100644
> --- a/drivers/gpu/drm/tiny/gm12u320.c
> +++ b/drivers/gpu/drm/tiny/gm12u320.c
> @@ -342,17 +342,6 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320)
> mutex_unlock(&gm12u320->fb_update.lock);
> }
>
> -static int gm12u320_fb_update_ready(struct gm12u320_device *gm12u320)
> -{
> - int ret;
> -
> - mutex_lock(&gm12u320->fb_update.lock);
> - ret = !gm12u320->fb_update.run || gm12u320->fb_update.fb != NULL;
> - mutex_unlock(&gm12u320->fb_update.lock);
> -
> - return ret;
> -}
> -
> static void gm12u320_fb_update_work(struct work_struct *work)
> {
> struct gm12u320_device *gm12u320 =
> @@ -426,7 +415,8 @@ static void gm12u320_fb_update_work(struct work_struct *work)
> * switches back to showing its logo.
> */
> wait_event_timeout(gm12u320->fb_update.waitq,
> - gm12u320_fb_update_ready(gm12u320),
> + !gm12u320->fb_update.run ||
> + gm12u320->fb_update.fb != NULL,
> IDLE_TIMEOUT);
> }
> return;
> --
> 2.22.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-08-11 16:45 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-08-11 14:37 [PATCH 1/2] drm: gm12u320: Do not take a mutex from a wait_event condition Hans de Goede
2019-08-11 14:37 ` [PATCH 2/2] drm: gm12u320: Add -ENODEV to list of errors to ignore Hans de Goede
2019-08-11 16:45 ` [PATCH 1/2] drm: gm12u320: Do not take a mutex from a wait_event condition Sam Ravnborg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox