[PATCH v2 1/2] platform/chrome: cros_ec_typec: Add role swap ops

[PATCH v2 1/2] platform/chrome: cros_ec_typec: Add role swap ops

[Radu Vele]

From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

Add the pr_set and dr_set typec_operations to registered typec ports.
This enables sysfs to control power and data role when the port is
capable of doing so.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Co-developed-by: Radu Vele <raduvele@google.com>
Signed-off-by: Radu Vele <raduvele@google.com>

---
  Changes in v2:
    - addressed comments from Tzung-Bi
    - after EC command to swap role is issued we check if the returned
      role in the response matches the target role. While testing this
      I observed that for power role swap the ec returns >0 but the
      returned role doesn't ever match the target role (but the role
      swap happens as expected). This may be an EC bug, I am
      investigating.
---
 drivers/platform/chrome/cros_ec_typec.c | 85 ++++++++++++++++++++++++-
 1 file changed, 84 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 7678e3d05fd3..e689bfaf95dc 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -58,8 +58,91 @@ static int cros_typec_enter_usb_mode(struct typec_port *tc_port, enum usb_mode m
 			  &req, sizeof(req), NULL, 0);
 }
 
+static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_role, u8 swap_type)
+{
+	struct cros_typec_port *port = typec_get_drvdata(tc_port);
+	struct cros_typec_data *data = port->typec_data;
+	struct ec_response_usb_pd_control_v2 resp;
+	struct ec_params_usb_pd_control req;
+	int role, ret;
+
+	/* Must be at least v1 to support role swap. */
+	if (!data->pd_ctrl_ver)
+		return -EOPNOTSUPP;
+
+	/* First query the state */
+	req.port = port->port_num;
+	req.role = USB_PD_CTRL_ROLE_NO_CHANGE;
+	req.mux = USB_PD_CTRL_MUX_NO_CHANGE;
+	req.swap = USB_PD_CTRL_SWAP_NONE;
+
+	ret = cros_ec_cmd(data->ec, data->pd_ctrl_ver, EC_CMD_USB_PD_CONTROL,
+				&req, sizeof(req), &resp, sizeof(resp));
+	if (ret < 0)
+		return ret;
+
+	switch (swap_type) {
+	case USB_PD_CTRL_SWAP_DATA:
+		role = (resp.role & PD_CTRL_RESP_ROLE_DATA) ? TYPEC_HOST :
+						TYPEC_DEVICE;
+		break;
+	case USB_PD_CTRL_SWAP_POWER:
+		role = (resp.role & PD_CTRL_RESP_ROLE_POWER) ? TYPEC_SOURCE :
+						TYPEC_SINK;
+		break;
+	default:
+		dev_warn(data->dev, "Unsupported role swap type %d\n", swap_type);
+		return -EOPNOTSUPP;
+	}
+
+	if (role == target_role)
+		return 0;
+
+	req.swap = swap_type;
+	ret = cros_ec_cmd(data->ec, data->pd_ctrl_ver, EC_CMD_USB_PD_CONTROL,
+				&req, sizeof(req), &resp, sizeof(resp));
+	if (ret < 0)
+		return ret;
+
+	switch (swap_type) {
+	case USB_PD_CTRL_SWAP_DATA:
+		role = resp.role & PD_CTRL_RESP_ROLE_DATA ? TYPEC_HOST : TYPEC_DEVICE;
+		if (role != target_role) {
+			dev_err(data->dev, "Data role swap failed despite EC returning success\n");
+			return -EIO;
+		}
+		typec_set_data_role(tc_port, target_role);
+		break;
+	case USB_PD_CTRL_SWAP_POWER:
+		role = resp.role & PD_CTRL_RESP_ROLE_POWER ? TYPEC_SOURCE : TYPEC_SINK;
+		if (role != target_role) {
+			dev_err(data->dev, "Power role swap failed despite EC returning success\n");
+			return -EIO;
+		}
+		typec_set_pwr_role(tc_port, target_role);
+		break;
+	default:
+		/* Should never execute */
+		break;
+	}
+
+	return 0;
+}
+
+static int cros_typec_dr_swap(struct typec_port *port, enum typec_data_role role)
+{
+	return cros_typec_perform_role_swap(port, role, USB_PD_CTRL_SWAP_DATA);
+}
+
+static int cros_typec_pr_swap(struct typec_port *port, enum typec_role role)
+{
+	return cros_typec_perform_role_swap(port, role, USB_PD_CTRL_SWAP_POWER);
+}
+
 static const struct typec_operations cros_typec_usb_mode_ops = {
-	.enter_usb_mode = cros_typec_enter_usb_mode
+	.enter_usb_mode = cros_typec_enter_usb_mode,
+	.dr_set = cros_typec_dr_swap,
+	.pr_set = cros_typec_pr_swap,
 };
 
 static int cros_typec_parse_port_props(struct typec_capability *cap,
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v2 2/2] platform/chrome: cros_ec_typec: Add lock per-port

