| Message ID | 20221212-uvc-race-v2-2-54496cc3b8ab@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | uvc: Fix race condition on uvc | expand |
On (22/12/13 15:35), Ricardo Ribalda wrote: [..] > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -559,7 +559,7 @@ struct uvc_device { > /* Status Interrupt Endpoint */ > struct usb_host_endpoint *int_ep; > struct urb *int_urb; > - u8 *status; > + u8 status[UVC_MAX_STATUS_SIZE]; Can we use `struct uvc_control_status status;` instead of open-coding it? Seems that this is what the code wants anyway: struct uvc_control_status *status = (struct uvc_control_status *)dev->status; And then we can drop casts in uvc_status_complete().
Hi Sergey Thanks for the review On Wed, 14 Dec 2022 at 01:40, Sergey Senozhatsky <senozhatsky@chromium.org> wrote: > > On (22/12/13 15:35), Ricardo Ribalda wrote: > [..] > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > @@ -559,7 +559,7 @@ struct uvc_device { > > /* Status Interrupt Endpoint */ > > struct usb_host_endpoint *int_ep; > > struct urb *int_urb; > > - u8 *status; > > + u8 status[UVC_MAX_STATUS_SIZE]; > > Can we use `struct uvc_control_status status;` instead of open-coding it? > Seems that this is what the code wants anyway: It can also be a `struct uvc_streaming_status` https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/uvc/uvc_status.c#n230 so we always need the casting :( > > struct uvc_control_status *status = > (struct uvc_control_status *)dev->status; > > And then we can drop casts in uvc_status_complete().
On (22/12/14 06:57), Ricardo Ribalda wrote: > > On (22/12/13 15:35), Ricardo Ribalda wrote: > > [..] > > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > > @@ -559,7 +559,7 @@ struct uvc_device { > > > /* Status Interrupt Endpoint */ > > > struct usb_host_endpoint *int_ep; > > > struct urb *int_urb; > > > - u8 *status; > > > + u8 status[UVC_MAX_STATUS_SIZE]; > > > > Can we use `struct uvc_control_status status;` instead of open-coding it? > > Seems that this is what the code wants anyway: > > It can also be a `struct uvc_streaming_status` > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/uvc/uvc_status.c#n230 > > so we always need the casting :( Then perhaps we can put both of them into anon union in struct uvc_device as stream_status and control_status?
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c index 09a5802dc974..52999b3b7c48 100644 --- a/drivers/media/usb/uvc/uvc_status.c +++ b/drivers/media/usb/uvc/uvc_status.c @@ -259,15 +259,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); @@ -296,7 +290,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 6a9b72d6789e..ccc7e3b60bf1 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -559,7 +559,7 @@ struct uvc_device { /* Status Interrupt Endpoint */ struct usb_host_endpoint *int_ep; struct urb *int_urb; - u8 *status; + u8 status[UVC_MAX_STATUS_SIZE]; bool flush_status; struct input_dev *input; char input_phys[64];
UVC_MAX_STATUS_SIZE is 16, simplify the code by inlining dev->status. Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_status.c | 9 +-------- drivers/media/usb/uvc/uvcvideo.h | 2 +- 2 files changed, 2 insertions(+), 9 deletions(-)