patch#1,#2,#3,#4,#5 use pm_runtime_resume_and_get() to replace pm_runtime_get_sync and pm_runtime_put_noidle.
This change is just to simplify the code, no actual functional changes.
Tian Tao (5): spi: Use pm_runtime_resume_and_get() to replace open coding spi: spi-mem: Use pm_runtime_resume_and_get() to replace open coding spi: imx: Use pm_runtime_resume_and_get() to replace open coding spi: spi-ti-qspi: Use pm_runtime_resume_and_get() to replace open coding spi: sprd: Use pm_runtime_resume_and_get() to replace open coding
drivers/spi/spi-imx.c | 6 ++---- drivers/spi/spi-mem.c | 3 +-- drivers/spi/spi-sprd.c | 3 +-- drivers/spi/spi-ti-qspi.c | 3 +-- drivers/spi/spi.c | 9 +++------ 5 files changed, 8 insertions(+), 16 deletions(-)
Found using coccicheck script under review at: https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
Signed-off-by: Tian Tao tiantao6@hisilicon.com --- drivers/spi/spi.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 57120de..2664b4a 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1486,9 +1486,8 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) mutex_lock(&ctlr->io_mutex);
if (!was_busy && ctlr->auto_runtime_pm) { - ret = pm_runtime_get_sync(ctlr->dev.parent); + ret = pm_runtime_resume_and_get(ctlr->dev.parent); if (ret < 0) { - pm_runtime_put_noidle(ctlr->dev.parent); dev_err(&ctlr->dev, "Failed to power device: %d\n", ret); mutex_unlock(&ctlr->io_mutex); @@ -3426,10 +3425,9 @@ int spi_setup(struct spi_device *spi) }
if (spi->controller->auto_runtime_pm && spi->controller->set_cs) { - status = pm_runtime_get_sync(spi->controller->dev.parent); + status = pm_runtime_resume_and_get(spi->controller->dev.parent); if (status < 0) { mutex_unlock(&spi->controller->io_mutex); - pm_runtime_put_noidle(spi->controller->dev.parent); dev_err(&spi->controller->dev, "Failed to power device: %d\n", status); return status; @@ -3491,10 +3489,9 @@ int spi_set_cs_timing(struct spi_device *spi, struct spi_delay *setup, mutex_lock(&spi->controller->io_mutex);
if (spi->controller->auto_runtime_pm) { - status = pm_runtime_get_sync(parent); + status = pm_runtime_resume_and_get(parent); if (status < 0) { mutex_unlock(&spi->controller->io_mutex); - pm_runtime_put_noidle(parent); dev_err(&spi->controller->dev, "Failed to power device: %d\n", status); return status;
Found using coccicheck script under review at: https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
Signed-off-by: Tian Tao tiantao6@hisilicon.com --- drivers/spi/spi-mem.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index 1513553e..ef20c41 100644 --- a/drivers/spi/spi-mem.c +++ b/drivers/spi/spi-mem.c @@ -257,9 +257,8 @@ static int spi_mem_access_start(struct spi_mem *mem) if (ctlr->auto_runtime_pm) { int ret;
- ret = pm_runtime_get_sync(ctlr->dev.parent); + ret = pm_runtime_resume_and_get(ctlr->dev.parent); if (ret < 0) { - pm_runtime_put_noidle(ctlr->dev.parent); dev_err(&ctlr->dev, "Failed to power device: %d\n", ret); return ret;
Found using coccicheck script under review at: https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
Signed-off-by: Tian Tao tiantao6@hisilicon.com --- drivers/spi/spi-imx.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index 39dc02e3..0bf8333 100644 --- a/drivers/spi/spi-imx.c +++ b/drivers/spi/spi-imx.c @@ -1514,9 +1514,8 @@ spi_imx_prepare_message(struct spi_master *master, struct spi_message *msg) struct spi_imx_data *spi_imx = spi_master_get_devdata(master); int ret;
- ret = pm_runtime_get_sync(spi_imx->dev); + ret = pm_runtime_resume_and_get(spi_imx->dev); if (ret < 0) { - pm_runtime_put_noidle(spi_imx->dev); dev_err(spi_imx->dev, "failed to enable clock\n"); return ret; } @@ -1723,9 +1722,8 @@ static int spi_imx_remove(struct platform_device *pdev)
spi_bitbang_stop(&spi_imx->bitbang);
- ret = pm_runtime_get_sync(spi_imx->dev); + ret = pm_runtime_resume_and_get(spi_imx->dev); if (ret < 0) { - pm_runtime_put_noidle(spi_imx->dev); dev_err(spi_imx->dev, "failed to enable clock\n"); return ret; }
On Thu, 20 May 2021 19:53:12 +0800 Tian Tao tiantao6@hisilicon.com wrote:
Found using coccicheck script under review at: https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
Signed-off-by: Tian Tao tiantao6@hisilicon.com
Not 100% sure, but I think there is a bug in the probe error path in that it will leave the runtime pm reference count elevated if we hit the goto out_runtime_pm_put;
It has incremented the counter wtih pm_runtime_get_no_resume() but there is no put in the error path to decrement it again.
@Mauro, could you check this one as you've stared at more of these than me?
drivers/spi/spi-imx.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index 39dc02e3..0bf8333 100644 --- a/drivers/spi/spi-imx.c +++ b/drivers/spi/spi-imx.c @@ -1514,9 +1514,8 @@ spi_imx_prepare_message(struct spi_master *master, struct spi_message *msg) struct spi_imx_data *spi_imx = spi_master_get_devdata(master); int ret;
- ret = pm_runtime_get_sync(spi_imx->dev);
- ret = pm_runtime_resume_and_get(spi_imx->dev); if (ret < 0) {
pm_runtime_put_noidle(spi_imx->dev);
Style in this file seems to prefer if (ret) where possible, so perhaps change it here as well?
dev_err(spi_imx->dev, "failed to enable clock\n"); return ret;} @@ -1723,9 +1722,8 @@ static int spi_imx_remove(struct platform_device *pdev)
spi_bitbang_stop(&spi_imx->bitbang);
- ret = pm_runtime_get_sync(spi_imx->dev);
- ret = pm_runtime_resume_and_get(spi_imx->dev); if (ret < 0) {
pm_runtime_put_noidle(spi_imx->dev);
Found using coccicheck script under review at: https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
Signed-off-by: Tian Tao tiantao6@hisilicon.com --- drivers/spi/spi-ti-qspi.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c index e06aafe..e6e8b06 100644 --- a/drivers/spi/spi-ti-qspi.c +++ b/drivers/spi/spi-ti-qspi.c @@ -172,9 +172,8 @@ static int ti_qspi_setup(struct spi_device *spi) dev_dbg(qspi->dev, "hz: %d, clock divider %d\n", qspi->spi_max_frequency, clk_div);
- ret = pm_runtime_get_sync(qspi->dev); + ret = pm_runtime_resume_and_get(qspi->dev); if (ret < 0) { - pm_runtime_put_noidle(qspi->dev); dev_err(qspi->dev, "pm_runtime_get_sync() failed\n"); return ret; }
On Thu, 20 May 2021 19:53:13 +0800 Tian Tao tiantao6@hisilicon.com wrote:
Found using coccicheck script under review at: https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
Signed-off-by: Tian Tao tiantao6@hisilicon.com
There is an unbalanced pm_runtime_put_sync() in the remove in this driver I think. At least I can find the paired get. It's not a bug, because pm_runtime won't decrement the counter if already zero, but it's not pretty.
Might be something subtle in SPI framework that needs that though....
drivers/spi/spi-ti-qspi.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c index e06aafe..e6e8b06 100644 --- a/drivers/spi/spi-ti-qspi.c +++ b/drivers/spi/spi-ti-qspi.c @@ -172,9 +172,8 @@ static int ti_qspi_setup(struct spi_device *spi) dev_dbg(qspi->dev, "hz: %d, clock divider %d\n", qspi->spi_max_frequency, clk_div);
- ret = pm_runtime_get_sync(qspi->dev);
- ret = pm_runtime_resume_and_get(qspi->dev); if (ret < 0) {
pm_runtime_put_noidle(qspi->dev);
Found using coccicheck script under review at: https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
Signed-off-by: Tian Tao tiantao6@hisilicon.com --- drivers/spi/spi-sprd.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/spi/spi-sprd.c b/drivers/spi/spi-sprd.c index 28e70db..65b8075 100644 --- a/drivers/spi/spi-sprd.c +++ b/drivers/spi/spi-sprd.c @@ -1008,9 +1008,8 @@ static int sprd_spi_remove(struct platform_device *pdev) struct sprd_spi *ss = spi_controller_get_devdata(sctlr); int ret;
- ret = pm_runtime_get_sync(ss->dev); + ret = pm_runtime_resume_and_get(ss->dev); if (ret < 0) { - pm_runtime_put_noidle(ss->dev); dev_err(ss->dev, "failed to resume SPI controller\n"); return ret; }
On Thu, 20 May 2021 19:53:14 +0800 Tian Tao tiantao6@hisilicon.com wrote:
Found using coccicheck script under review at: https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/
Signed-off-by: Tian Tao tiantao6@hisilicon.com
Seemed odd that this only called this in remove, so I took a closer look.
There is another call in probe. Do something similar for that one by adding an additional label in the error path.
The general approach in this driver is a bit of a mess because I think they will call clk_prepare_enable() twice in probe if pm_runtime is enabled. (once directly and once due to the pm_runtime_get_sync() call) It should probably be calling pm_runtime_get_noresume() in probe + a pm_runtime_set_active() call.
However that's a more complex patch set to attempt without hardware. You could try it though.
Jonathan
drivers/spi/spi-sprd.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/spi/spi-sprd.c b/drivers/spi/spi-sprd.c index 28e70db..65b8075 100644 --- a/drivers/spi/spi-sprd.c +++ b/drivers/spi/spi-sprd.c @@ -1008,9 +1008,8 @@ static int sprd_spi_remove(struct platform_device *pdev) struct sprd_spi *ss = spi_controller_get_devdata(sctlr); int ret;
- ret = pm_runtime_get_sync(ss->dev);
- ret = pm_runtime_resume_and_get(ss->dev); if (ret < 0) {
pm_runtime_put_noidle(ss->dev);
On Thu, 20 May 2021 19:53:09 +0800 Tian Tao tiantao6@hisilicon.com wrote:
patch#1,#2,#3,#4,#5 use pm_runtime_resume_and_get() to replace pm_runtime_get_sync and pm_runtime_put_noidle.
As that's all of them no need to say which patches here :)
This change is just to simplify the code, no actual functional changes.
What advantage does the new function have beyond saving a few lines of code? (harder to get wrong as no side effects).
Tian Tao (5): spi: Use pm_runtime_resume_and_get() to replace open coding spi: spi-mem: Use pm_runtime_resume_and_get() to replace open coding spi: imx: Use pm_runtime_resume_and_get() to replace open coding spi: spi-ti-qspi: Use pm_runtime_resume_and_get() to replace open coding spi: sprd: Use pm_runtime_resume_and_get() to replace open coding
drivers/spi/spi-imx.c | 6 ++---- drivers/spi/spi-mem.c | 3 +-- drivers/spi/spi-sprd.c | 3 +-- drivers/spi/spi-ti-qspi.c | 3 +-- drivers/spi/spi.c | 9 +++------ 5 files changed, 8 insertions(+), 16 deletions(-)
-
Jonathan Cameron -
Tian Tao