Re: [PATCH 1/2] drm: gm12u320: Do not take a mutex from a wait_event condition

[Sam Ravnborg <sam@ravnborg.org>]

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