* [PATCH 1/1] pinctrl: baytrail: Add spinlock usage to all read/write access
@ 2017-01-26 14:14 Alexander Stein
2017-01-26 20:52 ` Mika Westerberg
0 siblings, 1 reply; 5+ messages in thread
From: Alexander Stein @ 2017-01-26 14:14 UTC (permalink / raw)
To: Mika Westerberg, Heikki Krogerus, Linus Walleij
Cc: Alexander Stein, linux-gpio
According to VLI64 Intel Atom E3800 Specification Update (#329901)
concurrent read accesses may result in returning 0xffffffff and write
accesses may be dropped silently.
To workaround all accesses must be protected by locks.
Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
I actually had the case where the read access in byt_irq_unmask returned
0xffffffff. After OR'ing the trigger bits and writing 0xffffffff back to
BYT_CONF0_REG things started to act strange.
drivers/pinctrl/intel/pinctrl-baytrail.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index 6cce314..7294c88 100644
--- a/drivers/pinctrl/intel/pinctrl-baytrail.c
+++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
@@ -1594,6 +1594,7 @@ static void byt_gpio_irq_handler(struct irq_desc *desc)
void __iomem *reg;
unsigned long pending;
unsigned int virq;
+ unsigned long flags;
/* check from GPIO controller which pin triggered the interrupt */
for (base = 0; base < vg->chip.ngpio; base += 32) {
@@ -1606,7 +1607,9 @@ static void byt_gpio_irq_handler(struct irq_desc *desc)
continue;
}
+ raw_spin_lock_irqsave(&vg->lock, flags);
pending = readl(reg);
+ raw_spin_unlock_irqrestore(&vg->lock, flags);
for_each_set_bit(pin, &pending, 32) {
virq = irq_find_mapping(vg->chip.irqdomain, base + pin);
generic_handle_irq(virq);
@@ -1620,6 +1623,7 @@ static void byt_gpio_irq_init_hw(struct byt_gpio *vg)
void __iomem *reg;
u32 base, value;
int i;
+ unsigned long flags;
/*
* Clear interrupt triggers for all pins that are GPIOs and
@@ -1637,7 +1641,9 @@ static void byt_gpio_irq_init_hw(struct byt_gpio *vg)
continue;
}
+ raw_spin_lock_irqsave(&vg->lock, flags);
value = readl(reg);
+ raw_spin_unlock_irqrestore(&vg->lock, flags);
if ((value & BYT_PIN_MUX) == byt_get_gpio_mux(vg, i) &&
!(value & BYT_DIRECT_IRQ_EN)) {
byt_gpio_clear_triggering(vg, i);
@@ -1656,10 +1662,12 @@ static void byt_gpio_irq_init_hw(struct byt_gpio *vg)
continue;
}
+ raw_spin_lock_irqsave(&vg->lock, flags);
writel(0xffffffff, reg);
/* make sure trigger bits are cleared, if not then a pin
might be misconfigured in bios */
value = readl(reg);
+ raw_spin_unlock_irqrestore(&vg->lock, flags);
if (value)
dev_err(&vg->pdev->dev,
"GPIO interrupt error, pins misconfigured\n");
@@ -1828,7 +1836,9 @@ static int byt_gpio_suspend(struct device *dev)
struct platform_device *pdev = to_platform_device(dev);
struct byt_gpio *vg = platform_get_drvdata(pdev);
int i;
+ unsigned long flags;
+ raw_spin_lock_irqsave(&vg->lock, flags);
for (i = 0; i < vg->soc_data->npins; i++) {
void __iomem *reg;
u32 value;
@@ -1848,6 +1858,7 @@ static int byt_gpio_suspend(struct device *dev)
value = readl(reg) & BYT_VAL_RESTORE_MASK;
vg->saved_context[i].val = value;
}
+ raw_spin_unlock_irqrestore(&vg->lock, flags);
return 0;
}
@@ -1857,7 +1868,9 @@ static int byt_gpio_resume(struct device *dev)
struct platform_device *pdev = to_platform_device(dev);
struct byt_gpio *vg = platform_get_drvdata(pdev);
int i;
+ unsigned long flags;
+ raw_spin_lock_irqsave(&vg->lock, flags);
for (i = 0; i < vg->soc_data->npins; i++) {
void __iomem *reg;
u32 value;
@@ -1894,6 +1907,7 @@ static int byt_gpio_resume(struct device *dev)
}
}
}
+ raw_spin_unlock_irqrestore(&vg->lock, flags);
return 0;
}
--
2.10.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 1/1] pinctrl: baytrail: Add spinlock usage to all read/write access
2017-01-26 14:14 [PATCH 1/1] pinctrl: baytrail: Add spinlock usage to all read/write access Alexander Stein
@ 2017-01-26 20:52 ` Mika Westerberg
2017-01-27 9:26 ` Mika Westerberg
2017-01-30 6:50 ` Alexander Stein
0 siblings, 2 replies; 5+ messages in thread
From: Mika Westerberg @ 2017-01-26 20:52 UTC (permalink / raw)
To: Alexander Stein; +Cc: Heikki Krogerus, Linus Walleij, linux-gpio
On Thu, Jan 26, 2017 at 03:14:18PM +0100, Alexander Stein wrote:
> According to VLI64 Intel Atom E3800 Specification Update (#329901)
> concurrent read accesses may result in returning 0xffffffff and write
> accesses may be dropped silently.
> To workaround all accesses must be protected by locks.
>
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
> I actually had the case where the read access in byt_irq_unmask returned
> 0xffffffff. After OR'ing the trigger bits and writing 0xffffffff back to
> BYT_CONF0_REG things started to act strange.
>
> drivers/pinctrl/intel/pinctrl-baytrail.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
> index 6cce314..7294c88 100644
> --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
> @@ -1594,6 +1594,7 @@ static void byt_gpio_irq_handler(struct irq_desc *desc)
> void __iomem *reg;
> unsigned long pending;
> unsigned int virq;
> + unsigned long flags;
Can you move this variable after "pending" like:
unsigned long pending;
unsigned long flags;
unsigned int virq;
>
> /* check from GPIO controller which pin triggered the interrupt */
> for (base = 0; base < vg->chip.ngpio; base += 32) {
> @@ -1606,7 +1607,9 @@ static void byt_gpio_irq_handler(struct irq_desc *desc)
> continue;
> }
>
> + raw_spin_lock_irqsave(&vg->lock, flags);
> pending = readl(reg);
> + raw_spin_unlock_irqrestore(&vg->lock, flags);
> for_each_set_bit(pin, &pending, 32) {
> virq = irq_find_mapping(vg->chip.irqdomain, base + pin);
> generic_handle_irq(virq);
> @@ -1620,6 +1623,7 @@ static void byt_gpio_irq_init_hw(struct byt_gpio *vg)
> void __iomem *reg;
> u32 base, value;
> int i;
> + unsigned long flags;
Here also arrange it like
unsigned long flags;
void __iomem *reg;
u32 base, value;
int i;
>
> /*
> * Clear interrupt triggers for all pins that are GPIOs and
> @@ -1637,7 +1641,9 @@ static void byt_gpio_irq_init_hw(struct byt_gpio *vg)
> continue;
> }
>
> + raw_spin_lock_irqsave(&vg->lock, flags);
Is this really necessary as we are initializing the driver?
> value = readl(reg);
> + raw_spin_unlock_irqrestore(&vg->lock, flags);
> if ((value & BYT_PIN_MUX) == byt_get_gpio_mux(vg, i) &&
> !(value & BYT_DIRECT_IRQ_EN)) {
> byt_gpio_clear_triggering(vg, i);
> @@ -1656,10 +1662,12 @@ static void byt_gpio_irq_init_hw(struct byt_gpio *vg)
> continue;
> }
>
> + raw_spin_lock_irqsave(&vg->lock, flags);
> writel(0xffffffff, reg);
> /* make sure trigger bits are cleared, if not then a pin
> might be misconfigured in bios */
> value = readl(reg);
> + raw_spin_unlock_irqrestore(&vg->lock, flags);
> if (value)
> dev_err(&vg->pdev->dev,
> "GPIO interrupt error, pins misconfigured\n");
> @@ -1828,7 +1836,9 @@ static int byt_gpio_suspend(struct device *dev)
> struct platform_device *pdev = to_platform_device(dev);
> struct byt_gpio *vg = platform_get_drvdata(pdev);
> int i;
> + unsigned long flags;
Here also, please make it look like:
unsigned long flags;
int i;
> + raw_spin_lock_irqsave(&vg->lock, flags);
> for (i = 0; i < vg->soc_data->npins; i++) {
> void __iomem *reg;
> u32 value;
> @@ -1848,6 +1858,7 @@ static int byt_gpio_suspend(struct device *dev)
> value = readl(reg) & BYT_VAL_RESTORE_MASK;
> vg->saved_context[i].val = value;
> }
> + raw_spin_unlock_irqrestore(&vg->lock, flags);
>
> return 0;
> }
> @@ -1857,7 +1868,9 @@ static int byt_gpio_resume(struct device *dev)
> struct platform_device *pdev = to_platform_device(dev);
> struct byt_gpio *vg = platform_get_drvdata(pdev);
> int i;
> + unsigned long flags;
>
> + raw_spin_lock_irqsave(&vg->lock, flags);
> for (i = 0; i < vg->soc_data->npins; i++) {
> void __iomem *reg;
> u32 value;
> @@ -1894,6 +1907,7 @@ static int byt_gpio_resume(struct device *dev)
> }
> }
> }
> + raw_spin_unlock_irqrestore(&vg->lock, flags);
>
> return 0;
> }
> --
> 2.10.2
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/1] pinctrl: baytrail: Add spinlock usage to all read/write access
2017-01-26 20:52 ` Mika Westerberg
@ 2017-01-27 9:26 ` Mika Westerberg
2017-01-30 6:50 ` Alexander Stein
1 sibling, 0 replies; 5+ messages in thread
From: Mika Westerberg @ 2017-01-27 9:26 UTC (permalink / raw)
To: Alexander Stein; +Cc: Heikki Krogerus, Linus Walleij, linux-gpio
On Thu, Jan 26, 2017 at 10:52:19PM +0200, Mika Westerberg wrote:
> On Thu, Jan 26, 2017 at 03:14:18PM +0100, Alexander Stein wrote:
> > According to VLI64 Intel Atom E3800 Specification Update (#329901)
> > concurrent read accesses may result in returning 0xffffffff and write
> > accesses may be dropped silently.
> > To workaround all accesses must be protected by locks.
> >
> > Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> > ---
> > I actually had the case where the read access in byt_irq_unmask returned
> > 0xffffffff. After OR'ing the trigger bits and writing 0xffffffff back to
> > BYT_CONF0_REG things started to act strange.
> >
> > drivers/pinctrl/intel/pinctrl-baytrail.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
> > index 6cce314..7294c88 100644
> > --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
> > +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
> > @@ -1594,6 +1594,7 @@ static void byt_gpio_irq_handler(struct irq_desc *desc)
> > void __iomem *reg;
> > unsigned long pending;
> > unsigned int virq;
> > + unsigned long flags;
>
> Can you move this variable after "pending" like:
>
> unsigned long pending;
> unsigned long flags;
> unsigned int virq;
>
> >
> > /* check from GPIO controller which pin triggered the interrupt */
> > for (base = 0; base < vg->chip.ngpio; base += 32) {
> > @@ -1606,7 +1607,9 @@ static void byt_gpio_irq_handler(struct irq_desc *desc)
> > continue;
> > }
> >
> > + raw_spin_lock_irqsave(&vg->lock, flags);
Oh, and you don't need _irqsave() here as we already have interrupts
disabled here.
> > pending = readl(reg);
> > + raw_spin_unlock_irqrestore(&vg->lock, flags);
> > for_each_set_bit(pin, &pending, 32) {
> > virq = irq_find_mapping(vg->chip.irqdomain, base + pin);
> > generic_handle_irq(virq);
> > @@ -1620,6 +1623,7 @@ static void byt_gpio_irq_init_hw(struct byt_gpio *vg)
> > void __iomem *reg;
> > u32 base, value;
> > int i;
> > + unsigned long flags;
>
> Here also arrange it like
>
> unsigned long flags;
> void __iomem *reg;
> u32 base, value;
> int i;
>
> >
> > /*
> > * Clear interrupt triggers for all pins that are GPIOs and
> > @@ -1637,7 +1641,9 @@ static void byt_gpio_irq_init_hw(struct byt_gpio *vg)
> > continue;
> > }
> >
> > + raw_spin_lock_irqsave(&vg->lock, flags);
>
> Is this really necessary as we are initializing the driver?
>
> > value = readl(reg);
> > + raw_spin_unlock_irqrestore(&vg->lock, flags);
> > if ((value & BYT_PIN_MUX) == byt_get_gpio_mux(vg, i) &&
> > !(value & BYT_DIRECT_IRQ_EN)) {
> > byt_gpio_clear_triggering(vg, i);
> > @@ -1656,10 +1662,12 @@ static void byt_gpio_irq_init_hw(struct byt_gpio *vg)
> > continue;
> > }
> >
> > + raw_spin_lock_irqsave(&vg->lock, flags);
> > writel(0xffffffff, reg);
> > /* make sure trigger bits are cleared, if not then a pin
> > might be misconfigured in bios */
> > value = readl(reg);
> > + raw_spin_unlock_irqrestore(&vg->lock, flags);
> > if (value)
> > dev_err(&vg->pdev->dev,
> > "GPIO interrupt error, pins misconfigured\n");
> > @@ -1828,7 +1836,9 @@ static int byt_gpio_suspend(struct device *dev)
> > struct platform_device *pdev = to_platform_device(dev);
> > struct byt_gpio *vg = platform_get_drvdata(pdev);
> > int i;
> > + unsigned long flags;
>
> Here also, please make it look like:
>
> unsigned long flags;
> int i;
>
> > + raw_spin_lock_irqsave(&vg->lock, flags);
> > for (i = 0; i < vg->soc_data->npins; i++) {
> > void __iomem *reg;
> > u32 value;
> > @@ -1848,6 +1858,7 @@ static int byt_gpio_suspend(struct device *dev)
> > value = readl(reg) & BYT_VAL_RESTORE_MASK;
> > vg->saved_context[i].val = value;
> > }
> > + raw_spin_unlock_irqrestore(&vg->lock, flags);
For the suspend/resume parts I'm also not sure if it is possible to have
concurrent register access here. The userspace is already frozen and
this uses late pm ops meaning that most normal drivers (who could be
using GPIOs are already suspended as well).
> >
> > return 0;
> > }
> > @@ -1857,7 +1868,9 @@ static int byt_gpio_resume(struct device *dev)
> > struct platform_device *pdev = to_platform_device(dev);
> > struct byt_gpio *vg = platform_get_drvdata(pdev);
> > int i;
> > + unsigned long flags;
> >
> > + raw_spin_lock_irqsave(&vg->lock, flags);
> > for (i = 0; i < vg->soc_data->npins; i++) {
> > void __iomem *reg;
> > u32 value;
> > @@ -1894,6 +1907,7 @@ static int byt_gpio_resume(struct device *dev)
> > }
> > }
> > }
> > + raw_spin_unlock_irqrestore(&vg->lock, flags);
> >
> > return 0;
> > }
> > --
> > 2.10.2
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/1] pinctrl: baytrail: Add spinlock usage to all read/write access
2017-01-26 20:52 ` Mika Westerberg
2017-01-27 9:26 ` Mika Westerberg
@ 2017-01-30 6:50 ` Alexander Stein
2017-01-30 9:26 ` Mika Westerberg
1 sibling, 1 reply; 5+ messages in thread
From: Alexander Stein @ 2017-01-30 6:50 UTC (permalink / raw)
To: Mika Westerberg; +Cc: Heikki Krogerus, Linus Walleij, linux-gpio
Hi,
On Thursday 26 January 2017 22:52:19, Mika Westerberg wrote:
> On Thu, Jan 26, 2017 at 03:14:18PM +0100, Alexander Stein wrote:
> > According to VLI64 Intel Atom E3800 Specification Update (#329901)
> > concurrent read accesses may result in returning 0xffffffff and write
> > accesses may be dropped silently.
> > To workaround all accesses must be protected by locks.
> >
> > Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> > ---
> > I actually had the case where the read access in byt_irq_unmask returned
> > 0xffffffff. After OR'ing the trigger bits and writing 0xffffffff back to
> > BYT_CONF0_REG things started to act strange.
> >
> > drivers/pinctrl/intel/pinctrl-baytrail.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c
> > b/drivers/pinctrl/intel/pinctrl-baytrail.c index 6cce314..7294c88 100644
> > --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
> > +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
> > @@ -1594,6 +1594,7 @@ static void byt_gpio_irq_handler(struct irq_desc
> > *desc)>
> > void __iomem *reg;
> > unsigned long pending;
> > unsigned int virq;
> >
> > + unsigned long flags;
>
> Can you move this variable after "pending" like:
>
> unsigned long pending;
> unsigned long flags;
> unsigned int virq;
Sure, I understand why both unsigned longs are put together here...
> > /* check from GPIO controller which pin triggered the interrupt */
> > for (base = 0; base < vg->chip.ngpio; base += 32) {
> >
> [...]
> > @@ -1620,6 +1623,7 @@ static void byt_gpio_irq_init_hw(struct byt_gpio
> > *vg)
> >
> > void __iomem *reg;
> > u32 base, value;
> > int i;
> >
> > + unsigned long flags;
>
> Here also arrange it like
>
> unsigned long flags;
> void __iomem *reg;
> u32 base, value;
> int i;
but here I'm not so sure. What are the rules for variable declaration order?
> > /*
> >
> > * Clear interrupt triggers for all pins that are GPIOs and
> >
> > @@ -1637,7 +1641,9 @@ static void byt_gpio_irq_init_hw(struct byt_gpio
> > *vg)
> >
> > continue;
> >
> > }
> >
> > + raw_spin_lock_irqsave(&vg->lock, flags);
>
> Is this really necessary as we are initializing the driver?
I don't know. I guess this also depends if the previous call to
pinctrl_register (in byt_pinctrl_probe) could cause a call into that driver.
In the end I just wrapped _all_ accesses with locks, even suspend/resume
although I don't know if that would be necessary.
Best regards,
Alexander
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/1] pinctrl: baytrail: Add spinlock usage to all read/write access
2017-01-30 6:50 ` Alexander Stein
@ 2017-01-30 9:26 ` Mika Westerberg
0 siblings, 0 replies; 5+ messages in thread
From: Mika Westerberg @ 2017-01-30 9:26 UTC (permalink / raw)
To: Alexander Stein; +Cc: Heikki Krogerus, Linus Walleij, linux-gpio
On Mon, Jan 30, 2017 at 07:50:18AM +0100, Alexander Stein wrote:
> Hi,
>
> On Thursday 26 January 2017 22:52:19, Mika Westerberg wrote:
> > On Thu, Jan 26, 2017 at 03:14:18PM +0100, Alexander Stein wrote:
> > > According to VLI64 Intel Atom E3800 Specification Update (#329901)
> > > concurrent read accesses may result in returning 0xffffffff and write
> > > accesses may be dropped silently.
> > > To workaround all accesses must be protected by locks.
> > >
> > > Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> > > ---
> > > I actually had the case where the read access in byt_irq_unmask returned
> > > 0xffffffff. After OR'ing the trigger bits and writing 0xffffffff back to
> > > BYT_CONF0_REG things started to act strange.
> > >
> > > drivers/pinctrl/intel/pinctrl-baytrail.c | 14 ++++++++++++++
> > > 1 file changed, 14 insertions(+)
> > >
> > > diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c
> > > b/drivers/pinctrl/intel/pinctrl-baytrail.c index 6cce314..7294c88 100644
> > > --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
> > > +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
> > > @@ -1594,6 +1594,7 @@ static void byt_gpio_irq_handler(struct irq_desc
> > > *desc)>
> > > void __iomem *reg;
> > > unsigned long pending;
> > > unsigned int virq;
> > >
> > > + unsigned long flags;
> >
> > Can you move this variable after "pending" like:
> >
> > unsigned long pending;
> > unsigned long flags;
> > unsigned int virq;
>
> Sure, I understand why both unsigned longs are put together here...
>
> > > /* check from GPIO controller which pin triggered the interrupt */
> > > for (base = 0; base < vg->chip.ngpio; base += 32) {
> > >
> > [...]
> > > @@ -1620,6 +1623,7 @@ static void byt_gpio_irq_init_hw(struct byt_gpio
> > > *vg)
> > >
> > > void __iomem *reg;
> > > u32 base, value;
> > > int i;
> > >
> > > + unsigned long flags;
> >
> > Here also arrange it like
> >
> > unsigned long flags;
> > void __iomem *reg;
> > u32 base, value;
> > int i;
>
> but here I'm not so sure. What are the rules for variable declaration order?
Looks better that way.
> > > /*
> > >
> > > * Clear interrupt triggers for all pins that are GPIOs and
> > >
> > > @@ -1637,7 +1641,9 @@ static void byt_gpio_irq_init_hw(struct byt_gpio
> > > *vg)
> > >
> > > continue;
> > >
> > > }
> > >
> > > + raw_spin_lock_irqsave(&vg->lock, flags);
> >
> > Is this really necessary as we are initializing the driver?
>
> I don't know. I guess this also depends if the previous call to
> pinctrl_register (in byt_pinctrl_probe) could cause a call into that driver.
> In the end I just wrapped _all_ accesses with locks, even suspend/resume
> although I don't know if that would be necessary.
OK, let's try to lock only where we know we need it. I guess both the
initializiation and suspend/resume does not need locking. Other places
do.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-01-30 9:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-26 14:14 [PATCH 1/1] pinctrl: baytrail: Add spinlock usage to all read/write access Alexander Stein
2017-01-26 20:52 ` Mika Westerberg
2017-01-27 9:26 ` Mika Westerberg
2017-01-30 6:50 ` Alexander Stein
2017-01-30 9:26 ` Mika Westerberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).