linux-kernel - Re: [PATCH v4 18/18] platform/chrome: cros_ec

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: <Ztq6zf8n09ZcJNjT@google.com>
Date: Fri, 6 Sep 2024 08:18:21 +0000
From: Tzung-Bi Shih <tzungbi@...nel.org>
To: Stephen Boyd <swboyd@...omium.org>
Cc: chrome-platform@...ts.linux.dev, linux-kernel@...r.kernel.org,
	patches@...ts.linux.dev, devicetree@...r.kernel.org,
	Douglas Anderson <dianders@...omium.org>,
	Pin-yen Lin <treapking@...omium.org>,
	Andrzej Hajda <andrzej.hajda@...el.com>,
	Benson Leung <bleung@...omium.org>,
	Conor Dooley <conor+dt@...nel.org>, Daniel Vetter <daniel@...ll.ch>,
	David Airlie <airlied@...il.com>,
	Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
	dri-devel@...ts.freedesktop.org,
	Guenter Roeck <groeck@...omium.org>,
	Jernej Skrabec <jernej.skrabec@...il.com>,
	Jonas Karlman <jonas@...boo.se>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
	Lee Jones <lee@...nel.org>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Prashant Malani <pmalani@...omium.org>,
	Robert Foss <rfoss@...nel.org>, Rob Herring <robh+dt@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Daniel Scally <djrscally@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Ivan Orlov <ivan.orlov0322@...il.com>, linux-acpi@...r.kernel.org,
	linux-usb@...r.kernel.org,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	"Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Vinod Koul <vkoul@...nel.org>
Subject: Re: [PATCH v4 18/18] platform/chrome: cros_ec_typec: Handle lack of
 HPD information

On Wed, Sep 04, 2024 at 02:45:36PM -0700, Stephen Boyd wrote:
> Quoting Tzung-Bi Shih (2024-09-04 02:36:45)
> > On Sat, Aug 31, 2024 at 09:06:56PM -0700, Stephen Boyd wrote:
> > > +static void cros_typec_inject_hpd(struct cros_typec_data *typec,
> > > +                               struct ec_response_usb_pd_mux_info *resp,
> > > +                               struct cros_typec_port *port)
> > > +{
> > [...]
> > > +     /*
> > > +      * Only read the mux GPIO setting if we need to change the active port.
> > > +      * Otherwise, an active port is already set and HPD going high or low
> > > +      * doesn't change the muxed port until DP mode is exited.
> > > +      */
> > > +     if (!typec->active_dp_port) {
> >
> > Given that cros_typec_inject_hpd() is called before `typec->active_dp_port`
> > would be set (from previous patch "platform/chrome: ...  Support DP muxing"),
> > would it possibly wrongly fall into here at the beginning?  (E.g.:
> > cros_typec_probe() -> cros_typec_port_update() -> cros_typec_configure_mux()
> > -> cros_typec_inject_hpd().)
> 
> We wouldn't get here if 'hpd_asserted' is false though. We want to fall

Ack, I overlooked that.

> > > [...]
> > > +     /* Inject HPD from the GPIO state if EC firmware is broken. */
> > > +     if (typec->hpd_asserted)
> > > +             resp->flags |= USB_PD_MUX_HPD_LVL;
> >
> > `typec->hpd_asserted` is shared between all typec->ports[...].  Would it be
> > possible that a HPD is asserted for another port but not current `port`?
> > E.g.: cros_typec_inject_hpd() for port 2 and cros_typec_dp_bridge_hpd_notify()
> > gets called due to port 1 at the same time?
> 
> I'd like to avoid synchronizing the hpd notify and this injection code,
> if that's what you're asking. Thinking about this though, I've realized
> that it's broken even when HPD is working on the EC. Consider this
> scenario with two type-c ports C0 and C1:
>
> [...]

I understood it more: originally, I was wondering if it needs an array
`typec->hpd_asserted[...]` for storing HPD for each port.  But, no.

What cros_typec_dp_bridge_hpd_notify() get is just a connected/disconnected
signal.  It kicks off cros_typec_port_work() for finding which port is
supposed to trigger the event (with some logic with `active_dp_port`,
`mux_gpio`, and `hpd_asserted`).


Curious about one more scenario, is it possible:

Initially, no DP port and no mux is using:
  active_dp_port = NULL
  hpd_asserted = false
  mux_gpio = NULL

CPU A                                        CPU B
------------------------------------------------------------------------------
cros_typec_port_work()
  cros_typec_port_update(port_num=0)
                                             [C0 connected]
                                             cros_typec_dp_bridge_hpd_notify()
                                               hpd_asserted = true
  cros_typec_port_update(port_num=1)
    cros_typec_configure_mux(port_num=1)
      cros_typec_inject_hpd()
      active_dp_port = C1