[Radu Vele]

Add a lock associated to each port to protect port data against
concurrent access. Concurrency may result from sysfs commands
and ec events.

Signed-off-by: Radu Vele <raduvele@google.com>

---
  Changes in v2:
    - Followed Tzung-Bi's suggestion to use the guard mechanism for
      managing mutexes. This also removes the declaration of `ret`
      in the middle of the function that Benson pointed out.
---
 drivers/platform/chrome/cros_ec_typec.c | 10 ++++++++++
 drivers/platform/chrome/cros_ec_typec.h |  3 +++
 2 files changed, 13 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index e689bfaf95dc..f6a3d73a967f 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -7,7 +7,9 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/cleanup.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/platform_data/cros_ec_commands.h>
 #include <linux/platform_data/cros_usbpd_notify.h>
@@ -54,6 +56,7 @@ static int cros_typec_enter_usb_mode(struct typec_port *tc_port, enum usb_mode m
 		.mode_to_enter = CROS_EC_ALTMODE_USB4
 	};
 
+	guard(mutex)(&port->lock);
 	return cros_ec_cmd(port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
 			  &req, sizeof(req), NULL, 0);
 }
@@ -70,6 +73,8 @@ static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_r
 	if (!data->pd_ctrl_ver)
 		return -EOPNOTSUPP;
 
+	guard(mutex)(&port->lock);
+
 	/* First query the state */
 	req.port = port->port_num;
 	req.role = USB_PD_CTRL_ROLE_NO_CHANGE;
@@ -378,6 +383,7 @@ static void cros_unregister_ports(struct cros_typec_data *typec)
 		if (!typec->ports[i])
 			continue;
 
+		mutex_lock(&typec->ports[i]->lock);
 		cros_typec_remove_partner(typec, i);
 		cros_typec_remove_cable(typec, i);
 
@@ -386,6 +392,8 @@ static void cros_unregister_ports(struct cros_typec_data *typec)
 		typec_mux_put(typec->ports[i]->mux);
 		cros_typec_unregister_port_altmodes(typec->ports[i]);
 		typec_unregister_port(typec->ports[i]->port);
+		mutex_unlock(&typec->ports[i]->lock);
+		mutex_destroy(&typec->ports[i]->lock);
 	}
 }
 
@@ -480,6 +488,7 @@ static int cros_typec_init_ports(struct cros_typec_data *typec)
 			goto unregister_ports;
 		}
 
+		mutex_init(&cros_port->lock);
 		cros_port->port_num = port_num;
 		cros_port->typec_data = typec;
 		typec->ports[port_num] = cros_port;
@@ -1240,6 +1249,7 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
 		return -EINVAL;
 	}
 
+	guard(mutex)(&typec->ports[port_num]->lock);
 	req.port = port_num;
 	req.role = USB_PD_CTRL_ROLE_NO_CHANGE;
 	req.mux = USB_PD_CTRL_MUX_NO_CHANGE;
diff --git a/drivers/platform/chrome/cros_ec_typec.h b/drivers/platform/chrome/cros_ec_typec.h
index f9c31f04c102..d0a8a12ec91a 100644
--- a/drivers/platform/chrome/cros_ec_typec.h
+++ b/drivers/platform/chrome/cros_ec_typec.h
@@ -82,6 +82,9 @@ struct cros_typec_port {
 	struct usb_power_delivery_capabilities *partner_sink_caps;
 
 	struct cros_typec_data *typec_data;
+
+	/* Mutex to protect port data against concurrent access */
+	struct mutex lock;
 };
 
 #endif /*  __CROS_EC_TYPEC__ */
-- 
2.50.0.727.gbf7dc18ff4-goog

Re: (subset) [PATCH v2 1/2] platform/chrome: cros_ec_typec: Add role swap ops

[Tzung-Bi Shih]

On Fri, Jul 11, 2025 at 12:35:01AM +0000, Radu Vele wrote:
> Add the pr_set and dr_set typec_operations to registered typec ports.
> This enables sysfs to control power and data role when the port is
> capable of doing so.
> 
> [...]

Applied to

    https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next

[1/2] platform/chrome: cros_ec_typec: Add role swap ops
      commit: ab229c2b72c35739e8ffb70af11190ff40f38701

