linux-kernel - Re: [RFC][PATCH 2/3] usb: roles: Add usb role switch notifier.

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]
Date:   Fri, 18 Oct 2019 23:05:37 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     John Stultz <john.stultz@...aro.org>
Cc:     lkml <linux-kernel@...r.kernel.org>, Yu Chen <chenyu56@...wei.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Chunfeng Yun <chunfeng.yun@...iatek.com>,
        Felipe Balbi <balbi@...nel.org>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Jun Li <lijun.kernel@...il.com>,
        Valentin Schneider <valentin.schneider@....com>,
        Linux USB List <linux-usb@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>
Subject: Re: [RFC][PATCH 2/3] usb: roles: Add usb role switch notifier.

Hi,

On 18-10-2019 22:37, John Stultz wrote:
> On Fri, Oct 18, 2019 at 1:21 PM Hans de Goede <hdegoede@...hat.com> wrote:
>> On 18-10-2019 22:12, John Stultz wrote:
>>> On Fri, Oct 18, 2019 at 12:59 PM Hans de Goede <hdegoede@...hat.com> wrote:
>>>> On 18-10-2019 21:53, John Stultz wrote:
>>>>> On Fri, Oct 18, 2019 at 12:30 PM Hans de Goede <hdegoede@...hat.com> wrote:
>>>>>> Looking at drivers/usb/typec/tcpm/tcpci.c: tcpci_set_vconn I see that
>>>>>> there is a data struct with vendor specific callbacks and that the
>>>>>> drivers/usb/typec/tcpm/tcpci_rt1711h.c implements that.
>>>>>>
>>>>>> So you may want something similar here. But things are tricky here,
>>>>>> because when nothing is connected you want to provide Vbus for
>>>>>> the USB-A ports, which means that if someone then connects a
>>>>>> USB-A to C cable to connect the board to a PC (switching the port
>>>>>> to device mode) there will be a time when both sides are supplying
>>>>>> 5V if I remember the schedule correctly.
>>>>>
>>>>> Ok. Thanks for the pointer, I'll take a look at that to see if I can
>>>>> get it to work.
>>>>>
>>>>>> I think that the original hack might not be that bad, the whole hw
>>>>>> design seems so, erm, broken, that you probably cannot do proper
>>>>>> roleswapping anyways.  So just tying Vbus to host mode might be
>>>>>> fine, the question then becomes again how can some other piece
>>>>>> of code listen to the role-switch events...
>>>>>
>>>>> So, at least in the current approach (see the v3 series), I've
>>>>> basically set the hub driver as an role-switch intermediary, sitting
>>>>> between the calls from the tcpm to the dwc3 driver. It actually works
>>>>> better then the earlier notifier method (which had some issues with
>>>>> reliably establishing the initial state on boot).  Does that approach
>>>>> work for you?
>>>>
>>>> That sounds like it might be a nice solution. But I have not seen the
>>>> code, I think I was not Cc-ed on v3. Do you have a patchwork or
>>>> lore.kernel.org link for me?
>>>
>>> Oh! I think I had you on CC, maybe it got caught in your spam folder?
>>
>> More likely I just deleted mail to aggressively, sorry.
>>
>>> My apologies either way! The thread is here:
>>>     https://lore.kernel.org/lkml/20191016033340.1288-1-john.stultz@linaro.org/
>>>
>>> And the hub/role-switch-intermediary driver is here:
>>>     https://lore.kernel.org/lkml/20191016033340.1288-12-john.stultz@linaro.org/
>>
>> Hm, this looks very nice actually, much much better then the notifier stuff!
>>
>> As for your:
>>
>> "NOTE: It was noted that controlling the TYPEC_VBUS_POWER_OFF and
>> TYPEC_VBUS_POWER_ON values here is not reccomended. I'm looking
>> for a way to remove that bit from the logic here, but wanted to
>> still get feedback on this approach."
>>
>> Comment in the commit message, normally a type-c port would turn external Vbus
>> off until a sink is connected, IIRC the same Vbus is also used for the TupeA ports
>> on the hub, so that would mean those are unusable when nothing is connected to
>> the TypeC port, which is not what you want.
> 
> Uh, so I think for the HiKey960, the type-A ports on the hub are
> separately powered via the hub_power_ctrl(hisi_hikey_usb,
> HUB_VBUS_POWER_OFF/ON) call.
> 
> At least, with the current driver, the functionality is working as
> expected: remove the USB-C cable, and devices connected to the hub
> power on, plug something into the USB-C port and devices plugged into
> the hub shutdown.
> 
> But maybe I'm missing what you mean?

Ok, so double checking the schematic I do see separate Vbus-es for the
TypeC port and the TypeA ports, with the TypeC port one being controlled
by GPIO_202_VBUS_TYPEC. So ideally that gpio would be  controlled to
enable/disable vbus by the tcpm framework.

>> So I think that given the special case / hack-ish hw you have, that just setting
>> Vbus based on the role is ok(ish).
> 
> Ok. I'm happy to stick with what works here, since it is at least the
> oddness is isolated to the device specific hub driver.

Right, so for the Type-A ports Vbus controlled by PRT_CTL1 enabling it depending
on host vs devices mode makes sense. But the Type-C one really should be
controlled by the tcpm framework.

Regards,

Hans

p.s.

Sorry for the confusion I was under the impression that there was only 1
Vbus enable for both Type-A and Type-C ports.