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;
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); }
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);-- 2.22.0
dri-devel@lists.freedesktop.org
-
Hans de Goede -
Sam Ravnborg