[PATCH v2 0/4] DLN2 fixes related to suspend/resume

[PATCH v2 0/4] DLN2 fixes related to suspend/resume

[Octavian Purdila]

This 2nd patch set addresses Johan review comments:

 * Fix an issue introduced by v1 where we can get a use after free in
   the error path of probe

 * Add start/stop RX URBs helpers

 * move the suspend/resume routines above the module device table

 * used GFP_NOIO in resume

Octavian Purdila (4):
  gpio: dln2: fix issue when an IRQ is unmasked then enabled
  gpio: dln2: use bus_sync_unlock instead of scheduling work
  mfd: dln2: add start/stop RX URBs helpers
  mfd: dln2: add suspend/resume functionality

 drivers/gpio/gpio-dln2.c | 141 +++++++++++++++++++----------------------------
 drivers/mfd/dln2.c       |  71 ++++++++++++++++++++----
 2 files changed, 117 insertions(+), 95 deletions(-)

-- 
1.9.1

[PATCH v2 1/4] gpio: dln2: fix issue when an IRQ is unmasked then enabled

[Octavian Purdila]

As noticed during suspend/resume operations, the IRQ can be unmasked
then disabled in suspend and eventually enabled in resume, but without
being unmasked.

The current implementation does not take into account interactions
between mask/unmask and enable/disable interrupts, and thus in the
above scenarios the IRQs remain unactive.

To fix this we removed the enable/disable operations as they fallback
to mask/unmask anyway.

We also remove the pending bitmaks as it is already done in irq_data
(i.e. IRQS_PENDING).

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/gpio/gpio-dln2.c | 53 +++++++-----------------------------------------
 1 file changed, 7 insertions(+), 46 deletions(-)

diff --git a/drivers/gpio/gpio-dln2.c b/drivers/gpio/gpio-dln2.c
index 978b51e..28a62c4 100644
--- a/drivers/gpio/gpio-dln2.c
+++ b/drivers/gpio/gpio-dln2.c
@@ -64,9 +64,8 @@ struct dln2_gpio {
 	 */
 	DECLARE_BITMAP(output_enabled, DLN2_GPIO_MAX_PINS);
 
-	DECLARE_BITMAP(irqs_masked, DLN2_GPIO_MAX_PINS);
-	DECLARE_BITMAP(irqs_enabled, DLN2_GPIO_MAX_PINS);
-	DECLARE_BITMAP(irqs_pending, DLN2_GPIO_MAX_PINS);
+	/* active IRQs - not synced to hardware */
+	DECLARE_BITMAP(unmasked_irqs, DLN2_GPIO_MAX_PINS);
 	struct dln2_irq_work *irq_work;
 };
 
@@ -303,29 +302,19 @@ static void dln2_irq_work(struct work_struct *w)
 	struct dln2_gpio *dln2 = iw->dln2;
 	u8 type = iw->type & DLN2_GPIO_EVENT_MASK;
 
-	if (test_bit(iw->pin, dln2->irqs_enabled))
+	if (test_bit(iw->pin, dln2->unmasked_irqs))
 		dln2_gpio_set_event_cfg(dln2, iw->pin, type, 0);
 	else
 		dln2_gpio_set_event_cfg(dln2, iw->pin, DLN2_GPIO_EVENT_NONE, 0);
 }
 
-static void dln2_irq_enable(struct irq_data *irqd)
-{
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
-	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
-	int pin = irqd_to_hwirq(irqd);
-
-	set_bit(pin, dln2->irqs_enabled);
-	schedule_work(&dln2->irq_work[pin].work);
-}
-
-static void dln2_irq_disable(struct irq_data *irqd)
+static void dln2_irq_unmask(struct irq_data *irqd)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
 	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
 	int pin = irqd_to_hwirq(irqd);
 
-	clear_bit(pin, dln2->irqs_enabled);
+	set_bit(pin, dln2->unmasked_irqs);
 	schedule_work(&dln2->irq_work[pin].work);
 }
 
@@ -335,27 +324,8 @@ static void dln2_irq_mask(struct irq_data *irqd)
 	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
 	int pin = irqd_to_hwirq(irqd);
 
