Re: [PATCH v8 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On 4 November 2014 14:59, Rafael J. Wysocki wrote: > On Tuesday, November 04, 2014 10:11:35 AM Ulf Hansson wrote: >> On 4 November 2014 02:57, Rafael J. Wysocki wrote: >> > On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote: >> >> On Mon, 3 Nov 2014, Russell King - ARM Linux wrote: >> >> >> >> > That makes it pretty horrid from the point of view of having bus >> >> > management code, because we now have the management of the bus clock >> >> > split between the bus layer and the device driver. >> >> > >> >> > This is /really/ a problem for runtime PM. Runtime PM permits there >> >> > to be a bus layer involved - and runtime PM can also be coupled up >> >> > to PM domains as well. For all this stuff, the context which the >> >> > callbacks are called in depends on whether the driver itself has >> >> > marked the device as having IRQ-safe callbacks. >> >> > >> >> > That's fine, but the bus and PM domain level code then /really/ needs >> >> > to know what context they're being called in, so they know whether >> >> > they can sleep or not, or they must to be written to always use >> >> > non-sleeping functions so they work in both contexts. If we assume >> >> > the former, then that implies that the irq-safe flag must never change >> >> > state between a suspend and a resume. >> >> >> >> If a bus subsystem or PM domain is going to allow its drivers to choose >> >> between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the >> >> subsystem to come up with a way for drivers to indicate their choice. >> >> >> >> I tend to agree with Rafael that testing dev->power.irq_safe should be >> >> good enough, with no real need for a wrapper. But the subsystem can >> >> use a different mechanism if it wants. >> >> >> >> Bear in mind, however, that once the irq_safe flag has been set, the >> >> runtime PM core offers no way to turn it off again. >> > >> > There is a problem with it, though. Say, a driver handles a device that >> > may or may not be in a power domain. Or in other words, the power domain >> > the device is in may or may not be always on. If the domain is always on, >> > the runtime PM callbacks are IRQ-safe (they depend on the driver only). >> > If it isn't, they may not be IRQ-safe. How's the driver going to decide >> > whether or not to set power.irq_safe? >> >> From my point of view; the decision whether the driver will set the >> IRQ safe flag is in principle a software design choice. >> >> Currently genpd isn't able to power off, if one of its devices are IRQ >> safe configured. That's a limitation in genpd which we need to fix and >> it's on my TODO list. >> >> My point is thus, I don't think the driver should care about PM >> domains at all regarding using the IRQ safe option. Does that make >> sense? > > Yes, it does, and that's the heart of the problem above. The driver should > not care about wherther or not the device is in a power domain, but it needs > to know that when deciding whether or not to set power.irq_safe. Catch 22. Why is it catch22? The problem is supposed to be fixed in the generic PM domain. How we do that is a different question. Until genpd get fixed, the driver can still keep using irq_safe if they want to. It will only lead to limitations if the device is attached to a genpd. 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On Tuesday, November 04, 2014 10:11:35 AM Ulf Hansson wrote: > On 4 November 2014 02:57, Rafael J. Wysocki wrote: > > On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote: > >> On Mon, 3 Nov 2014, Russell King - ARM Linux wrote: > >> > >> > That makes it pretty horrid from the point of view of having bus > >> > management code, because we now have the management of the bus clock > >> > split between the bus layer and the device driver. > >> > > >> > This is /really/ a problem for runtime PM. Runtime PM permits there > >> > to be a bus layer involved - and runtime PM can also be coupled up > >> > to PM domains as well. For all this stuff, the context which the > >> > callbacks are called in depends on whether the driver itself has > >> > marked the device as having IRQ-safe callbacks. > >> > > >> > That's fine, but the bus and PM domain level code then /really/ needs > >> > to know what context they're being called in, so they know whether > >> > they can sleep or not, or they must to be written to always use > >> > non-sleeping functions so they work in both contexts. If we assume > >> > the former, then that implies that the irq-safe flag must never change > >> > state between a suspend and a resume. > >> > >> If a bus subsystem or PM domain is going to allow its drivers to choose > >> between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the > >> subsystem to come up with a way for drivers to indicate their choice. > >> > >> I tend to agree with Rafael that testing dev->power.irq_safe should be > >> good enough, with no real need for a wrapper. But the subsystem can > >> use a different mechanism if it wants. > >> > >> Bear in mind, however, that once the irq_safe flag has been set, the > >> runtime PM core offers no way to turn it off again. > > > > There is a problem with it, though. Say, a driver handles a device that > > may or may not be in a power domain. Or in other words, the power domain > > the device is in may or may not be always on. If the domain is always on, > > the runtime PM callbacks are IRQ-safe (they depend on the driver only). > > If it isn't, they may not be IRQ-safe. How's the driver going to decide > > whether or not to set power.irq_safe? > > From my point of view; the decision whether the driver will set the > IRQ safe flag is in principle a software design choice. > > Currently genpd isn't able to power off, if one of its devices are IRQ > safe configured. That's a limitation in genpd which we need to fix and > it's on my TODO list. > > My point is thus, I don't think the driver should care about PM > domains at all regarding using the IRQ safe option. Does that make > sense? Yes, it does, and that's the heart of the problem above. The driver should not care about wherther or not the device is in a power domain, but it needs to know that when deciding whether or not to set power.irq_safe. Catch 22. -- 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On 4 November 2014 02:57, Rafael J. Wysocki wrote: > On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote: >> On Mon, 3 Nov 2014, Russell King - ARM Linux wrote: >> >> > That makes it pretty horrid from the point of view of having bus >> > management code, because we now have the management of the bus clock >> > split between the bus layer and the device driver. >> > >> > This is /really/ a problem for runtime PM. Runtime PM permits there >> > to be a bus layer involved - and runtime PM can also be coupled up >> > to PM domains as well. For all this stuff, the context which the >> > callbacks are called in depends on whether the driver itself has >> > marked the device as having IRQ-safe callbacks. >> > >> > That's fine, but the bus and PM domain level code then /really/ needs >> > to know what context they're being called in, so they know whether >> > they can sleep or not, or they must to be written to always use >> > non-sleeping functions so they work in both contexts. If we assume >> > the former, then that implies that the irq-safe flag must never change >> > state between a suspend and a resume. >> >> If a bus subsystem or PM domain is going to allow its drivers to choose >> between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the >> subsystem to come up with a way for drivers to indicate their choice. >> >> I tend to agree with Rafael that testing dev->power.irq_safe should be >> good enough, with no real need for a wrapper. But the subsystem can >> use a different mechanism if it wants. >> >> Bear in mind, however, that once the irq_safe flag has been set, the >> runtime PM core offers no way to turn it off again. > > There is a problem with it, though. Say, a driver handles a device that > may or may not be in a power domain. Or in other words, the power domain > the device is in may or may not be always on. If the domain is always on, > the runtime PM callbacks are IRQ-safe (they depend on the driver only). > If it isn't, they may not be IRQ-safe. How's the driver going to decide > whether or not to set power.irq_safe? >From my point of view; the decision whether the driver will set the IRQ safe flag is in principle a software design choice. Currently genpd isn't able to power off, if one of its devices are IRQ safe configured. That's a limitation in genpd which we need to fix and it's on my TODO list. My point is thus, I don't think the driver should care about PM domains at all regarding using the IRQ safe option. Does that make sense? 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On wto, 2014-11-04 at 02:57 +0100, Rafael J. Wysocki wrote: > On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote: > > On Mon, 3 Nov 2014, Russell King - ARM Linux wrote: > > > > > That makes it pretty horrid from the point of view of having bus > > > management code, because we now have the management of the bus clock > > > split between the bus layer and the device driver. > > > > > > This is /really/ a problem for runtime PM. Runtime PM permits there > > > to be a bus layer involved - and runtime PM can also be coupled up > > > to PM domains as well. For all this stuff, the context which the > > > callbacks are called in depends on whether the driver itself has > > > marked the device as having IRQ-safe callbacks. > > > > > > That's fine, but the bus and PM domain level code then /really/ needs > > > to know what context they're being called in, so they know whether > > > they can sleep or not, or they must to be written to always use > > > non-sleeping functions so they work in both contexts. If we assume > > > the former, then that implies that the irq-safe flag must never change > > > state between a suspend and a resume. > > > > If a bus subsystem or PM domain is going to allow its drivers to choose > > between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the > > subsystem to come up with a way for drivers to indicate their choice. > > > > I tend to agree with Rafael that testing dev->power.irq_safe should be > > good enough, with no real need for a wrapper. But the subsystem can > > use a different mechanism if it wants. > > > > Bear in mind, however, that once the irq_safe flag has been set, the > > runtime PM core offers no way to turn it off again. > > There is a problem with it, though. Say, a driver handles a device that > may or may not be in a power domain. Or in other words, the power domain > the device is in may or may not be always on. If the domain is always on, > the runtime PM callbacks are IRQ-safe (they depend on the driver only). > If it isn't, they may not be IRQ-safe. How's the driver going to decide > whether or not to set power.irq_safe? The pl330 driver (being a part of amba bus) always wants IRQ safe callbacks. However other amba drivers may not. So actually the driver always sets one or another. The dualism is present only in the amba/bus.c driver which encapsulates the child drivers. 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On wto, 2014-11-04 at 02:57 +0100, Rafael J. Wysocki wrote: On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote: On Mon, 3 Nov 2014, Russell King - ARM Linux wrote: That makes it pretty horrid from the point of view of having bus management code, because we now have the management of the bus clock split between the bus layer and the device driver. This is /really/ a problem for runtime PM. Runtime PM permits there to be a bus layer involved - and runtime PM can also be coupled up to PM domains as well. For all this stuff, the context which the callbacks are called in depends on whether the driver itself has marked the device as having IRQ-safe callbacks. That's fine, but the bus and PM domain level code then /really/ needs to know what context they're being called in, so they know whether they can sleep or not, or they must to be written to always use non-sleeping functions so they work in both contexts. If we assume the former, then that implies that the irq-safe flag must never change state between a suspend and a resume. If a bus subsystem or PM domain is going to allow its drivers to choose between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the subsystem to come up with a way for drivers to indicate their choice. I tend to agree with Rafael that testing dev-power.irq_safe should be good enough, with no real need for a wrapper. But the subsystem can use a different mechanism if it wants. Bear in mind, however, that once the irq_safe flag has been set, the runtime PM core offers no way to turn it off again. There is a problem with it, though. Say, a driver handles a device that may or may not be in a power domain. Or in other words, the power domain the device is in may or may not be always on. If the domain is always on, the runtime PM callbacks are IRQ-safe (they depend on the driver only). If it isn't, they may not be IRQ-safe. How's the driver going to decide whether or not to set power.irq_safe? The pl330 driver (being a part of amba bus) always wants IRQ safe callbacks. However other amba drivers may not. So actually the driver always sets one or another. The dualism is present only in the amba/bus.c driver which encapsulates the child drivers. 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On 4 November 2014 02:57, Rafael J. Wysocki r...@rjwysocki.net wrote: On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote: On Mon, 3 Nov 2014, Russell King - ARM Linux wrote: That makes it pretty horrid from the point of view of having bus management code, because we now have the management of the bus clock split between the bus layer and the device driver. This is /really/ a problem for runtime PM. Runtime PM permits there to be a bus layer involved - and runtime PM can also be coupled up to PM domains as well. For all this stuff, the context which the callbacks are called in depends on whether the driver itself has marked the device as having IRQ-safe callbacks. That's fine, but the bus and PM domain level code then /really/ needs to know what context they're being called in, so they know whether they can sleep or not, or they must to be written to always use non-sleeping functions so they work in both contexts. If we assume the former, then that implies that the irq-safe flag must never change state between a suspend and a resume. If a bus subsystem or PM domain is going to allow its drivers to choose between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the subsystem to come up with a way for drivers to indicate their choice. I tend to agree with Rafael that testing dev-power.irq_safe should be good enough, with no real need for a wrapper. But the subsystem can use a different mechanism if it wants. Bear in mind, however, that once the irq_safe flag has been set, the runtime PM core offers no way to turn it off again. There is a problem with it, though. Say, a driver handles a device that may or may not be in a power domain. Or in other words, the power domain the device is in may or may not be always on. If the domain is always on, the runtime PM callbacks are IRQ-safe (they depend on the driver only). If it isn't, they may not be IRQ-safe. How's the driver going to decide whether or not to set power.irq_safe? From my point of view; the decision whether the driver will set the IRQ safe flag is in principle a software design choice. Currently genpd isn't able to power off, if one of its devices are IRQ safe configured. That's a limitation in genpd which we need to fix and it's on my TODO list. My point is thus, I don't think the driver should care about PM domains at all regarding using the IRQ safe option. Does that make sense? 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On Tuesday, November 04, 2014 10:11:35 AM Ulf Hansson wrote: On 4 November 2014 02:57, Rafael J. Wysocki r...@rjwysocki.net wrote: On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote: On Mon, 3 Nov 2014, Russell King - ARM Linux wrote: That makes it pretty horrid from the point of view of having bus management code, because we now have the management of the bus clock split between the bus layer and the device driver. This is /really/ a problem for runtime PM. Runtime PM permits there to be a bus layer involved - and runtime PM can also be coupled up to PM domains as well. For all this stuff, the context which the callbacks are called in depends on whether the driver itself has marked the device as having IRQ-safe callbacks. That's fine, but the bus and PM domain level code then /really/ needs to know what context they're being called in, so they know whether they can sleep or not, or they must to be written to always use non-sleeping functions so they work in both contexts. If we assume the former, then that implies that the irq-safe flag must never change state between a suspend and a resume. If a bus subsystem or PM domain is going to allow its drivers to choose between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the subsystem to come up with a way for drivers to indicate their choice. I tend to agree with Rafael that testing dev-power.irq_safe should be good enough, with no real need for a wrapper. But the subsystem can use a different mechanism if it wants. Bear in mind, however, that once the irq_safe flag has been set, the runtime PM core offers no way to turn it off again. There is a problem with it, though. Say, a driver handles a device that may or may not be in a power domain. Or in other words, the power domain the device is in may or may not be always on. If the domain is always on, the runtime PM callbacks are IRQ-safe (they depend on the driver only). If it isn't, they may not be IRQ-safe. How's the driver going to decide whether or not to set power.irq_safe? From my point of view; the decision whether the driver will set the IRQ safe flag is in principle a software design choice. Currently genpd isn't able to power off, if one of its devices are IRQ safe configured. That's a limitation in genpd which we need to fix and it's on my TODO list. My point is thus, I don't think the driver should care about PM domains at all regarding using the IRQ safe option. Does that make sense? Yes, it does, and that's the heart of the problem above. The driver should not care about wherther or not the device is in a power domain, but it needs to know that when deciding whether or not to set power.irq_safe. Catch 22. -- 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On 4 November 2014 14:59, Rafael J. Wysocki r...@rjwysocki.net wrote: On Tuesday, November 04, 2014 10:11:35 AM Ulf Hansson wrote: On 4 November 2014 02:57, Rafael J. Wysocki r...@rjwysocki.net wrote: On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote: On Mon, 3 Nov 2014, Russell King - ARM Linux wrote: That makes it pretty horrid from the point of view of having bus management code, because we now have the management of the bus clock split between the bus layer and the device driver. This is /really/ a problem for runtime PM. Runtime PM permits there to be a bus layer involved - and runtime PM can also be coupled up to PM domains as well. For all this stuff, the context which the callbacks are called in depends on whether the driver itself has marked the device as having IRQ-safe callbacks. That's fine, but the bus and PM domain level code then /really/ needs to know what context they're being called in, so they know whether they can sleep or not, or they must to be written to always use non-sleeping functions so they work in both contexts. If we assume the former, then that implies that the irq-safe flag must never change state between a suspend and a resume. If a bus subsystem or PM domain is going to allow its drivers to choose between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the subsystem to come up with a way for drivers to indicate their choice. I tend to agree with Rafael that testing dev-power.irq_safe should be good enough, with no real need for a wrapper. But the subsystem can use a different mechanism if it wants. Bear in mind, however, that once the irq_safe flag has been set, the runtime PM core offers no way to turn it off again. There is a problem with it, though. Say, a driver handles a device that may or may not be in a power domain. Or in other words, the power domain the device is in may or may not be always on. If the domain is always on, the runtime PM callbacks are IRQ-safe (they depend on the driver only). If it isn't, they may not be IRQ-safe. How's the driver going to decide whether or not to set power.irq_safe? From my point of view; the decision whether the driver will set the IRQ safe flag is in principle a software design choice. Currently genpd isn't able to power off, if one of its devices are IRQ safe configured. That's a limitation in genpd which we need to fix and it's on my TODO list. My point is thus, I don't think the driver should care about PM domains at all regarding using the IRQ safe option. Does that make sense? Yes, it does, and that's the heart of the problem above. The driver should not care about wherther or not the device is in a power domain, but it needs to know that when deciding whether or not to set power.irq_safe. Catch 22. Why is it catch22? The problem is supposed to be fixed in the generic PM domain. How we do that is a different question. Until genpd get fixed, the driver can still keep using irq_safe if they want to. It will only lead to limitations if the device is attached to a genpd. 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On pon, 2014-11-03 at 15:44 +, Russell King - ARM Linux wrote: > On Mon, Nov 03, 2014 at 10:41:02AM -0500, Alan Stern wrote: > > Bear in mind, however, that once the irq_safe flag has been set, the > > runtime PM core offers no way to turn it off again. > > Ah, I thought it did permit it to change both ways. In that case, we > don't need to validate that it doesn't change state on each call, and > we can just get away with checking its value. It cannot be unset but still it could be *set* during runtime (not only in probe). However that shouldn't happen between suspend-resume calls, so the solution of undoing suspend's work seems fine. I'll send a new patch doing this way. Without the wrapper. 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote: > On Mon, 3 Nov 2014, Russell King - ARM Linux wrote: > > > That makes it pretty horrid from the point of view of having bus > > management code, because we now have the management of the bus clock > > split between the bus layer and the device driver. > > > > This is /really/ a problem for runtime PM. Runtime PM permits there > > to be a bus layer involved - and runtime PM can also be coupled up > > to PM domains as well. For all this stuff, the context which the > > callbacks are called in depends on whether the driver itself has > > marked the device as having IRQ-safe callbacks. > > > > That's fine, but the bus and PM domain level code then /really/ needs > > to know what context they're being called in, so they know whether > > they can sleep or not, or they must to be written to always use > > non-sleeping functions so they work in both contexts. If we assume > > the former, then that implies that the irq-safe flag must never change > > state between a suspend and a resume. > > If a bus subsystem or PM domain is going to allow its drivers to choose > between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the > subsystem to come up with a way for drivers to indicate their choice. > > I tend to agree with Rafael that testing dev->power.irq_safe should be > good enough, with no real need for a wrapper. But the subsystem can > use a different mechanism if it wants. > > Bear in mind, however, that once the irq_safe flag has been set, the > runtime PM core offers no way to turn it off again. There is a problem with it, though. Say, a driver handles a device that may or may not be in a power domain. Or in other words, the power domain the device is in may or may not be always on. If the domain is always on, the runtime PM callbacks are IRQ-safe (they depend on the driver only). If it isn't, they may not be IRQ-safe. How's the driver going to decide whether or not to set power.irq_safe? 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
Russell King - ARM Linux writes: [...] > Would another possible solution be to remember the irq-safeness in the > suspend handler, and use that in the resume handler? Resume should > /always/ undo what the suspend handler previously did wrt clk API stuff. This seems more reasonable to me. Currently, the setting of irq_safe-ness is permanent (there's no exposed API to unset it after you set it.) so assuming it to be the same across suspend resume is a safe assumption. Kevin -- 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On Mon, Nov 03, 2014 at 10:41:02AM -0500, Alan Stern wrote: > Bear in mind, however, that once the irq_safe flag has been set, the > runtime PM core offers no way to turn it off again. Ah, I thought it did permit it to change both ways. In that case, we don't need to validate that it doesn't change state on each call, and we can just get away with checking its value. -- 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On Mon, 3 Nov 2014, Russell King - ARM Linux wrote: > That makes it pretty horrid from the point of view of having bus > management code, because we now have the management of the bus clock > split between the bus layer and the device driver. > > This is /really/ a problem for runtime PM. Runtime PM permits there > to be a bus layer involved - and runtime PM can also be coupled up > to PM domains as well. For all this stuff, the context which the > callbacks are called in depends on whether the driver itself has > marked the device as having IRQ-safe callbacks. > > That's fine, but the bus and PM domain level code then /really/ needs > to know what context they're being called in, so they know whether > they can sleep or not, or they must to be written to always use > non-sleeping functions so they work in both contexts. If we assume > the former, then that implies that the irq-safe flag must never change > state between a suspend and a resume. If a bus subsystem or PM domain is going to allow its drivers to choose between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the subsystem to come up with a way for drivers to indicate their choice. I tend to agree with Rafael that testing dev->power.irq_safe should be good enough, with no real need for a wrapper. But the subsystem can use a different mechanism if it wants. Bear in mind, however, that once the irq_safe flag has been set, the runtime PM core offers no way to turn it off again. Alan Stern -- 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On Mon, Nov 03, 2014 at 09:36:45AM +0100, Krzysztof Kozlowski wrote: > On sob, 2014-11-01 at 01:01 +, Russell King - ARM Linux wrote: > > Would another possible solution be to remember the irq-safeness in the > > suspend handler, and use that in the resume handler? Resume should > > /always/ undo what the suspend handler previously did wrt clk API stuff. > > I think the second solution could be more readable. The WARN_ON wouldn't > be needed. However this won't solve the two dual nature of runtime > callbacks. > > I wondered also about removing runtime PM callbacks from amba/bus.c > completely and moving this to child drivers. This way runtime PM would > be obvious in each driver case. That makes it pretty horrid from the point of view of having bus management code, because we now have the management of the bus clock split between the bus layer and the device driver. This is /really/ a problem for runtime PM. Runtime PM permits there to be a bus layer involved - and runtime PM can also be coupled up to PM domains as well. For all this stuff, the context which the callbacks are called in depends on whether the driver itself has marked the device as having IRQ-safe callbacks. That's fine, but the bus and PM domain level code then /really/ needs to know what context they're being called in, so they know whether they can sleep or not, or they must to be written to always use non-sleeping functions so they work in both contexts. If we assume the former, then that implies that the irq-safe flag must never change state between a suspend and a resume. -- 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On sob, 2014-11-01 at 01:01 +, Russell King - ARM Linux wrote:
> On Sat, Nov 01, 2014 at 12:55:14AM +, Russell King - ARM Linux wrote:
> > On Sat, Nov 01, 2014 at 01:45:47AM +0100, Rafael J. Wysocki wrote:
> > > On Monday, October 20, 2014 11:04:46 AM Krzysztof Kozlowski wrote:
> > > > @@ -198,8 +217,10 @@ static int amba_probe(struct device *dev)
> > > > pm_runtime_enable(dev);
> > > >
> > > > ret = pcdrv->probe(pcdev, id);
> > > > - if (ret == 0)
> > > > + if (ret == 0) {
> > > > + pcdev->irq_safe = pm_runtime_is_irq_safe(dev);
> > >
> > > This looks racy.
> > >
> > > Is it guaranteed that runtime PM callbacks won't be run for the device
> > > after pcdrv->probe() has returned and before setting pcdev->irq_safe?
> > > If not, inconsistent behavior may ensue.
> >
> > You are absolutely correct. So that knocks that idea on its head.
>
> Actually, I think we shouldn't give up hope here. Currently, we do this:
>
> pm_runtime_get_noresume(dev);
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
>
> ret = pcdrv->probe(pcdev, id);
>
> What we could do is:
>
> pm_runtime_get_noresume(dev);
> pm_runtime_get_noresume(dev);
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
>
> ret = pcdrv->probe(pcdev, id);
> if (ret == 0) {
> pcdev->irq_safe = pm_runtime_is_irq_safe(dev);
> pm_runtime_put(dev);
> break;
> }
>
> pm_runtime_disable(dev);
> pm_runtime_set_suspended(dev);
> pm_runtime_put_noidle(dev);
> pm_runtime_put_noidle(dev);
>
> which would ensure that we hold a usecount until after the probe function
> has returned. Would that work?
>
> I'll give you that it's pretty horrid.
> Would another possible solution be to remember the irq-safeness in the
> suspend handler, and use that in the resume handler? Resume should
> /always/ undo what the suspend handler previously did wrt clk API stuff.
I think the second solution could be more readable. The WARN_ON wouldn't
be needed. However this won't solve the two dual nature of runtime
callbacks.
I wondered also about removing runtime PM callbacks from amba/bus.c
completely and moving this to child drivers. This way runtime PM would
be obvious in each driver case.
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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On sob, 2014-11-01 at 01:01 +, Russell King - ARM Linux wrote:
On Sat, Nov 01, 2014 at 12:55:14AM +, Russell King - ARM Linux wrote:
On Sat, Nov 01, 2014 at 01:45:47AM +0100, Rafael J. Wysocki wrote:
On Monday, October 20, 2014 11:04:46 AM Krzysztof Kozlowski wrote:
@@ -198,8 +217,10 @@ static int amba_probe(struct device *dev)
pm_runtime_enable(dev);
ret = pcdrv-probe(pcdev, id);
- if (ret == 0)
+ if (ret == 0) {
+ pcdev-irq_safe = pm_runtime_is_irq_safe(dev);
This looks racy.
Is it guaranteed that runtime PM callbacks won't be run for the device
after pcdrv-probe() has returned and before setting pcdev-irq_safe?
If not, inconsistent behavior may ensue.
You are absolutely correct. So that knocks that idea on its head.
Actually, I think we shouldn't give up hope here. Currently, we do this:
pm_runtime_get_noresume(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
ret = pcdrv-probe(pcdev, id);
What we could do is:
pm_runtime_get_noresume(dev);
pm_runtime_get_noresume(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
ret = pcdrv-probe(pcdev, id);
if (ret == 0) {
pcdev-irq_safe = pm_runtime_is_irq_safe(dev);
pm_runtime_put(dev);
break;
}
pm_runtime_disable(dev);
pm_runtime_set_suspended(dev);
pm_runtime_put_noidle(dev);
pm_runtime_put_noidle(dev);
which would ensure that we hold a usecount until after the probe function
has returned. Would that work?
I'll give you that it's pretty horrid.
Would another possible solution be to remember the irq-safeness in the
suspend handler, and use that in the resume handler? Resume should
/always/ undo what the suspend handler previously did wrt clk API stuff.
I think the second solution could be more readable. The WARN_ON wouldn't
be needed. However this won't solve the two dual nature of runtime
callbacks.
I wondered also about removing runtime PM callbacks from amba/bus.c
completely and moving this to child drivers. This way runtime PM would
be obvious in each driver case.
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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On Mon, Nov 03, 2014 at 09:36:45AM +0100, Krzysztof Kozlowski wrote: On sob, 2014-11-01 at 01:01 +, Russell King - ARM Linux wrote: Would another possible solution be to remember the irq-safeness in the suspend handler, and use that in the resume handler? Resume should /always/ undo what the suspend handler previously did wrt clk API stuff. I think the second solution could be more readable. The WARN_ON wouldn't be needed. However this won't solve the two dual nature of runtime callbacks. I wondered also about removing runtime PM callbacks from amba/bus.c completely and moving this to child drivers. This way runtime PM would be obvious in each driver case. That makes it pretty horrid from the point of view of having bus management code, because we now have the management of the bus clock split between the bus layer and the device driver. This is /really/ a problem for runtime PM. Runtime PM permits there to be a bus layer involved - and runtime PM can also be coupled up to PM domains as well. For all this stuff, the context which the callbacks are called in depends on whether the driver itself has marked the device as having IRQ-safe callbacks. That's fine, but the bus and PM domain level code then /really/ needs to know what context they're being called in, so they know whether they can sleep or not, or they must to be written to always use non-sleeping functions so they work in both contexts. If we assume the former, then that implies that the irq-safe flag must never change state between a suspend and a resume. -- 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On Mon, 3 Nov 2014, Russell King - ARM Linux wrote: That makes it pretty horrid from the point of view of having bus management code, because we now have the management of the bus clock split between the bus layer and the device driver. This is /really/ a problem for runtime PM. Runtime PM permits there to be a bus layer involved - and runtime PM can also be coupled up to PM domains as well. For all this stuff, the context which the callbacks are called in depends on whether the driver itself has marked the device as having IRQ-safe callbacks. That's fine, but the bus and PM domain level code then /really/ needs to know what context they're being called in, so they know whether they can sleep or not, or they must to be written to always use non-sleeping functions so they work in both contexts. If we assume the former, then that implies that the irq-safe flag must never change state between a suspend and a resume. If a bus subsystem or PM domain is going to allow its drivers to choose between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the subsystem to come up with a way for drivers to indicate their choice. I tend to agree with Rafael that testing dev-power.irq_safe should be good enough, with no real need for a wrapper. But the subsystem can use a different mechanism if it wants. Bear in mind, however, that once the irq_safe flag has been set, the runtime PM core offers no way to turn it off again. Alan Stern -- 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On Mon, Nov 03, 2014 at 10:41:02AM -0500, Alan Stern wrote: Bear in mind, however, that once the irq_safe flag has been set, the runtime PM core offers no way to turn it off again. Ah, I thought it did permit it to change both ways. In that case, we don't need to validate that it doesn't change state on each call, and we can just get away with checking its value. -- 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
Russell King - ARM Linux li...@arm.linux.org.uk writes: [...] Would another possible solution be to remember the irq-safeness in the suspend handler, and use that in the resume handler? Resume should /always/ undo what the suspend handler previously did wrt clk API stuff. This seems more reasonable to me. Currently, the setting of irq_safe-ness is permanent (there's no exposed API to unset it after you set it.) so assuming it to be the same across suspend resume is a safe assumption. Kevin -- 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote: On Mon, 3 Nov 2014, Russell King - ARM Linux wrote: That makes it pretty horrid from the point of view of having bus management code, because we now have the management of the bus clock split between the bus layer and the device driver. This is /really/ a problem for runtime PM. Runtime PM permits there to be a bus layer involved - and runtime PM can also be coupled up to PM domains as well. For all this stuff, the context which the callbacks are called in depends on whether the driver itself has marked the device as having IRQ-safe callbacks. That's fine, but the bus and PM domain level code then /really/ needs to know what context they're being called in, so they know whether they can sleep or not, or they must to be written to always use non-sleeping functions so they work in both contexts. If we assume the former, then that implies that the irq-safe flag must never change state between a suspend and a resume. If a bus subsystem or PM domain is going to allow its drivers to choose between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the subsystem to come up with a way for drivers to indicate their choice. I tend to agree with Rafael that testing dev-power.irq_safe should be good enough, with no real need for a wrapper. But the subsystem can use a different mechanism if it wants. Bear in mind, however, that once the irq_safe flag has been set, the runtime PM core offers no way to turn it off again. There is a problem with it, though. Say, a driver handles a device that may or may not be in a power domain. Or in other words, the power domain the device is in may or may not be always on. If the domain is always on, the runtime PM callbacks are IRQ-safe (they depend on the driver only). If it isn't, they may not be IRQ-safe. How's the driver going to decide whether or not to set power.irq_safe? 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On pon, 2014-11-03 at 15:44 +, Russell King - ARM Linux wrote: On Mon, Nov 03, 2014 at 10:41:02AM -0500, Alan Stern wrote: Bear in mind, however, that once the irq_safe flag has been set, the runtime PM core offers no way to turn it off again. Ah, I thought it did permit it to change both ways. In that case, we don't need to validate that it doesn't change state on each call, and we can just get away with checking its value. It cannot be unset but still it could be *set* during runtime (not only in probe). However that shouldn't happen between suspend-resume calls, so the solution of undoing suspend's work seems fine. I'll send a new patch doing this way. Without the wrapper. 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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On Sat, Nov 01, 2014 at 12:55:14AM +, Russell King - ARM Linux wrote:
> On Sat, Nov 01, 2014 at 01:45:47AM +0100, Rafael J. Wysocki wrote:
> > On Monday, October 20, 2014 11:04:46 AM Krzysztof Kozlowski wrote:
> > > @@ -198,8 +217,10 @@ static int amba_probe(struct device *dev)
> > > pm_runtime_enable(dev);
> > >
> > > ret = pcdrv->probe(pcdev, id);
> > > - if (ret == 0)
> > > + if (ret == 0) {
> > > + pcdev->irq_safe = pm_runtime_is_irq_safe(dev);
> >
> > This looks racy.
> >
> > Is it guaranteed that runtime PM callbacks won't be run for the device
> > after pcdrv->probe() has returned and before setting pcdev->irq_safe?
> > If not, inconsistent behavior may ensue.
>
> You are absolutely correct. So that knocks that idea on its head.
Actually, I think we shouldn't give up hope here. Currently, we do this:
pm_runtime_get_noresume(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
ret = pcdrv->probe(pcdev, id);
What we could do is:
pm_runtime_get_noresume(dev);
pm_runtime_get_noresume(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
ret = pcdrv->probe(pcdev, id);
if (ret == 0) {
pcdev->irq_safe = pm_runtime_is_irq_safe(dev);
pm_runtime_put(dev);
break;
}
pm_runtime_disable(dev);
pm_runtime_set_suspended(dev);
pm_runtime_put_noidle(dev);
pm_runtime_put_noidle(dev);
which would ensure that we hold a usecount until after the probe function
has returned. Would that work?
I'll give you that it's pretty horrid.
Would another possible solution be to remember the irq-safeness in the
suspend handler, and use that in the resume handler? Resume should
/always/ undo what the suspend handler previously did wrt clk API stuff.
--
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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On Sat, Nov 01, 2014 at 01:45:47AM +0100, Rafael J. Wysocki wrote:
> On Monday, October 20, 2014 11:04:46 AM Krzysztof Kozlowski wrote:
> > @@ -198,8 +217,10 @@ static int amba_probe(struct device *dev)
> > pm_runtime_enable(dev);
> >
> > ret = pcdrv->probe(pcdev, id);
> > - if (ret == 0)
> > + if (ret == 0) {
> > + pcdev->irq_safe = pm_runtime_is_irq_safe(dev);
>
> This looks racy.
>
> Is it guaranteed that runtime PM callbacks won't be run for the device
> after pcdrv->probe() has returned and before setting pcdev->irq_safe?
> If not, inconsistent behavior may ensue.
You are absolutely correct. So that knocks that idea on its head.
--
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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On Monday, October 20, 2014 11:04:46 AM Krzysztof Kozlowski wrote:
> The AMBA bus driver defines runtime Power Management functions which
> disable and unprepare AMBA bus clock. This is problematic for runtime PM
> because unpreparing a clock might sleep so it is not interrupt safe.
>
> However some drivers may want to implement runtime PM functions in
> interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
> bus driver should only disable/enable the clock in runtime suspend and
> resume callbacks.
>
> Detect the device driver behavior after calling its probe function and
> store it. During runtime suspend/resume deal with clocks according to
> stored value.
>
> Signed-off-by: Krzysztof Kozlowski
> Reviewed-by: Ulf Hansson
> ---
> drivers/amba/bus.c | 29 +
> include/linux/amba/bus.h | 1 +
> 2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 47bbdc1b5be3..474434e1b1b9 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -95,8 +95,18 @@ static int amba_pm_runtime_suspend(struct device *dev)
> struct amba_device *pcdev = to_amba_device(dev);
> int ret = pm_generic_runtime_suspend(dev);
>
> - if (ret == 0 && dev->driver)
> - clk_disable_unprepare(pcdev->pclk);
> + if (ret == 0 && dev->driver) {
> + /*
> + * Drivers should not change pm_runtime_irq_safe()
> + * after probe.
> + */
> + WARN_ON(pcdev->irq_safe != pm_runtime_is_irq_safe(dev));
> +
> + if (pcdev->irq_safe)
> + clk_disable(pcdev->pclk);
> + else
> + clk_disable_unprepare(pcdev->pclk);
> + }
>
> return ret;
> }
> @@ -107,7 +117,16 @@ static int amba_pm_runtime_resume(struct device *dev)
> int ret;
>
> if (dev->driver) {
> - ret = clk_prepare_enable(pcdev->pclk);
> + /*
> + * Drivers should not change pm_runtime_irq_safe()
> + * after probe.
> + */
> + WARN_ON(pcdev->irq_safe != pm_runtime_is_irq_safe(dev));
> +
> + if (pcdev->irq_safe)
> + ret = clk_enable(pcdev->pclk);
> + else
> + ret = clk_prepare_enable(pcdev->pclk);
> /* Failure is probably fatal to the system, but... */
> if (ret)
> return ret;
> @@ -198,8 +217,10 @@ static int amba_probe(struct device *dev)
> pm_runtime_enable(dev);
>
> ret = pcdrv->probe(pcdev, id);
> - if (ret == 0)
> + if (ret == 0) {
> + pcdev->irq_safe = pm_runtime_is_irq_safe(dev);
This looks racy.
Is it guaranteed that runtime PM callbacks won't be run for the device
after pcdrv->probe() has returned and before setting pcdev->irq_safe?
If not, inconsistent behavior may ensue.
> break;
> + }
>
> pm_runtime_disable(dev);
> pm_runtime_set_suspended(dev);
> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> index ac02f9bd63dc..c4bae79851fb 100644
> --- a/include/linux/amba/bus.h
> +++ b/include/linux/amba/bus.h
> @@ -32,6 +32,7 @@ struct amba_device {
> struct clk *pclk;
> unsigned intperiphid;
> unsigned intirq[AMBA_NR_IRQS];
> + unsigned intirq_safe:1;
> };
>
> struct amba_driver {
>
--
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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On Monday, October 20, 2014 11:04:46 AM Krzysztof Kozlowski wrote:
The AMBA bus driver defines runtime Power Management functions which
disable and unprepare AMBA bus clock. This is problematic for runtime PM
because unpreparing a clock might sleep so it is not interrupt safe.
However some drivers may want to implement runtime PM functions in
interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
bus driver should only disable/enable the clock in runtime suspend and
resume callbacks.
Detect the device driver behavior after calling its probe function and
store it. During runtime suspend/resume deal with clocks according to
stored value.
Signed-off-by: Krzysztof Kozlowski k.kozlow...@samsung.com
Reviewed-by: Ulf Hansson ulf.hans...@linaro.org
---
drivers/amba/bus.c | 29 +
include/linux/amba/bus.h | 1 +
2 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 47bbdc1b5be3..474434e1b1b9 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -95,8 +95,18 @@ static int amba_pm_runtime_suspend(struct device *dev)
struct amba_device *pcdev = to_amba_device(dev);
int ret = pm_generic_runtime_suspend(dev);
- if (ret == 0 dev-driver)
- clk_disable_unprepare(pcdev-pclk);
+ if (ret == 0 dev-driver) {
+ /*
+ * Drivers should not change pm_runtime_irq_safe()
+ * after probe.
+ */
+ WARN_ON(pcdev-irq_safe != pm_runtime_is_irq_safe(dev));
+
+ if (pcdev-irq_safe)
+ clk_disable(pcdev-pclk);
+ else
+ clk_disable_unprepare(pcdev-pclk);
+ }
return ret;
}
@@ -107,7 +117,16 @@ static int amba_pm_runtime_resume(struct device *dev)
int ret;
if (dev-driver) {
- ret = clk_prepare_enable(pcdev-pclk);
+ /*
+ * Drivers should not change pm_runtime_irq_safe()
+ * after probe.
+ */
+ WARN_ON(pcdev-irq_safe != pm_runtime_is_irq_safe(dev));
+
+ if (pcdev-irq_safe)
+ ret = clk_enable(pcdev-pclk);
+ else
+ ret = clk_prepare_enable(pcdev-pclk);
/* Failure is probably fatal to the system, but... */
if (ret)
return ret;
@@ -198,8 +217,10 @@ static int amba_probe(struct device *dev)
pm_runtime_enable(dev);
ret = pcdrv-probe(pcdev, id);
- if (ret == 0)
+ if (ret == 0) {
+ pcdev-irq_safe = pm_runtime_is_irq_safe(dev);
This looks racy.
Is it guaranteed that runtime PM callbacks won't be run for the device
after pcdrv-probe() has returned and before setting pcdev-irq_safe?
If not, inconsistent behavior may ensue.
break;
+ }
pm_runtime_disable(dev);
pm_runtime_set_suspended(dev);
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index ac02f9bd63dc..c4bae79851fb 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -32,6 +32,7 @@ struct amba_device {
struct clk *pclk;
unsigned intperiphid;
unsigned intirq[AMBA_NR_IRQS];
+ unsigned intirq_safe:1;
};
struct amba_driver {
--
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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On Sat, Nov 01, 2014 at 01:45:47AM +0100, Rafael J. Wysocki wrote:
On Monday, October 20, 2014 11:04:46 AM Krzysztof Kozlowski wrote:
@@ -198,8 +217,10 @@ static int amba_probe(struct device *dev)
pm_runtime_enable(dev);
ret = pcdrv-probe(pcdev, id);
- if (ret == 0)
+ if (ret == 0) {
+ pcdev-irq_safe = pm_runtime_is_irq_safe(dev);
This looks racy.
Is it guaranteed that runtime PM callbacks won't be run for the device
after pcdrv-probe() has returned and before setting pcdev-irq_safe?
If not, inconsistent behavior may ensue.
You are absolutely correct. So that knocks that idea on its head.
--
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 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
On Sat, Nov 01, 2014 at 12:55:14AM +, Russell King - ARM Linux wrote:
On Sat, Nov 01, 2014 at 01:45:47AM +0100, Rafael J. Wysocki wrote:
On Monday, October 20, 2014 11:04:46 AM Krzysztof Kozlowski wrote:
@@ -198,8 +217,10 @@ static int amba_probe(struct device *dev)
pm_runtime_enable(dev);
ret = pcdrv-probe(pcdev, id);
- if (ret == 0)
+ if (ret == 0) {
+ pcdev-irq_safe = pm_runtime_is_irq_safe(dev);
This looks racy.
Is it guaranteed that runtime PM callbacks won't be run for the device
after pcdrv-probe() has returned and before setting pcdev-irq_safe?
If not, inconsistent behavior may ensue.
You are absolutely correct. So that knocks that idea on its head.
Actually, I think we shouldn't give up hope here. Currently, we do this:
pm_runtime_get_noresume(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
ret = pcdrv-probe(pcdev, id);
What we could do is:
pm_runtime_get_noresume(dev);
pm_runtime_get_noresume(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
ret = pcdrv-probe(pcdev, id);
if (ret == 0) {
pcdev-irq_safe = pm_runtime_is_irq_safe(dev);
pm_runtime_put(dev);
break;
}
pm_runtime_disable(dev);
pm_runtime_set_suspended(dev);
pm_runtime_put_noidle(dev);
pm_runtime_put_noidle(dev);
which would ensure that we hold a usecount until after the probe function
has returned. Would that work?
I'll give you that it's pretty horrid.
Would another possible solution be to remember the irq-safeness in the
suspend handler, and use that in the resume handler? Resume should
/always/ undo what the suspend handler previously did wrt clk API stuff.
--
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/
