| Message ID | 20230911140530.2995138-2-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3a63f86c6a6cb0601f0563a81574745da2979e3b |
| Headers | show |
| Series | usb: gadget: uvc: restart fixes | expand |
Hi Michael, Thank you for the patch. On Mon, Sep 11, 2023 at 04:05:28PM +0200, Michael Grzeschik wrote: > Since the uvc-video gadget driver is using the v4l2 interface, > the streamon and streamoff can be triggered at any times. To ensure > that the pump worker will be closed as soon the userspace is > calling streamoff we synchronize the state of the gadget ensuring > the pump worker to bail out. I'm sorry but I really dislike this. Not only does the patch fail to ensure real synchronization, as the uvcg_video_pump() function still runs asynchronously, it messes up the usage of the state field that now tracks the state both from a host point of view (which it was doing so far, updating the state based on callbacks from the UDC), and from a gadget userspace point of view. This lacks clarity and is confusing. Furthermore, the commit message doesn't even explain what issue is being fixed here. Greg, I think this series has been merged too soon :-( > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > --- > v1 -> v2: Fixed the missing uvc variable in uvcg_video_enable > > drivers/usb/gadget/function/uvc_video.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c > index 91af3b1ef0d412..4b68a3a9815d73 100644 > --- a/drivers/usb/gadget/function/uvc_video.c > +++ b/drivers/usb/gadget/function/uvc_video.c > @@ -384,13 +384,14 @@ static void uvcg_video_pump(struct work_struct *work) > struct uvc_video_queue *queue = &video->queue; > /* video->max_payload_size is only set when using bulk transfer */ > bool is_bulk = video->max_payload_size; > + struct uvc_device *uvc = video->uvc; > struct usb_request *req = NULL; > struct uvc_buffer *buf; > unsigned long flags; > bool buf_done; > int ret; > > - while (video->ep->enabled) { > + while (video->ep->enabled && uvc->state == UVC_STATE_STREAMING) { > /* > * Retrieve the first available USB request, protected by the > * request lock. > @@ -488,6 +489,7 @@ static void uvcg_video_pump(struct work_struct *work) > */ > int uvcg_video_enable(struct uvc_video *video, int enable) > { > + struct uvc_device *uvc = video->uvc; > unsigned int i; > int ret; > > @@ -498,6 +500,8 @@ int uvcg_video_enable(struct uvc_video *video, int enable) > } > > if (!enable) { > + uvc->state = UVC_STATE_CONNECTED; > + > cancel_work_sync(&video->pump); > uvcg_queue_cancel(&video->queue, 0); > > @@ -523,6 +527,8 @@ int uvcg_video_enable(struct uvc_video *video, int enable) > video->encode = video->queue.use_sg ? > uvc_video_encode_isoc_sg : uvc_video_encode_isoc; > > + uvc->state = UVC_STATE_STREAMING; > + You're now setting the state to UVC_STATE_STREAMING both here and in uvc_v4l2_streamon(). That's confusing. > video->req_int_count = 0; > > queue_work(video->async_wq, &video->pump);
On Thu, Oct 05, 2023 at 11:17:16AM +0300, Laurent Pinchart wrote: > Hi Michael, > > Thank you for the patch. > > On Mon, Sep 11, 2023 at 04:05:28PM +0200, Michael Grzeschik wrote: > > Since the uvc-video gadget driver is using the v4l2 interface, > > the streamon and streamoff can be triggered at any times. To ensure > > that the pump worker will be closed as soon the userspace is > > calling streamoff we synchronize the state of the gadget ensuring > > the pump worker to bail out. > > I'm sorry but I really dislike this. Not only does the patch fail to > ensure real synchronization, as the uvcg_video_pump() function still > runs asynchronously, it messes up the usage of the state field that now > tracks the state both from a host point of view (which it was doing so > far, updating the state based on callbacks from the UDC), and from a > gadget userspace point of view. This lacks clarity and is confusing. > Furthermore, the commit message doesn't even explain what issue is being > fixed here. > > Greg, I think this series has been merged too soon :-( Ok, I'll go revert them now, thanks for the review. greg k-h
On Thu, Oct 05, 2023 at 10:40:10AM +0200, Greg KH wrote: > On Thu, Oct 05, 2023 at 11:17:16AM +0300, Laurent Pinchart wrote: > > Hi Michael, > > > > Thank you for the patch. > > > > On Mon, Sep 11, 2023 at 04:05:28PM +0200, Michael Grzeschik wrote: > > > Since the uvc-video gadget driver is using the v4l2 interface, > > > the streamon and streamoff can be triggered at any times. To ensure > > > that the pump worker will be closed as soon the userspace is > > > calling streamoff we synchronize the state of the gadget ensuring > > > the pump worker to bail out. > > > > I'm sorry but I really dislike this. Not only does the patch fail to > > ensure real synchronization, as the uvcg_video_pump() function still > > runs asynchronously, it messes up the usage of the state field that now > > tracks the state both from a host point of view (which it was doing so > > far, updating the state based on callbacks from the UDC), and from a > > gadget userspace point of view. This lacks clarity and is confusing. > > Furthermore, the commit message doesn't even explain what issue is being > > fixed here. > > > > Greg, I think this series has been merged too soon :-( > > Ok, I'll go revert them now, thanks for the review. Or we can wait a day for Michael to reply, in case this can quickly be fixed on top for v6.7. I'm now reading on the loooon discussion from v1, and reviewing the other pending patches that try to tackle the same issue.
On Thu, Oct 05, 2023 at 11:48:05AM +0300, Laurent Pinchart wrote: > On Thu, Oct 05, 2023 at 10:40:10AM +0200, Greg KH wrote: > > On Thu, Oct 05, 2023 at 11:17:16AM +0300, Laurent Pinchart wrote: > > > Hi Michael, > > > > > > Thank you for the patch. > > > > > > On Mon, Sep 11, 2023 at 04:05:28PM +0200, Michael Grzeschik wrote: > > > > Since the uvc-video gadget driver is using the v4l2 interface, > > > > the streamon and streamoff can be triggered at any times. To ensure > > > > that the pump worker will be closed as soon the userspace is > > > > calling streamoff we synchronize the state of the gadget ensuring > > > > the pump worker to bail out. > > > > > > I'm sorry but I really dislike this. Not only does the patch fail to > > > ensure real synchronization, as the uvcg_video_pump() function still > > > runs asynchronously, it messes up the usage of the state field that now > > > tracks the state both from a host point of view (which it was doing so > > > far, updating the state based on callbacks from the UDC), and from a > > > gadget userspace point of view. This lacks clarity and is confusing. > > > Furthermore, the commit message doesn't even explain what issue is being > > > fixed here. > > > > > > Greg, I think this series has been merged too soon :-( > > > > Ok, I'll go revert them now, thanks for the review. > > Or we can wait a day for Michael to reply, in case this can quickly be > fixed on top for v6.7. I'm now reading on the loooon discussion from v1, > and reviewing the other pending patches that try to tackle the same > issue. I'd rather take a patchset that everyone agrees with, reverting was easy and now done. thanks, greg k-h
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c index 91af3b1ef0d412..4b68a3a9815d73 100644 --- a/drivers/usb/gadget/function/uvc_video.c +++ b/drivers/usb/gadget/function/uvc_video.c @@ -384,13 +384,14 @@ static void uvcg_video_pump(struct work_struct *work) struct uvc_video_queue *queue = &video->queue; /* video->max_payload_size is only set when using bulk transfer */ bool is_bulk = video->max_payload_size; + struct uvc_device *uvc = video->uvc; struct usb_request *req = NULL; struct uvc_buffer *buf; unsigned long flags; bool buf_done; int ret; - while (video->ep->enabled) { + while (video->ep->enabled && uvc->state == UVC_STATE_STREAMING) { /* * Retrieve the first available USB request, protected by the * request lock. @@ -488,6 +489,7 @@ static void uvcg_video_pump(struct work_struct *work) */ int uvcg_video_enable(struct uvc_video *video, int enable) { + struct uvc_device *uvc = video->uvc; unsigned int i; int ret; @@ -498,6 +500,8 @@ int uvcg_video_enable(struct uvc_video *video, int enable) } if (!enable) { + uvc->state = UVC_STATE_CONNECTED; + cancel_work_sync(&video->pump); uvcg_queue_cancel(&video->queue, 0); @@ -523,6 +527,8 @@ int uvcg_video_enable(struct uvc_video *video, int enable) video->encode = video->queue.use_sg ? uvc_video_encode_isoc_sg : uvc_video_encode_isoc; + uvc->state = UVC_STATE_STREAMING; + video->req_int_count = 0; queue_work(video->async_wq, &video->pump);
Since the uvc-video gadget driver is using the v4l2 interface, the streamon and streamoff can be triggered at any times. To ensure that the pump worker will be closed as soon the userspace is calling streamoff we synchronize the state of the gadget ensuring the pump worker to bail out. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- v1 -> v2: Fixed the missing uvc variable in uvcg_video_enable drivers/usb/gadget/function/uvc_video.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)