Skip to contentSkip to conversation

usb: gadget: pch_udc: Drop unneeded cpu_to_le32() call

PATCH In review Andy Shevchenko 5 participants 11 messages Mar 22, 2021 – Mar 25, 2021 USB

lore
Share & tools
lore
#0 lore raw

PATCH 1/6 usb: gadget: pch_udc: Drop unneeded cpu_to_le32() call

Either way ~0 will be in the correct byte order,
hence drop unneeded cpu_to_le32() call. Moreover,
it makes sparse happy, otherwise it complains:

pch_udc.c:1813:27: warning: incorrect type in assignment (different base types)
pch_udc.c:1813:27: expected unsigned int [usertype] dataptr
pch_udc.c:1813:27: got restricted __le32 [usertype]

Fixes f646cf94520e ("USB device driver of Topcliff PCH")
---
 drivers/usb/gadget/udc/pch_udc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/gadget/udc/pch_udc.c +1 -1
index a3c1fc924268..2e2dca391007 100644
@@ -1756,7 +1756,7 @@ static struct usb_request *pch_udc_alloc_request(struct usb_ep *usbep,
 	}
 	/* prevent from using desc. - set HOST BUSY */
 	dma_desc->status |= PCH_UDC_BS_HST_BSY;
-	dma_desc->dataptr = cpu_to_le32(DMA_ADDR_INVALID);
+	dma_desc->dataptr = DMA_ADDR_INVALID;
 	req->td_data = dma_desc;
 	req->td_data_last = dma_desc;
 	req->chain_len = 1;
Signature · 1 line
2.30.2
#1 lore raw

PATCH 2/6 usb: gardet: pch_udc: Check if driver is present before calling ->setup()

Since we have a separate routine for VBUS sense, the interrupt may occur before gadget driver is present. Hence, ->setup() call may oops the kernel:

[   55.245843] BUG: kernel NULL pointer dereference, address: 00000010
...
[   55.245843] EIP: pch_udc_isr.cold+0x162/0x33f
...
[   55.245843]  <IRQ>
[   55.245843]  ? pch_udc_svc_data_out+0x160/0x160

Check if driver is present before calling ->setup().

Fixes f646cf94520e ("USB device driver of Topcliff PCH")
---
 drivers/usb/gadget/udc/pch_udc.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)
drivers/usb/gadget/udc/pch_udc.c +18 -10
index 2e2dca391007..5bb83a2705ad 100644
@@ -2298,6 +2298,21 @@ static void pch_udc_svc_data_out(struct pch_udc_dev *dev, int ep_num)
 		pch_udc_set_dma(dev, DMA_DIR_RX);
 }
 
+static int pch_udc_gadget_setup(struct pch_udc_dev *dev)
+	__must_hold(&dev->lock)
+{
+	int rc;
+
+	/* In some cases we can get an interrupt before driver gets setup */
+	if (!dev->driver)
+		return -ESHUTDOWN;
+
+	spin_unlock(&dev->lock);
+	rc = dev->driver->setup(&dev->gadget, &dev->setup_data);
+	spin_lock(&dev->lock);
+	return rc;
+}
+
 /**
  * pch_udc_svc_control_in() - Handle Control IN endpoint interrupts
  * @dev:	Reference to the device structure
@@ -2369,15 +2384,12 @@ static void pch_udc_svc_control_out(struct pch_udc_dev *dev)
 			dev->gadget.ep0 = &dev->ep[UDC_EP0IN_IDX].ep;
 		else /* OUT */
 			dev->gadget.ep0 = &ep->ep;
-		spin_lock(&dev->lock);
 		/* If Mass storage Reset */
 		if ((dev->setup_data.bRequestType == 0x21) &&
 		    (dev->setup_data.bRequest == 0xFF))
 			dev->prot_stall = 0;
 		/* call gadget with setup data received */
