| Message ID | 20220207214026.1526151-4-pmalani@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | platform/chrome: cros_ec_typec: Reorganize mux configuration | expand |
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? On a related note, followed up the example scenario, which one of the understanding is the most applicable: 1) The disconnect followed by a connect is suboptimal. The patch cleans it. 2) The disconnect followed by a connect is a bug. The patch fixes it. > @@ -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? > @@ -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;`.
Hi Tzung-Bi, 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. > > On a related note, followed up the example scenario, which one of the > understanding is the most applicable: > 1) The disconnect followed by a connect is suboptimal. The patch cleans it. > 2) The disconnect followed by a connect is a bug. The patch fixes it. This one (number 2) > > > @@ -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. > > > @@ -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
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. 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().
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. > > > > > @@ -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
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
On Tue, Feb 08, 2022 at 02:58:51PM -0800, Prashant Malani wrote: > On Tue, Feb 8, 2022 at 10:35 AM Prashant Malani <pmalani@chromium.org> wrote: > > 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 Thanks for the link pointers. It would take me some time to understand the background for reviewing the patch though. But in general, I would suggest to change the title for a stronger signal that it fixes something (per [1]). Otherwise, the title and the description in cover letter looks like a move refactor. [1]: https://patchwork.kernel.org/project/chrome-platform/patch/20220207214026.1526151-4-pmalani@chromium.org/#24727676
On Tue, Feb 8, 2022 at 7:31 PM Tzung-Bi Shih <tzungbi@google.com> wrote: > > On Tue, Feb 08, 2022 at 02:58:51PM -0800, Prashant Malani wrote: > > On Tue, Feb 8, 2022 at 10:35 AM Prashant Malani <pmalani@chromium.org> wrote: > > > 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 > > Thanks for the link pointers. It would take me some time to understand the > background for reviewing the patch though. > > But in general, I would suggest to change the title for a stronger signal > that it fixes something (per [1]). Otherwise, the title and the description > in cover letter looks like a move refactor. I'm hesitant to use the term "fix" in the title; IMO the commit message describes the potential race involved. Since it is an unlikely edge case, brought about by limitations in how the EC updates its state, I'd like to avoid assigning it a "Fixes" label too. I can perhaps add some info to the cover letter, but I don't think that alone is worth a v3. I'll let this series hang for a few days; if there are other comments which necessitate a respin, I'll reword the cover letter. -Prashant > > [1]: https://patchwork.kernel.org/project/chrome-platform/patch/20220207214026.1526151-4-pmalani@chromium.org/#24727676
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c index 671d3569faf6..164c82f537dd 100644 --- a/drivers/platform/chrome/cros_ec_typec.c +++ b/drivers/platform/chrome/cros_ec_typec.c @@ -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); + dev_dbg(typec->dev, "Enabled %d: 0x%hhx\n", port_num, resp.enabled); dev_dbg(typec->dev, "Role %d: 0x%hhx\n", port_num, resp.role); dev_dbg(typec->dev, "Polarity %d: 0x%hhx\n", port_num, resp.polarity); @@ -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; }
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. Signed-off-by: Prashant Malani <pmalani@chromium.org> --- drivers/platform/chrome/cros_ec_typec.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)