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@xxxxxxxxxxxx> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx> --- media: uvcvideo: Code cleanup for dev->status Lets remove all the castings and open coding of offsets for it. To: Yunke Cao <yunkec@xxxxxxxxxxxx> To: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx> To: Max Staudt <mstaudt@xxxxxxxxxxxx> To: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> To: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> Cc: linux-media@xxxxxxxxxxxxxxx Cc: linux-kernel@xxxxxxxxxxxxxxx --- Changes in v4: - Fix sizeof() error. - Keep kzalloc(). Thanks Alan, Christoph and Jonathan - Reducing the cc: to: - Link to v3: https://lore.kernel.org/r/20221214-uvc-status-alloc-v3-0-9a67616cc549@xxxxxxxxxxxx Changes in v3: - Split the patch in two - Add linux-usb, Alan and Christoph for the allocation change. - Link to v2: https://lore.kernel.org/r/20221214-uvc-status-alloc-v2-0-3f1cba6fc734@xxxxxxxxxxxx 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@xxxxxxxxxxxx --- drivers/media/usb/uvc/uvc_status.c | 66 +++++++++++++------------------------- drivers/media/usb/uvc/uvcvideo.h | 25 +++++++++++++-- 2 files changed, 45 insertions(+), 46 deletions(-) diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c index 7518ffce22ed..00f4036d0683 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); - if (dev->status == NULL) + dev->status = kzalloc(sizeof(*dev->status), GFP_KERNEL); + if (!dev->status) 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]; --- base-commit: 0ec5a38bf8499f403f81cb81a0e3a60887d1993c change-id: 20221214-uvc-status-alloc-93becb783898 Best regards, -- Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
