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 <[email protected]>
---
drivers/platform/chrome/cros_ec_typec.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
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;
}
--
2.35.0.263.gb82422642f-goog
Hi Tzung-Bi,
On Mon, Feb 7, 2022 at 9:38 PM Tzung-Bi Shih <[email protected]> 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 <[email protected]> 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/[email protected]/
[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 10:35 AM Prashant Malani <[email protected]> wrote:
>
> On Tue, Feb 8, 2022 at 3:15 AM Tzung-Bi Shih <[email protected]> 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 <[email protected]> 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/[email protected]/
> > [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 8, 2022 at 7:31 PM Tzung-Bi Shih <[email protected]> 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 <[email protected]> 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/[email protected]/#24727676
On Tue, Feb 08, 2022 at 02:58:51PM -0800, Prashant Malani wrote:
> On Tue, Feb 8, 2022 at 10:35 AM Prashant Malani <[email protected]> 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/[email protected]/#24727676
On Tue, Feb 8, 2022 at 3:15 AM Tzung-Bi Shih <[email protected]> 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 <[email protected]> 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/[email protected]/
> [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 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;`.