| 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
| ||
|
Date: Sun, 25 Sep 2022 21:21:03 +0200
From: Ferry Toth <fntoth@...il.com>
To: Andrey Smirnov <andrew.smirnov@...il.com>,
Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Felipe Balbi <balbi@...nel.org>,
Thinh Nguyen <thinhn@...opsys.com>, linux-usb@...r.kernel.org,
linux-kernel@...r.kernel.org,
Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
Sven Peter <sven@...npeter.dev>
Subject: Re: [PATCH v4] usb: dwc3: Don't switch OTG -> peripheral if extcon is
present
Hi,
Promising results below.
Op 24-09-2022 om 23:29 schreef Ferry Toth:
> Hi,
>
> One more test
>
> Op 23-09-2022 om 20:23 schreef Andrey Smirnov:
>> On Fri, Sep 23, 2022 at 9:42 AM Andy Shevchenko
>> <andriy.shevchenko@...el.com> wrote:
>>> On Thu, Sep 22, 2022 at 04:32:55PM -0700, Andrey Smirnov wrote:
>>>> On Thu, Sep 22, 2022 at 3:23 AM Ferry Toth <fntoth@...il.com> wrote:
>>>>> On 22-09-2022 12:08, Andy Shevchenko wrote:
>>>>> On Sun, Apr 03, 2022 at 09:49:07AM -0700, Andrey Smirnov wrote:
>>> FYI: For now I sent a revert, but if we got a solution quicker we
>>> always
>>> can choose the course of actions.
>>>
>> I think we have another problem. This patch happened in parallel to mine
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.0-rc6&id=ab7aa2866d295438dc60522f85c5421c6b4f1507
>>
>>
>> so my changes didn't have that fix in mind and I think your revert
>> will not preserve that fix. Can you update your revert to take care of
>> that too, please?
>>
>> I'm really confused how the above commit could be followed up by:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/usb/dwc3/drd.c?h=v6.0-rc6&id=0f01017191384e3962fa31520a9fd9846c3d352f
>>
>>
>> the diffs in dwc3_drd_init seem contradictory
>>
>>>>> If the extcon device exists, get the mode from the extcon device. If
>>>>> the controller is DRD and the driver is unable to determine the mode,
>>>>> only then default the dr_mode to USB_DR_MODE_PERIPHERAL.
>>>>>
>>>>> According to Ferry (Cc'ed) this broke Intel Merrifield platform.
>>>>> Ferry, can you
>>>>> share bisect log?
>>>>>
>>>>> I can but not right now. But what I did was bisect between 5.18.0
>>>>> (good) and 5.19.0 (bad) then when I got near the culprit (~20
>>>>> remaining) based on the commit message I tried
>>>>> 0f01017191384e3962fa31520a9fd9846c3d352f "usb: dwc3: Don't switch
>>>>> OTG -> peripheral if extcon is present" (bad) and commit before
>>>>> that (good).
>>>>>
>>>>> The effect of the patch is that on Merrifield (I tested with Intel
>>>>> Edison Arduino board which has a HW switch to select between host
>>>>> and device mode) device mode works but in host mode USB is
>>>>> completely not working.
>>>>>
>>>>> Currently on host mode - when working - superfluous error messages
>>>>> from tusb1210 appear. When host mode is not working there are no
>>>>> tusb1210 messages in the logs / on the console at all. Seemingly
>>>>> tusb1210 is not probed, which points in the direction of a
>>>>> relation to extcon.
>>>>>
>>>>> Taking into account the late cycle, I would like to revert the
>>>>> change. And
>>>>> Ferry and I would help to test any other (non-regressive) approach).
>>>>>
>>>>> I have not yet tested if a simple revert fixes the problem but
>>>>> will tonight.
>>>>>
>>>>>
>>>>> I would be happy to test other approaches too.
>>>>
>>>> It's a bit hard for me to suggest an alternative approach without
>>>> knowing how things are breaking in this case. I'd love to order one of
>>>> those boards to repro and fix this on my end, but it looks like this
>>>> HW is EOLed and out of stock in most places. If you guys know how to
>>>> get my hands on those boards I'm all ears.
>>> There are still some second hand Intel Edison boards flying around
>>> (but maybe cost a bit more than expected) and there are also
>>> Dell Venue 7 3740 tablets based on the same platform/SoC. The latter
>>> option though requires more actions in order something to be boot
>>> there.
>>>
>> OK, I'll check e-bay just in case.
>>
>>> In any case, it's probably quicker to ask Ferry or me for testing.
>>> (Although currently I have no access to the board to test OTG, it's
>>> remote device which I can only power on and off and it has always
>>> be in host mode.)
>>>
>>>> Barring that, Ferry can you dig more into this failure? E.g. is it
>>>> this hunk
>>>>
>>>> @@ -85,7 +86,7 @@ static int dwc3_get_dr_mode(struct dwc3 *dwc)
>>>> * mode. If the controller supports DRD but the
>>>> dr_mode is not
>>>> * specified or set to OTG, then set the mode to
>>>> peripheral.
>>>> */
>>>> - if (mode == USB_DR_MODE_OTG &&
>>>> + if (mode == USB_DR_MODE_OTG && !dwc->edev &&
>>>> (!IS_ENABLED(CONFIG_USB_ROLE_SWITCH) ||
>>>> !device_property_read_bool(dwc->dev, "usb-role-switch")) &&
>>>> !DWC3_VER_IS_PRIOR(DWC3, 330A))
>>>> @@ -1632,6 +1633,51 @@ static void dwc3_check_params(struct dwc3 *dwc)
>>>> }
>>>> }
>>>>
>>>> that's problematic or moving
>>> I think you wanted to revert only this line and test?
>> Yes.
>>
>>>> static int dwc3_probe(struct platform_device *pdev)
>>>> {
>>>> struct device *dev = &pdev->dev;
>>>> @@ -1744,6 +1790,13 @@ static int dwc3_probe(struct platform_device
>>>> *pdev)
>>>> goto err2;
>>>> }
>>>>
>>>> + dwc->edev = dwc3_get_extcon(dwc);
>>>> + if (IS_ERR(dwc->edev)) {
>>>> + ret = PTR_ERR(dwc->edev);
>>>> + dev_err_probe(dwc->dev, ret, "failed to get
>>>> extcon\n");
>>>> + goto err3;
>>>> + }
>>>> +
>>>> ret = dwc3_get_dr_mode(dwc);
>>>> if (ret)
>>>> goto err3;
>>>>
>>>> to happen earlier?
>
> I tried moving dwc3_get_extcon after dwc3_get_dr_mode like so::
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 8c8e32651473..3bf370def546 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1843,6 +1843,10 @@ static int dwc3_probe(struct platform_device
> *pdev)
> goto err2;
> }
>
> + ret = dwc3_get_dr_mode(dwc);
> + if (ret)
> + goto err3;
> +
> dwc->edev = dwc3_get_extcon(dwc);
> if (IS_ERR(dwc->edev)) {
> ret = PTR_ERR(dwc->edev);
> @@ -1850,10 +1854,6 @@ static int dwc3_probe(struct platform_device
> *pdev)
> goto err3;
> }
>
> - ret = dwc3_get_dr_mode(dwc);
> - if (ret)
> - goto err3;
> -
> ret = dwc3_alloc_scratch_buffers(dwc);
> if (ret)
> goto err3;
After trying to understand the code a bit, I successfully tested the
following move:
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 8c8e32651473..4a38cff8cb16 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1843,13 +1843,6 @@ static int dwc3_probe(struct platform_device *pdev)
goto err2;
}
- dwc->edev = dwc3_get_extcon(dwc);
- if (IS_ERR(dwc->edev)) {
- ret = PTR_ERR(dwc->edev);
- dev_err_probe(dwc->dev, ret, "failed to get extcon\n");
- goto err3;
- }
-
ret = dwc3_get_dr_mode(dwc);
if (ret)
goto err3;
@@ -1867,6 +1860,13 @@ static int dwc3_probe(struct platform_device *pdev)
dwc3_check_params(dwc);
dwc3_debugfs_init(dwc);
+ dwc->edev = dwc3_get_extcon(dwc);
+ if (IS_ERR(dwc->edev)) {
+ ret = PTR_ERR(dwc->edev);
+ dev_err_probe(dwc->dev, ret, "failed to get extcon\n");
+ goto err5;
+ }
+
ret = dwc3_core_init_mode(dwc);
if (ret)
goto err5;
This moves dwc3_get_extcon() until after dwc3_core_init() but just
before dwc3_core_init_mode(). AFAIU initially dwc3_get_extcon() was
called from within dwc3_core_init_mode() but only for case
USB_DR_MODE_OTG. So with this change order of events is more or less
unchanged.
Due to move I modified goto to err5, not sure if that is correct.
Thoughts? Can we get something like this in quick or should we revert first?
Powered by blists - more mailing lists