Re: [PATCH -next] xhci: fix two places when dealing with return value of function xhci_check_args

[Mathias Nyman <mathias.nyman@linux.intel.com>]

Hi

On 28.1.2022 5.48, 谢泓宇 wrote:
> Hi Mathias,
> 
>> 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()?
> 

Yes. That is what it used to return. 
This is more about code maintaining practice than this specific patch.

Only make the necessary change to fix a bug, especially if the patch is going
to stable kernels. 
The change to return success ("0") instead of -EINVAL in xhci_urb_enqueue() for 
roothub URBs is irrelevant in fixing your issue.

Debugging future issues is a lot harder when there are small undocumented
unrelated functional changes scattered around bugfixing patches.

Other reason is that even if you can be almost certain xhci_urb_enqueue() won't
be called for roothub urbs for this kernel version, it's possible some old stable
kernel code looks very different, and this change can break that stable version.

Seemingly odd checks in code can indicate the old original code was flawed, and
quickly worked around by adding the odd check.
That kernel version might still depend on this odd check even if newer versions
are fixed properly.

>>
>> 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?

- For all reasons stated above
- Because it used to, and changing it doesn't fix anything
- Because urbs sent to xhci_urb_enqueue() should have a valid urb->dev->parent,
  if they don't have it then they are INVALID

> 
>     2) Should I change all the return statements about xhci_check_args() in drivers/usb/host/xhci.c?
> 
>     There are 6 of them.

Only make sure your patch doesn't change the functionality unnecessarily.
There are two places where we return -EINVAL if xhci_check_args() returns 0:
xhci_urb_enqueue() and xhci_check_streams_endpoint()
Keep that functionality.

Thanks
Mathias