diff mbox series

[v3,1/2] media: uvcvideo: Remove void casting for the status endpoint

Message ID 20221214-uvc-status-alloc-v3-1-9a67616cc549@chromium.org (mailing list archive)
State New, archived
Headers show
Series media: uvcvideo: Code cleanup for dev->status | expand

Commit Message

Ricardo Ribalda Dec. 15, 2022, 10:57 a.m. UTC 
Make the code more resiliant, by replacing the castings with proper
structure definitions and using offsetof() instead of open coding the
location of the data.

Suggested-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_status.c | 64 +++++++++++++-------------------------
 drivers/media/usb/uvc/uvcvideo.h   | 25 +++++++++++++--
 2 files changed, 44 insertions(+), 45 deletions(-)

Comments

Ricardo Ribalda Dec. 15, 2022, 11:07 a.m. UTC | #1 
On Thu, 15 Dec 2022 at 11:57, Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Make the code more resiliant, by replacing the castings with proper
> structure definitions and using offsetof() instead of open coding the
> location of the data.
>
> Suggested-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_status.c | 64 +++++++++++++-------------------------
>  drivers/media/usb/uvc/uvcvideo.h   | 25 +++++++++++++--
>  2 files changed, 44 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
> index 7518ffce22ed..dbaa9b07d77f 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,12 +239,12 @@ int uvc_status_init(struct uvc_device *dev)
>
>         uvc_input_init(dev);
>
> -       dev->status = kzalloc(UVC_MAX_STATUS_SIZE, GFP_KERNEL);
> +       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 == NULL) {
> +       if (!dev->int_urb) {
>                 kfree(dev->status);
>                 return -ENOMEM;
>         }
> @@ -281,7 +261,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,
this is obviously sizeof(*dev->status)

Sorry about that. Will resend, with other comments (if any)
>                 dev, interval);
>
>         return 0;
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index df93db259312..84326991ec36 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -51,8 +51,6 @@
>  #define UVC_URBS               5
>  /* Maximum number of packets per URB. */
>  #define UVC_MAX_PACKETS                32
> -/* Maximum status buffer size in bytes of interrupt URB. */
> -#define UVC_MAX_STATUS_SIZE    16
>
>  #define UVC_CTRL_CONTROL_TIMEOUT       5000
>  #define UVC_CTRL_STREAMING_TIMEOUT     5000
> @@ -527,6 +525,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 +577,8 @@ 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];
>
>
> --
> 2.39.0.314.g84b9a713c41-goog-b4-0.11.0-dev-696ae
diff mbox series

Patch

diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index 7518ffce22ed..dbaa9b07d77f 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,12 +239,12 @@  int uvc_status_init(struct uvc_device *dev)
 
 	uvc_input_init(dev);
 
-	dev->status = kzalloc(UVC_MAX_STATUS_SIZE, GFP_KERNEL);
+	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 == NULL) {
+	if (!dev->int_urb) {
 		kfree(dev->status);
 		return -ENOMEM;
 	}
@@ -281,7 +261,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;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index df93db259312..84326991ec36 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -51,8 +51,6 @@ 
 #define UVC_URBS		5
 /* Maximum number of packets per URB. */
 #define UVC_MAX_PACKETS		32
-/* Maximum status buffer size in bytes of interrupt URB. */
-#define UVC_MAX_STATUS_SIZE	16
 
 #define UVC_CTRL_CONTROL_TIMEOUT	5000
 #define UVC_CTRL_STREAMING_TIMEOUT	5000
@@ -527,6 +525,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 +577,8 @@  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];