| Message ID | 1693806261-12958-1-git-send-email-wentong.wu@intel.com |
|---|---|
| Headers | show |
| Series | Add Intel LJCA device driver | expand |
Hi Oliver, Thanks for your review > From: Oliver Neukum <oneukum@suse.com> > On 04.09.23 15:52, Wu, Wentong wrote: > >> From: Oliver Neukum <oneukum@suse.com> > > >> And we execute this. rx_urb is killed and does nothing. > >> I see nothing that terminates the waiting if you hit the wrong window. > > > > I think the auxiliary_device_delete will trigger the remove function > > of ljca client in his own sub system, and the delete() or remove() of > > every subsystem will not return until the ongoing transfer(probably > > blocked by above > > cmd_completion) complete. And that also makes sure no more transfers > > comes to there. > > Sure, you will not free used memory. But what allows you to be sure that you make any progress at all? > > That is that you will hang arbitrarily long in disconnect? This routine isn't called in an interrupt context, and it allows sleep or wait something before the real shutdown like many drivers' remove() or disconnect() do. If we want to speed up the disconnect(), below changes is to complete the cmd_completion if usb_kill_urb() has been called, but there is still possibility ljca client init one more transfer before auxiliary_device_delete() @@ -206,7 +206,11 @@ static void ljca_recv(struct urb *urb) if (urb->status) { /* sync/async unlink faults aren't errors */ - if (urb->status == -ENOENT || urb->status == -ECONNRESET || + if (urb->status == -ENOENT) { + complete(&adap->cmd_completion); + return; + } + if (urb->status == -ECONNRESET || urb->status == -ESHUTDOWN) return; perhaps we could add one more 'status' field in ljca_adapter to represent disconnect() happen or not, and it will be checked in the beginning of ljca_send() to avoid any new transfer. BR, Wentong > > Regards > Oliver
On Tue, Sep 05, 2023 at 08:53:43AM +0000, Wu, Wentong wrote: > > From: Oliver Neukum <oneukum@suse.com> > > > > On 05.09.23 04:20, Wu, Wentong wrote: > > > > Hi, > > > > >> That is that you will hang arbitrarily long in disconnect? > > > This routine isn't called in an interrupt context, and it allows sleep > > > or wait something before the real shutdown like many drivers' remove() > > > or > > > disconnect() do. > > > > It is, however, in the context of a kernel thread. We can wait, but not for > > arbitrary periods. > > AFAIK, this is very common. > > > > > > If we want to speed up the disconnect(), below changes is to complete > > > the cmd_completion if usb_kill_urb() has been called, but there is > > > still possibility ljca client init one more transfer before > > > auxiliary_device_delete() > > > > > > @@ -206,7 +206,11 @@ static void ljca_recv(struct urb *urb) > > > > > > if (urb->status) { > > > /* sync/async unlink faults aren't errors */ > > > - if (urb->status == -ENOENT || urb->status == -ECONNRESET || > > > + if (urb->status == -ENOENT) { > > > + complete(&adap->cmd_completion); > > > + return; > > > > I'd say you'd break suspend() by such a change. > > You cannot complete in the interrupt handler, unless you can determine why the > > URB is killed. > > With below status field in ljca_adapter to determine if it's killed by disconnect(). > > If this is preferred, I could cook the patch for review. > > If this is fixed, could you please help merge this usb-ljca driver so that it won't > block others which depends on this driver? Please relax, we can't do anything until after -rc1 is out, and for me, that includes reviewing the code. There is no rush, or deadline, here at all. It will be merged when it is acceptable. thanks, greg k-h
> From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org> > On Tue, Sep 05, 2023 at 08:53:43AM +0000, Wu, Wentong wrote: > > > From: Oliver Neukum <oneukum@suse.com> > > > > > > On 05.09.23 04:20, Wu, Wentong wrote: > > > > > > Hi, > > > > > > >> That is that you will hang arbitrarily long in disconnect? > > > > This routine isn't called in an interrupt context, and it allows > > > > sleep or wait something before the real shutdown like many > > > > drivers' remove() or > > > > disconnect() do. > > > > > > It is, however, in the context of a kernel thread. We can wait, but > > > not for arbitrary periods. > > > > AFAIK, this is very common. > > > > > > > > > If we want to speed up the disconnect(), below changes is to > > > > complete the cmd_completion if usb_kill_urb() has been called, but > > > > there is still possibility ljca client init one more transfer > > > > before > > > > auxiliary_device_delete() > > > > > > > > @@ -206,7 +206,11 @@ static void ljca_recv(struct urb *urb) > > > > > > > > if (urb->status) { > > > > /* sync/async unlink faults aren't errors */ > > > > - if (urb->status == -ENOENT || urb->status == -ECONNRESET || > > > > + if (urb->status == -ENOENT) { > > > > + complete(&adap->cmd_completion); > > > > + return; > > > > > > I'd say you'd break suspend() by such a change. > > > You cannot complete in the interrupt handler, unless you can > > > determine why the URB is killed. > > > > With below status field in ljca_adapter to determine if it's killed by disconnect(). > > > > If this is preferred, I could cook the patch for review. > > > > If this is fixed, could you please help merge this usb-ljca driver so > > that it won't block others which depends on this driver? > > Please relax, we can't do anything until after -rc1 is out, and for me, that > includes reviewing the code. Thanks for your review. > > There is no rush, or deadline, here at all. It will be merged when it is acceptable. Understood, thanks BR, Wentong > > thanks, > > greg k-h