spi/pxa2xx: use GFP_ATOMIC in sg table allocation
Subsystems (3)
PATCH 1/2 spi/pxa2xx: use GFP_ATOMIC in sg table allocation
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.
--- drivers/spi/spi-pxa2xx-dma.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/spi/spi-pxa2xx-dma.c +1 -1
@@ -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; }
Signature · 1 line
1.8.3.1
PATCH 2/2 spi/pxa2xx: use a flag to check if the device is runtime suspended
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.
--- drivers/spi/spi-pxa2xx.c | 4 +++- drivers/spi/spi-pxa2xx.h | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)
drivers/spi/spi-pxa2xx.c +3 -1
@@ -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
drivers/spi/spi-pxa2xx.h +1 -0
@@ -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; };
Signature · 1 line
1.8.3.1
Mark Brown
maintainerRe: [PATCH 2/2] spi/pxa2xx: use a flag to check if the device is runtime suspended
Quoted 4 lines · 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.
Quoted 8 lines
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
0xffffffffthat 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?
Re: [PATCH 2/2] spi/pxa2xx: use a flag to check if the device is runtime suspended
Quoted 14 lines · Mark Brown
Quoted 4 lines · 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.Quoted 8 lines
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
0xffffffffthat the driver misinterprets as an invalid state. When this happens user will see messages like below on the console:
Quoted 3 lines
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?
Mark Brown
maintainerRe: [PATCH 2/2] spi/pxa2xx: use a flag to check if the device is runtime suspended
Quoted 11 lines · Mika Westerberg
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).
Re: [PATCH 2/2] spi/pxa2xx: use a flag to check if the device is runtime suspended
In reply to Mark BrownQuoted 14 lines · Mark Brown
Quoted 4 lines · 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.Quoted 8 lines
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
0xffffffffthat the driver misinterprets as an invalid state. When this happens user will see messages like below on the console:
Quoted 3 lines
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.
Mark Brown
maintainerRe: [PATCH 2/2] spi/pxa2xx: use a flag to check if the device is runtime suspended
Quoted 11 lines · Russell King - ARM Linux
Quoted 3 lines · Mark Brown
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.
Re: [PATCH 2/2] spi/pxa2xx: use a flag to check if the device is runtime suspended
Quoted 20 lines · Mark Brown
Quoted 11 lines · Russell King - ARM Linux
Quoted 3 lines · Mark Brown
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).
Quoted 4 lines
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
Quoted 21 lines · Russell King - ARM Linux
Quoted 20 lines · Mark Brown
Quoted 11 lines · Russell King - ARM Linux
Quoted 3 lines · Mark Brown
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).
Quoted 8 lines
Quoted 4 lines
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.
Mark Brown
maintainerRe: [PATCH 2/2] spi/pxa2xx: use a flag to check if the device is runtime suspended
In reply to Russell King - ARM LinuxQuoted 3 lines · Russell King - ARM Linux
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.
Quoted 6 lines
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.
Quoted 8 lines
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.
Quoted 2 lines · Mika Westerberg
pxa2xx_spi_map_dma_buffer()gets called in tasklet context so we can't sleep when we allocate a new sg table. UseGFP_ATOMIChere instead.
Applied, thanks.