Re: [PATCH v4 18/18] platform/chrome: cros_ec_typec: Handle lack of HPD information

[Stephen Boyd <swboyd@chromium.org>]

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
into this case in the beginning, i.e. 'active_dp_port' is NULL, so that
we can read the mux and figure out which port is actually selected.

If we don't have a mux gpio we assume that we aren't muxing and that
there's only one port to begin with. I'll add a comment after the if
(mux_gpio) condition with this info.

>
> > [...]
> > +     /* 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:

	Plug in C1
	EC notifies AP
	AP queues cros_typec_port_work()
	HPD asserted
	EC picks C1 for DP // First to have hpd asserted
	EC notifies AP
	AP tries to queue cros_typec_port_work() but it's pending. Skip.
	Plug in C0
	EC notifies AP
	AP tries to queue cros_typec_port_work() but it's pending. Skip.
	HPD asserted
	EC notifies AP
	AP tries to queue cros_typec_port_work() but it's pending. Skip.
	Finally cros_typec_port_work() runs!
	 for (i = 0; i < typec->num_ports; i++) // typec->num_ports = 2
	  cros_typec_port_update(port_num=0)
	   cros_ec_cmd(EC_CMD_USB_PD_CONTROL.port=0) // In DP mode
	   cros_typec_configure_mux(port_num=0)
	    cros_ec_cmd(EC_CMD_USB_PD_MUX_INFO.port=0) // hpd asserted
	    if (!active_dp_port)
	     active_dp_port = port0

This is bad. The worker could be significantly delayed, although it's
really unlikely in practice. It would be better if the EC pushed a
message to AP about what happened, instead of having to query the EC
about the state of USB. Or the EC could have a sequence number or
something so AP could ask for the history of events. We can't fix all
the EC firmwares though, so we get what we get.

I think one solution would be to read the mux all the time and ignore
tracking the active port based on hpd state. If we do that then we don't
get tripped up by a delayed work iterating over both typec ports. The
logic will be a bit more complicated though, because we'll have to
consider all the ports when entering and exiting DP mode on one port so
that we don't assign DP to the wrong port.

Also, when hpd is broken on the EC I see an error message when I unplug
the DP cable. It's the "No valid DP mode provided." error from
cros_typec_enable_dp(). When I inject hpd that error goes away. I'll
need to look closer to understand why, but I suspect I'll need to keep
injecting hpd to avoid it.