| Message ID | 20190811143725.5951-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm: gm12u320: Do not take a mutex from a wait_event condition | expand |
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
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;
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(-)