[PATCH 1/2] spi/pxa2xx: use GFP_ATOMIC in sg table allocation

[PATCH 1/2] spi/pxa2xx: use GFP_ATOMIC in sg table allocation

[Mika Westerberg]

pxa2xx_spi_map_dma_buffer() gets called in tasklet context so we can't
sleep when we allocate a new sg table. Use GFP_ATOMIC here instead.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/spi/spi-pxa2xx-dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-pxa2xx-dma.c b/drivers/spi/spi-pxa2xx-dma.c
index f4cb744..3c0b551 100644
--- a/drivers/spi/spi-pxa2xx-dma.c
+++ b/drivers/spi/spi-pxa2xx-dma.c
@@ -59,7 +59,7 @@ static int pxa2xx_spi_map_dma_buffer(struct driver_data *drv_data,
 		int ret;
 
 		sg_free_table(sgt);
-		ret = sg_alloc_table(sgt, nents, GFP_KERNEL);
+		ret = sg_alloc_table(sgt, nents, GFP_ATOMIC);
 		if (ret)
 			return ret;
 	}
-- 
1.8.3.1

[PATCH 2/2] spi/pxa2xx: use a flag to check if the device is runtime suspended

[Mika Westerberg]

Current code calls pm_runtime_suspended() in the interrupt handler to check
if the device is suspended or not. However, runtime PM status of the device
is only set to suspended once all PM runtime suspend hooks have executed.

In our case we have the device bound to the ACPI power domain and its
runtime suspend hook will put the device to D3hot (or D3cold if possible).
This effectively means that the device is powered off before its state is
set to runtime suspended. During this time, it might get an interrupt that
is meant for another device (as the interrupt line is shared), and because
the device is powered off accessing its registers will return 0xffffffff
that the driver misinterprets as an invalid state. When this happens user
will see messages like below on the console:

	pxa2xx-spi INT33C0:00: bad message state in interrupt handler

Fix this by adding flag to the driver private data that indicates whether
the device is runtime suspended, and in that case return IRQ_NONE from the
interrupt handler.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/spi/spi-pxa2xx.c | 4 +++-
 drivers/spi/spi-pxa2xx.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index b92a5d0..a105c03 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -536,7 +536,7 @@ static irqreturn_t ssp_int(int irq, void *dev_id)
 	 * the IRQ was not for us (we shouldn't be RPM suspended when the
 	 * interrupt is enabled).
 	 */
-	if (pm_runtime_suspended(&drv_data->pdev->dev))
+	if (drv_data->suspended)
 		return IRQ_NONE;
 
 	sccr1_reg = read_SSCR1(reg);
@@ -1309,6 +1309,7 @@ static int pxa2xx_spi_runtime_suspend(struct device *dev)
 	struct driver_data *drv_data = dev_get_drvdata(dev);
 
 	clk_disable_unprepare(drv_data->ssp->clk);
+	drv_data->suspended = true;
 	return 0;
 }
 
@@ -1317,6 +1318,7 @@ static int pxa2xx_spi_runtime_resume(struct device *dev)
 	struct driver_data *drv_data = dev_get_drvdata(dev);
 
 	clk_prepare_enable(drv_data->ssp->clk);
+	drv_data->suspended = false;
 	return 0;
 }
 #endif
diff --git a/drivers/spi/spi-pxa2xx.h b/drivers/spi/spi-pxa2xx.h
index 5adc2a1..a5f8bb5 100644
--- a/drivers/spi/spi-pxa2xx.h
+++ b/drivers/spi/spi-pxa2xx.h
@@ -86,6 +86,7 @@ struct driver_data {
 	int (*read)(struct driver_data *drv_data);
 	irqreturn_t (*transfer_handler)(struct driver_data *drv_data);
 	void (*cs_control)(u32 command);
+	bool suspended;
 
 	void __iomem *lpss_base;
 };
-- 
1.8.3.1

Re: [PATCH 2/2] spi/pxa2xx: use a flag to check if the device is runtime suspended

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]

