linux-kernel - Re: [RESEND x2][PATCH 0/3] dwc2 fixes for edge cases on hikey

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: <CALAqxLXoSmKuGGROsWqas2pq4iXJ_jawJ+kkM19G3vQ2bNP0Hg@mail.gmail.com>
Date:   Thu, 12 Oct 2017 11:06:06 -0700
From:   John Stultz <john.stultz@...aro.org>
To:     Minas Harutyunyan <Minas.Harutyunyan@...opsys.com>
Cc:     John Youn <John.Youn@...opsys.com>,
        lkml <linux-kernel@...r.kernel.org>,
        Wei Xu <xuwei5@...ilicon.com>,
        Guodong Xu <guodong.xu@...aro.org>,
        Amit Pundir <amit.pundir@...aro.org>,
        YongQin Liu <yongqin.liu@...aro.org>,
        Douglas Anderson <dianders@...omium.org>,
        Chen Yu <chenyu56@...wei.com>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: Re: [RESEND x2][PATCH 0/3] dwc2 fixes for edge cases on hikey

On Thu, Oct 12, 2017 at 12:59 AM, Minas Harutyunyan
<Minas.Harutyunyan@...opsys.com> wrote:
>
> 1. Vardan's patch fixing issue when dwc2 switched from host to device
> mode. It's allow to make functional device after reconnecting without
> tracking UDC state.

While I'm sure Vardan's patch is useful, I worry that its resolving a
different issue then what I'm trying to address, as it doesn't seem to
help the problems I'm seeing.

> 2. I suppose that your patch "[RESEND x2][PATCH 1/3] usb: dwc2: Improve
> gadget state disconnection handling" not a good way to set correct UDC
> state. You added calling device mode functions dwc2_hsotg_disconnect()
> and dwc2_hsotg_core_init_disconnected() while core in Host mode and as
> result additional unwanted "mode mismatch" interrupts will be asserted.

Apologies, I'm not sure I'm understanding you here. Forgive me if I'm
misinterpreting your feedback.

So, the "usb: dwc2: Improve gadget state disconnection handling" isn't
itself doing the UDC state handling.

Personally I see it as improving a previously applied fix
(dad3f793f20f - usb: dwc2: Make sure we disconnect the gadget state).
 So instead of calling dwc2_hsotg_disconnect() in
dwc2_conn_id_status_change() when transitioning INTO device/B mode,
which was added due to earlier problems with state tracking (as when
we unplug the gadget cable, nothing else triggers the hsotg_disconnect
code), I'm instead suggesting we call dwc2_hsotg_disconnect() when we
transition into host/A mode.

This only allows us to do proper UDC state handling later, since we
properly run the disconnect code for device when we switch into host
mode.

If I'm understanding you, you seem to be objecting to this, as calling
dwc2_hsotg_disconnect() while we are transitioning to host mode can
cause "mode mismatch" interrupts. I've not seen this in practice with
this patch, but you know the logic better and it could be possible.

Now, I'm of course open to other approaches, but it seems that we need
*something* to call dwc2_hsotg_disconnect() when the otg cable is
removed (which currently just doesn't happen). The earlier patch
calling dwc2_hsotg_disconnect() when we are entering device/B mode
avoids the state tracking warnings but, doesn't seem correct (nor does
it allow for things like proper UDC state handling).

> 3. Function dwc2_conn_id_status_change() called when connector ID status
> changed. This interrupt asserted only when A-plug connected or
> disconnected. Connecting/disconnecting B-plug doesn't assert this interrupt.

Ok. What I'm seeing may be somewhat hardware specific then, as on
HiKey, we have a switch that enables a on-board USB hub when the OTG
plug is removed.  This may be the root of the issue, but I guess I'm
at a loss for how things should be handled here.

When the b-plug is disconnected, we need to do something to signal to
the core that we aren't connected, no?

And it seems that your point that the conn_id_status_change logic only
runs on the A-plug connect/disconnect mirrors the usage for the B plug
(ie: if A is disconnected, we enter B mode, thus if A is connected,
shouldn't we disable/disconnect B mode?). I suspect there's something
more subtle to your statement though, so if you could expand a bit so
I could better understand I'd appreciate it.

thanks
-john