Re: [PATCH 3/4] platform/chrome: cros_ec_typec: Configure muxes at start of port update

[Prashant Malani]

On Tue, Feb 8, 2022 at 10:35 AM Prashant Malani <pmalani@xxxxxxxxxxxx> wrote:
>
> On Tue, Feb 8, 2022 at 3:15 AM Tzung-Bi Shih <tzungbi@xxxxxxxxxx> wrote:
> >
> > On Mon, Feb 07, 2022 at 10:12:10PM -0800, Prashant Malani wrote:
> > > On Mon, Feb 7, 2022 at 9:38 PM Tzung-Bi Shih <tzungbi@xxxxxxxxxx> wrote:
> > > > On Mon, Feb 07, 2022 at 09:40:28PM +0000, Prashant Malani wrote:
> > > > > There are situations where the mux state reported by the Embedded
> > > > > Controller (EC), might lag the partner "connected" state. So, the mux
> > > > > state might still suggest that a partner is connected, while the PD
> > > > > "connected" state, being in Try.SNK (for example) suggests that the
> > > > > partner is disconnected.
> > > > >
> > > > > In such a scenario, we will end up sending a disconnect command to the
> > > > > mux driver, followed by a connect command, since the mux is configured
> > > > > later. Avoid this by configuring the mux before
> > > > > registering/disconnecting a partner.
> > > >
> > > > I failed to understand the description.  It looks like some protocol details.
> > > > Could you provide some brief explanation in the commit message?
> > >
> > > I'm not sure how else I can better elaborate on this in the commit message than
> > > as currently stated.
> > > Since the EC is an independent controller, the mux state *can* lag the
> > > "connected" state.
> > > So, as described in the commit message, when a disconnect happens, we could have
> > > a disconnect (since PD_CTRL contains the "connected state") followed
> > > by a configure_mux
> > > with the mux state still suggesting a connected device (the drivers
> > > which implement the
> > > mux/switch controls can misconstrue the old mux state) which results
> > > in a connect. This
> > > patch eliminates that.
> >
> > Pardon me if I ask, I am trying to understand why reorder the function calls
> > in cros_typec_port_update() can fix the issue.  And I am wondering if the
> > issue has fixed by the 4th patch in the series.
>
> It's not completely fixed by that; that is just an outstanding missing
> state update.
> If we just use just that patch, configure_mux() will still be executed
> before the code in patch 4 runs.
>
> >
> > To make sure I understand the issue correctly, imaging a "disconnect" event
> > in cros_typec_port_update() originally:
> >
> > a. Get pd_ctrl via EC_CMD_USB_PD_CONTROL[1].
> >
> > b. Call cros_typec_remove_partner() in cros_typec_set_port_params_v1()[2].
> > Is it the "disconnect" you were referring in the example?
> >
> > c. Get mux info via EC_CMD_USB_PD_MUX_INFO.
> > Did you mean the mux info might be stale which is "partner connected"?
>
> Yes, it can.
>
> >
> > d. Call cros_typec_enable_dp() in cros_typec_configure_mux()[3].
> > Does it result in another connect?
>
> It can occur much earlier, depending on what the mux state is (example: [1])
>
> >
> > [1]: https://elixir.bootlin.com/linux/v5.17-rc3/source/drivers/platform/chrome/cros_ec_typec.c#L955
> > [2]: https://elixir.bootlin.com/linux/v5.17-rc3/source/drivers/platform/chrome/cros_ec_typec.c#L628
> > [3]: https://elixir.bootlin.com/linux/v5.17-rc3/source/drivers/platform/chrome/cros_ec_typec.c#L548
> >
> > If the above understanding is correct, the patch fixes it by moving step b to
> > the last.  As a result, it won't have a "disconnect" -> "connect" transition.
>
> Yes
> >
> > Further questions:
> >
> > - If mux info from step c would be stale, won't it exit earlier in [4]?
> >
> > [4]: https://elixir.bootlin.com/linux/v5.17-rc3/source/drivers/platform/chrome/cros_ec_typec.c#L986
> >
> > - The 4th patch[5] sets mux_flags to USB_PD_MUX_NONE.  If it won't exit earlier
> >   from previous question, won't it fall into [6]?
>
> No. it depends on the mux flags and the pd_ctrl response.
>
> >
> > [5]: https://patchwork.kernel.org/project/chrome-platform/patch/20220207214026.1526151-5-pmalani@xxxxxxxxxxxx/
> > [6]: https://elixir.bootlin.com/linux/v5.17-rc3/source/drivers/platform/chrome/cros_ec_typec.c#L523
>
> This link [6] points to cros_typec_enable_usb4(); it's doesn't relate
> to your statement above.

Sorry, I misread where the link was pointing to. That said, it still
won't fall into the condition you quoted.
The configure_mux() is called first, then the
cros_typec_set_port_params_v1() is called.


>
> >
> > > > > @@ -965,6 +965,11 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
> > > > >       if (ret < 0)
> > > > >               return ret;
> > > > >
> > > > > +     /* Update the switches if they exist, according to requested state */
> > > > > +     ret = cros_typec_configure_mux(typec, port_num, &resp);
> > > > > +     if (ret)
> > > > > +             dev_warn(typec->dev, "Configure muxes failed, err = %d\n", ret);
> > > >
> > > > It used the fact that the function returns `ret` at the end.  After the move,
> > > > the block is no longer the last thing before function returns.
> > > >
> > > > Does it make more sense to return earlier if cros_typec_configure_mux() fails?
> > > > Does the rest of code need to be executed even if cros_typec_configure_mux()
> > > > fails?
> > >
> > > Yes, it should still be executed (we still need to update the port
> > > state). That is why the return is eliminated.
> >
> > Got it, as long as it is intended.
> >
> > > > > @@ -980,11 +985,6 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
> > > > >       if (typec->typec_cmd_supported)
> > > > >               cros_typec_handle_status(typec, port_num);
> > > > >
> > > > > -     /* Update the switches if they exist, according to requested state */
> > > > > -     ret = cros_typec_configure_mux(typec, port_num, &resp);
> > > > > -     if (ret)
> > > > > -             dev_warn(typec->dev, "Configure muxes failed, err = %d\n", ret);
> > > > > -
> > > > >       return ret;
> > > >
> > > > If the function decides to return earlier, it can be `return 0;`.
> > > Sure, I can change this in the next version
> >
> > No, I guess you would like to leave it as is to propagate return value from
> > cros_typec_configure_mux().
>
> No, it is better to not propogate that return value (we're doing it
> earlier, but there isn't
> anything the caller can do about it). We should just print a warn and
> still update the port
> state (userspace still reads the port state).
>
> In general, I think you may benefit from reading:
> - The entire cros_ec_typec driver
> - The EC Type C state machine [2] and interfaces [3][4]
>
> The above 2 will help understand how this entire stack works. Without
> it, it is challenging
> to process the flow (just from code review).
>
> If you have further questions our would like to better understand the
> drivers, feel free to reach
> out to me over IM/email. I don't think public list code review is the
> best option for this
> sort of explanation.
>
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/platform/chrome/cros_ec_typec.c#L549
> [2] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/ec/common/usbc/
> [3] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/ec/driver/usb_mux/usb_mux.c
> [4] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/ec/common/usb_pd_host_cmd.c