On Tue, Jun 18, 2013 at 05:29:45PM +0300, Mika Westerberg wrote:
> Current code calls pm_runtime_suspended() in the interrupt handler to check
> if the device is suspended or not. However, runtime PM status of the device
> is only set to suspended once all PM runtime suspend hooks have executed.

> In our case we have the device bound to the ACPI power domain and its
> runtime suspend hook will put the device to D3hot (or D3cold if possible).
> This effectively means that the device is powered off before its state is
> set to runtime suspended. During this time, it might get an interrupt that
> is meant for another device (as the interrupt line is shared), and because
> the device is powered off accessing its registers will return 0xffffffff
> that the driver misinterprets as an invalid state. When this happens user
> will see messages like below on the console:

This sounds like a problem which will affect a lot of devices and hence
ought to be handled better by the PM core or at least frameworks in
general.  Is it really device specific?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] spi/pxa2xx: use GFP_ATOMIC in sg table allocation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 226 bytes --]

On Tue, Jun 18, 2013 at 05:29:44PM +0300, Mika Westerberg wrote:
> pxa2xx_spi_map_dma_buffer() gets called in tasklet context so we can't
> sleep when we allocate a new sg table. Use GFP_ATOMIC here instead.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] spi/pxa2xx: use a flag to check if the device is runtime suspended

[Mika Westerberg]

On Tue, Jun 18, 2013 at 07:09:48PM +0100, Mark Brown wrote:
> On Tue, Jun 18, 2013 at 05:29:45PM +0300, Mika Westerberg wrote:
> > Current code calls pm_runtime_suspended() in the interrupt handler to check
> > if the device is suspended or not. However, runtime PM status of the device
> > is only set to suspended once all PM runtime suspend hooks have executed.
> 
> > In our case we have the device bound to the ACPI power domain and its
> > runtime suspend hook will put the device to D3hot (or D3cold if possible).
> > This effectively means that the device is powered off before its state is
> > set to runtime suspended. During this time, it might get an interrupt that
> > is meant for another device (as the interrupt line is shared), and because
> > the device is powered off accessing its registers will return 0xffffffff
> > that the driver misinterprets as an invalid state. When this happens user
> > will see messages like below on the console:
> 
> This sounds like a problem which will affect a lot of devices and hence
> ought to be handled better by the PM core or at least frameworks in
> general.  Is it really device specific?

No, it's not device specific. However, I've seen it only happen with the
SPI controller on Lynxpoint.

I agree that it is better handled outside of the driver (or provide some
API for drivers). One thing that immediately came to mind is
adding pm_runtime_suspending() that could be used here like:

ssp_int()
{
	if (pm_runtime_suspending(dev) || pm_runtime_suspending(dev))
		return IRQ_NONE;
	...

or something like that?

Re: [PATCH 2/2] spi/pxa2xx: use a flag to check if the device is runtime suspended

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 494 bytes --]

On Wed, Jun 19, 2013 at 10:44:37AM +0300, Mika Westerberg wrote:

> I agree that it is better handled outside of the driver (or provide some
> API for drivers). One thing that immediately came to mind is
> adding pm_runtime_suspending() that could be used here like:

> ssp_int()
> {
> 	if (pm_runtime_suspending(dev) || pm_runtime_suspending(dev))
> 		return IRQ_NONE;
> 	...

> or something like that?

Yes, something like that (probably include the suspended state in the
suspending check).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] spi/pxa2xx: use a flag to check if the device is runtime suspended

[Russell King - ARM Linux]

