re: drm/nouveau/pwr: initial implementation

re: drm/nouveau/pwr: initial implementation

[Dan Carpenter]

Hello Ben Skeggs,

The patch ff4b42c7532e: "drm/nouveau/pwr: initial implementation"
from Oct 15, 2013, leads to the following Smatch warning:
"drivers/gpu/drm/nouveau/core/subdev/pwr/base.c:48 nouveau_pwr_send()
	 warn: 'mutex:&subdev->mutex' is sometimes locked here and sometimes unlocked."

drivers/gpu/drm/nouveau/core/subdev/pwr/base.c
    35          /* we currently only support a single process at a time waiting
    36           * on a synchronous reply, take the PPWR mutex and tell the
    37           * receive handler what we're waiting for
    38           */
    39          if (reply) {
    40                  mutex_lock(&subdev->mutex);
    41                  ppwr->recv.message = message;
    42                  ppwr->recv.process = process;
    43          }
    44  
    45          /* wait for a free slot in the fifo */
    46          addr  = nv_rd32(ppwr, 0x10a4a0);
    47          if (!nv_wait_ne(ppwr, 0x10a4b0, 0xffffffff, addr ^ 8))
    48                  return -EBUSY;

Should we have an if (reply) mutex_unlock(&subdev->mutex) before the
return?

    49  

regards,
dan carpenter

Re: drm/nouveau/pwr: initial implementation

[Ben Skeggs]

----- Original Message -----
> From: "Dan Carpenter" <dan.carpenter@oracle.com>
> To: bskeggs@redhat.com
> Cc: dri-devel@lists.freedesktop.org
> Sent: Tuesday, 12 November, 2013 7:54:33 PM
> Subject: re: drm/nouveau/pwr: initial implementation
> 
> Hello Ben Skeggs,
Hey,

> 
> The patch ff4b42c7532e: "drm/nouveau/pwr: initial implementation"
> from Oct 15, 2013, leads to the following Smatch warning:
> "drivers/gpu/drm/nouveau/core/subdev/pwr/base.c:48 nouveau_pwr_send()
> 	 warn: 'mutex:&subdev->mutex' is sometimes locked here and sometimes
> 	 unlocked."
> 
> drivers/gpu/drm/nouveau/core/subdev/pwr/base.c
>     35          /* we currently only support a single process at a time
>     waiting
>     36           * on a synchronous reply, take the PPWR mutex and tell the
>     37           * receive handler what we're waiting for
>     38           */
>     39          if (reply) {
>     40                  mutex_lock(&subdev->mutex);
>     41                  ppwr->recv.message = message;
>     42                  ppwr->recv.process = process;
>     43          }
>     44
>     45          /* wait for a free slot in the fifo */
>     46          addr  = nv_rd32(ppwr, 0x10a4a0);
>     47          if (!nv_wait_ne(ppwr, 0x10a4b0, 0xffffffff, addr ^ 8))
>     48                  return -EBUSY;
> 
> Should we have an if (reply) mutex_unlock(&subdev->mutex) before the
> return?
Yes, we should.  It's been reported, no fix pushed to Linus' tree yet as the path that can hit it has been hardcoded as disabled for this release.

Thanks,
Ben.

> 
>     49
> 
> regards,
> dan carpenter
> 
>