[PATCH v2] media: uvcvideo: Do not alloc dev->status

[PATCH v2] media: uvcvideo: Do not alloc dev->status

[Ricardo Ribalda]

UVC_MAX_STATUS_SIZE is 16 bytes, simplify the code by inlining dev->status.

Now that we are at it, remove all the castings.

To avoid issues with non-coherent DMAs, give the memory the same
allocation as kmalloc.

This patch kind of reverts:
Fixes: a31a4055473b ("V4L/DVB:usbvideo:don't use part of buffer for USB transfer #4"

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
To: Ming Lei <tom.leiming@gmail.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
To: Yunke Cao <yunkec@chromium.org>
To: Sergey Senozhatsky <senozhatsky@chromium.org>
To: Max Staudt <mstaudt@google.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: linux-media@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
Changes in v2:
- using __aligned(), to keep the old alignment
- Adding Johnathan Cameron to:, as he has some similar experience with iio
- Adding Ming Lei, as this patch kind of revert his patch
- Link to v1: https://lore.kernel.org/r/20221214-uvc-status-alloc-v1-0-a0098ddc7c93@chromium.org
---
 drivers/media/usb/uvc/uvc_status.c | 69 ++++++++++++--------------------------
 drivers/media/usb/uvc/uvcvideo.h   | 28 +++++++++++++++-
 2 files changed, 48 insertions(+), 49 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index 7518ffce22ed..adf63e7616c9 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -73,38 +73,24 @@ static void uvc_input_report_key(struct uvc_device *dev, unsigned int code,
 /* --------------------------------------------------------------------------
  * Status interrupt endpoint
  */
-struct uvc_streaming_status {
-	u8	bStatusType;
-	u8	bOriginator;
-	u8	bEvent;
-	u8	bValue[];
-} __packed;
-
-struct uvc_control_status {
-	u8	bStatusType;
-	u8	bOriginator;
-	u8	bEvent;
-	u8	bSelector;
-	u8	bAttribute;
-	u8	bValue[];
-} __packed;
-
 static void uvc_event_streaming(struct uvc_device *dev,
-				struct uvc_streaming_status *status, int len)
+				struct uvc_status *status, int len)
 {
-	if (len < 3) {
+	if (len <= offsetof(struct uvc_status, bEvent)) {
 		uvc_dbg(dev, STATUS,
 			"Invalid streaming status event received\n");
 		return;
 	}
 
 	if (status->bEvent == 0) {
-		if (len < 4)
+		if (len <= offsetof(struct uvc_status, streaming))
 			return;
+
 		uvc_dbg(dev, STATUS, "Button (intf %u) %s len %d\n",
 			status->bOriginator,
-			status->bValue[0] ? "pressed" : "released", len);
-		uvc_input_report_key(dev, KEY_CAMERA, status->bValue[0]);
+			status->streaming.button ? "pressed" : "released", len);
+		uvc_input_report_key(dev, KEY_CAMERA,
+				     status->streaming.button);
 	} else {
 		uvc_dbg(dev, STATUS, "Stream %u error event %02x len %d\n",
 			status->bOriginator, status->bEvent, len);
@@ -131,7 +117,7 @@ static struct uvc_control *uvc_event_entity_find_ctrl(struct uvc_entity *entity,
 }
 
 static struct uvc_control *uvc_event_find_ctrl(struct uvc_device *dev,
-					const struct uvc_control_status *status,
+					const struct uvc_status *status,
 					struct uvc_video_chain **chain)
 {
 	list_for_each_entry((*chain), &dev->chains, list) {
@@ -143,7 +129,7 @@ static struct uvc_control *uvc_event_find_ctrl(struct uvc_device *dev,
 				continue;
 
 			ctrl = uvc_event_entity_find_ctrl(entity,
-							  status->bSelector);
+						     status->control.bSelector);
 			if (ctrl)
 				return ctrl;
 		}
@@ -153,7 +139,7 @@ static struct uvc_control *uvc_event_find_ctrl(struct uvc_device *dev,
 }
 
 static bool uvc_event_control(struct urb *urb,
-			      const struct uvc_control_status *status, int len)
+			      const struct uvc_status *status, int len)
 {
 	static const char *attrs[] = { "value", "info", "failure", "min", "max" };
 	struct uvc_device *dev = urb->context;
@@ -161,24 +147,24 @@ static bool uvc_event_control(struct urb *urb,
 	struct uvc_control *ctrl;
 
 	if (len < 6 || status->bEvent != 0 ||
-	    status->bAttribute >= ARRAY_SIZE(attrs)) {
+	    status->control.bAttribute >= ARRAY_SIZE(attrs)) {
 		uvc_dbg(dev, STATUS, "Invalid control status event received\n");
 		return false;
 	}
 
 	uvc_dbg(dev, STATUS, "Control %u/%u %s change len %d\n",
-		status->bOriginator, status->bSelector,
-		attrs[status->bAttribute], len);
+		status->bOriginator, status->control.bSelector,
+		attrs[status->control.bAttribute], len);
 
 	/* Find the control. */
 	ctrl = uvc_event_find_ctrl(dev, status, &chain);
 	if (!ctrl)
 		return false;
 
-	switch (status->bAttribute) {
+	switch (status->control.bAttribute) {
 	case UVC_CTRL_VALUE_CHANGE:
 		return uvc_ctrl_status_event_async(urb, chain, ctrl,
-						   status->bValue);
+						   status->control.bValue);
 
 	case UVC_CTRL_INFO_CHANGE:
 	case UVC_CTRL_FAILURE_CHANGE:
@@ -214,28 +200,22 @@ static void uvc_status_complete(struct urb *urb)
 
 	len = urb->actual_length;
 	if (len > 0) {
-		switch (dev->status[0] & 0x0f) {
+		switch (dev->status.bStatusType & 0x0f) {
 		case UVC_STATUS_TYPE_CONTROL: {
-			struct uvc_control_status *status =
-				(struct uvc_control_status *)dev->status;
-
-			if (uvc_event_control(urb, 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: {
-			struct uvc_streaming_status *status =
-				(struct uvc_streaming_status *)dev->status;
-
-			uvc_event_streaming(dev, status, len);
+			uvc_event_streaming(dev, &dev->status, len);
 			break;
 		}
 
 		default:
 			uvc_dbg(dev, STATUS, "Unknown status event type %u\n",
-				dev->status[0]);
+				dev->status.bStatusType);
 			break;
 		}
 	}
@@ -259,15 +239,9 @@ int uvc_status_init(struct uvc_device *dev)
 
 	uvc_input_init(dev);
 
-	dev->status = kzalloc(UVC_MAX_STATUS_SIZE, GFP_KERNEL);
-	if (dev->status == NULL)
-		return -ENOMEM;
-
 	dev->int_urb = usb_alloc_urb(0, GFP_KERNEL);
-	if (dev->int_urb == NULL) {
-		kfree(dev->status);
+	if (!dev->int_urb)
 		return -ENOMEM;
-	}
 
 	pipe = usb_rcvintpipe(dev->udev, ep->desc.bEndpointAddress);
 
@@ -281,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, UVC_MAX_STATUS_SIZE, uvc_status_complete,
+		&dev->status, sizeof(dev->status), uvc_status_complete,
 		dev, interval);
 
 	return 0;
@@ -296,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 df93db259312..5dfc2896ce88 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -527,6 +527,26 @@ struct uvc_device_info {
 	const struct uvc_control_mapping **mappings;
 };
 
+struct uvc_status_streaming {
+	u8	button;
+} __packed;
+
+struct uvc_status_control {
+	u8	bSelector;
+	u8	bAttribute;
+	u8	bValue[11];
+} __packed;
+
+struct uvc_status {
+	u8	bStatusType;
+	u8	bOriginator;
+	u8	bEvent;
+	union {
+		struct uvc_status_control control;
+		struct uvc_status_streaming streaming;
+	};
+} __packed;
+
 struct uvc_device {
 	struct usb_device *udev;
 	struct usb_interface *intf;
@@ -559,7 +579,7 @@ struct uvc_device {
 	/* Status Interrupt Endpoint */
 	struct usb_host_endpoint *int_ep;
 	struct urb *int_urb;
-	u8 *status;
+
 	struct input_dev *input;
 	char input_phys[64];
 
@@ -572,6 +592,12 @@ struct uvc_device {
 	} async_ctrl;
 
 	struct uvc_entity *gpio_unit;
+
+	/*
+	 * Ensure that status is aligned, making it safe to use with
+	 * non-coherent DMA.
+	 */
+	struct uvc_status status __aligned(ARCH_KMALLOC_MINALIGN);
 };
 
 enum uvc_handle_state {

---
base-commit: 0ec5a38bf8499f403f81cb81a0e3a60887d1993c
change-id: 20221214-uvc-status-alloc-93becb783898

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

Re: [PATCH v2] media: uvcvideo: Do not alloc dev->status

[Sergey Senozhatsky]

On (22/12/14 14:37), Ricardo Ribalda wrote:
[..]
> +struct uvc_status_streaming {
> +	u8	button;
> +} __packed;
> +
> +struct uvc_status_control {
> +	u8	bSelector;
> +	u8	bAttribute;
> +	u8	bValue[11];
> +} __packed;
> +
> +struct uvc_status {
> +	u8	bStatusType;
> +	u8	bOriginator;
> +	u8	bEvent;
> +	union {
> +		struct uvc_status_control control;
> +		struct uvc_status_streaming streaming;
> +	};
> +} __packed;
> +
>  struct uvc_device {
>  	struct usb_device *udev;
>  	struct usb_interface *intf;
> @@ -559,7 +579,7 @@ struct uvc_device {
>  	/* Status Interrupt Endpoint */
>  	struct usb_host_endpoint *int_ep;
>  	struct urb *int_urb;
> -	u8 *status;
> +
>  	struct input_dev *input;
>  	char input_phys[64];
>  
> @@ -572,6 +592,12 @@ struct uvc_device {
>  	} async_ctrl;
>  
>  	struct uvc_entity *gpio_unit;
> +
> +	/*
> +	 * Ensure that status is aligned, making it safe to use with
> +	 * non-coherent DMA.
> +	 */
> +	struct uvc_status status __aligned(ARCH_KMALLOC_MINALIGN);

	____cacheline_aligned ?

I don't see anyone using ARCH_KMALLOC_MINALIGN except for slab.h

Re: [PATCH v2] media: uvcvideo: Do not alloc dev->status

[Ricardo Ribalda]

Hi Sergey

Thanks for looking into this

On Thu, 15 Dec 2022 at 02:15, Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (22/12/14 14:37), Ricardo Ribalda wrote:
> [..]
> > +struct uvc_status_streaming {
> > +     u8      button;
> > +} __packed;
> > +
> > +struct uvc_status_control {
> > +     u8      bSelector;
> > +     u8      bAttribute;
> > +     u8      bValue[11];
> > +} __packed;
> > +
> > +struct uvc_status {
> > +     u8      bStatusType;
> > +     u8      bOriginator;
> > +     u8      bEvent;
> > +     union {
> > +             struct uvc_status_control control;
> > +             struct uvc_status_streaming streaming;
> > +     };
> > +} __packed;
> > +
> >  struct uvc_device {
> >       struct usb_device *udev;
> >       struct usb_interface *intf;
> > @@ -559,7 +579,7 @@ struct uvc_device {
> >       /* Status Interrupt Endpoint */
> >       struct usb_host_endpoint *int_ep;
> >       struct urb *int_urb;
> > -     u8 *status;
> > +
> >       struct input_dev *input;
> >       char input_phys[64];
> >
> > @@ -572,6 +592,12 @@ struct uvc_device {
> >       } async_ctrl;
> >
> >       struct uvc_entity *gpio_unit;
> > +
> > +     /*
> > +      * Ensure that status is aligned, making it safe to use with
> > +      * non-coherent DMA.
> > +      */
> > +     struct uvc_status status __aligned(ARCH_KMALLOC_MINALIGN);
>
>         ____cacheline_aligned ?
>
> I don't see anyone using ARCH_KMALLOC_MINALIGN except for slab.h

Seems like cacheline is not good enough:

https://github.com/torvalds/linux/commit/12c4efe3509b8018e76ea3ebda8227cb53bf5887
https://lore.kernel.org/all/20220405135758.774016-1-catalin.marinas@arm.com/

and ARCH_KMALLOC_MINALIGN is what we have today and is working...

But yeah, the name for that define is not the nicest :)

I added Jonathan Cameron, on cc, as he had to deal with something
similar for iio in case we are missing something


ps: and I thought this was an easy change :P

-- 
Ricardo Ribalda

Re: [PATCH v2] media: uvcvideo: Do not alloc dev->status

[Laurent Pinchart]

Hi Ricardo,

On Thu, Dec 15, 2022 at 08:59:14AM +0100, Ricardo Ribalda wrote:
> Hi Sergey
> 
> Thanks for looking into this
> 
> On Thu, 15 Dec 2022 at 02:15, Sergey Senozhatsky wrote:
> >
> > On (22/12/14 14:37), Ricardo Ribalda wrote:
> > [..]
> > > +struct uvc_status_streaming {
> > > +     u8      button;
> > > +} __packed;
> > > +
> > > +struct uvc_status_control {
> > > +     u8      bSelector;
> > > +     u8      bAttribute;
> > > +     u8      bValue[11];
> > > +} __packed;
> > > +
> > > +struct uvc_status {
> > > +     u8      bStatusType;
> > > +     u8      bOriginator;
> > > +     u8      bEvent;
> > > +     union {
> > > +             struct uvc_status_control control;
> > > +             struct uvc_status_streaming streaming;
> > > +     };
> > > +} __packed;
> > > +
> > >  struct uvc_device {
> > >       struct usb_device *udev;
> > >       struct usb_interface *intf;
> > > @@ -559,7 +579,7 @@ struct uvc_device {
> > >       /* Status Interrupt Endpoint */
> > >       struct usb_host_endpoint *int_ep;
> > >       struct urb *int_urb;
> > > -     u8 *status;
> > > +
> > >       struct input_dev *input;
> > >       char input_phys[64];
> > >
> > > @@ -572,6 +592,12 @@ struct uvc_device {
> > >       } async_ctrl;
> > >
> > >       struct uvc_entity *gpio_unit;
> > > +
> > > +     /*
> > > +      * Ensure that status is aligned, making it safe to use with
> > > +      * non-coherent DMA.
> > > +      */
> > > +     struct uvc_status status __aligned(ARCH_KMALLOC_MINALIGN);
> >
> >         ____cacheline_aligned ?
> >
> > I don't see anyone using ARCH_KMALLOC_MINALIGN except for slab.h
> 
> Seems like cacheline is not good enough:
> 
> https://github.com/torvalds/linux/commit/12c4efe3509b8018e76ea3ebda8227cb53bf5887
> https://lore.kernel.org/all/20220405135758.774016-1-catalin.marinas@arm.com/
> 
> and ARCH_KMALLOC_MINALIGN is what we have today and is working...
> 
> But yeah, the name for that define is not the nicest :)
> 
> I added Jonathan Cameron, on cc, as he had to deal with something
> similar for iio in case we are missing something

I'd like to get feedback on this from DMA and USB experts. Expanding the
CC list of the original patch would help (especially including the
linux-usb mailing list).

> ps: and I thought this was an easy change :P

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: uvcvideo: Do not alloc dev->status

[Laurent Pinchart]

Hi Ricardo,

On Thu, Dec 15, 2022 at 11:08:05AM +0200, Laurent Pinchart wrote:
> On Thu, Dec 15, 2022 at 08:59:14AM +0100, Ricardo Ribalda wrote:
> > On Thu, 15 Dec 2022 at 02:15, Sergey Senozhatsky wrote:
> > >
> > > On (22/12/14 14:37), Ricardo Ribalda wrote:
> > > [..]
> > > > +struct uvc_status_streaming {
> > > > +     u8      button;
> > > > +} __packed;
> > > > +
> > > > +struct uvc_status_control {
> > > > +     u8      bSelector;
> > > > +     u8      bAttribute;
> > > > +     u8      bValue[11];
> > > > +} __packed;
> > > > +
> > > > +struct uvc_status {
> > > > +     u8      bStatusType;
> > > > +     u8      bOriginator;
> > > > +     u8      bEvent;
> > > > +     union {
> > > > +             struct uvc_status_control control;
> > > > +             struct uvc_status_streaming streaming;
> > > > +     };
> > > > +} __packed;
> > > > +
> > > >  struct uvc_device {
> > > >       struct usb_device *udev;
> > > >       struct usb_interface *intf;
> > > > @@ -559,7 +579,7 @@ struct uvc_device {
> > > >       /* Status Interrupt Endpoint */
> > > >       struct usb_host_endpoint *int_ep;
> > > >       struct urb *int_urb;
> > > > -     u8 *status;
> > > > +
> > > >       struct input_dev *input;
> > > >       char input_phys[64];
> > > >
> > > > @@ -572,6 +592,12 @@ struct uvc_device {
> > > >       } async_ctrl;
> > > >
> > > >       struct uvc_entity *gpio_unit;
> > > > +
> > > > +     /*
> > > > +      * Ensure that status is aligned, making it safe to use with
> > > > +      * non-coherent DMA.
> > > > +      */
> > > > +     struct uvc_status status __aligned(ARCH_KMALLOC_MINALIGN);
> > >
> > >         ____cacheline_aligned ?
> > >
> > > I don't see anyone using ARCH_KMALLOC_MINALIGN except for slab.h
> > 
> > Seems like cacheline is not good enough:
> > 
> > https://github.com/torvalds/linux/commit/12c4efe3509b8018e76ea3ebda8227cb53bf5887
> > https://lore.kernel.org/all/20220405135758.774016-1-catalin.marinas@arm.com/
> > 
> > and ARCH_KMALLOC_MINALIGN is what we have today and is working...
> > 
> > But yeah, the name for that define is not the nicest :)
> > 
> > I added Jonathan Cameron, on cc, as he had to deal with something
> > similar for iio in case we are missing something
> 
> I'd like to get feedback on this from DMA and USB experts. Expanding the
> CC list of the original patch would help (especially including the
> linux-usb mailing list).

Also, do we need the allocation change ? It doesn't seem to simplify the
code that much, neither in terms of lines of code

>  2 files changed, 48 insertions(+), 49 deletions(-)

nor in terms of complexity. Maybe we could keep the union and offsetof
changes, and drop the allocation change ? In any case, those are two
different changes, so I'd split them in two patches at least.

> > ps: and I thought this was an easy change :P

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: uvcvideo: Do not alloc dev->status

[Jonathan Cameron]

On Thu, 15 Dec 2022 11:11:40 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Ricardo,
> 
> On Thu, Dec 15, 2022 at 11:08:05AM +0200, Laurent Pinchart wrote:
> > On Thu, Dec 15, 2022 at 08:59:14AM +0100, Ricardo Ribalda wrote:  
> > > On Thu, 15 Dec 2022 at 02:15, Sergey Senozhatsky wrote:  
> > > >
> > > > On (22/12/14 14:37), Ricardo Ribalda wrote:
> > > > [..]  
> > > > > +struct uvc_status_streaming {
> > > > > +     u8      button;
> > > > > +} __packed;
> > > > > +
> > > > > +struct uvc_status_control {
> > > > > +     u8      bSelector;
> > > > > +     u8      bAttribute;
> > > > > +     u8      bValue[11];
> > > > > +} __packed;
> > > > > +
> > > > > +struct uvc_status {
> > > > > +     u8      bStatusType;
> > > > > +     u8      bOriginator;
> > > > > +     u8      bEvent;
> > > > > +     union {
> > > > > +             struct uvc_status_control control;
> > > > > +             struct uvc_status_streaming streaming;
> > > > > +     };
> > > > > +} __packed;
> > > > > +
> > > > >  struct uvc_device {
> > > > >       struct usb_device *udev;
> > > > >       struct usb_interface *intf;
> > > > > @@ -559,7 +579,7 @@ struct uvc_device {
> > > > >       /* Status Interrupt Endpoint */
> > > > >       struct usb_host_endpoint *int_ep;
> > > > >       struct urb *int_urb;
> > > > > -     u8 *status;
> > > > > +
> > > > >       struct input_dev *input;
> > > > >       char input_phys[64];
> > > > >
> > > > > @@ -572,6 +592,12 @@ struct uvc_device {
> > > > >       } async_ctrl;
> > > > >
> > > > >       struct uvc_entity *gpio_unit;
> > > > > +
> > > > > +     /*
> > > > > +      * Ensure that status is aligned, making it safe to use with
> > > > > +      * non-coherent DMA.
> > > > > +      */
> > > > > +     struct uvc_status status __aligned(ARCH_KMALLOC_MINALIGN);  
> > > >
> > > >         ____cacheline_aligned ?
> > > >
> > > > I don't see anyone using ARCH_KMALLOC_MINALIGN except for slab.h  
> > > 
> > > Seems like cacheline is not good enough:
> > > 
> > > https://github.com/torvalds/linux/commit/12c4efe3509b8018e76ea3ebda8227cb53bf5887
> > > https://lore.kernel.org/all/20220405135758.774016-1-catalin.marinas@arm.com/
> > > 
> > > and ARCH_KMALLOC_MINALIGN is what we have today and is working...
> > > 
> > > But yeah, the name for that define is not the nicest :)
> > > 
> > > I added Jonathan Cameron, on cc, as he had to deal with something
> > > similar for iio in case we are missing something  
> > 
> > I'd like to get feedback on this from DMA and USB experts. Expanding the
> > CC list of the original patch would help (especially including the
> > linux-usb mailing list).  
> 
> Also, do we need the allocation change ? It doesn't seem to simplify the
> code that much, neither in terms of lines of code
> 
> >  2 files changed, 48 insertions(+), 49 deletions(-)  
> 
> nor in terms of complexity. Maybe we could keep the union and offsetof
> changes, and drop the allocation change ? In any case, those are two
> different changes, so I'd split them in two patches at least.
> 
> > > ps: and I thought this was an easy change :P  
> 
+CC Catalin who is driving effort to change what we should do here to avoid
wasting space on systems where ARCH_KMALLOC_MINALIGN is currently 128 bytes.

I don't know the precise requirements for this particular allocation, but
if it's about ensuring the data doesn't share a cacheline with anything else in
the structure then the problem is that ____cacheline_aligned is the
size of a line in the L1 cache.  It's not uncommon for microarchitectures to have
a larger cacheline size for L3 and above.  Most of the time that doesn't
matter as they maintain correct coherence (all the ARM servers are fine
I think - ours has 128 byte cachelines in L3, Fujitsu have parts with
256 byte cachelines in L3), but guess what, there are Qualcomm(?) parts where the
L1 cacheline is 64 bytes, but the l3 cacheline is 128 bytes and don't
deal with the hardware coherence issues. For those we need to ensure that
a DMA safe buffer is in it's own 128 byte cacheline, but ___cacheline_aligned
on arm64 only does 64 bytes.  Currently ARCH_KMALLOC_MINALIGN enforces the
larger guarantee and is available on all architectures unlike
ARCH_DMA_MINALIGN which is not yet.

Catalin is working to replace this, so the required guarantees may change,
but we still need something backportable.

When I sent a bunch of fixes for Input Dmitry asked for a general
___dma_minalign (naming to be bikeshedded) define.  So far there are a few
subsystems carrying their own local equivalent (IIO moved to
IIO_DMA_MINALIGN define) in the interests of reducing the pain of
changing this in future. A central definition is another option.

Jonathan

 

Re: [PATCH v2] media: uvcvideo: Do not alloc dev->status

[Ricardo Ribalda]

Hi Jonathan

On Thu, 15 Dec 2022 at 12:45, Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Thu, 15 Dec 2022 11:11:40 +0200
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
>
> > Hi Ricardo,
> >
> > On Thu, Dec 15, 2022 at 11:08:05AM +0200, Laurent Pinchart wrote:
> > > On Thu, Dec 15, 2022 at 08:59:14AM +0100, Ricardo Ribalda wrote:
> > > > On Thu, 15 Dec 2022 at 02:15, Sergey Senozhatsky wrote:
> > > > >
> > > > > On (22/12/14 14:37), Ricardo Ribalda wrote:
> > > > > [..]
> > > > > > +struct uvc_status_streaming {
> > > > > > +     u8      button;
> > > > > > +} __packed;
> > > > > > +
> > > > > > +struct uvc_status_control {
> > > > > > +     u8      bSelector;
> > > > > > +     u8      bAttribute;
> > > > > > +     u8      bValue[11];
> > > > > > +} __packed;
> > > > > > +
> > > > > > +struct uvc_status {
> > > > > > +     u8      bStatusType;
> > > > > > +     u8      bOriginator;
> > > > > > +     u8      bEvent;
> > > > > > +     union {
> > > > > > +             struct uvc_status_control control;
> > > > > > +             struct uvc_status_streaming streaming;
> > > > > > +     };
> > > > > > +} __packed;
> > > > > > +
> > > > > >  struct uvc_device {
> > > > > >       struct usb_device *udev;
> > > > > >       struct usb_interface *intf;
> > > > > > @@ -559,7 +579,7 @@ struct uvc_device {
> > > > > >       /* Status Interrupt Endpoint */
> > > > > >       struct usb_host_endpoint *int_ep;
> > > > > >       struct urb *int_urb;
> > > > > > -     u8 *status;
> > > > > > +
> > > > > >       struct input_dev *input;
> > > > > >       char input_phys[64];
> > > > > >
> > > > > > @@ -572,6 +592,12 @@ struct uvc_device {
> > > > > >       } async_ctrl;
> > > > > >
> > > > > >       struct uvc_entity *gpio_unit;
> > > > > > +
> > > > > > +     /*
> > > > > > +      * Ensure that status is aligned, making it safe to use with
> > > > > > +      * non-coherent DMA.
> > > > > > +      */
> > > > > > +     struct uvc_status status __aligned(ARCH_KMALLOC_MINALIGN);
> > > > >
> > > > >         ____cacheline_aligned ?
> > > > >
> > > > > I don't see anyone using ARCH_KMALLOC_MINALIGN except for slab.h
> > > >
> > > > Seems like cacheline is not good enough:
> > > >
> > > > https://github.com/torvalds/linux/commit/12c4efe3509b8018e76ea3ebda8227cb53bf5887
> > > > https://lore.kernel.org/all/20220405135758.774016-1-catalin.marinas@arm.com/
> > > >
> > > > and ARCH_KMALLOC_MINALIGN is what we have today and is working...
> > > >
> > > > But yeah, the name for that define is not the nicest :)
> > > >
> > > > I added Jonathan Cameron, on cc, as he had to deal with something
> > > > similar for iio in case we are missing something
> > >
> > > I'd like to get feedback on this from DMA and USB experts. Expanding the
> > > CC list of the original patch would help (especially including the
> > > linux-usb mailing list).
> >
> > Also, do we need the allocation change ? It doesn't seem to simplify the
> > code that much, neither in terms of lines of code
> >
> > >  2 files changed, 48 insertions(+), 49 deletions(-)
> >
> > nor in terms of complexity. Maybe we could keep the union and offsetof
> > changes, and drop the allocation change ? In any case, those are two
> > different changes, so I'd split them in two patches at least.
> >
> > > > ps: and I thought this was an easy change :P
> >
> +CC Catalin who is driving effort to change what we should do here to avoid
> wasting space on systems where ARCH_KMALLOC_MINALIGN is currently 128 bytes.
>
> I don't know the precise requirements for this particular allocation, but
> if it's about ensuring the data doesn't share a cacheline with anything else in
> the structure then the problem is that ____cacheline_aligned is the
> size of a line in the L1 cache.  It's not uncommon for microarchitectures to have
> a larger cacheline size for L3 and above.  Most of the time that doesn't
> matter as they maintain correct coherence (all the ARM servers are fine
> I think - ours has 128 byte cachelines in L3, Fujitsu have parts with
> 256 byte cachelines in L3), but guess what, there are Qualcomm(?) parts where the
> L1 cacheline is 64 bytes, but the l3 cacheline is 128 bytes and don't
> deal with the hardware coherence issues. For those we need to ensure that
> a DMA safe buffer is in it's own 128 byte cacheline, but ___cacheline_aligned
> on arm64 only does 64 bytes.  Currently ARCH_KMALLOC_MINALIGN enforces the
> larger guarantee and is available on all architectures unlike
> ARCH_DMA_MINALIGN which is not yet.
>
> Catalin is working to replace this, so the required guarantees may change,
> but we still need something backportable.
>
> When I sent a bunch of fixes for Input Dmitry asked for a general
> ___dma_minalign (naming to be bikeshedded) define.  So far there are a few
> subsystems carrying their own local equivalent (IIO moved to
> IIO_DMA_MINALIGN define) in the interests of reducing the pain of
> changing this in future. A central definition is another option.
>

Thanks a lot for the explanation!

> Jonathan
>
>


-- 
Ricardo Ribalda