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]

Message-ID: <CALAqxLW5vYpGY1yLTvRefiX6anOGsPWW_Dc1Rj8kdf9+BDm0kw@mail.gmail.com>
Date:   Fri, 18 Oct 2019 13:37:39 -0700
From:   John Stultz <john.stultz@...aro.org>
To:     Hans de Goede <hdegoede@...hat.com>
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.

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?

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

thanks
-john