Re: "usbip: Implement SG support to vhci-hcd and stub driver" causes a deadlock

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 2595 bytes --]

On Wed, Dec 11, 2019 at 03:27:05PM +0900, Suwan Kim wrote:
> On Wed, Dec 11, 2019 at 04:20:32AM +0100, Marek Marczykowski-Górecki wrote:
> > On Wed, Dec 11, 2019 at 12:07:54PM +0900, Suwan Kim wrote:
> > > HCD should giveback URB to driver calling usb_hcd_giveback_urb().
> > > But in the case of transaction error, vhci_recv_ret_submit()
> > > terminates without giveback URB. So, I think the error path in
> > > usbip_recv_xbuff and usbip_recv_iso should unlink URB from ep and
> > > insert proper error code in urb->status that indicates error
> > > status to driver and handle giveback URB to driver.
> > > 
> > > Then hub_irq doesn't need to flush error URB.
> > > That will be helpful to graceful connection shutdown.
> > > 
> > > 
> > > static void vhci_recv_ret_submit(struct vhci_device *vdev,
> > > 				 struct usbip_header *pdu)
> > > ...
> > > ...
> > > 	if (usbip_recv_xbuff(ud, urb) < 0) {
> > > 		urb->status = -EPIPE;
> > > 		goto error;	// goto error path
> > > 	}
> > > 
> > > 	/* recv iso_packet_descriptor */
> > > 	if (usbip_recv_iso(ud, urb) < 0) {
> > > 		urb->status = -EPIPE;
> > > 		goto error;	// goto error path
> > > 	}
> > > ...
> > > ...
> > > error://error path
> > > 	spin_lock_irqsave(&vhci->lock, flags);
> > > 	usb_hcd_unlink_urb_from_ep(vhci_hcd_to_hcd(vhci_hcd), urb);
> > > 	spin_unlock_irqrestore(&vhci->lock, flags);
> > > 
> > > 	usb_hcd_giveback_urb(vhci_hcd_to_hcd(vhci_hcd), urb, urb->status);
> > > 
> > > 	usbip_dbg_vhci_rx("Leave\n");
> > > }
> > 
> > Yup, that works! Now the error is handled gracefully instead of hanging.
> 
> Marek, Could you test if it works too?

Yes, this seems to work too.


> Regards,
> Suwan Kim
> 
> 
> diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c
> index 33f8972ba842..19a807e398eb 100644
> --- a/drivers/usb/usbip/vhci_rx.c
> +++ b/drivers/usb/usbip/vhci_rx.c
> @@ -78,11 +78,11 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
> 
>         /* recv transfer buffer */
>         if (usbip_recv_xbuff(ud, urb) < 0)
> -               return;
> +               urb->status = -EPIPE;
> 
>         /* recv iso_packet_descriptor */
>         if (usbip_recv_iso(ud, urb) < 0)
> -               return;
> +               urb->status = -EPIPE;
> 
>         /* restore the padding in iso packets */
>         usbip_pad_iso(ud, urb);

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]