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.