Thanks!

Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Add lock per-port

[Tzung-Bi Shih]

On Fri, Jul 11, 2025 at 12:35:02AM +0000, Radu Vele wrote:
> Add a lock associated to each port to protect port data against
> concurrent access. Concurrency may result from sysfs commands
> and ec events.

I realized the critical sections are way too large.  What exactly data the
lock tries to protect?  Is the race possibility introduced by any previous
commits?  Please provide more context.

Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Add lock per-port

[Radu Vele]

On Fri, Jul 11, 2025 at 6:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> On Fri, Jul 11, 2025 at 12:35:02AM +0000, Radu Vele wrote:
> > Add a lock associated to each port to protect port data against
> > concurrent access. Concurrency may result from sysfs commands
> > and ec events.
>
> I realized the critical sections are way too large.  What exactly data the
> lock tries to protect?  Is the race possibility introduced by any previous
> commits?  Please provide more context.

With the implementation of the role swap operations from the previous
commit (and also enter usb mode from another recent commit) we
introduce the possibility of concurrent access to the cros_ec_typec port
data from the userspace (e.g. trigger a power role swap from sysfs) vs
from EC events (e.g. partner triggered a role swap that we accept).
This is the main reason to propose a per-port lock. This way we ensure
we protect the state of each port in the cros_ec_typec driver.

Re: (subset) [PATCH v2 1/2] platform/chrome: cros_ec_typec: Add role swap ops

[Radu Vele]

On Fri, Jul 11, 2025 at 6:11 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> On Fri, Jul 11, 2025 at 12:35:01AM +0000, Radu Vele wrote:
> > Add the pr_set and dr_set typec_operations to registered typec ports.
> > This enables sysfs to control power and data role when the port is
> > capable of doing so.
> >
> > [...]
>
> Applied to
>
>     https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
>
> [1/2] platform/chrome: cros_ec_typec: Add role swap ops
>       commit: ab229c2b72c35739e8ffb70af11190ff40f38701
>
> Thanks!

Thanks, please note that in case of the power role swap I started a discussion
with the EC team to verify the assumption we introduce here (the resp.role
populated by `cros_ec_cmd` should be equal to the target role if the operation
is successful). During testing I saw that the `resp.role` was not equal
to the target role but the role swap operation was successful (I
verified in sysfs
that the power role is the expected one).

In case they advise that our assumption is not correct we should remove the
assert resp.role == target_role for the power role.

Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Add lock per-port

[Tzung-Bi Shih]

On Mon, Jul 14, 2025 at 10:32:03AM +0200, Radu Vele wrote:
> On Fri, Jul 11, 2025 at 6:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> >
> > On Fri, Jul 11, 2025 at 12:35:02AM +0000, Radu Vele wrote:
> > > Add a lock associated to each port to protect port data against
> > > concurrent access. Concurrency may result from sysfs commands
> > > and ec events.
> >
> > I realized the critical sections are way too large.  What exactly data the
> > lock tries to protect?  Is the race possibility introduced by any previous
> > commits?  Please provide more context.
> 
> With the implementation of the role swap operations from the previous
> commit (and also enter usb mode from another recent commit) we
> introduce the possibility of concurrent access to the cros_ec_typec port
> data from the userspace (e.g. trigger a power role swap from sysfs) vs
> from EC events (e.g. partner triggered a role swap that we accept).
> This is the main reason to propose a per-port lock. This way we ensure
> we protect the state of each port in the cros_ec_typec driver.

To make sure I understand, did you mean the lock tries to prevent from
sending multiple commands to EC at a time?  If yes, does it still need
if the underlying ec_dev is guranteed that [1]?

[1] https://elixir.bootlin.com/linux/v6.15/source/drivers/platform/chrome/cros_ec_proto.c#L661

By taking the following hunk the patch adds as an example:

@@ -54,6 +56,7 @@  static int cros_typec_enter_usb_mode(struct typec_port *tc_port, enum usb_mode m
 		.mode_to_enter = CROS_EC_ALTMODE_USB4
 	};
 
+	guard(mutex)(&port->lock);
 	return cros_ec_cmd(port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
 			  &req, sizeof(req), NULL, 0);

It seems the lock doesn't protect any data but the command transfer.

Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Add lock per-port

[Radu Vele]

