diff mbox series

[v2,2/2] media: uvcvideo: Do not alloc dev->status

Message ID 20221212-uvc-race-v2-2-54496cc3b8ab@chromium.org
State New
Headers
Series uvc: Fix race condition on uvc |

Commit Message

Ricardo Ribalda Dec. 13, 2022, 2:35 p.m. UTC 
  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(-)

Comments

Sergey Senozhatsky Dec. 14, 2022, 12:39 a.m. UTC | #1 
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().
Ricardo Ribalda Dec. 14, 2022, 5:57 a.m. UTC | #2 
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().
Sergey Senozhatsky Dec. 14, 2022, 6:02 a.m. UTC | #3 
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 mbox series

Patch

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];