-		setup_supported = dev->driver->setup(&dev->gadget,
-						     &dev->setup_data);
-		spin_unlock(&dev->lock);
+		setup_supported = pch_udc_gadget_setup(dev);
 
 		if (dev->setup_data.bRequestType & USB_DIR_IN) {
 			ep->td_data->status = (ep->td_data->status &
@@ -2625,9 +2637,7 @@ static void pch_udc_svc_intf_interrupt(struct pch_udc_dev *dev)
 		dev->ep[i].halted = 0;
 	}
 	dev->stall = 0;
-	spin_unlock(&dev->lock);
-	dev->driver->setup(&dev->gadget, &dev->setup_data);
-	spin_lock(&dev->lock);
+	pch_udc_gadget_setup(dev);
 }
 
 /**
@@ -2662,9 +2672,7 @@ static void pch_udc_svc_cfg_interrupt(struct pch_udc_dev *dev)
 	dev->stall = 0;
 
 	/* call gadget zero with setup data received */
-	spin_unlock(&dev->lock);
-	dev->driver->setup(&dev->gadget, &dev->setup_data);
-	spin_lock(&dev->lock);
+	pch_udc_gadget_setup(dev);
 }
 
 /**
Signature · 1 line
2.30.2
#2 lore raw

PATCH 3/6 usb: gadget: pch_udc: Revert d3cb25a12138 completely

In reply to Andy Shevchenko

The commit d3cb25a12138 ("usb: gadget: udc: fix spin_lock in pch_udc") obviously was not thought through and had made the situation even worse than it was before. Two changes after almost reverted it. but a few leftovers have been left as it. With this revert d3cb25a12138 completely.

While at it, narrow down the scope of unlocked section to prevent potential race when prot_stall is assigned.

Fixes d3cb25a12138 ("usb: gadget: udc: fix spin_lock in pch_udc")
Fixes 9903b6bedd38 ("usb: gadget: pch-udc: fix lock")
Fixes 1d23d16a88e6 ("usb: gadget: pch_udc: reorder spin_[un]lock to avoid deadlock")
Cc Iago Abal <[email protected]>
---
 drivers/usb/gadget/udc/pch_udc.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
drivers/usb/gadget/udc/pch_udc.c +13 -4
index 5bb83a2705ad..5421075df01c 100644
@@ -596,18 +596,22 @@ static void pch_udc_reconnect(struct pch_udc_dev *dev)
 static inline void pch_udc_vbus_session(struct pch_udc_dev *dev,
 					  int is_active)
 {
+	unsigned long		iflags;
+
+	spin_lock_irqsave(&dev->lock, iflags);
 	if (is_active) {
 		pch_udc_reconnect(dev);
 		dev->vbus_session = 1;
 	} else {
 		if (dev->driver && dev->driver->disconnect) {
-			spin_lock(&dev->lock);
+			spin_unlock_irqrestore(&dev->lock, iflags);
 			dev->driver->disconnect(&dev->gadget);
-			spin_unlock(&dev->lock);
+			spin_lock_irqsave(&dev->lock, iflags);
 		}
 		pch_udc_set_disconnect(dev);
 		dev->vbus_session = 0;
 	}
+	spin_unlock_irqrestore(&dev->lock, iflags);
 }
 
 /**
@@ -1166,20 +1170,25 @@ static int pch_udc_pcd_selfpowered(struct usb_gadget *gadget, int value)
 static int pch_udc_pcd_pullup(struct usb_gadget *gadget, int is_on)
 {
 	struct pch_udc_dev	*dev;
+	unsigned long		iflags;
 
 	if (!gadget)
 		return -EINVAL;
+
 	dev = container_of(gadget, struct pch_udc_dev, gadget);
+
+	spin_lock_irqsave(&dev->lock, iflags);
 	if (is_on) {
 		pch_udc_reconnect(dev);
 	} else {
 		if (dev->driver && dev->driver->disconnect) {
-			spin_lock(&dev->lock);
+			spin_unlock_irqrestore(&dev->lock, iflags);
 			dev->driver->disconnect(&dev->gadget);
-			spin_unlock(&dev->lock);
+			spin_lock_irqsave(&dev->lock, iflags);
 		}
 		pch_udc_set_disconnect(dev);
 	}
+	spin_unlock_irqrestore(&dev->lock, iflags);
 
 	return 0;
 }
Signature · 1 line
2.30.2
#3 lore raw

PATCH 4/6 usb: gadget: pch_udc: Move pch_udc_init() to satisfy kernel doc

In reply to Andy Shevchenko

Kernel doc and the content described by it shouldn't be teared apart. Otherwise validator is not happy:

.../pch_udc.c:573: warning: expecting prototype for pch_udc_reconnect(). Prototype was for pch_udc_init() instead

Fixes 1c575d2d2e3f ("usb: gadget: pch_udc: Fix usb/gadget/pch_udc: Fix ether gadget connect/disconnect issue")
---
 drivers/usb/gadget/udc/pch_udc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/gadget/udc/pch_udc.c +2 -1
index 5421075df01c..984c9299d2c6 100644
@@ -563,12 +563,13 @@ static void pch_udc_clear_disconnect(struct pch_udc_dev *dev)
 	pch_udc_bit_clr(dev, UDC_DEVCTL_ADDR, UDC_DEVCTL_RES);
 }
 
+static void pch_udc_init(struct pch_udc_dev *dev);
+
 /**
  * pch_udc_reconnect() - This API initializes usb device controller,
  *						and clear the disconnect status.
  * @dev:		Reference to pch_udc_regs structure
  */
