[v3,2/2] media: uvcvideo: Do not alloc dev->status
Commit Message
UVC_MAX_STATUS_SIZE is 16 bytes, it makes more sense to have it inlined
than dynamically allocate it.
To avoid issues with non-coherent DMAs, give the memory the same
allocation as kmalloc.
This patch kind of reverts:
a31a4055473b ("V4L/DVB:usbvideo:don't use part of buffer for USB transfer #4")
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/usb/uvc/uvc_status.c | 19 ++++++-------------
drivers/media/usb/uvc/uvcvideo.h | 6 +++++-
2 files changed, 11 insertions(+), 14 deletions(-)
Comments
On Thu, Dec 15, 2022 at 11:57:19AM +0100, Ricardo Ribalda wrote:
> + /*
> + * Ensure that status is aligned, making it safe to use with
> + * non-coherent DMA.
> + */
> + struct uvc_status status __aligned(ARCH_KMALLOC_MINALIGN);
This should be ARCH_DMA_MINALIGN, not ARCH_KMALLOC_MINALIGN.
Note that without an __aligned tag on the next member as well, those
next members might get cache corrupted.
>
> struct input_dev *input;
.. and without also aligning the next member you'll might still
corrupt everything adter the DMAed member.
That's the reason why I generall advocate against playing these
__aligned games as they can easily go wrong if someone reorders
the structure.
Hi Christoph
On Thu, 15 Dec 2022 at 13:48, Christoph Hellwig <hch@lst.de> wrote:
>
> On Thu, Dec 15, 2022 at 11:57:19AM +0100, Ricardo Ribalda wrote:
> > + /*
> > + * Ensure that status is aligned, making it safe to use with
> > + * non-coherent DMA.
> > + */
> > + struct uvc_status status __aligned(ARCH_KMALLOC_MINALIGN);
>
> This should be ARCH_DMA_MINALIGN, not ARCH_KMALLOC_MINALIGN.
>
> Note that without an __aligned tag on the next member as well, those
> next members might get cache corrupted.
>
> >
> > struct input_dev *input;
>
> .. and without also aligning the next member you'll might still
> corrupt everything adter the DMAed member.
>
> That's the reason why I generall advocate against playing these
> __aligned games as they can easily go wrong if someone reorders
> the structure.
Thanks a lot for the explanation. I agree, we should keep the
allocation as it is :). Sorry for the noise
Best regards!
@@ -200,22 +200,22 @@ static void uvc_status_complete(struct urb *urb)
len = urb->actual_length;
if (len > 0) {
- switch (dev->status->bStatusType & 0x0f) {
+ switch (dev->status.bStatusType & 0x0f) {
case UVC_STATUS_TYPE_CONTROL: {
- if (uvc_event_control(urb, dev->status, len))
+ if (uvc_event_control(urb, &dev->status, len))
/* The URB will be resubmitted in work context. */
return;
break;
}
case UVC_STATUS_TYPE_STREAMING: {
- uvc_event_streaming(dev, dev->status, len);
+ uvc_event_streaming(dev, &dev->status, len);
break;
}
default:
uvc_dbg(dev, STATUS, "Unknown status event type %u\n",
- dev->status->bStatusType);
+ dev->status.bStatusType);
break;
}
}
@@ -239,15 +239,9 @@ int uvc_status_init(struct uvc_device *dev)
uvc_input_init(dev);
- dev->status = kzalloc(sizeof(*dev->status), GFP_KERNEL);
- if (dev->status == NULL)
- return -ENOMEM;
-
dev->int_urb = usb_alloc_urb(0, GFP_KERNEL);
- if (!dev->int_urb) {
- kfree(dev->status);
+ if (!dev->int_urb)
return -ENOMEM;
- }
pipe = usb_rcvintpipe(dev->udev, ep->desc.bEndpointAddress);
@@ -261,7 +255,7 @@ int uvc_status_init(struct uvc_device *dev)
interval = fls(interval) - 1;
usb_fill_int_urb(dev->int_urb, dev->udev, pipe,
- dev->status, sizeof(dev->status), uvc_status_complete,
+ &dev->status, sizeof(dev->status), uvc_status_complete,
dev, interval);
return 0;
@@ -276,7 +270,6 @@ void uvc_status_unregister(struct uvc_device *dev)
void uvc_status_cleanup(struct uvc_device *dev)
{
usb_free_urb(dev->int_urb);
- kfree(dev->status);
}
int uvc_status_start(struct uvc_device *dev, gfp_t flags)
@@ -577,7 +577,11 @@ struct uvc_device {
/* Status Interrupt Endpoint */
struct usb_host_endpoint *int_ep;
struct urb *int_urb;
- struct uvc_status *status;
+ /*
+ * Ensure that status is aligned, making it safe to use with
+ * non-coherent DMA.
+ */
+ struct uvc_status status __aligned(ARCH_KMALLOC_MINALIGN);
struct input_dev *input;
char input_phys[64];