Hi Mathias,
On 2022/1/27 17:43, Mathias Nyman wrote:
On 26.1.2022 14.49, Hongyu Xie wrote:
Anyway, why is this unique to this one driver? Why does it not show up
for any other driver?
The whole thing is not about a particular driver. The thing is xhci_urb_enqueue shouldn't change the return value of xhci_check_args from -ENODEV to -EINVAL. Many other drivers only check if the return value of xchi_check_args is <= 0.
Agree, lets return -ENODEV when appropriate.
The whole point is, if xhci_check_args returns value A, xhci_urb_enqueque
shouldn't return any
other value, because that will change some driver's behavior(like r8152.c).
But you are changing how the code currently works. Are you sure you
want to have this "succeed" if this is on a root hub?
Yes, I'm changing how the code currently works but not on a root hub.
2."So if 0 is returned, you will now return that here, is that ok?
That is a change in functionality.
But this can only ever be the case for a root hub, is that ok?"
It's the same logic, but now xhci_urb_enqueue can return -ENODEV if xHC is
halted.
If it happens on a root hub, xhci_urb_enqueue won't be called.
3."Again, this means all is good? Why is this being called for a root hub?"
It is the same logic with the old one, but now xhci_check_streams_endpoint
can return -ENODEV if xHC is halted.
This still feels wrong to me, but I'll let the maintainer decide, as I
don't understand why a root hub is special here.
Thanks please. usb_submit_urb will call usb_hcd_submit_urb. And usb_hcd_submit_urb will call rh_urb_enqueue if it's on a root hub instead of calling hcd->driver->urb_enqueue(which is xhci_urb_enqueue in this case).
xhci_urb_enqueue() shouldn't be called for roothub urbs, but if it is then we
should continue to return -EINVAL
xhci_urb_enqueue() won't be called for roothub urbs, only for none
roothub urbs(see usb_hcd_submit_urb()).
So xhci_urb_enqueue() will not get 0 from xhci_check_args().
Still return -EINVAL if xhci_check_args() returns 0 in xhci_urb_enqueue()?
xhci_check_args() should be rewritten later, but first we want a targeted fix
that can go to stable.
Your original patch would be ok after following modification:
if (ret <= 0)
return ret ? ret : -EINVAL;
I have two questions:
1) Why return -EINVAL for roothub urbs?
2) Should I change all the return statements about
xhci_check_args() in drivers/usb/host/xhci.c?
There are 6 of them.
Thanks
-Mathias