On Tue, Jun 18, 2013 at 07:09:48PM +0100, Mark Brown wrote:
> On Tue, Jun 18, 2013 at 05:29:45PM +0300, Mika Westerberg wrote:
> > Current code calls pm_runtime_suspended() in the interrupt handler to check
> > if the device is suspended or not. However, runtime PM status of the device
> > is only set to suspended once all PM runtime suspend hooks have executed.
> 
> > In our case we have the device bound to the ACPI power domain and its
> > runtime suspend hook will put the device to D3hot (or D3cold if possible).
> > This effectively means that the device is powered off before its state is
> > set to runtime suspended. During this time, it might get an interrupt that
> > is meant for another device (as the interrupt line is shared), and because
> > the device is powered off accessing its registers will return 0xffffffff
> > that the driver misinterprets as an invalid state. When this happens user
> > will see messages like below on the console:
> 
> This sounds like a problem which will affect a lot of devices and hence
> ought to be handled better by the PM core or at least frameworks in
> general.  Is it really device specific?

It's always been something that has been recommended to be dealt with
by the driver.  If reading the interrupt status you read ~0, then it
likely is because the device is powered down or removed from the system.

PCMCIA drivers have done this for years.

Re: [PATCH 2/2] spi/pxa2xx: use a flag to check if the device is runtime suspended

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1269 bytes --]

On Wed, Jun 19, 2013 at 10:25:08AM +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 18, 2013 at 07:09:48PM +0100, Mark Brown wrote:

> > This sounds like a problem which will affect a lot of devices and hence
> > ought to be handled better by the PM core or at least frameworks in
> > general.  Is it really device specific?

> It's always been something that has been recommended to be dealt with
> by the driver.  If reading the interrupt status you read ~0, then it
> likely is because the device is powered down or removed from the system.

> PCMCIA drivers have done this for years.

I know, some PCI devices too.  It's not just an issue for memory mapped
devices, the same thing happens with devices on other buses - there's a
whole bunch of issues around moving out of the various suspend states
and getting interrupts (things like getting an interrupt controller
waking up and delivering interrupts before the control bus for a device
connected to it has woken up).  

The driver does need to be the one deciding what to do about being in
suspend but we really ought to be able to do something without having to
interact with the hardware partly just for neatness but more because on
general buses the error handling is too painful.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] spi/pxa2xx: use a flag to check if the device is runtime suspended

[Russell King - ARM Linux]

On Wed, Jun 19, 2013 at 10:39:38AM +0100, Mark Brown wrote:
> On Wed, Jun 19, 2013 at 10:25:08AM +0100, Russell King - ARM Linux wrote:
> > On Tue, Jun 18, 2013 at 07:09:48PM +0100, Mark Brown wrote:
> 
> > > This sounds like a problem which will affect a lot of devices and hence
> > > ought to be handled better by the PM core or at least frameworks in
> > > general.  Is it really device specific?
> 
> > It's always been something that has been recommended to be dealt with
> > by the driver.  If reading the interrupt status you read ~0, then it
> > likely is because the device is powered down or removed from the system.
> 
> > PCMCIA drivers have done this for years.
> 
> I know, some PCI devices too.  It's not just an issue for memory mapped
> devices, the same thing happens with devices on other buses - there's a
> whole bunch of issues around moving out of the various suspend states
> and getting interrupts (things like getting an interrupt controller
> waking up and delivering interrupts before the control bus for a device
> connected to it has woken up).  
> 
> The driver does need to be the one deciding what to do about being in
> suspend but we really ought to be able to do something without having to
> interact with the hardware partly just for neatness but more because on
> general buses the error handling is too painful.

And that's why doing it by "read the ISR and check its value" is the
best way, and not doing the "what state does the kernel think this
device is in".

The latter may be fine if the device is only connected to a non-shared
interrupt, but as soon as you start sharing interrupts, it fails - what
do you do if the device signals an interrupt on a level-sensitive input
but the kernel's state says that it's not yet active?  The answer -
you spin forever entering the same interrupt handler until the IRQ
input gets disabled permanently until you reboot the system.

As far as device sequencing, that is where the tree topology of the
device model is supposed to save you - the parenting of devices is
supposed to reflect their relationship, and the way things like PM
and runtime PM work, those relationships dictate the order in which
PM operations occur.  For instance, with runtime PM, a parent will
not be placed into a suspend state unless its children are already
suspended, unless the parent signals that it is independent of the
childs states.

