| Message ID | 1397493665-912-1-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Monday, April 14, 2014, 6:41:05 PM, you wrote: > Currently stk1160_read_reg() uses a stack-allocated char to get the > read control value. This is wrong because usb_control_msg() requires > a kmalloc-ed buffer, and a DMA-API warning is produced: > WARNING: CPU: 0 PID: 1376 at lib/dma-debug.c:1153 check_for_stack+0xa0/0x100() > ehci-pci 0000:00:0a.0: DMA-API: device driver maps memory fromstack [addr=ffff88003d0b56bf] > This commit fixes such issue by using a 'usb_ctrl_read' field embedded > in the device's struct to pass the value. In addition, we introduce a > mutex to protect the value. > While here, let's remove the urb_buf array which was meant for a similar > purpose, but never really used. > Reported-by: Sander Eikelenboom <linux@eikelenboom.it> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > --- > Sander, Hans: > Does this cause any regressions, other than the DMA-API warning? > In that case, we can consider this as suitable for -stable. Thanks ! Will test and report back later. -- Sander > drivers/media/usb/stk1160/stk1160-core.c | 8 +++++++- > drivers/media/usb/stk1160/stk1160.h | 3 ++- > 2 files changed, 9 insertions(+), 2 deletions(-) > diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c > index 34a26e0..cce91e7 100644 > --- a/drivers/media/usb/stk1160/stk1160-core.c > +++ b/drivers/media/usb/stk1160/stk1160-core.c > @@ -69,15 +69,20 @@ int stk1160_read_reg(struct stk1160 *dev, u16 reg, u8 *value) > int pipe = usb_rcvctrlpipe(dev->udev, 0); > > *value = 0; > + > + mutex_lock(&dev->urb_ctrl_lock); > ret = usb_control_msg(dev->udev, pipe, 0x00, > USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, > - 0x00, reg, value, sizeof(u8), HZ); > + 0x00, reg, &dev->urb_ctrl_read, sizeof(u8), HZ); > if (ret < 0) { > stk1160_err("read failed on reg 0x%x (%d)\n", > reg, ret); > + mutex_unlock(&dev->urb_ctrl_lock); > return ret; > } > > + *value = dev->urb_ctrl_read; > + mutex_unlock(&dev->urb_ctrl_lock); > return 0; > } > > @@ -322,6 +327,7 @@ static int stk1160_probe(struct usb_interface *interface, > * because we register the device node as the *last* thing. > */ > spin_lock_init(&dev->buf_lock); > + mutex_init(&dev->urb_ctrl_lock); > mutex_init(&dev->v4l_lock); > mutex_init(&dev->vb_queue_lock); > > diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h > index 05b05b1..8886be4 100644 > --- a/drivers/media/usb/stk1160/stk1160.h > +++ b/drivers/media/usb/stk1160/stk1160.h > @@ -143,7 +143,7 @@ struct stk1160 { > int num_alt; > > struct stk1160_isoc_ctl isoc_ctl; > - char urb_buf[255]; /* urb control msg buffer */ > + char urb_ctrl_read; > > /* frame properties */ > int width; /* current frame width */ > @@ -159,6 +159,7 @@ struct stk1160 { > struct i2c_adapter i2c_adap; > struct i2c_client i2c_client; > > + struct mutex urb_ctrl_lock; /* protects urb_ctrl_read */ > struct mutex v4l_lock; > struct mutex vb_queue_lock; > spinlock_t buf_lock; -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 14 Apr 2014, Ezequiel Garcia wrote: > Currently stk1160_read_reg() uses a stack-allocated char to get the > read control value. This is wrong because usb_control_msg() requires > a kmalloc-ed buffer, and a DMA-API warning is produced: > > WARNING: CPU: 0 PID: 1376 at lib/dma-debug.c:1153 check_for_stack+0xa0/0x100() > ehci-pci 0000:00:0a.0: DMA-API: device driver maps memory fromstack [addr=ffff88003d0b56bf] > > This commit fixes such issue by using a 'usb_ctrl_read' field embedded > in the device's struct to pass the value. In addition, we introduce a > mutex to protect the value. This isn't right either. The buffer must be allocated in its own cache line; it must not be part of a larger structure. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Apr 14, Alan Stern wrote: > On Mon, 14 Apr 2014, Ezequiel Garcia wrote: > > > Currently stk1160_read_reg() uses a stack-allocated char to get the > > read control value. This is wrong because usb_control_msg() requires > > a kmalloc-ed buffer, and a DMA-API warning is produced: > > > > WARNING: CPU: 0 PID: 1376 at lib/dma-debug.c:1153 check_for_stack+0xa0/0x100() > > ehci-pci 0000:00:0a.0: DMA-API: device driver maps memory fromstack [addr=ffff88003d0b56bf] > > > > This commit fixes such issue by using a 'usb_ctrl_read' field embedded > > in the device's struct to pass the value. In addition, we introduce a > > mutex to protect the value. > > This isn't right either. The buffer must be allocated in its own cache > line; it must not be part of a larger structure. > In that case, we can simply allocate 1 byte using kmalloc(). We won't be needing the mutex and it'll ensure proper cache alignment, right?
On Mon, 14 Apr 2014, Ezequiel Garcia wrote: > On Apr 14, Alan Stern wrote: > > On Mon, 14 Apr 2014, Ezequiel Garcia wrote: > > > > > Currently stk1160_read_reg() uses a stack-allocated char to get the > > > read control value. This is wrong because usb_control_msg() requires > > > a kmalloc-ed buffer, and a DMA-API warning is produced: > > > > > > WARNING: CPU: 0 PID: 1376 at lib/dma-debug.c:1153 check_for_stack+0xa0/0x100() > > > ehci-pci 0000:00:0a.0: DMA-API: device driver maps memory fromstack [addr=ffff88003d0b56bf] > > > > > > This commit fixes such issue by using a 'usb_ctrl_read' field embedded > > > in the device's struct to pass the value. In addition, we introduce a > > > mutex to protect the value. > > > > This isn't right either. The buffer must be allocated in its own cache > > line; it must not be part of a larger structure. > > > > In that case, we can simply allocate 1 byte using kmalloc(). We won't > be needing the mutex and it'll ensure proper cache alignment, right? Yes, that will work fine. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c index 34a26e0..cce91e7 100644 --- a/drivers/media/usb/stk1160/stk1160-core.c +++ b/drivers/media/usb/stk1160/stk1160-core.c @@ -69,15 +69,20 @@ int stk1160_read_reg(struct stk1160 *dev, u16 reg, u8 *value) int pipe = usb_rcvctrlpipe(dev->udev, 0); *value = 0; + + mutex_lock(&dev->urb_ctrl_lock); ret = usb_control_msg(dev->udev, pipe, 0x00, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, - 0x00, reg, value, sizeof(u8), HZ); + 0x00, reg, &dev->urb_ctrl_read, sizeof(u8), HZ); if (ret < 0) { stk1160_err("read failed on reg 0x%x (%d)\n", reg, ret); + mutex_unlock(&dev->urb_ctrl_lock); return ret; } + *value = dev->urb_ctrl_read; + mutex_unlock(&dev->urb_ctrl_lock); return 0; } @@ -322,6 +327,7 @@ static int stk1160_probe(struct usb_interface *interface, * because we register the device node as the *last* thing. */ spin_lock_init(&dev->buf_lock); + mutex_init(&dev->urb_ctrl_lock); mutex_init(&dev->v4l_lock); mutex_init(&dev->vb_queue_lock); diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h index 05b05b1..8886be4 100644 --- a/drivers/media/usb/stk1160/stk1160.h +++ b/drivers/media/usb/stk1160/stk1160.h @@ -143,7 +143,7 @@ struct stk1160 { int num_alt; struct stk1160_isoc_ctl isoc_ctl; - char urb_buf[255]; /* urb control msg buffer */ + char urb_ctrl_read; /* frame properties */ int width; /* current frame width */ @@ -159,6 +159,7 @@ struct stk1160 { struct i2c_adapter i2c_adap; struct i2c_client i2c_client; + struct mutex urb_ctrl_lock; /* protects urb_ctrl_read */ struct mutex v4l_lock; struct mutex vb_queue_lock; spinlock_t buf_lock;
Currently stk1160_read_reg() uses a stack-allocated char to get the read control value. This is wrong because usb_control_msg() requires a kmalloc-ed buffer, and a DMA-API warning is produced: WARNING: CPU: 0 PID: 1376 at lib/dma-debug.c:1153 check_for_stack+0xa0/0x100() ehci-pci 0000:00:0a.0: DMA-API: device driver maps memory fromstack [addr=ffff88003d0b56bf] This commit fixes such issue by using a 'usb_ctrl_read' field embedded in the device's struct to pass the value. In addition, we introduce a mutex to protect the value. While here, let's remove the urb_buf array which was meant for a similar purpose, but never really used. Reported-by: Sander Eikelenboom <linux@eikelenboom.it> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> --- Sander, Hans: Does this cause any regressions, other than the DMA-API warning? In that case, we can consider this as suitable for -stable. drivers/media/usb/stk1160/stk1160-core.c | 8 +++++++- drivers/media/usb/stk1160/stk1160.h | 3 ++- 2 files changed, 9 insertions(+), 2 deletions(-)