Skip to contentSkip to conversation

usb: typec: altmodes/displayport: create sysfs nodes after assigning driver data

PATCH In review RD Babiera 2 participants 3 messages Feb 26, 2024 – Feb 28, 2024

Subsystems (4)
Stable branch USB USB Type-C class USB Type-C bus for alternate modes
lore
Share & tools
lore

RD Babiera

author
#0 lore raw

PATCH v2 1/1 usb: typec: altmodes/displayport: create sysfs nodes after assigning driver data

The DisplayPort driver's sysfs nodes may be present to the userspace before typec_altmode_set_drvdata() completes in dp_altmode_probe. This means that a sysfs read can trigger a NULL pointer error by deferencing dp->hpd in hpd_show or dp->lock in pin_assignment_show, as dev_get_drvdata() returns NULL in those cases.

Create sysfs nodes after typec_altmode_set_drvdata call.

Fixes 0e3bb7d6894d ("usb: typec: Add driver for DisplayPort alternate mode")
Cc [email protected]
Signed-off-by RD Babiera <[email protected]>

---
Changes from v1:
* Moved sysfs node creation instead of NULL checking dev_get_drvdata().

---
 drivers/usb/typec/altmodes/displayport.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/usb/typec/altmodes/displayport.c +4 -4
index 5a80776c7255..5bbdd2c04237 100644
@@ -731,10 +731,6 @@ int dp_altmode_probe(struct typec_altmode *alt)
 	      DP_CAP_PIN_ASSIGN_DFP_D(alt->vdo)))
 		return -ENODEV;
 
-	ret = sysfs_create_group(&alt->dev.kobj, &dp_altmode_group);
-	if (ret)
-		return ret;
-
 	dp = devm_kzalloc(&alt->dev, sizeof(*dp), GFP_KERNEL);
 	if (!dp)
 		return -ENOMEM;

@@ -766,6 +762,10 @@ int dp_altmode_probe(struct typec_altmode *alt)
 	if (plug)
 		typec_altmode_set_drvdata(plug, dp);
 
+	ret = sysfs_create_group(&alt->dev.kobj, &dp_altmode_group);
+	if (ret)
+		return ret;
+
 	dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
 	schedule_work(&dp->work);

base-commit: a560a5672826fc1e057068bda93b3d4c98d037a2

Signature · 1 line
2.44.0.rc1.240.g4c46232300-goog
maintainer
#1 lore raw
Quoted 34 lines · RD Babiera

The DisplayPort driver's sysfs nodes may be present to the userspace before typec_altmode_set_drvdata() completes in dp_altmode_probe. This means that a sysfs read can trigger a NULL pointer error by deferencing dp->hpd in hpd_show or dp->lock in pin_assignment_show, as dev_get_drvdata() returns NULL in those cases.

Create sysfs nodes after typec_altmode_set_drvdata call.

Fixes 0e3bb7d6894d ("usb: typec: Add driver for DisplayPort alternate mode")
Cc [email protected]
Signed-off-by RD Babiera <[email protected]>

---
Changes from v1:
* Moved sysfs node creation instead of NULL checking dev_get_drvdata().

---
 drivers/usb/typec/altmodes/displayport.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/usb/typec/altmodes/displayport.c +0 -4
index 5a80776c7255..5bbdd2c04237 100644
@@ -731,10 +731,6 @@ int dp_altmode_probe(struct typec_altmode *alt)
 	      DP_CAP_PIN_ASSIGN_DFP_D(alt->vdo)))
 		return -ENODEV;
 
-	ret = sysfs_create_group(&alt->dev.kobj, &dp_altmode_group);
-	if (ret)
-		return ret;
-
 	dp = devm_kzalloc(&alt->dev, sizeof(*dp), GFP_KERNEL);
 	if (!dp)
 		return -ENOMEM;

drivers/usb/typec/altmodes/displayport.c int dp_altmode_probe(struct typec_altmode *alt) · line 768

Quoted 7 lines
drivers/usb/typec/altmodes/displayport.c +4 -0
@@ -766,6 +762,10 @@ int dp_altmode_probe(struct typec_altmode *alt)
 	if (plug)
 		typec_altmode_set_drvdata(plug, dp);
 
+	ret = sysfs_create_group(&alt->dev.kobj, &dp_altmode_group);
+	if (ret)
+		return ret;
+

Now I am going to push back again and ask why you are even attempting to create sysfs files "by hand" here at all?

Why is this just not set to be a default group? That way the group is managed properly by the driver core and the driver doesn't have to worry about ANY of this at all. Bonus is that you remove the "you raced with userspace and lost" problem that this code still has even with the change you made here.

Huge hint, if you EVER have to call sysfs_*() from a driver, usually something is wrong (there are a few exceptions, this is not one.)

thanks,

greg k-h

author
#2 lore raw
Quoted 8 lines · Greg KH

Now I am going to push back again and ask why you are even attempting to create sysfs files "by hand" here at all?

Why is this just not set to be a default group? That way the group is managed properly by the driver core and the driver doesn't have to worry about ANY of this at all. Bonus is that you remove the "you raced with userspace and lost" problem that this code still has even with the change you made here.

To answer both questions, the driver had always created its sysfs nodes manually so I did not suspect this was not the preferred way to handle driver sysfs creation until I found your article on default attributes after your email. Will fix the patch up.

thanks again,
rd