Now, what might be a common pitfall with that is if you have a device
which is runtime PM enabled but its children aren't - does that block
or allow the parent device.  I've seen at least one instance where
it means that the children don't, so that's an argument that runtime
PM is either broken, or we need greater coverage with runtime PM
conversions.

Re: [PATCH 2/2] spi/pxa2xx: use a flag to check if the device is runtime suspended

[Mika Westerberg]

On Wed, Jun 19, 2013 at 11:05:15AM +0100, Russell King - ARM Linux wrote:
> On Wed, Jun 19, 2013 at 10:39:38AM +0100, Mark Brown wrote:
> > On Wed, Jun 19, 2013 at 10:25:08AM +0100, Russell King - ARM Linux wrote:
> > > On Tue, Jun 18, 2013 at 07:09:48PM +0100, Mark Brown wrote:
> > 
> > > > This sounds like a problem which will affect a lot of devices and hence
> > > > ought to be handled better by the PM core or at least frameworks in
> > > > general.  Is it really device specific?
> > 
> > > It's always been something that has been recommended to be dealt with
> > > by the driver.  If reading the interrupt status you read ~0, then it
> > > likely is because the device is powered down or removed from the system.
> > 
> > > PCMCIA drivers have done this for years.
> > 
> > I know, some PCI devices too.  It's not just an issue for memory mapped
> > devices, the same thing happens with devices on other buses - there's a
> > whole bunch of issues around moving out of the various suspend states
> > and getting interrupts (things like getting an interrupt controller
> > waking up and delivering interrupts before the control bus for a device
> > connected to it has woken up).  
> > 
> > The driver does need to be the one deciding what to do about being in
> > suspend but we really ought to be able to do something without having to
> > interact with the hardware partly just for neatness but more because on
> > general buses the error handling is too painful.
> 
> And that's why doing it by "read the ISR and check its value" is the
> best way, and not doing the "what state does the kernel think this
> device is in".

This sounds the simplest thing that solves the problem with the Lynxpoint
SPI controller driver. Then I don't need to add any new flags to the
private structure but just check that if reading the status register
returns ~0 and bail out in that case.

Re: [PATCH 2/2] spi/pxa2xx: use a flag to check if the device is runtime suspended

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2060 bytes --]

On Wed, Jun 19, 2013 at 11:05:15AM +0100, Russell King - ARM Linux wrote:

> And that's why doing it by "read the ISR and check its value" is the
> best way, and not doing the "what state does the kernel think this
> device is in".

Not entirely, and of course that's not always an option either.

> The latter may be fine if the device is only connected to a non-shared
> interrupt, but as soon as you start sharing interrupts, it fails - what
> do you do if the device signals an interrupt on a level-sensitive input
> but the kernel's state says that it's not yet active?  The answer -
> you spin forever entering the same interrupt handler until the IRQ
> input gets disabled permanently until you reboot the system.

Or we teach the interrupt core how to handle it better - checking for
I/O failures works in the case where the device is genuinely asleep and
we can see I/O failures without undue pain but starts to fall over with
other scenarios.

> As far as device sequencing, that is where the tree topology of the
> device model is supposed to save you - the parenting of devices is
> supposed to reflect their relationship, and the way things like PM
> and runtime PM work, those relationships dictate the order in which
> PM operations occur.  For instance, with runtime PM, a parent will
> not be placed into a suspend state unless its children are already
> suspended, unless the parent signals that it is independent of the
> childs states.

This doesn't fix everything (though it's a lot better with deferred
probing since things now mostly end up in the actual dependency order),
there's still some nasty issues especially around devices which can
interrupt from suspend states.  If that can happen then you can get the
interrupt controller being awake and delivering the interrupt while the
device (or worse, the control bus for the device) is not able to
interact with it.  This is a different problem in that the interrupt
genuinely is firing, it's just that the control path isn't available
yet, but it's the same general class of things.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]