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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Dec 14, 2022 at 02:45:31PM +0200, Laurent Pinchart wrote:
> Hi Ricardo,
> 
> Thank you for the patch.
> 
> On Wed, Dec 14, 2022 at 12:23:41PM +0100, Ricardo Ribalda wrote:
> > UVC_MAX_STATUS_SIZE is 16, simplify the code by inlining dev->status.
> 
> The reason why uvc_status is allocated dynamically is to ensure cache
> line alignment, as the USB host controller may DMA to the buffer.

See

commit a31a4055473bf0a7b2b06cb2262347200d0711e1
Author: Ming Lei <tom.leiming@xxxxxxxxx>
Date:   Tue Sep 16 03:32:20 2008 -0300

    V4L/DVB:usbvideo:don't use part of buffer for USB transfer #4

    The status[] is part of  uvc_device structure. We can't make sure
    the address of status is at a cache-line boundary in all archs,so
    status[] might share a cache-line with some fields in uvc_structure.
    This can lead to some cache coherence issues(http://lwn.net/Articles/2265/).
    Use dynamically allocated buffer instead.

    Signed-off-by: Ming Lei <tom.leiming@xxxxxxxxx>
    Acked-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxx>
    Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>

> > Now that we are at it, remove all the castings.
> > 
> > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > ---
> > To: Yunke Cao <yunkec@xxxxxxxxxxxx>
> > To: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
> > To: Max Staudt <mstaudt@xxxxxxxxxx>
> > To: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > To: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> > Cc: linux-media@xxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > ---
> >  drivers/media/usb/uvc/uvc_status.c | 69 ++++++++++++--------------------------
> >  drivers/media/usb/uvc/uvcvideo.h   | 22 +++++++++++-
> >  2 files changed, 42 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..cdd2e328acc2 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 uvc_status status;
> >  	struct input_dev *input;
> >  	char input_phys[64];
> >  
> > 
> > ---
> > base-commit: 0ec5a38bf8499f403f81cb81a0e3a60887d1993c
> > change-id: 20221214-uvc-status-alloc-93becb783898

-- 
Regards,

Laurent Pinchart



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux