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:
signature.asc
Description: PGP signature
