[PATCH 0/3] DLN2 fixes related to suspend/resume

[PATCH 0/3] DLN2 fixes related to suspend/resume

[Octavian Purdila]

First patch in the series fixes a GPIO IRQ issues found during
suspend/resume testing, the next simplifies a bit the IRQ code and the
last adds support for suspend/resume to DLN2 to avoid a crash during
suspend caused by the fact that we cant unplug a GPIO controller while
it is in use.

As suggested by Johan, I have tested the suspend/resume on barebone
hardware, in addition to KVM USB pass-through and reset_resume routine
is not neccessary on barebone hardware. 

It looks like with KVM USB pass-through the emulated port is reseted
during suspend/resume regardless of the state of the physical port.

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

 drivers/gpio/gpio-dln2.c | 141 +++++++++++++++++++----------------------------
 drivers/mfd/dln2.c       |  41 +++++++++++++-
 2 files changed, 94 insertions(+), 88 deletions(-)

-- 
1.9.1

[PATCH 1/3] 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

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

[Linus Walleij]

On Thu, Dec 11, 2014 at 2:02 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>

Patch applied for fixes.

Yours,
Linus Walleij

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

[Linus Walleij]

On Wed, Dec 31, 2014 at 9:55 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Dec 11, 2014 at 2:02 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>
>
> Patch applied for fixes.

Bah now that I see there are several versions of the patch set
floating around and also MFD patches I don't quite understand
how acute this is or how it's to be applied.

- Are these regression fixes or nice to have for next kernel
 release?

- Are the GPIO patches independent of the MFD patch?

Yours,
Linus Walleij

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

[Octavian Purdila]

On Mon, Jan 5, 2015 at 6:55 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Dec 31, 2014 at 9:55 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Thu, Dec 11, 2014 at 2:02 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>
> >
> > Patch applied for fixes.
>
> Bah now that I see there are several versions of the patch set
> floating around and also MFD patches I don't quite understand
> how acute this is or how it's to be applied.

Hi Linus,

Oops I did not noticed you applied the first version. It should not
matter anyway since I did not make any modifications to the GPIO
patches in the second version - I just doubled checked it now.

>
> - Are these regression fixes or nice to have for next kernel
>  release?
>

The first patch is a fix. The second is more of a cleanup patch.

> - Are the GPIO patches independent of the MFD patch?
>

Yes, the GPIO patches are independent of the MFD patches.

[PATCH 2/3] 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

Re: [PATCH 2/3] gpio: dln2: use bus_sync_unlock instead of scheduling work

[Linus Walleij]

On Thu, Dec 11, 2014 at 2:02 PM, Octavian Purdila
<octavian.purdila@intel.com> wrote:

> 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>

Patch applied for fixes.

Yours,
Linus Walleij

Re: [PATCH 2/3] gpio: dln2: use bus_sync_unlock instead of scheduling work

[Octavian Purdila]

On Thu, Jan 1, 2015 at 9:56 AM, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Dec 11, 2014 at 2:02 PM, Octavian Purdila
> <octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>
>> 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
> Patch applied for fixes.
>

Hi Linus,

I don't see the patch applied, could you please consider it for the
-for-next branch?

Thanks,
Tavi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] gpio: dln2: use bus_sync_unlock instead of scheduling work

[Octavian Purdila]

On Thu, Jan 1, 2015 at 9:56 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Dec 11, 2014 at 2:02 PM, Octavian Purdila
> <octavian.purdila@intel.com> wrote:
>
>> 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>
>
> Patch applied for fixes.
>

Hi Linus,

I don't see the patch applied, could you please consider it for the
-for-next branch?

Thanks,
Tavi

Re: [PATCH 2/3] gpio: dln2: use bus_sync_unlock instead of scheduling work

[Linus Walleij]

On Fri, Jan 9, 2015 at 12:40 AM, Octavian Purdila
<octavian.purdila@intel.com> wrote:
> On Thu, Jan 1, 2015 at 9:56 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Thu, Dec 11, 2014 at 2:02 PM, Octavian Purdila
>> <octavian.purdila@intel.com> wrote:
>>
>>> 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>
>>
>> Patch applied for fixes.
>>
>
> Hi Linus,
>
> I don't see the patch applied, could you please consider it for the
> -for-next branch?

