| Message ID | 20221214-uvc-status-alloc-v3-2-9a67616cc549@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: uvcvideo: Code cleanup for dev->status | expand |
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!
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c index dbaa9b07d77f..adf63e7616c9 100644 --- a/drivers/media/usb/uvc/uvc_status.c +++ b/drivers/media/usb/uvc/uvc_status.c @@ -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) diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 84326991ec36..b0abc70ca58e 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -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];
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(-)