-static void pch_udc_init(struct pch_udc_dev *dev);
 static void pch_udc_reconnect(struct pch_udc_dev *dev)
 {
 	pch_udc_init(dev);
Signature · 1 line
2.30.2
#4 lore raw

Re: [PATCH v1 4/6] usb: gadget: pch_udc: Move pch_udc_init() to satisfy kernel doc

Hi!

Quoted 1 line · Andy Shevchenko

Kernel doc and the content described by it shouldn't be teared apart.

s/teared/torn/?

Quoted 6 lines

Otherwise validator is not happy:

.../pch_udc.c:573: warning: expecting prototype for pch_udc_reconnect(). Prototype was for pch_udc_init() instead

Fixes 1c575d2d2e3f ("usb: gadget: pch_udc: Fix usb/gadget/pch_udc: Fix ether gadget connect/disconnect issue")

[...]

MBR, Sergei

#5 lore raw

Re: [PATCH v1 4/6] usb: gadget: pch_udc: Move pch_udc_init() to satisfy kernel doc

Quoted 5 lines · Sergei Shtylyov
Quoted 1 line · Andy Shevchenko

Kernel doc and the content described by it shouldn't be teared apart.

    s/teared/torn/?

Thanks!
I will change if the maintainer asks to resend or if it will be another version for some other reason.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#6 lore raw

PATCH 5/6 usb: gadget: pch_udc: Initialize device pointer before use

In reply to Andy Shevchenko

During conversion to use GPIO descriptors the device pointer, which is applied to devm_gpiod_get(), is not yet initialized.

Move initialization in the ->probe() in order to have it set before use.

Fixes e20849a8c883 ("usb: gadget: pch_udc: Convert to use GPIO descriptors")
---
 drivers/usb/gadget/udc/pch_udc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/usb/gadget/udc/pch_udc.c +3 -2
index 984c9299d2c6..1c8c1070eb28 100644
@@ -1370,6 +1370,7 @@ static irqreturn_t pch_vbus_gpio_irq(int irq, void *data)
  */
 static int pch_vbus_gpio_init(struct pch_udc_dev *dev)
 {
+	struct device *d = &dev->pdev->dev;
 	int err;
 	int irq_num = 0;
 	struct gpio_desc *gpiod;
@@ -1378,7 +1379,7 @@ static int pch_vbus_gpio_init(struct pch_udc_dev *dev)
 	dev->vbus_gpio.intr = 0;
 
 	/* Retrieve the GPIO line from the USB gadget device */
-	gpiod = devm_gpiod_get(dev->gadget.dev.parent, NULL, GPIOD_IN);
+	gpiod = devm_gpiod_get(d, NULL, GPIOD_IN);
 	if (IS_ERR(gpiod))
 		return PTR_ERR(gpiod);
 	gpiod_set_consumer_name(gpiod, "pch_vbus");
@@ -3081,6 +3082,7 @@ static int pch_udc_probe(struct pci_dev *pdev,
 	if (retval)
 		return retval;
 
+	dev->pdev = pdev;
 	pci_set_drvdata(pdev, dev);
 
 	/* Determine BAR based on PCI ID */
@@ -3122,7 +3124,6 @@ static int pch_udc_probe(struct pci_dev *pdev,
 
 	/* device struct setup */
 	spin_lock_init(&dev->lock);
-	dev->pdev = pdev;
 	dev->gadget.ops = &pch_udc_ops;
 
 	retval = init_dma_pools(dev);
Signature · 1 line
2.30.2
#7 lore raw

PATCH 6/6 usb: gadget: pch_udc: Provide a GPIO line used on Intel Minnowboard (v1)

In reply to Andy Shevchenko

Intel Minnowboard (v1) uses SCH GPIO line SUS7 (i.e. 12) for VBUS sense. Provide a DMI based quirk to have it's being used.

Fixes e20849a8c883 ("usb: gadget: pch_udc: Convert to use GPIO descriptors")
---
 drivers/usb/gadget/udc/pch_udc.c | 71 +++++++++++++++++++++++++-------
 1 file changed, 57 insertions(+), 14 deletions(-)
drivers/usb/gadget/udc/pch_udc.c +57 -14
index 1c8c1070eb28..e5f3c6157d53 100644
@@ -7,12 +7,14 @@
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/delay.h>
+#include <linux/dmi.h>
 #include <linux/errno.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/machine.h>
 #include <linux/list.h>
 #include <linux/interrupt.h>
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
-#include <linux/gpio/consumer.h>
 #include <linux/irq.h>
 
 #define PCH_VBUS_PERIOD		3000	/* VBUS polling period (msec) */
@@ -1360,6 +1362,43 @@ static irqreturn_t pch_vbus_gpio_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static struct gpiod_lookup_table minnowboard_udc_gpios = {
+	.dev_id		= "0000:02:02.4",
+	.table		= {
+		GPIO_LOOKUP("sch_gpio.33158", 12, NULL, GPIO_ACTIVE_HIGH),
+		{}
+	},
+};
+
+static const struct dmi_system_id pch_udc_gpio_dmi_table[] = {
+	{
+		.ident = "MinnowBoard",
+		.matches = {
+			DMI_MATCH(DMI_BOARD_NAME, "MinnowBoard"),
+		},
+		.driver_data = &minnowboard_udc_gpios,
+	},
+	{ }
+};
+
+static void pch_vbus_gpio_remove_table(void *table)
+{
+	gpiod_remove_lookup_table(table);
+}
+
+static int pch_vbus_gpio_add_table(struct pch_udc_dev *dev)
+{
+	struct device *d = &dev->pdev->dev;
+	const struct dmi_system_id *dmi;
+
+	dmi = dmi_first_match(pch_udc_gpio_dmi_table);
+	if (!dmi)
+		return 0;
+
+	gpiod_add_lookup_table(dmi->driver_data);
+	return devm_add_action_or_reset(d, pch_vbus_gpio_remove_table, dmi->driver_data);
+}
+
 /**
  * pch_vbus_gpio_init() - This API initializes GPIO port detecting VBUS.
  * @dev:		Reference to the driver structure
@@ -1378,8 +1417,12 @@ static int pch_vbus_gpio_init(struct pch_udc_dev *dev)
 	dev->vbus_gpio.port = NULL;
 	dev->vbus_gpio.intr = 0;
 
+	err = pch_vbus_gpio_add_table(dev);
+	if (err)
+		return err;
+
 	/* Retrieve the GPIO line from the USB gadget device */
-	gpiod = devm_gpiod_get(d, NULL, GPIOD_IN);
+	gpiod = devm_gpiod_get_optional(d, NULL, GPIOD_IN);
 	if (IS_ERR(gpiod))
 		return PTR_ERR(gpiod);
 	gpiod_set_consumer_name(gpiod, "pch_vbus");
@@ -2889,14 +2932,20 @@ static void pch_udc_pcd_reinit(struct pch_udc_dev *dev)
  * @dev:	Reference to the driver structure
  *
  * Return codes:
- *	0: Success
+ *	0:		Success
+ *	-%ERRNO:	All kind of errors when retrieving VBUS GPIO
  */
 static int pch_udc_pcd_init(struct pch_udc_dev *dev)
 {
+	int ret;
+
 	pch_udc_init(dev);
 	pch_udc_pcd_reinit(dev);
-	pch_vbus_gpio_init(dev);
-	return 0;
+
+	ret = pch_vbus_gpio_init(dev);
+	if (ret)
+		pch_udc_exit(dev);
+	return ret;
 }
 
 /**
@@ -3098,16 +3147,10 @@ static int pch_udc_probe(struct pci_dev *pdev,
 
 	dev->base_addr = pcim_iomap_table(pdev)[bar];
 
-	/*
-	 * FIXME: add a GPIO descriptor table to pdev.dev using
-	 * gpiod_add_descriptor_table() from <linux/gpio/machine.h> based on
-	 * the PCI subsystem ID. The system-dependent GPIO is necessary for
-	 * VBUS operation.
-	 */
-
 	/* initialize the hardware */
-	if (pch_udc_pcd_init(dev))
-		return -ENODEV;
+	retval = pch_udc_pcd_init(dev);
+	if (retval)
+		return retval;
 
 	pci_enable_msi(pdev);
 
Signature · 1 line
2.30.2
Reviewed-by on v1 6/6
#8 lore raw

Re: [PATCH v1 6/6] usb: gadget: pch_udc: Provide a GPIO line used on Intel Minnowboard (v1)

Quoted 5 lines · Andy Shevchenko

Intel Minnowboard (v1) uses SCH GPIO line SUS7 (i.e. 12) for VBUS sense. Provide a DMI based quirk to have it's being used.

Fixes e20849a8c883 ("usb: gadget: pch_udc: Convert to use GPIO descriptors")

Excellent solution!

Yours,
Linus Walleij

#9 lore raw
In reply to Andy Shevchenko
Quoted 19 lines · Andy Shevchenko

Either way ~0 will be in the correct byte order,
hence drop unneeded cpu_to_le32() call. Moreover,
it makes sparse happy, otherwise it complains:

pch_udc.c:1813:27: warning: incorrect type in assignment (different base types)
pch_udc.c:1813:27: expected unsigned int [usertype] dataptr
pch_udc.c:1813:27: got restricted __le32 [usertype]

Fixes f646cf94520e ("USB device driver of Topcliff PCH")
---
 drivers/usb/gadget/udc/pch_udc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/gadget/udc/pch_udc.c +0 -0
index a3c1fc924268..2e2dca391007 100644

drivers/usb/gadget/udc/pch_udc.c static struct usb_request *pch_udc_alloc_request(struct usb_ep *usbep, · line 1762

Quoted 8 lines
drivers/usb/gadget/udc/pch_udc.c +1 -1
@@ -1756,7 +1756,7 @@ static struct usb_request *pch_udc_alloc_request(struct usb_ep *usbep,
 	}
 	/* prevent from using desc. - set HOST BUSY */
 	dma_desc->status |= PCH_UDC_BS_HST_BSY;
-	dma_desc->dataptr = cpu_to_le32(DMA_ADDR_INVALID);
+	dma_desc->dataptr = DMA_ADDR_INVALID;
 	req->td_data = dma_desc;
 	req->td_data_last = dma_desc;
 	req->chain_len = 1;

With this series applied, I get the following build warning:

drivers/usb/gadget/udc/pch_udc.c: In function ‘pch_udc_alloc_request’:
drivers/usb/gadget/udc/pch_udc.c:208:26: warning: conversion from ‘long long unsigned int’ to ‘u32’ {aka ‘unsigned int’} changes value from ‘18446744073709551615’ to ‘4294967295’ [-Woverflow]
208 | #define DMA_ADDR_INVALID (~(dma_addr_t)0)
| ^
drivers/usb/gadget/udc/pch_udc.c:1813:22: note: in expansion of macro ‘DMA_ADDR_INVALID’

 1813 |  dma_desc->dataptr = DMA_ADDR_INVALID;
      |                      ^~~~~~~~~~~~~~~~

Please fix up and resend a new version of this series.

thanks,

greg k-h

#10 lore raw
Quoted 36 lines · Greg Kroah-Hartman
Quoted 19 lines · Andy Shevchenko

Either way ~0 will be in the correct byte order,
hence drop unneeded cpu_to_le32() call. Moreover,
it makes sparse happy, otherwise it complains:

pch_udc.c:1813:27: warning: incorrect type in assignment (different base types)
pch_udc.c:1813:27: expected unsigned int [usertype] dataptr
pch_udc.c:1813:27: got restricted __le32 [usertype]

Fixes f646cf94520e ("USB device driver of Topcliff PCH")
---
 drivers/usb/gadget/udc/pch_udc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/gadget/udc/pch_udc.c +0 -0
index a3c1fc924268..2e2dca391007 100644

drivers/usb/gadget/udc/pch_udc.c static struct usb_request *pch_udc_alloc_request(struct usb_ep *usbep, · line 1762

Quoted 8 lines
drivers/usb/gadget/udc/pch_udc.c +1 -1
@@ -1756,7 +1756,7 @@ static struct usb_request *pch_udc_alloc_request(struct usb_ep *usbep,
 	}
 	/* prevent from using desc. - set HOST BUSY */
 	dma_desc->status |= PCH_UDC_BS_HST_BSY;
-	dma_desc->dataptr = cpu_to_le32(DMA_ADDR_INVALID);
+	dma_desc->dataptr = DMA_ADDR_INVALID;
 	req->td_data = dma_desc;
 	req->td_data_last = dma_desc;
 	req->chain_len = 1;

With this series applied, I get the following build warning:

drivers/usb/gadget/udc/pch_udc.c: In function ‘pch_udc_alloc_request’:
drivers/usb/gadget/udc/pch_udc.c:208:26: warning: conversion from ‘long long unsigned int’ to ‘u32’ {aka ‘unsigned int’} changes value from ‘18446744073709551615’ to ‘4294967295’ [-Woverflow]
208 | #define DMA_ADDR_INVALID (~(dma_addr_t)0)
| ^
drivers/usb/gadget/udc/pch_udc.c:1813:22: note: in expansion of macro ‘DMA_ADDR_INVALID’

Quoted 5 lines
 1813 |  dma_desc->dataptr = DMA_ADDR_INVALID;
      |                      ^~~~~~~~~~~~~~~~

Please fix up and resend a new version of this series.

Oops, thanks! Now I understand how I missed this (I have another patch to test that effectively removed that line AFAIR. In any case, I will retest and resend.

Signature · 2 lines
With Best Regards,
Andy Shevchenko