Ah sorry I thought only the first patch was a critical fix.
Applied this to fixes too now. Working on queueing the
GPIO fixes.

Yours,
Linus Walleij

Re: [PATCH 2/3] gpio: dln2: use bus_sync_unlock instead of scheduling work

[Octavian Purdila]

On Fri, Jan 9, 2015 at 7:58 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Jan 9, 2015 at 12:40 AM, Octavian Purdila
> <octavian.purdila@intel.com> wrote:
>> On Thu, Jan 1, 2015 at 9:56 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Thu, Dec 11, 2014 at 2:02 PM, Octavian Purdila
>>> <octavian.purdila@intel.com> wrote:
>>>
>>>> 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>
>>>
>>> Patch applied for fixes.
>>>
>>
>> Hi Linus,
>>
>> I don't see the patch applied, could you please consider it for the
>> -for-next branch?
>
> Ah sorry I thought only the first patch was a critical fix.
> Applied this to fixes too now. Working on queueing the
> GPIO fixes.

Hi Linus,

Its not a critical fix, its just a clean-up. I thought it would go in
the the for-next branch (for 3.20) and since I did not see it there I
thought it got lost.

Sorry for any confusion I may have caused, I am not familiar with the
GPIO branch work-flow.

[PATCH 3/3] 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 | 41 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
index 6d49685..08c403c 100644
--- a/drivers/mfd/dln2.c
+++ b/drivers/mfd/dln2.c
@@ -587,7 +587,6 @@ 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]);
 	}
@@ -665,9 +664,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 +694,16 @@ static void dln2_disconnect(struct usb_interface *interface)
 	/* wait for transfers to end */
 	wait_event(dln2->disconnect_wq, !dln2->active_transfers);
 
+	for (i = 0; i < DLN2_MAX_URBS; i++)
+		usb_kill_urb(dln2->rx_urb[i]);
+}
+
+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);
@@ -767,11 +775,38 @@ static const struct usb_device_id dln2_table[] = {
 
 MODULE_DEVICE_TABLE(usb, dln2_table);
 
+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);
+	int i;
+	int ret = 0;
+
+	dln2->disconnect = false;
+
+	for (i = 0; i < DLN2_MAX_URBS; i++) {
+		ret = usb_submit_urb(dln2->rx_urb[i], GFP_KERNEL);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
 static struct usb_driver dln2_driver = {
 	.name = "dln2",
 	.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 3/3] mfd: dln2: add suspend/resume functionality

[Johan Hovold]

On Thu, Dec 11, 2014 at 03:02:31PM +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>
> ---
>  drivers/mfd/dln2.c | 41 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mfd/dln2.c b/drivers/mfd/dln2.c
> index 6d49685..08c403c 100644
> --- a/drivers/mfd/dln2.c
> +++ b/drivers/mfd/dln2.c
> @@ -587,7 +587,6 @@ 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]);
>  	}

Now dln2_free will no longer stop the urbs before releasing them and
hence you can get a use after free in the error path of probe where
dln2_free is called after you have submitted the urbs.

Please be more careful.

Splitting allocation and submission (and reuse that helper in resume)
and adding a stop_rx_urbs helper might be a good idea.

> @@ -665,9 +664,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 +694,16 @@ static void dln2_disconnect(struct usb_interface *interface)
>  	/* wait for transfers to end */
>  	wait_event(dln2->disconnect_wq, !dln2->active_transfers);
>  
> +	for (i = 0; i < DLN2_MAX_URBS; i++)
> +		usb_kill_urb(dln2->rx_urb[i]);
> +}
> +
> +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);
> @@ -767,11 +775,38 @@ static const struct usb_device_id dln2_table[] = {
>  
>  MODULE_DEVICE_TABLE(usb, dln2_table);

I believe I already asked you to place the new callbacks above the id
table.

> +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);
> +	int i;
> +	int ret = 0;
> +
> +	dln2->disconnect = false;
> +
> +	for (i = 0; i < DLN2_MAX_URBS; i++) {
> +		ret = usb_submit_urb(dln2->rx_urb[i], GFP_KERNEL);

You cannot use GFP_KERNEL in resume, use GFP_NOIO.

> +		if (ret)

Add a dev_err here.

> +			break;
> +	}
> +
> +	return ret;
> +}

Johan