linux-kernel - Re: [PATCH v1 1/1] usb: ulpi: defer ulpi_register on ulpi_read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <48e7b906-d7e3-46e3-e2bc-71512a1e64aa@gmail.com>
Date:   Thu, 10 Nov 2022 21:38:39 +0100
From:   Ferry Toth <fntoth@...il.com>
To:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Cc:     "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Sean Anderson <sean.anderson@...o.com>,
        Liu Shixin <liushixin2@...wei.com>,
        Andrey Smirnov <andrew.smirnov@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v1 1/1] usb: ulpi: defer ulpi_register on ulpi_read_id
 timeout

Hi

Op 10-11-2022 om 13:45 schreef Ferry Toth:
> (sorry sent html with previous attempt)
> 
> On 10-11-2022 01:06, Thinh Nguyen wrote:
>> Hi Ferry,
>>
>> On Wed, Nov 09, 2022, Ferry Toth wrote:
>>> Since commit 0f010171
>>> Dual Role support on Intel Merrifield platform broke due to rearranging
>>> the call to dwc3_get_extcon().
>>>
>>> It appears to be caused by ulpi_read_id() on the first test write 
>>> failing
>>> with -ETIMEDOUT. Currently ulpi_read_id() expects to discover the phy 
>>> via
>>> DT when the test write fails and returns 0 in that case even if DT 
>>> does not
>>> provide the phy. Due to the timeout being masked dwc3 probe continues by
>>> calling dwc3_core_soft_reset() followed by dwc3_get_extcon() which 
>>> happens
>>> to return -EPROBE_DEFER. On deferred probe ulpi_read_id() finally 
>>> succeeds.
>>>
>>> This patch changes ulpi_read_id() to return -ETIMEDOUT when it occurs 
>>> and
>>> catches the error in dwc3_core_init(). It handles the error by calling
>>> dwc3_core_soft_reset() after which it requests -EPROBE_DEFER. On 
>>> deferred
>>> probe ulpi_read_id() again succeeds.
>>>
>>> Signed-off-by: Ferry Toth<ftoth@...londelft.nl>
>>> ---
>>>   drivers/usb/common/ulpi.c | 5 +++--
>>>   drivers/usb/dwc3/core.c   | 5 ++++-
>>>   2 files changed, 7 insertions(+), 3 deletions(-)
>>>
>> Can you split the dwc3 change and ulpi change to separate patches?
> 
> Thanks for your comments.
> 
> I will send v2
> 
>>> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
>>> index d7c8461976ce..d8f22bc2f9d0 100644
>>> --- a/drivers/usb/common/ulpi.c
>>> +++ b/drivers/usb/common/ulpi.c
>>> @@ -206,8 +206,9 @@ static int ulpi_read_id(struct ulpi *ulpi)
>>>       /* Test the interface */
>>>       ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa);
>>> -    if (ret < 0)
>>> -        goto err;
>>> +    if (ret < 0) {
>>> +        return ret;
>>> +    }
>>>       ret = ulpi_read(ulpi, ULPI_SCRATCH);
>>>       if (ret < 0)
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 648f1c570021..e293ef70039b 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -1106,8 +1106,11 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>>       if (!dwc->ulpi_ready) {
>>>           ret = dwc3_core_ulpi_init(dwc);
>>> -        if (ret)
>>> +        if (ret) {
>>> +            dwc3_core_soft_reset(dwc);
>> We shouldn't need to do soft reset here. The controller shouldn't be at
>> a bad/incorrect state at this point to warrant a soft-reset. There will
>> be a soft-reset when it goes through the initialization again.
> 
> It doesn't go through the initialization again unless we set 
> -EPROBE_DEFER. And when we make ulpi_read_id() return -EPROBE_DEFER it 
> will goto err0 here, so skips dwc3_core_soft_reset.
> 
> Do you mean you prefer something like:
> 
> if (ret) {
> 
>      if (ret == -ETIMEDOUT) ret = -EPROBE_DEFER;
> 
>      else goto err0;
> 
> }

I just tested, and calling dwc3_core_soft_reset() proves to be necessary 
as we need to goto err0 directly after. Else ret is overwritten and 
-EPROBE_DEFER lost.

>>> +            ret = -EPROBE_DEFER;
>> We shouldn't automatically set every error status to correspond to
>> -EPROBE_DEFER. Check only the approapriate error codes (-ETIMEDOUT +
>> any other?).
> Other could be -ENOMEM. I think no need to do any new handling for that.
>>>               goto err0;
>>> +        }
>>>           dwc->ulpi_ready = true;
>>>       }
>>> -- 
>>> 2.34.1
>>>
>> Thanks,
>> Thinh