Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option
Hi Russell, On Monday 03 November 2014 17:04:08 Russell King - ARM Linux wrote: > On Mon, Nov 03, 2014 at 09:57:28PM +0530, Vinod Koul wrote: > > On Sat, Nov 01, 2014 at 02:29:42AM +0200, Laurent Pinchart wrote: > >> Many other drivers suffer from the same problem. While I won't reject > >> your proposed fix, I would prefer a more generic approach. > >> > >> One option that has been discussed previously was to use a work queue to > >> delay starting the DMA transfer to an interruptible context where > >> pm_runtime_get_sync() could be called. However, as Russell pointed out > >> [1], > >> even that won't work in all cases as the DMA slave might need the > >> transfer to be started before enabling part of its hardware (OMAP audio > >> seem to be such a case). > >> > >> I've heard a rumor of a possible DMA engine rework to forbid calling the > >> descriptor preparation API from atomic context. This could be used as a > >> base to implement runtime PM, as DMA slave drivers should not prepare > >> descriptors if they don't need to use them. However that's a long term > >> plan, and we need a solution sooner than that. > > > > Well it is not a rumour :) > > > > I have been contemplating that now that async_tx will be killed so we dont > > have to worry about that usage. For the slave dma usage, we can change the > > prepare API to be non atomic. I think the users will be okay with > > approach. This way drivers can use runtime pm calls in prepare. > > Except we /do/ have a fair number of places where the prep calls are made > from atomic contexts, particularly in serial drivers. You'd need to > introduce a tasklet into almost every serial driver which doesn't > already have one to restart RX DMA after an error or pause. Eg, > > drivers/tty/serial/amba-pl011.c > drivers/tty/serial/pch_uart.c > drivers/tty/serial/imx.c I wonder whether it would be possible to decouple descriptor allocation and descriptor initialization/preparation. If drivers could allocate descriptors and reuse them after they complete, not only would it lower the memory management pressure by getting rid of alloc/free during transmission, but it would also make it possible to easily allocate the transaction descriptors beforehand in non-atomic context. > Probably also: > > drivers/net/ethernet/micrel/ks8842.c > > There could well be other places as well, I've not gone through and > checked exhaustively. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option
On Mon, Nov 03, 2014 at 06:59:37PM +0200, Laurent Pinchart wrote: > > > Many other drivers suffer from the same problem. While I won't reject your > > > proposed fix, I would prefer a more generic approach. > > > > > > One option that has been discussed previously was to use a work queue to > > > delay starting the DMA transfer to an interruptible context where > > > pm_runtime_get_sync() could be called. However, as Russell pointed out > > > [1], > > > even that won't work in all cases as the DMA slave might need the transfer > > > to be started before enabling part of its hardware (OMAP audio seem to be > > > such a case). > > > > > > I've heard a rumor of a possible DMA engine rework to forbid calling the > > > descriptor preparation API from atomic context. This could be used as a > > > base to implement runtime PM, as DMA slave drivers should not prepare > > > descriptors if they don't need to use them. However that's a long term > > > plan, and we need a solution sooner than that. > > > > Well it is not a rumour :) > > I didn't want to put too much pressure on you by quoting you on that :-) > > > I have been contemplating that now that async_tx will be killed so we dont > > have to worry about that usage. For the slave dma usage, we can change the > > prepare API to be non atomic. I think the users will be okay with approach. > > This way drivers can use runtime pm calls in prepare. > > I'd certainly welcome that. There's a couple of users we will need to fix as > they call the prepare API from an atomic context. I'm thinking about ASoC > drivers in particular. ASoC is impler to fix as we can do this is ASoC prepare which is not atomic. So we would need to split prepare and submit on that > Do you have any time line in mind ? Not yet, perhpaps the one after next merge window will be good target. -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option
On Mon, Nov 03, 2014 at 05:04:08PM +, Russell King - ARM Linux wrote: > On Mon, Nov 03, 2014 at 09:57:28PM +0530, Vinod Koul wrote: > > On Sat, Nov 01, 2014 at 02:29:42AM +0200, Laurent Pinchart wrote: > > > Many other drivers suffer from the same problem. While I won't reject > > > your > > > proposed fix, I would prefer a more generic approach. > > > > > > One option that has been discussed previously was to use a work queue to > > > delay > > > starting the DMA transfer to an interruptible context where > > > pm_runtime_get_sync() could be called. However, as Russell pointed out > > > [1], > > > even that won't work in all cases as the DMA slave might need the > > > transfer to > > > be started before enabling part of its hardware (OMAP audio seem to be > > > such a > > > case). > > > > > > I've heard a rumor of a possible DMA engine rework to forbid calling the > > > descriptor preparation API from atomic context. This could be used as a > > > base > > > to implement runtime PM, as DMA slave drivers should not prepare > > > descriptors > > > if they don't need to use them. However that's a long term plan, and we > > > need a > > > solution sooner than that. > > > > Well it is not a rumour :) > > > > I have been contemplating that now that async_tx will be killed so we dont > > have to worry about that usage. For the slave dma usage, we can change the > > prepare API to be non atomic. I think the users will be okay with approach. > > This way drivers can use runtime pm calls in prepare. > > Except we /do/ have a fair number of places where the prep calls are made > from atomic contexts, particularly in serial drivers. You'd need to > introduce a tasklet into almost every serial driver which doesn't > already have one to restart RX DMA after an error or pause. Eg, > > drivers/tty/serial/amba-pl011.c > drivers/tty/serial/pch_uart.c > drivers/tty/serial/imx.c > > Probably also: > > drivers/net/ethernet/micrel/ks8842.c > > There could well be other places as well, I've not gone through and > checked exhaustively. Yes that would be required to be done first. Or is there any better alternative proposal, am all ears -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option
On Mon, Nov 03, 2014 at 09:57:28PM +0530, Vinod Koul wrote: > On Sat, Nov 01, 2014 at 02:29:42AM +0200, Laurent Pinchart wrote: > > Many other drivers suffer from the same problem. While I won't reject your > > proposed fix, I would prefer a more generic approach. > > > > One option that has been discussed previously was to use a work queue to > > delay > > starting the DMA transfer to an interruptible context where > > pm_runtime_get_sync() could be called. However, as Russell pointed out [1], > > even that won't work in all cases as the DMA slave might need the transfer > > to > > be started before enabling part of its hardware (OMAP audio seem to be such > > a > > case). > > > > I've heard a rumor of a possible DMA engine rework to forbid calling the > > descriptor preparation API from atomic context. This could be used as a > > base > > to implement runtime PM, as DMA slave drivers should not prepare > > descriptors > > if they don't need to use them. However that's a long term plan, and we > > need a > > solution sooner than that. > > Well it is not a rumour :) > > I have been contemplating that now that async_tx will be killed so we dont > have to worry about that usage. For the slave dma usage, we can change the > prepare API to be non atomic. I think the users will be okay with approach. > This way drivers can use runtime pm calls in prepare. Except we /do/ have a fair number of places where the prep calls are made from atomic contexts, particularly in serial drivers. You'd need to introduce a tasklet into almost every serial driver which doesn't already have one to restart RX DMA after an error or pause. Eg, drivers/tty/serial/amba-pl011.c drivers/tty/serial/pch_uart.c drivers/tty/serial/imx.c Probably also: drivers/net/ethernet/micrel/ks8842.c There could well be other places as well, I've not gone through and checked exhaustively. -- FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option
Hi Vinod, On Monday 03 November 2014 21:57:28 Vinod Koul wrote: > On Sat, Nov 01, 2014 at 02:29:42AM +0200, Laurent Pinchart wrote: > > On Friday 31 October 2014 15:40:16 Krzysztof Kozlowski wrote: > >> On pią, 2014-10-31 at 15:22 +0100, Pavel Machek wrote: > >>> On Fri 2014-10-31 10:14:55, Krzysztof Kozlowski wrote: > On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: > > Add a simple getter pm_runtime_is_irq_safe() for querying whether > > runtime PM IRQ safe was set or not. > > > > Various bus drivers implementing runtime PM may use choose to > > suspend differently based on IRQ safeness status of child driver > > (e.g. do not unprepare the clock if IRQ safe is not set). > > > > Signed-off-by: Krzysztof Kozlowski > > Reviewed-by: Ulf Hansson > > Rafael, Len, Pavel, > > Is proposed API ok? Do you have any comments? > > I'll upload whole patchset to Russell's patch tracking system. > However an ack from PM maintainer is probably needed. > >>> > >>> I don't like the API. Having callbacks work in different context (irq > >>> / noirq) based on what another function reports is ugly. > >>> > >>> What is the penalty if we always decide callbacks are not IRQ safe? > >> > >> Then pm_runtime_get_sync() could not be called in atomic context. The > >> pl330 runtime PM would have to be completely reworked because one > >> pm_runtime_get_sync() is called in device_issue_pending which cannot > >> sleep (at least in non preemptible kernels). Probably this can be solved > >> some way... > > > > Many other drivers suffer from the same problem. While I won't reject your > > proposed fix, I would prefer a more generic approach. > > > > One option that has been discussed previously was to use a work queue to > > delay starting the DMA transfer to an interruptible context where > > pm_runtime_get_sync() could be called. However, as Russell pointed out > > [1], > > even that won't work in all cases as the DMA slave might need the transfer > > to be started before enabling part of its hardware (OMAP audio seem to be > > such a case). > > > > I've heard a rumor of a possible DMA engine rework to forbid calling the > > descriptor preparation API from atomic context. This could be used as a > > base to implement runtime PM, as DMA slave drivers should not prepare > > descriptors if they don't need to use them. However that's a long term > > plan, and we need a solution sooner than that. > > Well it is not a rumour :) I didn't want to put too much pressure on you by quoting you on that :-) > I have been contemplating that now that async_tx will be killed so we dont > have to worry about that usage. For the slave dma usage, we can change the > prepare API to be non atomic. I think the users will be okay with approach. > This way drivers can use runtime pm calls in prepare. I'd certainly welcome that. There's a couple of users we will need to fix as they call the prepare API from an atomic context. I'm thinking about ASoC drivers in particular. Do you have any time line in mind ? > > I've been toying with the idea of adding explicit open/close (or whatever > > we would call them) operations to the DMA engine API. Those would be used > > by DMA slave drivers to signal that they will start/stop using the DMA > > engine. > > > > If (1) we must start the DMA synchronously with a DMA slave call, (2) need > > to sleep to handle PM, and (3) don't want to keep the DMA engine powered > > for as long as one channel is requested, then we need to turn at least > > preparation as not callable in atomic context, or introduce a new > > operation. > > > > [1] http://www.spinics.net/lists/dmaengine/msg01548.html > > > > > >>> --- a/Documentation/power/runtime_pm.txt > > > >>> +++ b/Documentation/power/runtime_pm.txt > > > >>> > > > >>> @@ -468,6 +468,10 @@ drivers/base/power/runtime.c and > > > >>> > > > >>> include/linux/pm_runtime.h: > > > >>> - set the power.irq_safe flag for the device, causing the > > > >>>runtime-PM callbacks to be invoked with interrupts off > > > >>> > > > >>> + bool pm_runtime_is_irq_safe(struct device *dev); > > > >>> +- return true if power.irq_safe flag was set for the device, > > > >>> causing > > > >>> + the runtime-PM callbacks to be invoked with interrupts off > > > >>> + > > > >>> > > > >>>void pm_runtime_mark_last_busy(struct device *dev); > > > >>> - set the power.last_busy field to the current time -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option
On Sat, Nov 01, 2014 at 02:29:42AM +0200, Laurent Pinchart wrote: > Hi Krzysztof, > > On Friday 31 October 2014 15:40:16 Krzysztof Kozlowski wrote: > > On pią, 2014-10-31 at 15:22 +0100, Pavel Machek wrote: > > > On Fri 2014-10-31 10:14:55, Krzysztof Kozlowski wrote: > > >> On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: > > >>> Add a simple getter pm_runtime_is_irq_safe() for querying whether > > >>> runtime PM IRQ safe was set or not. > > >>> > > >>> Various bus drivers implementing runtime PM may use choose to suspend > > >>> differently based on IRQ safeness status of child driver (e.g. do not > > >>> unprepare the clock if IRQ safe is not set). > > >>> > > >>> Signed-off-by: Krzysztof Kozlowski > > >>> Reviewed-by: Ulf Hansson > > >> > > >> Rafael, Len, Pavel, > > >> > > >> Is proposed API ok? Do you have any comments? > > >> > > >> I'll upload whole patchset to Russell's patch tracking system. However > > >> an ack from PM maintainer is probably needed. > > > > > > I don't like the API. Having callbacks work in different context (irq > > > / noirq) based on what another function reports is ugly. > > > > > > What is the penalty if we always decide callbacks are not IRQ safe? > > > > Then pm_runtime_get_sync() could not be called in atomic context. The > > pl330 runtime PM would have to be completely reworked because one > > pm_runtime_get_sync() is called in device_issue_pending which cannot > > sleep (at least in non preemptible kernels). Probably this can be solved > > some way... > > Many other drivers suffer from the same problem. While I won't reject your > proposed fix, I would prefer a more generic approach. > > One option that has been discussed previously was to use a work queue to > delay > starting the DMA transfer to an interruptible context where > pm_runtime_get_sync() could be called. However, as Russell pointed out [1], > even that won't work in all cases as the DMA slave might need the transfer to > be started before enabling part of its hardware (OMAP audio seem to be such a > case). > > I've heard a rumor of a possible DMA engine rework to forbid calling the > descriptor preparation API from atomic context. This could be used as a base > to implement runtime PM, as DMA slave drivers should not prepare descriptors > if they don't need to use them. However that's a long term plan, and we need > a > solution sooner than that. Well it is not a rumour :) I have been contemplating that now that async_tx will be killed so we dont have to worry about that usage. For the slave dma usage, we can change the prepare API to be non atomic. I think the users will be okay with approach. This way drivers can use runtime pm calls in prepare. -- ~Vinod > > I've been toying with the idea of adding explicit open/close (or whatever we > would call them) operations to the DMA engine API. Those would be used by DMA > slave drivers to signal that they will start/stop using the DMA engine. > > If (1) we must start the DMA synchronously with a DMA slave call, (2) need to > sleep to handle PM, and (3) don't want to keep the DMA engine powered for as > long as one channel is requested, then we need to turn at least preparation > as > not callable in atomic context, or introduce a new operation. > > [1] http://www.spinics.net/lists/dmaengine/msg01548.html > > > >>> --- a/Documentation/power/runtime_pm.txt > > >>> +++ b/Documentation/power/runtime_pm.txt > > >>> > > >>> @@ -468,6 +468,10 @@ drivers/base/power/runtime.c and > > >>> include/linux/pm_runtime.h: > > >>> - set the power.irq_safe flag for the device, causing the > > >>> runtime-PM > > >>> > > >>>callbacks to be invoked with interrupts off > > >>> > > >>> + bool pm_runtime_is_irq_safe(struct device *dev); > > >>> +- return true if power.irq_safe flag was set for the device, > > >>> causing > > >>> + the runtime-PM callbacks to be invoked with interrupts off > > >>> + > > >>> > > >>>void pm_runtime_mark_last_busy(struct device *dev); > > >>> > > >>> - set the power.last_busy field to the current time > > -- > Regards, > > Laurent Pinchart > -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option
On sob, 2014-11-01 at 02:29 +0200, Laurent Pinchart wrote: > Hi Krzysztof, > > On Friday 31 October 2014 15:40:16 Krzysztof Kozlowski wrote: > > On pią, 2014-10-31 at 15:22 +0100, Pavel Machek wrote: > > > On Fri 2014-10-31 10:14:55, Krzysztof Kozlowski wrote: > > >> On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: > > >>> Add a simple getter pm_runtime_is_irq_safe() for querying whether > > >>> runtime PM IRQ safe was set or not. > > >>> > > >>> Various bus drivers implementing runtime PM may use choose to suspend > > >>> differently based on IRQ safeness status of child driver (e.g. do not > > >>> unprepare the clock if IRQ safe is not set). > > >>> > > >>> Signed-off-by: Krzysztof Kozlowski > > >>> Reviewed-by: Ulf Hansson > > >> > > >> Rafael, Len, Pavel, > > >> > > >> Is proposed API ok? Do you have any comments? > > >> > > >> I'll upload whole patchset to Russell's patch tracking system. However > > >> an ack from PM maintainer is probably needed. > > > > > > I don't like the API. Having callbacks work in different context (irq > > > / noirq) based on what another function reports is ugly. > > > > > > What is the penalty if we always decide callbacks are not IRQ safe? > > > > Then pm_runtime_get_sync() could not be called in atomic context. The > > pl330 runtime PM would have to be completely reworked because one > > pm_runtime_get_sync() is called in device_issue_pending which cannot > > sleep (at least in non preemptible kernels). Probably this can be solved > > some way... > > Many other drivers suffer from the same problem. While I won't reject your > proposed fix, I would prefer a more generic approach. > > One option that has been discussed previously was to use a work queue to > delay > starting the DMA transfer to an interruptible context where > pm_runtime_get_sync() could be called. However, as Russell pointed out [1], > even that won't work in all cases as the DMA slave might need the transfer to > be started before enabling part of its hardware (OMAP audio seem to be such a > case). > > I've heard a rumor of a possible DMA engine rework to forbid calling the > descriptor preparation API from atomic context. This could be used as a base > to implement runtime PM, as DMA slave drivers should not prepare descriptors > if they don't need to use them. However that's a long term plan, and we need > a > solution sooner than that. > > I've been toying with the idea of adding explicit open/close (or whatever we > would call them) operations to the DMA engine API. Those would be used by DMA > slave drivers to signal that they will start/stop using the DMA engine. > > If (1) we must start the DMA synchronously with a DMA slave call, (2) need to > sleep to handle PM, and (3) don't want to keep the DMA engine powered for as > long as one channel is requested, then we need to turn at least preparation > as > not callable in atomic context, or introduce a new operation. > > [1] http://www.spinics.net/lists/dmaengine/msg01548.html That makes sense. However I am not familiar with DMA core code as much as I think it would be needed to make such generic changes :). I'll stick to one driver for now. Thanks for comments! Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option
On sob, 2014-11-01 at 01:42 +0100, Rafael J. Wysocki wrote: > On Friday, October 31, 2014 11:04:52 PM Russell King - ARM Linux wrote: > > On Sat, Nov 01, 2014 at 12:11:05AM +0100, Rafael J. Wysocki wrote: > > > [CC list trimmed + added Kevin Hilman] > > > > > > On Monday, October 20, 2014 11:04:44 AM Krzysztof Kozlowski wrote: > > > > Add a simple getter pm_runtime_is_irq_safe() for querying whether > > > > runtime > > > > PM IRQ safe was set or not. > > > > > > > > Various bus drivers implementing runtime PM may use choose to suspend > > > > differently based on IRQ safeness status of child driver (e.g. do not > > > > unprepare the clock if IRQ safe is not set). > > > > > > > > Signed-off-by: Krzysztof Kozlowski > > > > Reviewed-by: Ulf Hansson > > > > > > So why do we need to add the wrapper? > > > > > > And it goes kind of against the intention which was to set irq_safe when > > > we knew that the callbacks were safe to be executed from interrupt context > > > and not when we wished that to be the case. > > > > This was provided in the covering email - I quote: > > > > This patchset adds runtime and system PM to the pl330 driver. > > > > The runtime PM of pl330 driver requires interrupt safe suspend/resume > > callbacks which is in conflict with current amba bus driver. > > The latter also unprepares and prepares the AMBA bus clock which > > is not safe for atomic context. > > > > The patchset solves this in patch 3/5 by handling clocks in different > > way if device driver set interrupt safe runtime PM. > > So I'm still unsure why we need the wrapper. IMHO this check in particular: > > WARN_ON(pcdev->irq_safe != pm_runtime_is_irq_safe(dev)); > > (and should that be WARN_ON_ONCE(), for that matter?), looks better this way: > > WARN_ON(pcdev->irq_safe != dev->power.irq_safe); > > and so on, pretty much. I used the wrapper only to hide the actual code behind interface but it don't really matter to me. > Besides, these special "irq safe" code paths in the bus type look > considerably ugly to me. I'd probably use an "irq safe" PM domain for > that device and put it in there instead of doing the > > pcdev->irq_safe = pm_runtime_is_irq_safe(dev); > > thing in amba_probe(). But that's just me. :-) The device is not attached to any domain and there is no hardware domain matching. Thanks for feedback! Best regards, Krzysztof > > There's one weak point in [3/5], but let me comment it in there. > > Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option
Hi Krzysztof, On Friday 31 October 2014 15:40:16 Krzysztof Kozlowski wrote: > On pią, 2014-10-31 at 15:22 +0100, Pavel Machek wrote: > > On Fri 2014-10-31 10:14:55, Krzysztof Kozlowski wrote: > >> On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: > >>> Add a simple getter pm_runtime_is_irq_safe() for querying whether > >>> runtime PM IRQ safe was set or not. > >>> > >>> Various bus drivers implementing runtime PM may use choose to suspend > >>> differently based on IRQ safeness status of child driver (e.g. do not > >>> unprepare the clock if IRQ safe is not set). > >>> > >>> Signed-off-by: Krzysztof Kozlowski > >>> Reviewed-by: Ulf Hansson > >> > >> Rafael, Len, Pavel, > >> > >> Is proposed API ok? Do you have any comments? > >> > >> I'll upload whole patchset to Russell's patch tracking system. However > >> an ack from PM maintainer is probably needed. > > > > I don't like the API. Having callbacks work in different context (irq > > / noirq) based on what another function reports is ugly. > > > > What is the penalty if we always decide callbacks are not IRQ safe? > > Then pm_runtime_get_sync() could not be called in atomic context. The > pl330 runtime PM would have to be completely reworked because one > pm_runtime_get_sync() is called in device_issue_pending which cannot > sleep (at least in non preemptible kernels). Probably this can be solved > some way... Many other drivers suffer from the same problem. While I won't reject your proposed fix, I would prefer a more generic approach. One option that has been discussed previously was to use a work queue to delay starting the DMA transfer to an interruptible context where pm_runtime_get_sync() could be called. However, as Russell pointed out [1], even that won't work in all cases as the DMA slave might need the transfer to be started before enabling part of its hardware (OMAP audio seem to be such a case). I've heard a rumor of a possible DMA engine rework to forbid calling the descriptor preparation API from atomic context. This could be used as a base to implement runtime PM, as DMA slave drivers should not prepare descriptors if they don't need to use them. However that's a long term plan, and we need a solution sooner than that. I've been toying with the idea of adding explicit open/close (or whatever we would call them) operations to the DMA engine API. Those would be used by DMA slave drivers to signal that they will start/stop using the DMA engine. If (1) we must start the DMA synchronously with a DMA slave call, (2) need to sleep to handle PM, and (3) don't want to keep the DMA engine powered for as long as one channel is requested, then we need to turn at least preparation as not callable in atomic context, or introduce a new operation. [1] http://www.spinics.net/lists/dmaengine/msg01548.html > >>> --- a/Documentation/power/runtime_pm.txt > >>> +++ b/Documentation/power/runtime_pm.txt > >>> > >>> @@ -468,6 +468,10 @@ drivers/base/power/runtime.c and > >>> include/linux/pm_runtime.h: > >>> - set the power.irq_safe flag for the device, causing the > >>> runtime-PM > >>> > >>>callbacks to be invoked with interrupts off > >>> > >>> + bool pm_runtime_is_irq_safe(struct device *dev); > >>> +- return true if power.irq_safe flag was set for the device, > >>> causing > >>> + the runtime-PM callbacks to be invoked with interrupts off > >>> + > >>> > >>>void pm_runtime_mark_last_busy(struct device *dev); > >>> > >>> - set the power.last_busy field to the current time -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option
On Friday, October 31, 2014 11:04:52 PM Russell King - ARM Linux wrote: > On Sat, Nov 01, 2014 at 12:11:05AM +0100, Rafael J. Wysocki wrote: > > [CC list trimmed + added Kevin Hilman] > > > > On Monday, October 20, 2014 11:04:44 AM Krzysztof Kozlowski wrote: > > > Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime > > > PM IRQ safe was set or not. > > > > > > Various bus drivers implementing runtime PM may use choose to suspend > > > differently based on IRQ safeness status of child driver (e.g. do not > > > unprepare the clock if IRQ safe is not set). > > > > > > Signed-off-by: Krzysztof Kozlowski > > > Reviewed-by: Ulf Hansson > > > > So why do we need to add the wrapper? > > > > And it goes kind of against the intention which was to set irq_safe when > > we knew that the callbacks were safe to be executed from interrupt context > > and not when we wished that to be the case. > > This was provided in the covering email - I quote: > > This patchset adds runtime and system PM to the pl330 driver. > > The runtime PM of pl330 driver requires interrupt safe suspend/resume > callbacks which is in conflict with current amba bus driver. > The latter also unprepares and prepares the AMBA bus clock which > is not safe for atomic context. > > The patchset solves this in patch 3/5 by handling clocks in different > way if device driver set interrupt safe runtime PM. So I'm still unsure why we need the wrapper. IMHO this check in particular: WARN_ON(pcdev->irq_safe != pm_runtime_is_irq_safe(dev)); (and should that be WARN_ON_ONCE(), for that matter?), looks better this way: WARN_ON(pcdev->irq_safe != dev->power.irq_safe); and so on, pretty much. Besides, these special "irq safe" code paths in the bus type look considerably ugly to me. I'd probably use an "irq safe" PM domain for that device and put it in there instead of doing the pcdev->irq_safe = pm_runtime_is_irq_safe(dev); thing in amba_probe(). But that's just me. :-) There's one weak point in [3/5], but let me comment it in there. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option
On Sat, Nov 01, 2014 at 12:11:05AM +0100, Rafael J. Wysocki wrote: > [CC list trimmed + added Kevin Hilman] > > On Monday, October 20, 2014 11:04:44 AM Krzysztof Kozlowski wrote: > > Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime > > PM IRQ safe was set or not. > > > > Various bus drivers implementing runtime PM may use choose to suspend > > differently based on IRQ safeness status of child driver (e.g. do not > > unprepare the clock if IRQ safe is not set). > > > > Signed-off-by: Krzysztof Kozlowski > > Reviewed-by: Ulf Hansson > > So why do we need to add the wrapper? > > And it goes kind of against the intention which was to set irq_safe when > we knew that the callbacks were safe to be executed from interrupt context > and not when we wished that to be the case. This was provided in the covering email - I quote: This patchset adds runtime and system PM to the pl330 driver. The runtime PM of pl330 driver requires interrupt safe suspend/resume callbacks which is in conflict with current amba bus driver. The latter also unprepares and prepares the AMBA bus clock which is not safe for atomic context. The patchset solves this in patch 3/5 by handling clocks in different way if device driver set interrupt safe runtime PM. -- FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option
[CC list trimmed + added Kevin Hilman]
On Monday, October 20, 2014 11:04:44 AM Krzysztof Kozlowski wrote:
> Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime
> PM IRQ safe was set or not.
>
> Various bus drivers implementing runtime PM may use choose to suspend
> differently based on IRQ safeness status of child driver (e.g. do not
> unprepare the clock if IRQ safe is not set).
>
> Signed-off-by: Krzysztof Kozlowski
> Reviewed-by: Ulf Hansson
So why do we need to add the wrapper?
And it goes kind of against the intention which was to set irq_safe when
we knew that the callbacks were safe to be executed from interrupt context
and not when we wished that to be the case.
> ---
> Documentation/power/runtime_pm.txt | 4
> include/linux/pm_runtime.h | 6 ++
> 2 files changed, 10 insertions(+)
>
> diff --git a/Documentation/power/runtime_pm.txt
> b/Documentation/power/runtime_pm.txt
> index f32ce5419573..397b81593142 100644
> --- a/Documentation/power/runtime_pm.txt
> +++ b/Documentation/power/runtime_pm.txt
> @@ -468,6 +468,10 @@ drivers/base/power/runtime.c and
> include/linux/pm_runtime.h:
> - set the power.irq_safe flag for the device, causing the runtime-PM
>callbacks to be invoked with interrupts off
>
> + bool pm_runtime_is_irq_safe(struct device *dev);
> +- return true if power.irq_safe flag was set for the device, causing
> + the runtime-PM callbacks to be invoked with interrupts off
> +
>void pm_runtime_mark_last_busy(struct device *dev);
> - set the power.last_busy field to the current time
>
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 367f49b9a1c9..44d74f0f182e 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -128,6 +128,11 @@ static inline void pm_runtime_mark_last_busy(struct
> device *dev)
> ACCESS_ONCE(dev->power.last_busy) = jiffies;
> }
>
> +static inline bool pm_runtime_is_irq_safe(struct device *dev)
> +{
> + return dev->power.irq_safe;
> +}
> +
> #else /* !CONFIG_PM_RUNTIME */
>
> static inline int __pm_runtime_idle(struct device *dev, int rpmflags)
> @@ -167,6 +172,7 @@ static inline bool pm_runtime_enabled(struct device *dev)
> { return false; }
>
> static inline void pm_runtime_no_callbacks(struct device *dev) {}
> static inline void pm_runtime_irq_safe(struct device *dev) {}
> +static inline bool pm_runtime_is_irq_safe(struct device *dev) { return
> false; }
>
> static inline bool pm_runtime_callbacks_present(struct device *dev) { return
> false; }
> static inline void pm_runtime_mark_last_busy(struct device *dev) {}
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option
On pią, 2014-10-31 at 15:22 +0100, Pavel Machek wrote: > On Fri 2014-10-31 10:14:55, Krzysztof Kozlowski wrote: > > On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: > > > Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime > > > PM IRQ safe was set or not. > > > > > > Various bus drivers implementing runtime PM may use choose to suspend > > > differently based on IRQ safeness status of child driver (e.g. do not > > > unprepare the clock if IRQ safe is not set). > > > > > > Signed-off-by: Krzysztof Kozlowski > > > Reviewed-by: Ulf Hansson > > > > Rafael, Len, Pavel, > > > > Is proposed API ok? Do you have any comments? > > > > I'll upload whole patchset to Russell's patch tracking system. However > > an ack from PM maintainer is probably needed. > > I don't like the API. Having callbacks work in different context (irq > / noirq) based on what another function reports is ugly. > > What is the penalty if we always decide callbacks are not IRQ safe? Then pm_runtime_get_sync() could not be called in atomic context. The pl330 runtime PM would have to be completely reworked because one pm_runtime_get_sync() is called in device_issue_pending which cannot sleep (at least in non preemptible kernels). Probably this can be solved some way... Best regards, Krzysztof > > > > --- a/Documentation/power/runtime_pm.txt > > > +++ b/Documentation/power/runtime_pm.txt > > > @@ -468,6 +468,10 @@ drivers/base/power/runtime.c and > > > include/linux/pm_runtime.h: > > > - set the power.irq_safe flag for the device, causing the runtime-PM > > >callbacks to be invoked with interrupts off > > > > > > + bool pm_runtime_is_irq_safe(struct device *dev); > > > +- return true if power.irq_safe flag was set for the device, causing > > > + the runtime-PM callbacks to be invoked with interrupts off > > > + > > >void pm_runtime_mark_last_busy(struct device *dev); > > > - set the power.last_busy field to the current time > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option
On Fri 2014-10-31 10:14:55, Krzysztof Kozlowski wrote: > On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: > > Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime > > PM IRQ safe was set or not. > > > > Various bus drivers implementing runtime PM may use choose to suspend > > differently based on IRQ safeness status of child driver (e.g. do not > > unprepare the clock if IRQ safe is not set). > > > > Signed-off-by: Krzysztof Kozlowski > > Reviewed-by: Ulf Hansson > > Rafael, Len, Pavel, > > Is proposed API ok? Do you have any comments? > > I'll upload whole patchset to Russell's patch tracking system. However > an ack from PM maintainer is probably needed. I don't like the API. Having callbacks work in different context (irq / noirq) based on what another function reports is ugly. What is the penalty if we always decide callbacks are not IRQ safe? Pavel > > --- a/Documentation/power/runtime_pm.txt > > +++ b/Documentation/power/runtime_pm.txt > > @@ -468,6 +468,10 @@ drivers/base/power/runtime.c and > > include/linux/pm_runtime.h: > > - set the power.irq_safe flag for the device, causing the runtime-PM > >callbacks to be invoked with interrupts off > > > > + bool pm_runtime_is_irq_safe(struct device *dev); > > +- return true if power.irq_safe flag was set for the device, causing > > + the runtime-PM callbacks to be invoked with interrupts off > > + > >void pm_runtime_mark_last_busy(struct device *dev); > > - set the power.last_busy field to the current time > > -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option
On 31 October 2014 10:33, Russell King - ARM Linux wrote: > On Fri, Oct 31, 2014 at 10:29:36AM +0100, Ulf Hansson wrote: >> On 31 October 2014 10:14, Krzysztof Kozlowski >> wrote: >> > On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: >> >> Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime >> >> PM IRQ safe was set or not. >> >> >> >> Various bus drivers implementing runtime PM may use choose to suspend >> >> differently based on IRQ safeness status of child driver (e.g. do not >> >> unprepare the clock if IRQ safe is not set). >> >> >> >> Signed-off-by: Krzysztof Kozlowski >> >> Reviewed-by: Ulf Hansson >> > >> > Rafael, Len, Pavel, >> > >> > Is proposed API ok? Do you have any comments? >> > >> > I'll upload whole patchset to Russell's patch tracking system. However >> > an ack from PM maintainer is probably needed. >> >> I would actually prefer the opposite. It's better if we can take these >> patches through Rafaels tree. >> >> 1) I have posted patches for the amba bus, these may have merge >> conflicts with your changes. >> 2) We may start using the new runtime PM API within genpd as of now. > > Stop this broken thinking. The solution is simple. > > Put the generic runtime PM changes into the PM tree in a separate /clean/ > branch. Merge them into the PM-next branch. Notify me where I can > pull that /clean/ branch. > > I pull that clean branch into my tree, and then apply the AMBA specific > patches on top. > > That means Rafael has the PM changes, I have the dependent PM changes, > and I also have the AMBA changes, and no one ends up carrying changes > which are inappropriate. Okay, I have no concerns with this approach. I have also checked the potential upcoming merge conflict in the amba bus, since I doubt you will pull in bigger changes from his tree!? Anyway, that conflict should be trivial to resolve if that happens. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option
On Fri, Oct 31, 2014 at 10:29:36AM +0100, Ulf Hansson wrote: > On 31 October 2014 10:14, Krzysztof Kozlowski wrote: > > On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: > >> Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime > >> PM IRQ safe was set or not. > >> > >> Various bus drivers implementing runtime PM may use choose to suspend > >> differently based on IRQ safeness status of child driver (e.g. do not > >> unprepare the clock if IRQ safe is not set). > >> > >> Signed-off-by: Krzysztof Kozlowski > >> Reviewed-by: Ulf Hansson > > > > Rafael, Len, Pavel, > > > > Is proposed API ok? Do you have any comments? > > > > I'll upload whole patchset to Russell's patch tracking system. However > > an ack from PM maintainer is probably needed. > > I would actually prefer the opposite. It's better if we can take these > patches through Rafaels tree. > > 1) I have posted patches for the amba bus, these may have merge > conflicts with your changes. > 2) We may start using the new runtime PM API within genpd as of now. Stop this broken thinking. The solution is simple. Put the generic runtime PM changes into the PM tree in a separate /clean/ branch. Merge them into the PM-next branch. Notify me where I can pull that /clean/ branch. I pull that clean branch into my tree, and then apply the AMBA specific patches on top. That means Rafael has the PM changes, I have the dependent PM changes, and I also have the AMBA changes, and no one ends up carrying changes which are inappropriate. -- FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option
On pią, 2014-10-31 at 10:29 +0100, Ulf Hansson wrote: > On 31 October 2014 10:14, Krzysztof Kozlowski wrote: > > On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: > >> Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime > >> PM IRQ safe was set or not. > >> > >> Various bus drivers implementing runtime PM may use choose to suspend > >> differently based on IRQ safeness status of child driver (e.g. do not > >> unprepare the clock if IRQ safe is not set). > >> > >> Signed-off-by: Krzysztof Kozlowski > >> Reviewed-by: Ulf Hansson > > > > Rafael, Len, Pavel, > > > > Is proposed API ok? Do you have any comments? > > > > I'll upload whole patchset to Russell's patch tracking system. However > > an ack from PM maintainer is probably needed. > > I would actually prefer the opposite. It's better if we can take these > patches through Rafaels tree. > > 1) I have posted patches for the amba bus, these may have merge > conflicts with your changes. > 2) We may start using the new runtime PM API within genpd as of now. I don't mind and it makes sense to me. This way I need an ack from Russell or Vinod... Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option
On 31 October 2014 10:14, Krzysztof Kozlowski wrote: > On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: >> Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime >> PM IRQ safe was set or not. >> >> Various bus drivers implementing runtime PM may use choose to suspend >> differently based on IRQ safeness status of child driver (e.g. do not >> unprepare the clock if IRQ safe is not set). >> >> Signed-off-by: Krzysztof Kozlowski >> Reviewed-by: Ulf Hansson > > Rafael, Len, Pavel, > > Is proposed API ok? Do you have any comments? > > I'll upload whole patchset to Russell's patch tracking system. However > an ack from PM maintainer is probably needed. I would actually prefer the opposite. It's better if we can take these patches through Rafaels tree. 1) I have posted patches for the amba bus, these may have merge conflicts with your changes. 2) We may start using the new runtime PM API within genpd as of now. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option
On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote:
> Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime
> PM IRQ safe was set or not.
>
> Various bus drivers implementing runtime PM may use choose to suspend
> differently based on IRQ safeness status of child driver (e.g. do not
> unprepare the clock if IRQ safe is not set).
>
> Signed-off-by: Krzysztof Kozlowski
> Reviewed-by: Ulf Hansson
Rafael, Len, Pavel,
Is proposed API ok? Do you have any comments?
I'll upload whole patchset to Russell's patch tracking system. However
an ack from PM maintainer is probably needed.
Best regards,
Krzysztof
> ---
> Documentation/power/runtime_pm.txt | 4
> include/linux/pm_runtime.h | 6 ++
> 2 files changed, 10 insertions(+)
>
> diff --git a/Documentation/power/runtime_pm.txt
> b/Documentation/power/runtime_pm.txt
> index f32ce5419573..397b81593142 100644
> --- a/Documentation/power/runtime_pm.txt
> +++ b/Documentation/power/runtime_pm.txt
> @@ -468,6 +468,10 @@ drivers/base/power/runtime.c and
> include/linux/pm_runtime.h:
> - set the power.irq_safe flag for the device, causing the runtime-PM
>callbacks to be invoked with interrupts off
>
> + bool pm_runtime_is_irq_safe(struct device *dev);
> +- return true if power.irq_safe flag was set for the device, causing
> + the runtime-PM callbacks to be invoked with interrupts off
> +
>void pm_runtime_mark_last_busy(struct device *dev);
> - set the power.last_busy field to the current time
>
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 367f49b9a1c9..44d74f0f182e 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -128,6 +128,11 @@ static inline void pm_runtime_mark_last_busy(struct
> device *dev)
> ACCESS_ONCE(dev->power.last_busy) = jiffies;
> }
>
> +static inline bool pm_runtime_is_irq_safe(struct device *dev)
> +{
> + return dev->power.irq_safe;
> +}
> +
> #else /* !CONFIG_PM_RUNTIME */
>
> static inline int __pm_runtime_idle(struct device *dev, int rpmflags)
> @@ -167,6 +172,7 @@ static inline bool pm_runtime_enabled(struct device *dev)
> { return false; }
>
> static inline void pm_runtime_no_callbacks(struct device *dev) {}
> static inline void pm_runtime_irq_safe(struct device *dev) {}
> +static inline bool pm_runtime_is_irq_safe(struct device *dev) { return
> false; }
>
> static inline bool pm_runtime_callbacks_present(struct device *dev) { return
> false; }
> static inline void pm_runtime_mark_last_busy(struct device *dev) {}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
