| Message ID | 20240825035422.900370-1-jon.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: rockchip: Avoid redundant clock disable in pm operation | expand |
(NB: I have several nearly identical copies of this email. I'm replying to the latest one I see.) Hi Jon, On Sun, Aug 25, 2024 at 11:54:22AM +0800, Jon Lin wrote: > Fix WARN_ON: > [ 22.869352][ T1885] clk_spi0 already unprepared > [ 22.869379][ T1885] WARNING: CPU: 3 PID: 1885 at drivers/clk/clk.c:813 clk_core_unprepare+0xbc4 > [ 22.869380][ T1885] Modules linked in: bcmdhd dhd_static_buf > [ 22.869391][ T1885] CPU: 3 PID: 1885 Comm: Binder:355_2 Tainted: G W 5.10.66 #59 > [ 22.869393][ T1885] Hardware name: Rockchip RK3588 EVB1 LP4 V10 Board (DT) > [ 22.869397][ T1885] pstate: 60400009 (nZCv daif +PAN -UAO -TCO BTYPE=--) > [ 22.869401][ T1885] pc : clk_core_unprepare+0xbc/0x214 > [ 22.869404][ T1885] lr : clk_core_unprepare+0xbc/0x214 I appreciate the snippet of a WARNING trace, but I'd also appreciate some actual explanation of what the problem is, and why you're solving it this way. > Fixes: e882575efc77 ("spi: rockchip: Suspend and resume the bus during NOIRQ_SYSTEM_SLEEP_PM ops") > Signed-off-by: Jon Lin <jon.lin@rock-chips.com> > --- > > drivers/spi/spi-rockchip.c | 57 +++++++++++++++++--------------------- > 1 file changed, 26 insertions(+), 31 deletions(-) > > diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c > index e1ecd96c7858..043a7739c330 100644 > --- a/drivers/spi/spi-rockchip.c > +++ b/drivers/spi/spi-rockchip.c > +#ifdef CONFIG_PM_SLEEP > +static int rockchip_spi_suspend(struct device *dev) > { > + int ret; > struct spi_controller *ctlr = dev_get_drvdata(dev); > - struct rockchip_spi *rs = spi_controller_get_devdata(ctlr); > > - clk_disable_unprepare(rs->spiclk); > - clk_disable_unprepare(rs->apb_pclk); > + ret = spi_controller_suspend(ctlr); > + if (ret < 0) > + return ret; > + > + /* Avoid redundant clock disable */ > + if (!pm_runtime_status_suspended(dev)) > + rockchip_spi_runtime_suspend(dev); It seems like you'd really be served well by pm_runtime_force_{suspend,resume}() here, and in fact, that's what this driver used to use before the breaking change (commit e882575efc77). Why aren't you just going back to using it? (This is the kind of thing I might expect in your commit message -- reasoning as to why you're doing what you're doing.) And in fact, I already submitted a patch that resolves the above problem and does exactly that: https://lore.kernel.org/all/20240823214235.1718769-1-briannorris@chromium.org/ [PATCH] spi: rockchip: Resolve unbalanced runtime PM / system PM handling Do you see any problem with it? Thanks, Brian > + pinctrl_pm_select_sleep_state(dev); > > return 0; > }
On 2024/8/27 6:27, Brian Norris wrote: > (NB: I have several nearly identical copies of this email. I'm replying > to the latest one I see.) > > Hi Jon, > > On Sun, Aug 25, 2024 at 11:54:22AM +0800, Jon Lin wrote: >> Fix WARN_ON: >> [ 22.869352][ T1885] clk_spi0 already unprepared >> [ 22.869379][ T1885] WARNING: CPU: 3 PID: 1885 at drivers/clk/clk.c:813 clk_core_unprepare+0xbc4 >> [ 22.869380][ T1885] Modules linked in: bcmdhd dhd_static_buf >> [ 22.869391][ T1885] CPU: 3 PID: 1885 Comm: Binder:355_2 Tainted: G W 5.10.66 #59 >> [ 22.869393][ T1885] Hardware name: Rockchip RK3588 EVB1 LP4 V10 Board (DT) >> [ 22.869397][ T1885] pstate: 60400009 (nZCv daif +PAN -UAO -TCO BTYPE=--) >> [ 22.869401][ T1885] pc : clk_core_unprepare+0xbc/0x214 >> [ 22.869404][ T1885] lr : clk_core_unprepare+0xbc/0x214 > > I appreciate the snippet of a WARNING trace, but I'd also appreciate > some actual explanation of what the problem is, and why you're solving > it this way. > Thank you for the reminder. >> Fixes: e882575efc77 ("spi: rockchip: Suspend and resume the bus during NOIRQ_SYSTEM_SLEEP_PM ops") >> Signed-off-by: Jon Lin <jon.lin@rock-chips.com> >> --- >> >> drivers/spi/spi-rockchip.c | 57 +++++++++++++++++--------------------- >> 1 file changed, 26 insertions(+), 31 deletions(-) >> >> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c >> index e1ecd96c7858..043a7739c330 100644 >> --- a/drivers/spi/spi-rockchip.c >> +++ b/drivers/spi/spi-rockchip.c > >> +#ifdef CONFIG_PM_SLEEP >> +static int rockchip_spi_suspend(struct device *dev) >> { >> + int ret; >> struct spi_controller *ctlr = dev_get_drvdata(dev); >> - struct rockchip_spi *rs = spi_controller_get_devdata(ctlr); >> >> - clk_disable_unprepare(rs->spiclk); >> - clk_disable_unprepare(rs->apb_pclk); >> + ret = spi_controller_suspend(ctlr); >> + if (ret < 0) >> + return ret; >> + >> + /* Avoid redundant clock disable */ >> + if (!pm_runtime_status_suspended(dev)) >> + rockchip_spi_runtime_suspend(dev); > > It seems like you'd really be served well by > pm_runtime_force_{suspend,resume}() here, and in fact, that's what this > driver used to use before the breaking change (commit > e882575efc77). Why aren't you just going back to using it? (This is the > kind of thing I might expect in your commit message -- reasoning as to > why you're doing what you're doing.) > > And in fact, I already submitted a patch that resolves the above problem > and does exactly that: > > https://lore.kernel.org/all/20240823214235.1718769-1-briannorris@chromium.org/ > [PATCH] spi: rockchip: Resolve unbalanced runtime PM / system PM handling > > Do you see any problem with it? > I have reviewed your submission and although the code has been simplified, the execution efficiency has decreased. So although it is a commonly used processing solution for SPI Upstream, I still hope to retain a more efficiency approach as I submitted. > Thanks, > Brian > >> + pinctrl_pm_select_sleep_state(dev); >> >> return 0; >> } >
On Mon, Aug 26, 2024 at 6:33 PM Jon Lin <jon.lin@rock-chips.com> wrote: > On 2024/8/27 6:27, Brian Norris wrote: > > It seems like you'd really be served well by > > pm_runtime_force_{suspend,resume}() here, and in fact, that's what this > > driver used to use before the breaking change (commit > > e882575efc77). Why aren't you just going back to using it? (This is the > > kind of thing I might expect in your commit message -- reasoning as to > > why you're doing what you're doing.) > > > > And in fact, I already submitted a patch that resolves the above problem > > and does exactly that: > > > > https://lore.kernel.org/all/20240823214235.1718769-1-briannorris@chromium.org/ > > [PATCH] spi: rockchip: Resolve unbalanced runtime PM / system PM handling > > > > Do you see any problem with it? > > > > I have reviewed your submission and although the code has been > simplified, the execution efficiency has decreased. So although it is a > commonly used processing solution for SPI Upstream, I still hope to > retain a more efficiency approach as I submitted. What do you mean by "efficiency"? You mean because there's indirection, via the PM runtime framework? If so, I doubt that's a priority for this piece of functionality -- simplicity is more important than a function call or two when talking about system suspend. Additionally, simplicity has additional benefits -- it heads off questions that your more complex code doesn't address. For example, are runtime PM and system PM mutually exclusive? Do we have to coordinate with any pending autosuspend? (Reading through https://docs.kernel.org/power/runtime_pm.html, I believe these are not actually concerns, but it's really not obvious and takes a bit of reading.) But your patch makes it more likely that runtime and system PM states get out of sync. Anyway, if the patches really are equivalent, I suppose it can be the maintainer's choice as to which is preferable. Brian
Hi Jon, On Sat, Aug 24, 2024 at 8:55 PM Jon Lin <jon.lin@rock-chips.com> wrote: > --- a/drivers/spi/spi-rockchip.c > +++ b/drivers/spi/spi-rockchip.c > +#ifdef CONFIG_PM_SLEEP > +static int rockchip_spi_suspend(struct device *dev) > { > + int ret; > struct spi_controller *ctlr = dev_get_drvdata(dev); > - struct rockchip_spi *rs = spi_controller_get_devdata(ctlr); > > - clk_disable_unprepare(rs->spiclk); > - clk_disable_unprepare(rs->apb_pclk); > + ret = spi_controller_suspend(ctlr); > + if (ret < 0) > + return ret; > + > + /* Avoid redundant clock disable */ > + if (!pm_runtime_status_suspended(dev)) > + rockchip_spi_runtime_suspend(dev); rockchip_spi_runtime_suspend() returns an error code. I understand that it doesn't actually fail in practice, but you should probably check it anyway. > + > + pinctrl_pm_select_sleep_state(dev); > > return 0; > } > > -static int rockchip_spi_runtime_resume(struct device *dev) > +static int rockchip_spi_resume(struct device *dev) > { > int ret; > struct spi_controller *ctlr = dev_get_drvdata(dev); > - struct rockchip_spi *rs = spi_controller_get_devdata(ctlr); > > - ret = clk_prepare_enable(rs->apb_pclk); > - if (ret < 0) > - return ret; > + pinctrl_pm_select_default_state(dev); > > - ret = clk_prepare_enable(rs->spiclk); > + if (!pm_runtime_status_suspended(dev)) { > + ret = rockchip_spi_runtime_resume(dev); > + if (ret < 0) > + return ret; > + } > + > + ret = spi_controller_resume(ctlr); > if (ret < 0) > - clk_disable_unprepare(rs->apb_pclk); > + rockchip_spi_runtime_suspend(dev); I don't think this is valid error handling. AFAIK, failing the resume() function doesn't actually "disable" the device in any way (it's just informative at best), so we probably shouldn't disable clocks here. Otherwise, you might leave the clock disabled while the runtime PM framework thinks it's enabled. Brian > > return 0; > } > -- > 2.34.1 >
On 2024/8/27 10:59, Brian Norris wrote: > On Mon, Aug 26, 2024 at 6:33 PM Jon Lin <jon.lin@rock-chips.com> wrote: >> On 2024/8/27 6:27, Brian Norris wrote: >>> It seems like you'd really be served well by >>> pm_runtime_force_{suspend,resume}() here, and in fact, that's what this >>> driver used to use before the breaking change (commit >>> e882575efc77). Why aren't you just going back to using it? (This is the >>> kind of thing I might expect in your commit message -- reasoning as to >>> why you're doing what you're doing.) >>> >>> And in fact, I already submitted a patch that resolves the above problem >>> and does exactly that: >>> >>> https://lore.kernel.org/all/20240823214235.1718769-1-briannorris@chromium.org/ >>> [PATCH] spi: rockchip: Resolve unbalanced runtime PM / system PM handling >>> >>> Do you see any problem with it? >>> >> >> I have reviewed your submission and although the code has been >> simplified, the execution efficiency has decreased. So although it is a >> commonly used processing solution for SPI Upstream, I still hope to >> retain a more efficiency approach as I submitted. > > What do you mean by "efficiency"? You mean because there's > indirection, via the PM runtime framework? If so, I doubt that's a > priority for this piece of functionality -- simplicity is more > important than a function call or two when talking about system > suspend. > Your code is fine, and I have tested it locally. The interface simplification is indeed in line with the direction of community development, and your solution can be used as a solution. > Additionally, simplicity has additional benefits -- it heads off > questions that your more complex code doesn't address. For example, > are runtime PM and system PM mutually exclusive? Do we have to > coordinate with any pending autosuspend? (Reading through > https://docs.kernel.org/power/runtime_pm.html, I believe these are not > actually concerns, but it's really not obvious and takes a bit of > reading.) But your patch makes it more likely that runtime and system > PM states get out of sync. > > Anyway, if the patches really are equivalent, I suppose it can be the > maintainer's choice as to which is preferable. > > Brian >
diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c index e1ecd96c7858..043a7739c330 100644 --- a/drivers/spi/spi-rockchip.c +++ b/drivers/spi/spi-rockchip.c @@ -940,33 +940,24 @@ static void rockchip_spi_remove(struct platform_device *pdev) spi_controller_put(ctlr); } -#ifdef CONFIG_PM_SLEEP -static int rockchip_spi_suspend(struct device *dev) +#ifdef CONFIG_PM +static int rockchip_spi_runtime_suspend(struct device *dev) { - int ret; struct spi_controller *ctlr = dev_get_drvdata(dev); struct rockchip_spi *rs = spi_controller_get_devdata(ctlr); - ret = spi_controller_suspend(ctlr); - if (ret < 0) - return ret; - clk_disable_unprepare(rs->spiclk); clk_disable_unprepare(rs->apb_pclk); - pinctrl_pm_select_sleep_state(dev); - return 0; } -static int rockchip_spi_resume(struct device *dev) +static int rockchip_spi_runtime_resume(struct device *dev) { int ret; struct spi_controller *ctlr = dev_get_drvdata(dev); struct rockchip_spi *rs = spi_controller_get_devdata(ctlr); - pinctrl_pm_select_default_state(dev); - ret = clk_prepare_enable(rs->apb_pclk); if (ret < 0) return ret; @@ -975,41 +966,45 @@ static int rockchip_spi_resume(struct device *dev) if (ret < 0) clk_disable_unprepare(rs->apb_pclk); - ret = spi_controller_resume(ctlr); - if (ret < 0) { - clk_disable_unprepare(rs->spiclk); - clk_disable_unprepare(rs->apb_pclk); - } - return 0; } -#endif /* CONFIG_PM_SLEEP */ +#endif /* CONFIG_PM */ -#ifdef CONFIG_PM -static int rockchip_spi_runtime_suspend(struct device *dev) +#ifdef CONFIG_PM_SLEEP +static int rockchip_spi_suspend(struct device *dev) { + int ret; struct spi_controller *ctlr = dev_get_drvdata(dev); - struct rockchip_spi *rs = spi_controller_get_devdata(ctlr); - clk_disable_unprepare(rs->spiclk); - clk_disable_unprepare(rs->apb_pclk); + ret = spi_controller_suspend(ctlr); + if (ret < 0) + return ret; + + /* Avoid redundant clock disable */ + if (!pm_runtime_status_suspended(dev)) + rockchip_spi_runtime_suspend(dev); + + pinctrl_pm_select_sleep_state(dev); return 0; } -static int rockchip_spi_runtime_resume(struct device *dev) +static int rockchip_spi_resume(struct device *dev) { int ret; struct spi_controller *ctlr = dev_get_drvdata(dev); - struct rockchip_spi *rs = spi_controller_get_devdata(ctlr); - ret = clk_prepare_enable(rs->apb_pclk); - if (ret < 0) - return ret; + pinctrl_pm_select_default_state(dev); - ret = clk_prepare_enable(rs->spiclk); + if (!pm_runtime_status_suspended(dev)) { + ret = rockchip_spi_runtime_resume(dev); + if (ret < 0) + return ret; + } + + ret = spi_controller_resume(ctlr); if (ret < 0) - clk_disable_unprepare(rs->apb_pclk); + rockchip_spi_runtime_suspend(dev); return 0; }
Fix WARN_ON: [ 22.869352][ T1885] clk_spi0 already unprepared [ 22.869379][ T1885] WARNING: CPU: 3 PID: 1885 at drivers/clk/clk.c:813 clk_core_unprepare+0xbc4 [ 22.869380][ T1885] Modules linked in: bcmdhd dhd_static_buf [ 22.869391][ T1885] CPU: 3 PID: 1885 Comm: Binder:355_2 Tainted: G W 5.10.66 #59 [ 22.869393][ T1885] Hardware name: Rockchip RK3588 EVB1 LP4 V10 Board (DT) [ 22.869397][ T1885] pstate: 60400009 (nZCv daif +PAN -UAO -TCO BTYPE=--) [ 22.869401][ T1885] pc : clk_core_unprepare+0xbc/0x214 [ 22.869404][ T1885] lr : clk_core_unprepare+0xbc/0x214 Fixes: e882575efc77 ("spi: rockchip: Suspend and resume the bus during NOIRQ_SYSTEM_SLEEP_PM ops") Signed-off-by: Jon Lin <jon.lin@rock-chips.com> --- drivers/spi/spi-rockchip.c | 57 +++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 31 deletions(-)