Skip to contentSkip to conversation

usb/usbip: fix wrong data added to platform device

PATCH In review Hongren Zheng 4 participants 4 messages Oct 14, 2023 – Oct 16, 2023

Subsystems (2)
USB USB over IP driver
lore
Share & tools
lore

Hongren Zheng

author reviewer
#0 lore raw

PATCH v2 1/1 usb/usbip: fix wrong data added to platform device

.data of platform_device_info will be copied into .platform_data of struct device via platform_device_add_data.

However, vhcis[i] contains a spinlock, is dynamically allocated and used by other code, so it is not meant to be copied. The workaround was to use void *vhci as an agent, but it was removed in the commit suggested below.

This patch adds back the workaround and changes the way of using platform_data accordingly.

Reported-by [email protected]
Closes https://lore.kernel.org/r/[email protected]/ open here
Reported-by [email protected]
Closes https://lore.kernel.org/r/[email protected]/ open here
Fixes b8aaf639b403 ("usbip: Use platform_device_register_full()")
Tested-by [email protected]
Link https://lore.kernel.org/r/[email protected]/ open here
Signed-off-by Hongren Zheng <[email protected]>
---
 drivers/usb/usbip/vhci_hcd.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

v2:
code style change suggested by Andy Shevchenko

drivers/usb/usbip/vhci_hcd.c +8 -7
index f845b91848b9..82650c11e451 100644
@@ -1139,7 +1139,7 @@ static int hcd_name_to_id(const char *name)
 
 static int vhci_setup(struct usb_hcd *hcd)
 {
-	struct vhci *vhci = dev_get_platdata(hcd->self.controller);
+	struct vhci *vhci = *((void **)dev_get_platdata(hcd->self.controller));
 
 	if (usb_hcd_is_primary_hcd(hcd)) {
 		vhci->vhci_hcd_hs = hcd_to_vhci_hcd(hcd);

@@ -1257,7 +1257,7 @@ static int vhci_get_frame_number(struct usb_hcd *hcd)
 /* FIXME: suspend/resume */
 static int vhci_bus_suspend(struct usb_hcd *hcd)
 {
-	struct vhci *vhci = dev_get_platdata(hcd->self.controller);
+	struct vhci *vhci = *((void **)dev_get_platdata(hcd->self.controller));
 	unsigned long flags;
 
 	dev_dbg(&hcd->self.root_hub->dev, "%s\n", __func__);

@@ -1271,7 +1271,7 @@ static int vhci_bus_suspend(struct usb_hcd *hcd)
 
 static int vhci_bus_resume(struct usb_hcd *hcd)
 {
-	struct vhci *vhci = dev_get_platdata(hcd->self.controller);
+	struct vhci *vhci = *((void **)dev_get_platdata(hcd->self.controller));
 	int rc = 0;
 	unsigned long flags;
 

@@ -1338,7 +1338,7 @@ static const struct hc_driver vhci_hc_driver = {
 
 static int vhci_hcd_probe(struct platform_device *pdev)
 {
-	struct vhci             *vhci = dev_get_platdata(&pdev->dev);
+	struct vhci             *vhci = *((void **)dev_get_platdata(&pdev->dev));
 	struct usb_hcd		*hcd_hs;
 	struct usb_hcd		*hcd_ss;
 	int			ret;

@@ -1396,7 +1396,7 @@ static int vhci_hcd_probe(struct platform_device *pdev)
 
 static void vhci_hcd_remove(struct platform_device *pdev)
 {
-	struct vhci *vhci = dev_get_platdata(&pdev->dev);
+	struct vhci *vhci = *((void **)dev_get_platdata(&pdev->dev));
 
 	/*
 	 * Disconnects the root hub,

@@ -1431,7 +1431,7 @@ static int vhci_hcd_suspend(struct platform_device *pdev, pm_message_t state)
 	if (!hcd)
 		return 0;
 
-	vhci = dev_get_platdata(hcd->self.controller);
+	vhci = *((void **)dev_get_platdata(hcd->self.controller));
 
 	spin_lock_irqsave(&vhci->lock, flags);
 

@@ -1522,10 +1522,11 @@ static int __init vhci_hcd_init(void)
 		goto err_driver_register;
 
 	for (i = 0; i < vhci_num_controllers; i++) {
+		void *vhci = &vhcis[i];
 		struct platform_device_info pdevinfo = {
 			.name = driver_name,
 			.id = i,
-			.data = &vhcis[i],
+			.data = &vhci,
 			.size_data = sizeof(void *),
 		};
Signature · 1 line
2.37.2
Reviewed-by on v2 1/1
#1 lore raw
Quoted 2 lines · Hongren Zheng

.data of platform_device_info will be copied into .platform_data of struct device via platform_device_add_data.

platform_device_add_data()

Quoted 7 lines

However, vhcis[i] contains a spinlock, is dynamically allocated and used by other code, so it is not meant to be copied. The workaround was to use void *vhci as an agent, but it was removed in the commit suggested below.

This patch adds back the workaround and changes the way of using platform_data accordingly.

Good learning to me, thank you for the fix!

Reviewed-by Andy Shevchenko <[email protected]>
Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer Acked-by
#2 lore raw
Quoted 11 lines · Andy Shevchenko
Quoted 2 lines · Hongren Zheng

.data of platform_device_info will be copied into .platform_data of struct device via platform_device_add_data.

platform_device_add_data()

Quoted 5 lines

However, vhcis[i] contains a spinlock, is dynamically allocated and used by other code, so it is not meant to be copied. The workaround was to use void *vhci as an agent, but it was removed in the commit suggested below.

Quoted 6 lines
Quoted 2 lines

This patch adds back the workaround and changes the way of using platform_data accordingly.

Good learning to me, thank you for the fix!

Reviewed-by Andy Shevchenko <[email protected]>

Thank you both.

Acked-by Shuah Khan <[email protected]>

thanks,
-- Shuah

maintainer
#3 lore raw
In reply to Hongren Zheng
Quoted 2 lines · Hongren Zheng

.data of platform_device_info will be copied into .platform_data of struct device via platform_device_add_data.

That is crazy.

Quoted 4 lines

However, vhcis[i] contains a spinlock, is dynamically allocated and used by other code, so it is not meant to be copied. The workaround was to use void *vhci as an agent, but it was removed in the commit suggested below.

Again, crazy, I'm amazed it works at all.

Quoted 11 lines

This patch adds back the workaround and changes the way of using platform_data accordingly.

Reported-by [email protected]
Closes https://lore.kernel.org/r/[email protected]/ open here
Reported-by [email protected]
Closes https://lore.kernel.org/r/[email protected]/ open here
Fixes b8aaf639b403 ("usbip: Use platform_device_register_full()")
Tested-by [email protected]
Link https://lore.kernel.org/r/[email protected]/ open here
Signed-off-by Hongren Zheng <[email protected]>
Quoted 3 lines
---
 drivers/usb/usbip/vhci_hcd.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

I'll take this now, as it fixes a reported issue, but really, the root problem should be fixed instead. This is not how to treat platform data at all.

thanks,

greg k-h