Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <4f1bd7fb-050b-ddb3-da69-1e6d32a5a7a1@kylinos.cn>
Date:   Wed, 9 Feb 2022 10:47:04 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH -next] xhci: fix two places when dealing with return value
 of function xhci_check_args
Content-Language: en-US
To:     Mathias Nyman <mathias.nyman@linux.intel.com>,
        Greg KH <gregkh@linuxfoundation.org>
Cc:     Hongyu Xie <xy521521@gmail.com>, mathias.nyman@intel.com,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
        125707942@qq.com, stable@vger.kernel.org
References: <20220126094126.923798-1-xy521521@gmail.com>
 <YfEZFtf9K8pFC8Mw@kroah.com>
 <c7f6a8bb-76b6-cd2d-7551-b599a8276f5c@kylinos.cn>
 <YfEnbRW3oU0ouGqH@kroah.com>
 <e86972d3-e4a0-ad81-45ea-21137e3bfcb6@kylinos.cn>
 <7af5b318-b1ac-0c74-1782-04ba50a3b5fa@linux.intel.com>
 <ce40f4cd-a110-80b1-f766-e94dd8cedc7e@kylinos.cn>
 <6da59964-ce0e-c202-8a9c-a753a1908f3e@linux.intel.com>
From:   =?UTF-8?B?6LCi5rOT5a6H?= <xiehongyu1@kylinos.cn>
In-Reply-To: <6da59964-ce0e-c202-8a9c-a753a1908f3e@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Mathias,

On 2022/1/28 17:48, Mathias Nyman wrote:
> 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

I'll fix these in v2.

Thanks

Hongyu Xie