Messages in this thread | | | From | Prashant Malani <> | Date | Tue, 8 Feb 2022 14:58:51 -0800 | Subject | Re: [PATCH 3/4] platform/chrome: cros_ec_typec: Configure muxes at start of port update |
| |
On Tue, Feb 8, 2022 at 10:35 AM Prashant Malani <pmalani@chromium.org> wrote: > > On Tue, Feb 8, 2022 at 3:15 AM Tzung-Bi Shih <tzungbi@google.com> 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@google.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. > > 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@chromium.org/ > > [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
| |