Message-ID: <YgJQ30D8nc4LWvUy@google.com>
Date:   Tue, 8 Feb 2022 19:15:43 +0800
From:   Tzung-Bi Shih <tzungbi@...gle.com>
To:     Prashant Malani <pmalani@...omium.org>
Cc:     linux-kernel@...r.kernel.org, Benson Leung <bleung@...omium.org>,
        Guenter Roeck <groeck@...omium.org>,
        "open list:CHROMEOS EC USB TYPE-C DRIVER" 
        <chrome-platform@...ts.linux.dev>
Subject: Re: [PATCH 3/4] platform/chrome: cros_ec_typec: Configure muxes at
 start of port update

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@...gle.com> 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.

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"?

d. Call cros_typec_enable_dp() in cros_typec_configure_mux()[3].
Does it result in another connect?

[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.

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]?

[5]: https://patchwork.kernel.org/project/chrome-platform/patch/20220207214026.1526151-5-pmalani@chromium.org/
[6]: https://elixir.bootlin.com/linux/v5.17-rc3/source/drivers/platform/chrome/cros_ec_typec.c#L523

> > > @@ -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().

Powered by blists - more mailing lists