diff mbox series

[4/4] usb: typec: ucsi: Hold con->lock for the entire duration of ucsi_register_port()

Message ID 20200809141904.4317-5-hdegoede@redhat.com (mailing list archive)
State New, archived
Headers show
Series usb: typec: ucsi: Fix various locking issues | expand

Commit Message

Hans de Goede Aug. 9, 2020, 2:19 p.m. UTC 
Commit 081da1325d35 ("usb: typec: ucsi: displayport: Fix a potential race
during registration") made the ucsi code hold con->lock in
ucsi_register_displayport(). But we really don't want any interactions
with the connector to run before the port-registration process is fully
complete.

This commit moves the taking of con->lock from ucsi_register_displayport()
into ucsi_register_port() to achieve this.

Cc: stable@vger.kernel.org
Fixes: 081da1325d35 ("usb: typec: ucsi: displayport: Fix a potential race during registration")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/ucsi/displayport.c |  9 +-------
 drivers/usb/typec/ucsi/ucsi.c        | 31 ++++++++++++++++++++--------
 2 files changed, 23 insertions(+), 17 deletions(-)

Comments

Sasha Levin Aug. 13, 2020, 4:25 p.m. UTC | #1 
Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: 081da1325d35 ("usb: typec: ucsi: displayport: Fix a potential race during registration").

The bot has tested the following trees: v5.8, v5.7.14, v5.4.57.

v5.8: Build OK!
v5.7.14: Failed to apply! Possible dependencies:
    4dbc6a4ef06d ("usb: typec: ucsi: save power data objects in PD mode")
    992a60ed0d5e ("usb: typec: ucsi: register with power_supply class")

v5.4.57: Failed to apply! Possible dependencies:
    2ede55468ca8 ("usb: typec: ucsi: Remove the old API")
    3cf657f07918 ("usb: typec: ucsi: Remove all bit-fields")
    470ce43a1a81 ("usb: typec: ucsi: Remove struct ucsi_control")
    4dbc6a4ef06d ("usb: typec: ucsi: save power data objects in PD mode")
    6df475f804e6 ("usb: typec: ucsi: Start using struct typec_operations")
    992a60ed0d5e ("usb: typec: ucsi: register with power_supply class")
    bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?
Sasha Levin Aug. 19, 2020, 11:56 p.m. UTC | #2 
Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: 081da1325d35 ("usb: typec: ucsi: displayport: Fix a potential race during registration").

The bot has tested the following trees: v5.8.1, v5.7.15, v5.4.58.

v5.8.1: Build OK!
v5.7.15: Failed to apply! Possible dependencies:
    4dbc6a4ef06d ("usb: typec: ucsi: save power data objects in PD mode")
    992a60ed0d5e ("usb: typec: ucsi: register with power_supply class")

v5.4.58: Failed to apply! Possible dependencies:
    2ede55468ca8 ("usb: typec: ucsi: Remove the old API")
    3cf657f07918 ("usb: typec: ucsi: Remove all bit-fields")
    470ce43a1a81 ("usb: typec: ucsi: Remove struct ucsi_control")
    4dbc6a4ef06d ("usb: typec: ucsi: save power data objects in PD mode")
    6df475f804e6 ("usb: typec: ucsi: Start using struct typec_operations")
    992a60ed0d5e ("usb: typec: ucsi: register with power_supply class")
    bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?
Sasha Levin Aug. 26, 2020, 1:53 p.m. UTC | #3 
Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: 081da1325d35 ("usb: typec: ucsi: displayport: Fix a potential race during registration").

The bot has tested the following trees: v5.8.2, v5.7.16, v5.4.59.

v5.8.2: Build OK!
v5.7.16: Failed to apply! Possible dependencies:
    4dbc6a4ef06d ("usb: typec: ucsi: save power data objects in PD mode")
    992a60ed0d5e ("usb: typec: ucsi: register with power_supply class")

v5.4.59: Failed to apply! Possible dependencies:
    2ede55468ca8 ("usb: typec: ucsi: Remove the old API")
    3cf657f07918 ("usb: typec: ucsi: Remove all bit-fields")
    470ce43a1a81 ("usb: typec: ucsi: Remove struct ucsi_control")
    4dbc6a4ef06d ("usb: typec: ucsi: save power data objects in PD mode")
    6df475f804e6 ("usb: typec: ucsi: Start using struct typec_operations")
    992a60ed0d5e ("usb: typec: ucsi: register with power_supply class")
    bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?
diff mbox series

Patch

diff --git a/drivers/usb/typec/ucsi/displayport.c b/drivers/usb/typec/ucsi/displayport.c
index 048381c058a5..261131c9e37c 100644
--- a/drivers/usb/typec/ucsi/displayport.c
+++ b/drivers/usb/typec/ucsi/displayport.c
@@ -288,8 +288,6 @@  struct typec_altmode *ucsi_register_displayport(struct ucsi_connector *con,
 	struct typec_altmode *alt;
 	struct ucsi_dp *dp;
 
-	mutex_lock(&con->lock);
-
 	/* We can't rely on the firmware with the capabilities. */
 	desc->vdo |= DP_CAP_DP_SIGNALING | DP_CAP_RECEPTACLE;
 
@@ -298,15 +296,12 @@  struct typec_altmode *ucsi_register_displayport(struct ucsi_connector *con,
 	desc->vdo |= all_assignments << 16;
 
 	alt = typec_port_register_altmode(con->port, desc);
-	if (IS_ERR(alt)) {
-		mutex_unlock(&con->lock);
+	if (IS_ERR(alt))
 		return alt;
-	}
 
 	dp = devm_kzalloc(&alt->dev, sizeof(*dp), GFP_KERNEL);
 	if (!dp) {
 		typec_unregister_altmode(alt);
-		mutex_unlock(&con->lock);
 		return ERR_PTR(-ENOMEM);
 	}
 
@@ -319,7 +314,5 @@  struct typec_altmode *ucsi_register_displayport(struct ucsi_connector *con,
 	alt->ops = &ucsi_displayport_ops;
 	typec_altmode_set_drvdata(alt, dp);
 
-	mutex_unlock(&con->lock);
-
 	return alt;
 }
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 182e34aa65ed..2999217c8109 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -898,12 +898,15 @@  static int ucsi_register_port(struct ucsi *ucsi, int index)
 	con->num = index + 1;
 	con->ucsi = ucsi;
 
+	/* Delay other interactions with the con until registration is complete */
+	mutex_lock(&con->lock);
+
 	/* Get connector capability */
 	command = UCSI_GET_CONNECTOR_CAPABILITY;
 	command |= UCSI_CONNECTOR_NUMBER(con->num);
 	ret = ucsi_send_command(ucsi, command, &con->cap, sizeof(con->cap));
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DRP)
 		cap->data = TYPEC_PORT_DRD;
@@ -935,26 +938,32 @@  static int ucsi_register_port(struct ucsi *ucsi, int index)
 
 	ret = ucsi_register_port_psy(con);
 	if (ret)
-		return ret;
+		goto out;
 
 	/* Register the connector */
 	con->port = typec_register_port(ucsi->dev, cap);
-	if (IS_ERR(con->port))
-		return PTR_ERR(con->port);
+	if (IS_ERR(con->port)) {
+		ret = PTR_ERR(con->port);
+		goto out;
+	}
 
 	/* Alternate modes */
 	ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_CON);
-	if (ret)
+	if (ret) {
 		dev_err(ucsi->dev, "con%d: failed to register alt modes\n",
 			con->num);
+		goto out;
+	}
 
 	/* Get the status */
 	command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num);
 	ret = ucsi_send_command(ucsi, command, &con->status, sizeof(con->status));
 	if (ret < 0) {
 		dev_err(ucsi->dev, "con%d: failed to get status\n", con->num);
-		return 0;
+		ret = 0;
+		goto out;
 	}
+	ret = 0; /* ucsi_send_command() returns length on success */
 
 	switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) {
 	case UCSI_CONSTAT_PARTNER_TYPE_UFP:
@@ -979,17 +988,21 @@  static int ucsi_register_port(struct ucsi *ucsi, int index)
 
 	if (con->partner) {
 		ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_SOP);
-		if (ret)
+		if (ret) {
 			dev_err(ucsi->dev,
 				"con%d: failed to register alternate modes\n",
 				con->num);
-		else
+			ret = 0;
+		} else {
 			ucsi_altmode_update_active(con);
+		}
 	}
 
 	trace_ucsi_register_port(con->num, &con->status);
 
-	return 0;
+out:
+	mutex_unlock(&con->lock);
+	return ret;
 }
 
 /**