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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
On Thu, Dec 11, 2014 at 2:02 PM, Octavian Purdila
<[email protected]> 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 <[email protected]>
Patch applied for fixes.
Yours,
Linus Walleij
On Thu, Dec 11, 2014 at 2:02 PM, Octavian Purdila
<[email protected]> 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 <[email protected]>
Patch applied for fixes.
Yours,
Linus Walleij