Re: [PATCH v2 28/31] cpia2_usb: don't use stack for DMA
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 > Signed-off-by: Mauro Carvalho Chehab > --- > 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
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(&cam->v4l2_lock); if (waitqueue_active(&cam->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( &wq, &__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 --- 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(&cam->wq_stream)) - wake_up_interruptible(&cam->wq_stream); + wake_up_interruptible(&cam->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