[PATCH 0/4] platform/chrome: cros_ec_typec: Reorganize mux configuration

[PATCH 0/4] platform/chrome: cros_ec_typec: Reorganize mux configuration

[Prashant Malani]

This is a short series that reorganizes when mux configuration occurs
during Type C port updates. The first 2 patches are minor refactors
which move some of the mux update logic to within
cros_typec_configure_mux(). The third patch moves
cros_typec_configure_mux() itself to be earlier in
cros_typec_port_update().

The final patch updates the stashed mux flag when a partner removal has
occured.

Prashant Malani (4):
  platform/chrome: cros_ec_typec: Move mux flag checks
  platform/chrome: cros_ec_typec: Get mux state inside configure_mux
  platform/chrome: cros_ec_typec: Configure muxes at start of port
    update
  platform/chrome: cros_ec_typec: Update mux flags during partner
    removal

 drivers/platform/chrome/cros_ec_typec.c | 76 +++++++++++--------------
 1 file changed, 34 insertions(+), 42 deletions(-)

-- 
2.35.0.263.gb82422642f-goog

[PATCH 1/4] platform/chrome: cros_ec_typec: Move mux flag checks

[Prashant Malani]

Move mux and role flag checks inside of cros_typec_configure_mux(),
which is a more logical location for them.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
 drivers/platform/chrome/cros_ec_typec.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index c6f17e3ef72d..445da4f122e7 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -519,7 +519,14 @@ static int cros_typec_configure_mux(struct cros_typec_data *typec, int port_num,
 	struct cros_typec_port *port = typec->ports[port_num];
 	struct ec_params_usb_pd_mux_ack mux_ack;
 	enum typec_orientation orientation;
-	int ret;
+	int ret = 0;
+
+	/* No change needs to be made, let's exit early. */
+	if (port->mux_flags == mux_flags && port->role == pd_ctrl->role)
+		return 0;
+
+	port->mux_flags = mux_flags;
+	port->role = pd_ctrl->role;
 
 	if (mux_flags == USB_PD_MUX_NONE) {
 		ret = cros_typec_usb_disconnect_state(port);
@@ -983,13 +990,6 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
 		return 0;
 	}
 
-	/* No change needs to be made, let's exit early. */
-	if (typec->ports[port_num]->mux_flags == mux_resp.flags &&
-	    typec->ports[port_num]->role == resp.role)
-		return 0;
-
-	typec->ports[port_num]->mux_flags = mux_resp.flags;
-	typec->ports[port_num]->role = resp.role;
 	ret = cros_typec_configure_mux(typec, port_num, mux_resp.flags, &resp);
 	if (ret)
 		dev_warn(typec->dev, "Configure muxes failed, err = %d\n", ret);
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 1/4] platform/chrome: cros_ec_typec: Move mux flag checks

[Tzung-Bi Shih]

On Mon, Feb 07, 2022 at 09:40:24PM +0000, Prashant Malani wrote:
> Move mux and role flag checks inside of cros_typec_configure_mux(),
> which is a more logical location for them.

nit: s/Move/Moves/.

