usb: gadget: pch_udc: Drop unneeded cpu_to_le32() call
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]
--- drivers/usb/gadget/udc/pch_udc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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 ;
Signature · 1 line
2.30.2
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().
--- drivers/usb/gadget/udc/pch_udc.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)
drivers/usb/gadget/udc/pch_udc.c +18 -10
@@ -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
PATCH 3/6 usb: gadget: pch_udc: Revert d3cb25a12138 completely
In reply to Andy ShevchenkoThe 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.
--- drivers/usb/gadget/udc/pch_udc.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
drivers/usb/gadget/udc/pch_udc.c +13 -4
@@ -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
PATCH 4/6 usb: gadget: pch_udc: Move pch_udc_init() to satisfy kernel doc
In reply to Andy ShevchenkoKernel 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
--- drivers/usb/gadget/udc/pch_udc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/gadget/udc/pch_udc.c +2 -1
@@ -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
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 forpch_udc_reconnect(). Prototype was forpch_udc_init()insteadFixes 1c575d2d2e3f ("usb: gadget: pch_udc: Fix usb/gadget/pch_udc: Fix ether gadget connect/disconnect issue")Signed-off-by Andy Shevchenko <[email protected]>
[...]
MBR, Sergei
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
PATCH 5/6 usb: gadget: pch_udc: Initialize device pointer before use
In reply to Andy ShevchenkoDuring 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.
--- drivers/usb/gadget/udc/pch_udc.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/usb/gadget/udc/pch_udc.c +3 -2
@@ -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
PATCH 6/6 usb: gadget: pch_udc: Provide a GPIO line used on Intel Minnowboard (v1)
In reply to Andy ShevchenkoIntel Minnowboard (v1) uses SCH GPIO line SUS7 (i.e. 12) for VBUS sense. Provide a DMI based quirk to have it's being used.
--- drivers/usb/gadget/udc/pch_udc.c | 71 +++++++++++++++++++++++++------- 1 file changed, 57 insertions(+), 14 deletions(-)
drivers/usb/gadget/udc/pch_udc.c +57 -14
@@ -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
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")Signed-off-by Andy Shevchenko <[email protected]>
Excellent solution!
Yours,
Linus Walleij
Greg Kroah-Hartman
maintainerQuoted 19 lines · Andy Shevchenko
Either way ~0 will be in the correct byte order,
hence drop unneededcpu_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")Signed-off-by Andy Shevchenko <[email protected]>--- drivers/usb/gadget/udc/pch_udc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/usb/gadget/udc/pch_udc.c +0
-0
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
Quoted 36 lines · Greg Kroah-Hartman
Quoted 19 lines · Andy Shevchenko
Either way ~0 will be in the correct byte order,
hence drop unneededcpu_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")Signed-off-by Andy Shevchenko <[email protected]>--- drivers/usb/gadget/udc/pch_udc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/usb/gadget/udc/pch_udc.c +0
-0drivers/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 | #defineDMA_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