usb: gadget: pch_udc: Replace cpu_to_le32() by lower_32_bits()
PATCH v2 1/7 usb: gadget: pch_udc: Replace cpu_to_le32() by lower_32_bits()
Either way ~0 will be in the correct byte order, hence replace cpu_to_le32() by lower_32_bits(). 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]
---
v2: fixed 64-bit build (Greg)
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 =lower_32_bits (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 v2 2/7 usb: gadget: 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().
---
v2: fixed typo in the Subject
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 v2 3/7 usb: gadget: pch_udc: Check for DMA mapping error
In reply to Andy ShevchenkoDMA mapping might fail, we have to check it with dma_mapping_error(). Otherwise DMA-API is not happy:
DMA-API: pch_udc 0000:02:02.4: device driver failed to check map error[device address=0x00000000027ee678] [size=64 bytes] [mapped as single]
---
v2: new patch
drivers/usb/gadget/udc/pch_udc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/gadget/udc/pch_udc.c +1 -1
@@ -2946,7 +2946,7 @@ static int init_dma_pools(struct pch_udc_dev *dev) dev->dma_addr =dma_map_single (&dev->pdev->dev, ep0out_buf,UDC_EP0OUT_BUFF_SIZE *4 ,DMA_FROM_DEVICE ); -return 0 ; +return dma_mapping_error (&dev->pdev->dev, dev->dma_addr); } static int pch_udc_start(struct usb_gadget *g,
Signature · 1 line
2.30.2
PATCH v2 4/7 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 torn apart. Otherwise validator is not happy:
.../pch_udc.c:573: warning: expecting prototype for pch_udc_reconnect(). Prototype was for pch_udc_init() instead
---
v2: Fixed typo in the commit message (Sergei)
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
PATCH v2 5/7 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.
---
v2: no changes
drivers/usb/gadget/udc/pch_udc.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
drivers/usb/gadget/udc/pch_udc.c +13 -4
@@ -597,18 +597,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); }/**
@@ -1167,20 +1171,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 v2 6/7 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.
---
v2: no changes
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
Re: [PATCH v2 6/7] usb: gadget: pch_udc: Initialize device pointer before use
Quoted 7 lines · 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")Signed-off-by Andy Shevchenko <[email protected]>
Ooops sorry.
Yours,
Linus Walleij
PATCH v2 7/7 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.
---
v2: no changes
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 v2 7/7] 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]>
Yours,
Linus Walleij
Quoted 10 lines · Andy Shevchenko
Either way ~0 will be in the correct byte order, hence replace
cpu_to_le32()bylower_32_bits(). 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]>
Nice fix! Also easier to understand.
Yours,
Linus Walleij