> @@ -519,7 +519,14 @@ static int cros_typec_configure_mux(struct cros_typec_data *typec, int port_num,
>  	struct cros_typec_port *port = typec->ports[port_num];
>  	struct ec_params_usb_pd_mux_ack mux_ack;
>  	enum typec_orientation orientation;
> -	int ret;
> +	int ret = 0;

The change looks irrelevant to the patch.

Re: [PATCH 1/4] platform/chrome: cros_ec_typec: Move mux flag checks

[Prashant Malani]

Hi Tzung-Bi,

On Mon, Feb 7, 2022 at 9:33 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> On Mon, Feb 07, 2022 at 09:40:24PM +0000, Prashant Malani wrote:
> > Move mux and role flag checks inside of cros_typec_configure_mux(),
> > which is a more logical location for them.
>
> nit: s/Move/Moves/.

Move is fine. Please see:
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#describe-your-changes

Quoting from the above: " Describe your changes in imperative mood,"

>
> > @@ -519,7 +519,14 @@ static int cros_typec_configure_mux(struct cros_typec_data *typec, int port_num,
> >       struct cros_typec_port *port = typec->ports[port_num];
> >       struct ec_params_usb_pd_mux_ack mux_ack;
> >       enum typec_orientation orientation;
> > -     int ret;
> > +     int ret = 0;
>
> The change looks irrelevant to the patch.
Ack, I missed eliminating this while cleaning up the patches. Will fix
it in the next version.

[PATCH 2/4] platform/chrome: cros_ec_typec: Get mux state inside configure_mux

[Prashant Malani]

Move the function which gets current mux state inside the
cros_typec_configure_mux() function. It is better to group those
bits of functionality together, and it makes it easier to move around
cros_typec_configure_mux() later.

While we are doing this, also inline the cros_typec_get_mux_info() inside
of cros_typec_configure_mux().

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
 drivers/platform/chrome/cros_ec_typec.c | 55 +++++++++++--------------
 1 file changed, 23 insertions(+), 32 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 445da4f122e7..671d3569faf6 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -513,27 +513,38 @@ static int cros_typec_enable_usb4(struct cros_typec_data *typec,
 }
 
 static int cros_typec_configure_mux(struct cros_typec_data *typec, int port_num,
-				uint8_t mux_flags,
 				struct ec_response_usb_pd_control_v2 *pd_ctrl)
 {
 	struct cros_typec_port *port = typec->ports[port_num];
+	struct ec_response_usb_pd_mux_info mux_resp;
+	struct ec_params_usb_pd_mux_info req = {
+		.port = port_num,
+	};
 	struct ec_params_usb_pd_mux_ack mux_ack;
 	enum typec_orientation orientation;
 	int ret = 0;
 
+	ret = cros_ec_command(typec->ec, 0, EC_CMD_USB_PD_MUX_INFO,
+			      &req, sizeof(req), &mux_resp, sizeof(mux_resp));
+	if (ret < 0) {
+		dev_warn(typec->dev, "Failed to get mux info for port: %d, err = %d\n",
+			 port_num, ret);
+		return ret;
+	}
+
 	/* No change needs to be made, let's exit early. */
-	if (port->mux_flags == mux_flags && port->role == pd_ctrl->role)
+	if (port->mux_flags == mux_resp.flags && port->role == pd_ctrl->role)
 		return 0;
 
-	port->mux_flags = mux_flags;
+	port->mux_flags = mux_resp.flags;
 	port->role = pd_ctrl->role;
 
-	if (mux_flags == USB_PD_MUX_NONE) {
+	if (port->mux_flags == USB_PD_MUX_NONE) {
 		ret = cros_typec_usb_disconnect_state(port);
 		goto mux_ack;
 	}
 
-	if (mux_flags & USB_PD_MUX_POLARITY_INVERTED)
+	if (port->mux_flags & USB_PD_MUX_POLARITY_INVERTED)
 		orientation = TYPEC_ORIENTATION_REVERSE;
 	else
 		orientation = TYPEC_ORIENTATION_NORMAL;
@@ -548,22 +559,22 @@ static int cros_typec_configure_mux(struct cros_typec_data *typec, int port_num,
 	if (ret)
 		return ret;
 
-	if (mux_flags & USB_PD_MUX_USB4_ENABLED) {
+	if (port->mux_flags & USB_PD_MUX_USB4_ENABLED) {
 		ret = cros_typec_enable_usb4(typec, port_num, pd_ctrl);
-	} else if (mux_flags & USB_PD_MUX_TBT_COMPAT_ENABLED) {
+	} else if (port->mux_flags & USB_PD_MUX_TBT_COMPAT_ENABLED) {
 		ret = cros_typec_enable_tbt(typec, port_num, pd_ctrl);
-	} else if (mux_flags & USB_PD_MUX_DP_ENABLED) {
+	} else if (port->mux_flags & USB_PD_MUX_DP_ENABLED) {
 		ret = cros_typec_enable_dp(typec, port_num, pd_ctrl);
-	} else if (mux_flags & USB_PD_MUX_SAFE_MODE) {
+	} else if (port->mux_flags & USB_PD_MUX_SAFE_MODE) {
 		ret = cros_typec_usb_safe_state(port);
-	} else if (mux_flags & USB_PD_MUX_USB_ENABLED) {
+	} else if (port->mux_flags & USB_PD_MUX_USB_ENABLED) {
 		port->state.alt = NULL;
 		port->state.mode = TYPEC_STATE_USB;
 		ret = typec_mux_set(port->mux, &port->state);
 	} else {
 		dev_dbg(typec->dev,
 			"Unrecognized mode requested, mux flags: %x\n",
-			mux_flags);
+			port->mux_flags);
 	}
 
 mux_ack:
@@ -638,17 +649,6 @@ static void cros_typec_set_port_params_v1(struct cros_typec_data *typec,
 	}
 }
 
-static int cros_typec_get_mux_info(struct cros_typec_data *typec, int port_num,
-				   struct ec_response_usb_pd_mux_info *resp)
-{
-	struct ec_params_usb_pd_mux_info req = {
-		.port = port_num,
-	};
-
-	return cros_ec_command(typec->ec, 0, EC_CMD_USB_PD_MUX_INFO, &req,
-			       sizeof(req), resp, sizeof(*resp));
-}
-
 /*
  * Helper function to register partner/plug altmodes.
  */
@@ -946,7 +946,6 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
 {
 	struct ec_params_usb_pd_control req;
 	struct ec_response_usb_pd_control_v2 resp;
-	struct ec_response_usb_pd_mux_info mux_resp;
 	int ret;
 
 	if (port_num < 0 || port_num >= typec->num_ports) {
@@ -982,15 +981,7 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
 		cros_typec_handle_status(typec, port_num);
 
 	/* Update the switches if they exist, according to requested state */
-	ret = cros_typec_get_mux_info(typec, port_num, &mux_resp);
-	if (ret < 0) {
-		dev_warn(typec->dev,
-			 "Failed to get mux info for port: %d, err = %d\n",
-			 port_num, ret);
-		return 0;
-	}
-
-	ret = cros_typec_configure_mux(typec, port_num, mux_resp.flags, &resp);
+	ret = cros_typec_configure_mux(typec, port_num, &resp);
 	if (ret)
 		dev_warn(typec->dev, "Configure muxes failed, err = %d\n", ret);
 
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 2/4] platform/chrome: cros_ec_typec: Get mux state inside configure_mux

[Tzung-Bi Shih]

On Mon, Feb 07, 2022 at 09:40:26PM +0000, Prashant Malani wrote:
> Move the function which gets current mux state inside the
> cros_typec_configure_mux() function. It is better to group those
> bits of functionality together, and it makes it easier to move around
> cros_typec_configure_mux() later.

nit: s/Move/Moves/.

>  static int cros_typec_configure_mux(struct cros_typec_data *typec, int port_num,
> -				uint8_t mux_flags,
>  				struct ec_response_usb_pd_control_v2 *pd_ctrl)
>  {
>  	struct cros_typec_port *port = typec->ports[port_num];
> +	struct ec_response_usb_pd_mux_info mux_resp;
> +	struct ec_params_usb_pd_mux_info req = {
> +		.port = port_num,
> +	};
>  	struct ec_params_usb_pd_mux_ack mux_ack;
>  	enum typec_orientation orientation;
>  	int ret = 0;
>  
> +	ret = cros_ec_command(typec->ec, 0, EC_CMD_USB_PD_MUX_INFO,
> +			      &req, sizeof(req), &mux_resp, sizeof(mux_resp));

It was `req` and `resp` in cros_typec_get_mux_info().  However, `mux_resp` for
separating from `struct ec_response_usb_pd_control_v2 resp` in
cros_typec_port_update().

It would be neat to be either {`req`, `resp`} or {`mux_req`, `mux_resp`} in
cros_typec_configure_mux().

Re: [PATCH 2/4] platform/chrome: cros_ec_typec: Get mux state inside configure_mux

[Prashant Malani]

On Mon, Feb 7, 2022 at 9:35 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> On Mon, Feb 07, 2022 at 09:40:26PM +0000, Prashant Malani wrote:
> > Move the function which gets current mux state inside the
> > cros_typec_configure_mux() function. It is better to group those
> > bits of functionality together, and it makes it easier to move around
> > cros_typec_configure_mux() later.
>
> nit: s/Move/Moves/.

Move is fine. Please see:
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#describe-your-changes

Quoting from the above: " Describe your changes in imperative mood,"

>
> >  static int cros_typec_configure_mux(struct cros_typec_data *typec, int port_num,
> > -                             uint8_t mux_flags,
> >                               struct ec_response_usb_pd_control_v2 *pd_ctrl)
> >  {
> >       struct cros_typec_port *port = typec->ports[port_num];
> > +     struct ec_response_usb_pd_mux_info mux_resp;
> > +     struct ec_params_usb_pd_mux_info req = {
> > +             .port = port_num,
> > +     };
> >       struct ec_params_usb_pd_mux_ack mux_ack;
> >       enum typec_orientation orientation;
> >       int ret = 0;
> >
> > +     ret = cros_ec_command(typec->ec, 0, EC_CMD_USB_PD_MUX_INFO,
> > +                           &req, sizeof(req), &mux_resp, sizeof(mux_resp));
>
> It was `req` and `resp` in cros_typec_get_mux_info().  However, `mux_resp` for
> separating from `struct ec_response_usb_pd_control_v2 resp` in
> cros_typec_port_update().
>
> It would be neat to be either {`req`, `resp`} or {`mux_req`, `mux_resp`} in
> cros_typec_configure_mux().

I'll change it to resp (instead of mux_resp)

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

[Prashant Malani]

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

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

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

[Tzung-Bi Shih]

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;`.

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

[Prashant Malani]

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

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

[Tzung-Bi Shih]

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

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

[Prashant Malani]

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

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@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

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

[Tzung-Bi Shih]

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

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

[Prashant Malani]

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

[PATCH 4/4] platform/chrome: cros_ec_typec: Update mux flags during partner removal

[Prashant Malani]

In cros_typec_remove_partner(), we call
cros_typec_usb_disconnect_state() which sets the switches/muxes to be in
a disconnected state. This also happens in cros_typec_configure_mux().
However, unlike there, here the mux_flags variable hasn't been updated
to reflect that a disconnection has occurred. Update the flag here
accordingly.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
 drivers/platform/chrome/cros_ec_typec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 164c82f537dd..ce37b6abe69f 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -228,6 +228,7 @@ static void cros_typec_remove_partner(struct cros_typec_data *typec,
 	cros_typec_unregister_altmodes(typec, port_num, true);
 
 	cros_typec_usb_disconnect_state(port);
+	port->mux_flags = USB_PD_MUX_NONE;
 
 	typec_unregister_partner(port->partner);
 	port->partner = NULL;
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 4/4] platform/chrome: cros_ec_typec: Update mux flags during partner removal

[Tzung-Bi Shih]

On Mon, Feb 07, 2022 at 09:40:30PM +0000, Prashant Malani wrote:
> In cros_typec_remove_partner(), we call
> cros_typec_usb_disconnect_state() which sets the switches/muxes to be in
> a disconnected state. This also happens in cros_typec_configure_mux().
> However, unlike there, here the mux_flags variable hasn't been updated
> to reflect that a disconnection has occurred. Update the flag here
> accordingly.

It is fine (at least to me) usually.

Since you would need to respin the series anyway, s/Update/Updates/.

Re: [PATCH 4/4] platform/chrome: cros_ec_typec: Update mux flags during partner removal

[Prashant Malani]

Hi Tzung-Bi,

Thanks for reviewing the series.

On Mon, Feb 7, 2022 at 9:38 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> On Mon, Feb 07, 2022 at 09:40:30PM +0000, Prashant Malani wrote:
> > In cros_typec_remove_partner(), we call
> > cros_typec_usb_disconnect_state() which sets the switches/muxes to be in
> > a disconnected state. This also happens in cros_typec_configure_mux().
> > However, unlike there, here the mux_flags variable hasn't been updated
> > to reflect that a disconnection has occurred. Update the flag here
> > accordingly.
>
> It is fine (at least to me) usually.
>
> Since you would need to respin the series anyway, s/Update/Updates/.

Again, "update" is fine. Please see:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

Quoting from the above: " Describe your changes in imperative mood,"

So, imperative usage is actually preferred, and "updates/makes" is discouraged.

(I grabbed an older version of the documentation in my other replies,
but the description is the same).

Thanks!

Re: [PATCH 4/4] platform/chrome: cros_ec_typec: Update mux flags during partner removal

[Tzung-Bi Shih]

On Mon, Feb 07, 2022 at 10:15:12PM -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:30PM +0000, Prashant Malani wrote:
> > > In cros_typec_remove_partner(), we call
> > > cros_typec_usb_disconnect_state() which sets the switches/muxes to be in
> > > a disconnected state. This also happens in cros_typec_configure_mux().
> > > However, unlike there, here the mux_flags variable hasn't been updated
> > > to reflect that a disconnection has occurred. Update the flag here
> > > accordingly.
> >
> > It is fine (at least to me) usually.
> >
> > Since you would need to respin the series anyway, s/Update/Updates/.
> 
> Again, "update" is fine. Please see:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
> 
> Quoting from the above: " Describe your changes in imperative mood,"
> 
> So, imperative usage is actually preferred, and "updates/makes" is discouraged.

Ack, thanks for correcting my so-long-time misunderstanding.