On Tue, Jul 15, 2025 at 8:07 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> On Mon, Jul 14, 2025 at 10:32:03AM +0200, Radu Vele wrote:
> > On Fri, Jul 11, 2025 at 6:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> > >
> > > On Fri, Jul 11, 2025 at 12:35:02AM +0000, Radu Vele wrote:
> > > > Add a lock associated to each port to protect port data against
> > > > concurrent access. Concurrency may result from sysfs commands
> > > > and ec events.
> > >
> > > I realized the critical sections are way too large.  What exactly data the
> > > lock tries to protect?  Is the race possibility introduced by any previous
> > > commits?  Please provide more context.
> >
> > With the implementation of the role swap operations from the previous
> > commit (and also enter usb mode from another recent commit) we
> > introduce the possibility of concurrent access to the cros_ec_typec port
> > data from the userspace (e.g. trigger a power role swap from sysfs) vs
> > from EC events (e.g. partner triggered a role swap that we accept).
> > This is the main reason to propose a per-port lock. This way we ensure
> > we protect the state of each port in the cros_ec_typec driver.
>
> To make sure I understand, did you mean the lock tries to prevent from
> sending multiple commands to EC at a time?  If yes, does it still need
> if the underlying ec_dev is guranteed that [1]?

Not really, as I noticed that both the ec and Type-C connector class
use their own mutexes.

My intention with the mutexes is to avoid race conditions in the case
when a role swap is in progress but at the same time there is a
`cros_port_update` that modifies the state of the port. Another
example I have in mind is when the port is being unregistered and
a role swap is issued.

Please let me know what you think.

>
> [1] https://elixir.bootlin.com/linux/v6.15/source/drivers/platform/chrome/cros_ec_proto.c#L661
>
> By taking the following hunk the patch adds as an example:
>
> @@ -54,6 +56,7 @@  static int cros_typec_enter_usb_mode(struct typec_port *tc_port, enum usb_mode m
>                 .mode_to_enter = CROS_EC_ALTMODE_USB4
>         };
>
> +       guard(mutex)(&port->lock);
>         return cros_ec_cmd(port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
>                           &req, sizeof(req), NULL, 0);
>
> It seems the lock doesn't protect any data but the command transfer.

Thanks for pointing that out, indeed I can remove the lock in this
function.

Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Add lock per-port

[Tzung-Bi Shih]

On Wed, Jul 16, 2025 at 01:39:53PM +0200, Radu Vele wrote:
> On Tue, Jul 15, 2025 at 8:07 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> >
> > On Mon, Jul 14, 2025 at 10:32:03AM +0200, Radu Vele wrote:
> > > On Fri, Jul 11, 2025 at 6:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> > > >
> > > > On Fri, Jul 11, 2025 at 12:35:02AM +0000, Radu Vele wrote:
> > > > > Add a lock associated to each port to protect port data against
> > > > > concurrent access. Concurrency may result from sysfs commands
> > > > > and ec events.
> > > >
> > > > I realized the critical sections are way too large.  What exactly data the
> > > > lock tries to protect?  Is the race possibility introduced by any previous
> > > > commits?  Please provide more context.
> > >
> > > With the implementation of the role swap operations from the previous
> > > commit (and also enter usb mode from another recent commit) we
> > > introduce the possibility of concurrent access to the cros_ec_typec port
> > > data from the userspace (e.g. trigger a power role swap from sysfs) vs
> > > from EC events (e.g. partner triggered a role swap that we accept).
> > > This is the main reason to propose a per-port lock. This way we ensure
> > > we protect the state of each port in the cros_ec_typec driver.
> >
> > To make sure I understand, did you mean the lock tries to prevent from
> > sending multiple commands to EC at a time?  If yes, does it still need
> > if the underlying ec_dev is guranteed that [1]?
> 
> Not really, as I noticed that both the ec and Type-C connector class
> use their own mutexes.
> 
> My intention with the mutexes is to avoid race conditions in the case
> when a role swap is in progress but at the same time there is a
> `cros_port_update` that modifies the state of the port. Another
> example I have in mind is when the port is being unregistered and
> a role swap is issued.

The critical section is too large to understand which fields in the data
structure it tries to protect.  Please review again whether the lock is
needed or not and shrink the critical section if possible.

If taking the changes in cros_typec_perform_role_swap() as another example:

    static int cros_typec_perform_role_swap(...)
    {
        ...
        struct ec_params_usb_pd_control req;

        guard(mutex)(&port->lock);

        req...
        cros_ec_cmd(...)

        req...
        cros_ec_cmd(...)

        switch (...) {

            typec_set_data_role...

            typec_set_pwr_role...
        }
    }

The `req` and `cros_ec_cmd` obviously don't need to protect.  Does
typec_set_data_role() and typec_set_pwr_role() need to protect from
concurrent calling?