Re: [PATCH v2 28/31] cpia2_usb: don't use stack for DMA

[Kosuke Tatsukawa]

Hi,

> The USB control messages require DMA to work. We cannot pass
> a stack-allocated buffer, as it is not warranted that the
> stack would be into a DMA enabled area.
> 
> Signed-off-by: Mauro Carvalho Chehab <mche...@osg.samsung.com>
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
> ---
>  drivers/media/usb/cpia2/cpia2_usb.c | 32 +---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/usb/cpia2/cpia2_usb.c 
> b/drivers/media/usb/cpia2/cpia2_usb.c
> index 13620cdf0599..417d683b237d 100644
> --- a/drivers/media/usb/cpia2/cpia2_usb.c
> +++ b/drivers/media/usb/cpia2/cpia2_usb.c
> @@ -545,10 +545,19 @@ static void free_sbufs(struct camera_data *cam)
>  static int write_packet(struct usb_device *udev,
>   u8 request, u8 * registers, u16 start, size_t size)
>  {
> + unsigned char *buf;
> + int ret;
> +
>   if (!registers || size <= 0)
>   return -EINVAL;
>  
> - return usb_control_msg(udev,
> + buf = kmalloc(size, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + memcpy(buf, registers, size);
> +
> + ret = usb_control_msg(udev,
>  usb_sndctrlpipe(udev, 0),
>  request,
>  USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> @@ -557,6 +566,9 @@ static int write_packet(struct usb_device *udev,
>  registers,   /* buffer */
   =

I think you also want to change the argument to usb_control_msg() from
"registers" to "buf" in write_packet().


>  size,
>  HZ);
> +
> + kfree(buf);
> + return ret;
>  }
..
---
Kosuke TATSUKAWA  | 1st Platform Software Division
  | NEC Solution Innovators
  | ta...@ab.jp.nec.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] media: fix waitqueue_active without memory barrier in cpia2 driver

[Kosuke Tatsukawa]

cpia2_usb_disconnect() seems to be missing a memory barrier which might
cause the waker to not notice the waiter and miss sending a wake_up as
in the following figure.

cpia2_usb_disconnectsync

mutex_unlock(>v4l2_lock);
if (waitqueue_active(>wq_stream))
/* The CPU might reorder the test for
   the waitqueue up here, before
   prior writes complete */
/* wait_event_interruptible */
 /* __wait_event_interruptible */
  /* ___wait_event */
  long __int = prepare_to_wait_event(
, &__wait, state);
  if (!cam->streaming ||
frame->status == FRAME_READY)
cam->curbuff->status = FRAME_READY;
cam->curbuff->length = 0;
  schedule()


The attached patch removes the call to waitqueue_active() leaving just
wake_up() behind.  This fixes the problem because the call to
spin_lock_irqsave() in wake_up() will be an ACQUIRE operation.

I found this issue when I was looking through the linux source code
for places calling waitqueue_active() before wake_up*(), but without
preceding memory barriers, after sending a patch to fix a similar
issue in drivers/tty/n_tty.c  (Details about the original issue can be
found here: https://lkml.org/lkml/2015/9/28/849).

Signed-off-by: Kosuke Tatsukawa <ta...@ab.jp.nec.com>
---
 drivers/media/usb/cpia2/cpia2_usb.c |3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/cpia2/cpia2_usb.c 
b/drivers/media/usb/cpia2/cpia2_usb.c
index 351a78a..c1aa1ab 100644
--- a/drivers/media/usb/cpia2/cpia2_usb.c
+++ b/drivers/media/usb/cpia2/cpia2_usb.c
@@ -890,8 +890,7 @@ static void cpia2_usb_disconnect(struct usb_interface *intf)
DBG("Wakeup waiting processes\n");
cam->curbuff->status = FRAME_READY;
cam->curbuff->length = 0;
-   if (waitqueue_active(>wq_stream))
-   wake_up_interruptible(>wq_stream);
+   wake_up_interruptible(>wq_stream);
}
 
DBG("Releasing interface\n");
-- 
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html