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(-)

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];

-- 
2.39.0.314.g84b9a713c41-goog-b4-0.11.0-dev-696ae

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!



-- 
Ricardo Ribalda