-	set_bit(pin, dln2->irqs_masked);
-}
-
-static void dln2_irq_unmask(struct irq_data *irqd)
-{
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
-	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
-	struct device *dev = dln2->gpio.dev;
-	int pin = irqd_to_hwirq(irqd);
-
-	if (test_and_clear_bit(pin, dln2->irqs_pending)) {
-		int irq;
-
-		irq = irq_find_mapping(dln2->gpio.irqdomain, pin);
-		if (!irq) {
-			dev_err(dev, "pin %d not mapped to IRQ\n", pin);
-			return;
-		}
-
-		generic_handle_irq(irq);
-	}
+	clear_bit(pin, dln2->unmasked_irqs);
+	schedule_work(&dln2->irq_work[pin].work);
 }
 
 static int dln2_irq_set_type(struct irq_data *irqd, unsigned type)
@@ -389,8 +359,6 @@ static int dln2_irq_set_type(struct irq_data *irqd, unsigned type)
 
 static struct irq_chip dln2_gpio_irqchip = {
 	.name = "dln2-irq",
-	.irq_enable = dln2_irq_enable,
-	.irq_disable = dln2_irq_disable,
 	.irq_mask = dln2_irq_mask,
 	.irq_unmask = dln2_irq_unmask,
 	.irq_set_type = dln2_irq_set_type,
@@ -425,13 +393,6 @@ static void dln2_gpio_event(struct platform_device *pdev, u16 echo,
 		return;
 	}
 
-	if (!test_bit(pin, dln2->irqs_enabled))
-		return;
-	if (test_bit(pin, dln2->irqs_masked)) {
-		set_bit(pin, dln2->irqs_pending);
-		return;
-	}
-
 	switch (dln2->irq_work[pin].type) {
 	case DLN2_GPIO_EVENT_CHANGE_RISING:
 		if (event->value)
-- 
1.9.1

[PATCH v2 2/4] gpio: dln2: use bus_sync_unlock instead of scheduling work

[Octavian Purdila]

Use the irq_chip bus_sync_unlock method to update hardware registers
instead of scheduling work from the mask/unmask methods. This simplifies
a bit the driver and make it more uniform with the other GPIO IRQ
drivers.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/gpio/gpio-dln2.c | 92 +++++++++++++++++++++++++++---------------------
 1 file changed, 51 insertions(+), 41 deletions(-)

diff --git a/drivers/gpio/gpio-dln2.c b/drivers/gpio/gpio-dln2.c
index 28a62c4..2fdb138 100644
--- a/drivers/gpio/gpio-dln2.c
+++ b/drivers/gpio/gpio-dln2.c
@@ -47,13 +47,6 @@
 
 #define DLN2_GPIO_MAX_PINS 32
 
-struct dln2_irq_work {
-	struct work_struct work;
-	struct dln2_gpio *dln2;
-	int pin;
-	int type;
-};
-
 struct dln2_gpio {
 	struct platform_device *pdev;
 	struct gpio_chip gpio;
@@ -66,7 +59,10 @@ struct dln2_gpio {
 
 	/* active IRQs - not synced to hardware */
 	DECLARE_BITMAP(unmasked_irqs, DLN2_GPIO_MAX_PINS);
-	struct dln2_irq_work *irq_work;
+	/* active IRQS - synced to hardware */
+	DECLARE_BITMAP(enabled_irqs, DLN2_GPIO_MAX_PINS);
+	int irq_type[DLN2_GPIO_MAX_PINS];
+	struct mutex irq_lock;
 };
 
 struct dln2_gpio_pin {
@@ -296,18 +292,6 @@ static int dln2_gpio_set_event_cfg(struct dln2_gpio *dln2, unsigned pin,
 				&req, sizeof(req));
 }
 
-static void dln2_irq_work(struct work_struct *w)
-{
-	struct dln2_irq_work *iw = container_of(w, struct dln2_irq_work, work);
-	struct dln2_gpio *dln2 = iw->dln2;
-	u8 type = iw->type & DLN2_GPIO_EVENT_MASK;
-
-	if (test_bit(iw->pin, dln2->unmasked_irqs))
-		dln2_gpio_set_event_cfg(dln2, iw->pin, type, 0);
-	else
-		dln2_gpio_set_event_cfg(dln2, iw->pin, DLN2_GPIO_EVENT_NONE, 0);
-}
-
 static void dln2_irq_unmask(struct irq_data *irqd)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
@@ -315,7 +299,6 @@ static void dln2_irq_unmask(struct irq_data *irqd)
 	int pin = irqd_to_hwirq(irqd);
 
 	set_bit(pin, dln2->unmasked_irqs);
-	schedule_work(&dln2->irq_work[pin].work);
 }
 
 static void dln2_irq_mask(struct irq_data *irqd)
@@ -325,7 +308,6 @@ static void dln2_irq_mask(struct irq_data *irqd)
 	int pin = irqd_to_hwirq(irqd);
 
 	clear_bit(pin, dln2->unmasked_irqs);
-	schedule_work(&dln2->irq_work[pin].work);
 }
 
 static int dln2_irq_set_type(struct irq_data *irqd, unsigned type)
@@ -336,19 +318,19 @@ static int dln2_irq_set_type(struct irq_data *irqd, unsigned type)
 
 	switch (type) {
 	case IRQ_TYPE_LEVEL_HIGH:
-		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_LVL_HIGH;
+		dln2->irq_type[pin] = DLN2_GPIO_EVENT_LVL_HIGH;
 		break;
 	case IRQ_TYPE_LEVEL_LOW:
-		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_LVL_LOW;
+		dln2->irq_type[pin] = DLN2_GPIO_EVENT_LVL_LOW;
 		break;
 	case IRQ_TYPE_EDGE_BOTH:
-		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_CHANGE;
+		dln2->irq_type[pin] = DLN2_GPIO_EVENT_CHANGE;
 		break;
 	case IRQ_TYPE_EDGE_RISING:
-		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_CHANGE_RISING;
+		dln2->irq_type[pin] = DLN2_GPIO_EVENT_CHANGE_RISING;
 		break;
 	case IRQ_TYPE_EDGE_FALLING:
-		dln2->irq_work[pin].type = DLN2_GPIO_EVENT_CHANGE_FALLING;
+		dln2->irq_type[pin] = DLN2_GPIO_EVENT_CHANGE_FALLING;
 		break;
 	default:
 		return -EINVAL;
@@ -357,11 +339,50 @@ static int dln2_irq_set_type(struct irq_data *irqd, unsigned type)
 	return 0;
 }
 
+static void dln2_irq_bus_lock(struct irq_data *irqd)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
+
+	mutex_lock(&dln2->irq_lock);
+}
+
+static void dln2_irq_bus_unlock(struct irq_data *irqd)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct dln2_gpio *dln2 = container_of(gc, struct dln2_gpio, gpio);
+	int pin = irqd_to_hwirq(irqd);
+	int enabled, unmasked;
+	unsigned type;
+	int ret;
+
+	enabled = test_bit(pin, dln2->enabled_irqs);
+	unmasked = test_bit(pin, dln2->unmasked_irqs);
+
+	if (enabled != unmasked) {
+		if (unmasked) {
+			type = dln2->irq_type[pin] & DLN2_GPIO_EVENT_MASK;
+			set_bit(pin, dln2->enabled_irqs);
+		} else {
+			type = DLN2_GPIO_EVENT_NONE;
+			clear_bit(pin, dln2->enabled_irqs);
+		}
+
+		ret = dln2_gpio_set_event_cfg(dln2, pin, type, 0);
+		if (ret)
+			dev_err(dln2->gpio.dev, "failed to set event\n");
+	}
+
+	mutex_unlock(&dln2->irq_lock);
+}
+
 static struct irq_chip dln2_gpio_irqchip = {
 	.name = "dln2-irq",
 	.irq_mask = dln2_irq_mask,
 	.irq_unmask = dln2_irq_unmask,
 	.irq_set_type = dln2_irq_set_type,
+	.irq_bus_lock = dln2_irq_bus_lock,
+	.irq_bus_sync_unlock = dln2_irq_bus_unlock,
 };
 
 static void dln2_gpio_event(struct platform_device *pdev, u16 echo,
@@ -393,7 +414,7 @@ static void dln2_gpio_event(struct platform_device *pdev, u16 echo,
 		return;
 	}
 
-	switch (dln2->irq_work[pin].type) {
+	switch (dln2->irq_type[pin]) {
 	case DLN2_GPIO_EVENT_CHANGE_RISING:
 		if (event->value)
 			generic_handle_irq(irq);
@@ -412,7 +433,7 @@ static int dln2_gpio_probe(struct platform_device *pdev)
 	struct dln2_gpio *dln2;
 	struct device *dev = &pdev->dev;
 	int pins;
-	int i, ret;
+	int ret;
 
 	pins = dln2_gpio_get_pin_count(pdev);
 	if (pins < 0) {
@@ -428,15 +449,7 @@ static int dln2_gpio_probe(struct platform_device *pdev)
 	if (!dln2)
 		return -ENOMEM;
 
-	dln2->irq_work = devm_kcalloc(&pdev->dev, pins,
-				      sizeof(struct dln2_irq_work), GFP_KERNEL);
-	if (!dln2->irq_work)
-		return -ENOMEM;
-	for (i = 0; i < pins; i++) {
-		INIT_WORK(&dln2->irq_work[i].work, dln2_irq_work);
-		dln2->irq_work[i].pin = i;
-		dln2->irq_work[i].dln2 = dln2;
-	}
+	mutex_init(&dln2->irq_lock);
 
 	dln2->pdev = pdev;
 
@@ -490,11 +503,8 @@ out:
 static int dln2_gpio_remove(struct platform_device *pdev)
 {
 	struct dln2_gpio *dln2 = platform_get_drvdata(pdev);
-	int i;
 
 	dln2_unregister_event_cb(pdev, DLN2_GPIO_CONDITION_MET_EV);
-	for (i = 0; i < dln2->gpio.ngpio; i++)
-		flush_work(&dln2->irq_work[i].work);
 	gpiochip_remove(&dln2->gpio);
 
 	return 0;
-- 
1.9.1

[PATCH v2 3/4] mfd: dln2: add start/stop RX URBs helpers

[Octavian Purdila]

This is in preparation for adding suspend / resume support.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/mfd/dln2.c | 51 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 10 deletions(-)

diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
index 6d49685..75358d2 100644
--- a/drivers/mfd/dln2.c
+++ b/drivers/mfd/dln2.c
@@ -587,12 +587,19 @@ static void dln2_free_rx_urbs(struct dln2_dev *dln2)
 	int i;
 
 	for (i = 0; i < DLN2_MAX_URBS; i++) {
-		usb_kill_urb(dln2->rx_urb[i]);
 		usb_free_urb(dln2->rx_urb[i]);
 		kfree(dln2->rx_buf[i]);
 	}
 }
 
+static void dln2_stop_rx_urbs(struct dln2_dev *dln2)
+{
+	int i;
+
+	for (i = 0; i < DLN2_MAX_URBS; i++)
+		usb_kill_urb(dln2->rx_urb[i]);
+}
+
 static void dln2_free(struct dln2_dev *dln2)
 {
 	dln2_free_rx_urbs(dln2);
@@ -604,9 +611,7 @@ static int dln2_setup_rx_urbs(struct dln2_dev *dln2,
 			      struct usb_host_interface *hostif)
 {
 	int i;
-	int ret;
 	const int rx_max_size = DLN2_RX_BUF_SIZE;
-	struct device *dev = &dln2->interface->dev;
 
 	for (i = 0; i < DLN2_MAX_URBS; i++) {
 		dln2->rx_buf[i] = kmalloc(rx_max_size, GFP_KERNEL);
@@ -621,7 +626,19 @@ static int dln2_setup_rx_urbs(struct dln2_dev *dln2,
 				  usb_rcvbulkpipe(dln2->usb_dev, dln2->ep_in),
 				  dln2->rx_buf[i], rx_max_size, dln2_rx, dln2);
 
-		ret = usb_submit_urb(dln2->rx_urb[i], GFP_KERNEL);
+	}
+
+	return 0;
+}
+
+static int dln2_start_rx_urbs(struct dln2_dev *dln2, gfp_t gfp)
+{
+	struct device *dev = &dln2->interface->dev;
+	int ret;
+	int i;
+
+	for (i = 0; i < DLN2_MAX_URBS; i++) {
+		ret = usb_submit_urb(dln2->rx_urb[i], gfp);
 		if (ret < 0) {
 			dev_err(dev, "failed to submit RX URB: %d\n", ret);
 			return ret;
@@ -665,9 +682,8 @@ static const struct mfd_cell dln2_devs[] = {
 	},
 };
 
-static void dln2_disconnect(struct usb_interface *interface)
+static void dln2_stop(struct dln2_dev *dln2)
 {
-	struct dln2_dev *dln2 = usb_get_intfdata(interface);
 	int i, j;
 
 	/* don't allow starting new transfers */
@@ -696,6 +712,14 @@ static void dln2_disconnect(struct usb_interface *interface)
 	/* wait for transfers to end */
 	wait_event(dln2->disconnect_wq, !dln2->active_transfers);
 
+	dln2_stop_rx_urbs(dln2);
+}
+static void dln2_disconnect(struct usb_interface *interface)
+{
+	struct dln2_dev *dln2 = usb_get_intfdata(interface);
+
+	dln2_stop(dln2);
+
 	mfd_remove_devices(&interface->dev);
 
 	dln2_free(dln2);
@@ -738,23 +762,30 @@ static int dln2_probe(struct usb_interface *interface,
 
 	ret = dln2_setup_rx_urbs(dln2, hostif);
 	if (ret)
-		goto out_cleanup;
+		goto out_free;
+
+	ret = dln2_start_rx_urbs(dln2, GFP_KERNEL);
+	if (ret)
+		goto out_stop_rx;
 
 	ret = dln2_hw_init(dln2);
 	if (ret < 0) {
 		dev_err(dev, "failed to initialize hardware\n");
-		goto out_cleanup;
+		goto out_stop_rx;
 	}
 
 	ret = mfd_add_hotplug_devices(dev, dln2_devs, ARRAY_SIZE(dln2_devs));
 	if (ret != 0) {
 		dev_err(dev, "failed to add mfd devices to core\n");
-		goto out_cleanup;
+		goto out_stop_rx;
 	}
 
 	return 0;
 
-out_cleanup:
+out_stop_rx:
+	dln2_stop_rx_urbs(dln2);
+
+out_free:
 	dln2_free(dln2);
 
 	return ret;
-- 
1.9.1

[PATCH v2 4/4] mfd: dln2: add suspend/resume functionality

[Octavian Purdila]

Without suspend/resume functionality in the USB driver the USB core
will disconnect and reconnect the DLN2 port and because the GPIO
framework does not yet support removal of an in-use controller a
suspend/resume operation will result in a crash.

This patch provides suspend and resume functions for the DLN2 driver
so that the above scenario is avoided.

Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
---
 drivers/mfd/dln2.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
index 75358d2..f9c4a0b 100644
--- a/drivers/mfd/dln2.c
+++ b/drivers/mfd/dln2.c
@@ -791,6 +791,24 @@ out_free:
 	return ret;
 }
 
+static int dln2_suspend(struct usb_interface *iface, pm_message_t message)
+{
+	struct dln2_dev *dln2 = usb_get_intfdata(iface);
+
+	dln2_stop(dln2);
+
+	return 0;
+}
+
+static int dln2_resume(struct usb_interface *iface)
+{
+	struct dln2_dev *dln2 = usb_get_intfdata(iface);
+
+	dln2->disconnect = false;
+
+	return dln2_start_rx_urbs(dln2, GFP_NOIO);
+}
+
 static const struct usb_device_id dln2_table[] = {
 	{ USB_DEVICE(0xa257, 0x2013) },
 	{ }
@@ -803,6 +821,8 @@ static struct usb_driver dln2_driver = {
 	.probe = dln2_probe,
 	.disconnect = dln2_disconnect,
 	.id_table = dln2_table,
+	.suspend = dln2_suspend,
+	.resume = dln2_resume,
 };
 
 module_usb_driver(dln2_driver);
-- 
1.9.1

Re: [PATCH v2 1/4] gpio: dln2: fix issue when an IRQ is unmasked then enabled

[Alexandre Courbot]

On Wed, Dec 17, 2014 at 12:57 AM, Octavian Purdila
<octavian.purdila@intel.com> wrote:
> As noticed during suspend/resume operations, the IRQ can be unmasked
> then disabled in suspend and eventually enabled in resume, but without
> being unmasked.
>
> The current implementation does not take into account interactions
> between mask/unmask and enable/disable interrupts, and thus in the
> above scenarios the IRQs remain unactive.
>
> To fix this we removed the enable/disable operations as they fallback
> to mask/unmask anyway.
>
> We also remove the pending bitmaks as it is already done in irq_data
> (i.e. IRQS_PENDING).
>
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> ---
>  drivers/gpio/gpio-dln2.c | 53 +++++++-----------------------------------------
>  1 file changed, 7 insertions(+), 46 deletions(-)

Yum, less code!

Acked-by: Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH v2 1/4] gpio: dln2: fix issue when an IRQ is unmasked then enabled

[Linus Walleij]

On Tue, Dec 16, 2014 at 4:57 PM, Octavian Purdila
<octavian.purdila@intel.com> wrote:

> As noticed during suspend/resume operations, the IRQ can be unmasked
> then disabled in suspend and eventually enabled in resume, but without
> being unmasked.
>
> The current implementation does not take into account interactions
> between mask/unmask and enable/disable interrupts, and thus in the
> above scenarios the IRQs remain unactive.
>
> To fix this we removed the enable/disable operations as they fallback
> to mask/unmask anyway.
>
> We also remove the pending bitmaks as it is already done in irq_data
> (i.e. IRQS_PENDING).
>
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>

This patch applied for fixes.

Yours,
Linus Walleij

Re: [PATCH v2 3/4] mfd: dln2: add start/stop RX URBs helpers

[Johan Hovold]

On Tue, Dec 16, 2014 at 05:57:14PM +0200, Octavian Purdila wrote:
> This is in preparation for adding suspend / resume support.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> ---
>  drivers/mfd/dln2.c | 51 +++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 41 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> index 6d49685..75358d2 100644
> --- a/drivers/mfd/dln2.c
> +++ b/drivers/mfd/dln2.c
> @@ -587,12 +587,19 @@ static void dln2_free_rx_urbs(struct dln2_dev *dln2)
>  	int i;
>  
>  	for (i = 0; i < DLN2_MAX_URBS; i++) {
> -		usb_kill_urb(dln2->rx_urb[i]);
>  		usb_free_urb(dln2->rx_urb[i]);
>  		kfree(dln2->rx_buf[i]);
>  	}
>  }
>  
> +static void dln2_stop_rx_urbs(struct dln2_dev *dln2)
> +{
> +	int i;
> +
> +	for (i = 0; i < DLN2_MAX_URBS; i++)
> +		usb_kill_urb(dln2->rx_urb[i]);
> +}
> +
>  static void dln2_free(struct dln2_dev *dln2)
>  {
>  	dln2_free_rx_urbs(dln2);
> @@ -604,9 +611,7 @@ static int dln2_setup_rx_urbs(struct dln2_dev *dln2,
>  			      struct usb_host_interface *hostif)
>  {
>  	int i;
> -	int ret;
>  	const int rx_max_size = DLN2_RX_BUF_SIZE;
> -	struct device *dev = &dln2->interface->dev;
>  
>  	for (i = 0; i < DLN2_MAX_URBS; i++) {
>  		dln2->rx_buf[i] = kmalloc(rx_max_size, GFP_KERNEL);
> @@ -621,7 +626,19 @@ static int dln2_setup_rx_urbs(struct dln2_dev *dln2,
>  				  usb_rcvbulkpipe(dln2->usb_dev, dln2->ep_in),
>  				  dln2->rx_buf[i], rx_max_size, dln2_rx, dln2);
>  

You should remove this stray newline as well. 

> -		ret = usb_submit_urb(dln2->rx_urb[i], GFP_KERNEL);
> +	}
> +
> +	return 0;
> +}

Otherwise,

Reviewed-by: Johan Hovold <johan@kernel.org>

Johan

Re: [PATCH v2 4/4] mfd: dln2: add suspend/resume functionality

[Johan Hovold]

On Tue, Dec 16, 2014 at 05:57:15PM +0200, Octavian Purdila wrote:
> Without suspend/resume functionality in the USB driver the USB core
> will disconnect and reconnect the DLN2 port and because the GPIO
> framework does not yet support removal of an in-use controller a
> suspend/resume operation will result in a crash.
> 
> This patch provides suspend and resume functions for the DLN2 driver
> so that the above scenario is avoided.
> 
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>

This patch looks good now, but how did you say this device was powered?

If powered by vbus you cannot assume that the device maintains it's
state over a suspend cycle, something which would complicate matters
quite a bit...

Thanks,
Johan

Re: [PATCH v2 4/4] mfd: dln2: add suspend/resume functionality

[Octavian Purdila]

On Wed, Jan 7, 2015 at 11:26 PM, Johan Hovold <johan@kernel.org> wrote:
> On Tue, Dec 16, 2014 at 05:57:15PM +0200, Octavian Purdila wrote:
>> Without suspend/resume functionality in the USB driver the USB core
>> will disconnect and reconnect the DLN2 port and because the GPIO
>> framework does not yet support removal of an in-use controller a
>> suspend/resume operation will result in a crash.
>>
>> This patch provides suspend and resume functions for the DLN2 driver
>> so that the above scenario is avoided.
>>
>> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
>
> This patch looks good now, but how did you say this device was powered?
>
> If powered by vbus you cannot assume that the device maintains it's
> state over a suspend cycle, something which would complicate matters
> quite a bit...
>

Yes, the device is powered by VBUS. During my tests, depending on the
host and USB port, VBUS is sometimes preserved - and this is the case
this patch addresses, and sometimes is not, but in that case because
no reset_resume routine is implemented the resume path will go through
the disconnect/reconnect process. This second case is not addressed by
the patch, as I think in this second case fixing the GPIO framework to
support the removal of an in-use device is the best way to go.

Re: [PATCH v2 4/4] mfd: dln2: add suspend/resume functionality

[Johan Hovold]

On Thu, Jan 08, 2015 at 02:16:15AM +1300, Octavian Purdila wrote:
> On Wed, Jan 7, 2015 at 11:26 PM, Johan Hovold <johan@kernel.org> wrote:
> > On Tue, Dec 16, 2014 at 05:57:15PM +0200, Octavian Purdila wrote:
> >> Without suspend/resume functionality in the USB driver the USB core
> >> will disconnect and reconnect the DLN2 port and because the GPIO
> >> framework does not yet support removal of an in-use controller a
> >> suspend/resume operation will result in a crash.
> >>
> >> This patch provides suspend and resume functions for the DLN2 driver
> >> so that the above scenario is avoided.
> >>
> >> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> >
> > This patch looks good now, but how did you say this device was powered?
> >
> > If powered by vbus you cannot assume that the device maintains it's
> > state over a suspend cycle, something which would complicate matters
> > quite a bit...
> 
> Yes, the device is powered by VBUS. During my tests, depending on the
> host and USB port, VBUS is sometimes preserved - and this is the case
> this patch addresses, and sometimes is not, but in that case because
> no reset_resume routine is implemented the resume path will go through
> the disconnect/reconnect process.

Yes, it depends on the host controller and I believe the common case is
to drop VBUS.

> This second case is not addressed by
> the patch, as I think in this second case fixing the GPIO framework to
> support the removal of an in-use device is the best way to go.

I agree.

But please do mention that this case is not handled in the commit
message.

Thanks,
Johan

Re: [PATCH v2 3/4] mfd: dln2: add start/stop RX URBs helpers

[Lee Jones]

On Tue, 16 Dec 2014, Octavian Purdila wrote:

> This is in preparation for adding suspend / resume support.

Please re-submit this set with the Acks you have received.

Also draft a cover-letter with the current status of set, what you
need next etc.

> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
> ---
>  drivers/mfd/dln2.c | 51 +++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 41 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> index 6d49685..75358d2 100644
> --- a/drivers/mfd/dln2.c
> +++ b/drivers/mfd/dln2.c
> @@ -587,12 +587,19 @@ static void dln2_free_rx_urbs(struct dln2_dev *dln2)
>  	int i;
>  
>  	for (i = 0; i < DLN2_MAX_URBS; i++) {
> -		usb_kill_urb(dln2->rx_urb[i]);
>  		usb_free_urb(dln2->rx_urb[i]);
>  		kfree(dln2->rx_buf[i]);
>  	}
>  }
>  
> +static void dln2_stop_rx_urbs(struct dln2_dev *dln2)
> +{
> +	int i;
> +
> +	for (i = 0; i < DLN2_MAX_URBS; i++)
> +		usb_kill_urb(dln2->rx_urb[i]);
> +}
> +
>  static void dln2_free(struct dln2_dev *dln2)
>  {
>  	dln2_free_rx_urbs(dln2);
> @@ -604,9 +611,7 @@ static int dln2_setup_rx_urbs(struct dln2_dev *dln2,
>  			      struct usb_host_interface *hostif)
>  {
>  	int i;
> -	int ret;
>  	const int rx_max_size = DLN2_RX_BUF_SIZE;
> -	struct device *dev = &dln2->interface->dev;
>  
>  	for (i = 0; i < DLN2_MAX_URBS; i++) {
>  		dln2->rx_buf[i] = kmalloc(rx_max_size, GFP_KERNEL);
> @@ -621,7 +626,19 @@ static int dln2_setup_rx_urbs(struct dln2_dev *dln2,
>  				  usb_rcvbulkpipe(dln2->usb_dev, dln2->ep_in),
>  				  dln2->rx_buf[i], rx_max_size, dln2_rx, dln2);
>  
> -		ret = usb_submit_urb(dln2->rx_urb[i], GFP_KERNEL);
> +	}
> +
> +	return 0;
> +}
> +
> +static int dln2_start_rx_urbs(struct dln2_dev *dln2, gfp_t gfp)
> +{
> +	struct device *dev = &dln2->interface->dev;
> +	int ret;
> +	int i;
> +
> +	for (i = 0; i < DLN2_MAX_URBS; i++) {
> +		ret = usb_submit_urb(dln2->rx_urb[i], gfp);
>  		if (ret < 0) {
>  			dev_err(dev, "failed to submit RX URB: %d\n", ret);
>  			return ret;
> @@ -665,9 +682,8 @@ static const struct mfd_cell dln2_devs[] = {
>  	},
>  };
>  
> -static void dln2_disconnect(struct usb_interface *interface)
> +static void dln2_stop(struct dln2_dev *dln2)
>  {
> -	struct dln2_dev *dln2 = usb_get_intfdata(interface);
>  	int i, j;
>  
>  	/* don't allow starting new transfers */
> @@ -696,6 +712,14 @@ static void dln2_disconnect(struct usb_interface *interface)
>  	/* wait for transfers to end */
>  	wait_event(dln2->disconnect_wq, !dln2->active_transfers);
>  
> +	dln2_stop_rx_urbs(dln2);
> +}
> +static void dln2_disconnect(struct usb_interface *interface)
> +{
> +	struct dln2_dev *dln2 = usb_get_intfdata(interface);
> +
> +	dln2_stop(dln2);
> +
>  	mfd_remove_devices(&interface->dev);
>  
>  	dln2_free(dln2);
> @@ -738,23 +762,30 @@ static int dln2_probe(struct usb_interface *interface,
>  
>  	ret = dln2_setup_rx_urbs(dln2, hostif);
>  	if (ret)
> -		goto out_cleanup;
> +		goto out_free;
> +
> +	ret = dln2_start_rx_urbs(dln2, GFP_KERNEL);
> +	if (ret)
> +		goto out_stop_rx;
>  
>  	ret = dln2_hw_init(dln2);
>  	if (ret < 0) {
>  		dev_err(dev, "failed to initialize hardware\n");
> -		goto out_cleanup;
> +		goto out_stop_rx;
>  	}
>  
>  	ret = mfd_add_hotplug_devices(dev, dln2_devs, ARRAY_SIZE(dln2_devs));
>  	if (ret != 0) {
>  		dev_err(dev, "failed to add mfd devices to core\n");
> -		goto out_cleanup;
> +		goto out_stop_rx;
>  	}
>  
>  	return 0;
>  
> -out_cleanup:
> +out_stop_rx:
> +	dln2_stop_rx_urbs(dln2);
> +
> +out_free:
>  	dln2_free(dln2);
>  
>  	return ret;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog