* [PATCH v3 00/79] Address some issues with PM runtime at media subsystem
@ 2021-04-27 10:25 Mauro Carvalho Chehab
2021-04-27 10:26 ` [PATCH v3 25/79] staging: media: vde: use pm_runtime_resume_and_get() Mauro Carvalho Chehab
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Mauro Carvalho Chehab @ 2021-04-27 10:25 UTC (permalink / raw)
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Lad, Prabhakar,
Paul J. Murphy, Alexandre Torgue, Andrzej Hajda,
Andrzej Pietrasiewicz, Andy Gross, Benoit Parrot, Bingbu Cao,
Bjorn Andersson, Chen-Yu Tsai, Chiranjeevi Rapolu,
Dafna Hirschfeld, Dan Scally, Daniele Alessandrelli,
Dave Stevenson, Dmitry Osipenko, Dongchun Zhu, Ezequiel Garcia,
Fabio Estevam, Heiko Stuebner, Helen Koike, Hyungwoo Yang,
Jacek Anaszewski, Jacob Chen, Jacopo Mondi, Jernej Skrabec,
Jonathan Hunter, Krzysztof Kozlowski, Leon Luo,
Manivannan Sadhasivam, Marek Szyprowski, Matt Ranostay,
Matthias Brugger, Mauro Carvalho Chehab, Maxime Coquelin,
Maxime Ripard, NXP Linux Team, Paul Kocialkowski,
Pengutronix Kernel Team, Philipp Zabel, Ricardo Ribalda,
Robert Foss, Rui Miguel Silva, Sakari Ailus, Sascha Hauer,
Shawn Guo, Shawn Tu, Shunqian Zheng, Sowjanya Komatineni,
Stanimir Varbanov, Steve Longerbeam, Sylwester Nawrocki,
Sylwester Nawrocki, Thierry Reding, Tianshu Qiu, Todor Tomov,
Wenyou Yang, Yong Zhi, devel, linux-arm-kernel, linux-arm-msm,
linux-kernel, linux-media, linux-mediatek, linux-renesas-soc,
linux-rockchip, linux-samsung-soc, linux-stm32, linux-tegra
During the review of the patches from unm.edu, one of the patterns
I noticed is the amount of patches trying to fix pm_runtime_get_sync()
calls.
After analyzing the feedback from version 1 of this series, I noticed
a few other weird behaviors at the PM runtime resume code. So, this
series start addressing some bugs and issues at the current code.
Then, it gets rid of pm_runtime_get_sync() at the media subsystem
(with 2 exceptions).
It should be noticed that
Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
added a new method to does a pm_runtime get, which increments
the usage count only on success.
The rationale of getting rid of pm_runtime_get_sync() is:
1. despite its name, this is actually a PM runtime resume call,
but some developers didn't seem to realize that, as I got this
pattern on some drivers:
pm_runtime_get_sync(&client->dev);
pm_runtime_disable(&client->dev);
pm_runtime_set_suspended(&client->dev);
pm_runtime_put_noidle(&client->dev);
It makes no sense to resume PM just to suspend it again ;-)
2. Usual *_get() methods only increment their use count on success,
but pm_runtime_get_sync() increments it unconditionally. Due to
that, several drivers were mistakenly not calling
pm_runtime_put_noidle() when it fails;
3. The name of the new variant is a lot clearer:
pm_runtime_resume_and_get()
As its same clearly says that this is a PM runtime resume function,
that also increments the usage counter on success;
4. Consistency: we did similar changes subsystem wide with
for instance strlcpy() and strcpy() that got replaced by
strscpy(). Having all drivers using the same known-to-be-safe
methods is a good thing;
5. Prevent newer drivers to copy-and-paste a code that it would
be easier to break if they don't truly understand what's behind
the scenes.
This series replace places pm_runtime_get_sync(), by calling
pm_runtime_resume_and_get() instead.
This should help to avoid future mistakes like that, as people
tend to use the existing drivers as examples for newer ones.
compile-tested only.
Patches 1 to 7 fix some issues that already exists at the current
PM runtime code;
patches 8 to 20 fix some usage_count problems that still exists
at the media subsystem;
patches 21 to 78 repaces pm_runtime_get_sync() by
pm_runtime_resume_and_get();
Patch 79 (and a hunk on patch 78) documents the two exceptions
where pm_runtime_get_sync() will still be used for now.
---
v3: - fix a compilation error;
v2: - addressed pointed issues and fixed a few other PM issues.
Mauro Carvalho Chehab (79):
media: venus: fix PM runtime logic at venus_sys_error_handler()
media: i2c: ccs-core: return the right error code at suspend
media: i2c: mt9m001: don't resume at remove time
media: i2c: ov7740: don't resume at remove time
media: i2c: video-i2c: don't resume at remove time
media: exynos-gsc: don't resume at remove time
media: atmel: properly get pm_runtime
media: marvel-ccic: fix some issues when getting pm_runtime
media: mdk-mdp: fix pm_runtime_get_sync() usage count
media: rcar_fdp1: fix pm_runtime_get_sync() usage count
media: rga-buf: use pm_runtime_resume_and_get()
media: renesas-ceu: Properly check for PM errors
media: s5p: fix pm_runtime_get_sync() usage count
media: am437x: fix pm_runtime_get_sync() usage count
media: sh_vou: fix pm_runtime_get_sync() usage count
media: mtk-vcodec: fix pm_runtime_get_sync() usage count
media: s5p-jpeg: fix pm_runtime_get_sync() usage count
media: delta-v4l2: fix pm_runtime_get_sync() usage count
media: sun8i_rotate: fix pm_runtime_get_sync() usage count
staging: media: rkvdec: fix pm_runtime_get_sync() usage count
staging: media: atomisp_fops: use pm_runtime_resume_and_get()
staging: media: imx7-mipi-csis: use pm_runtime_resume_and_get()
staging: media: ipu3: use pm_runtime_resume_and_get()
staging: media: cedrus_video: use pm_runtime_resume_and_get()
staging: media: vde: use pm_runtime_resume_and_get()
staging: media: csi: use pm_runtime_resume_and_get()
staging: media: vi: use pm_runtime_resume_and_get()
media: i2c: ak7375: use pm_runtime_resume_and_get()
media: i2c: ccs-core: use pm_runtime_resume_and_get()
media: i2c: dw9714: use pm_runtime_resume_and_get()
media: i2c: dw9768: use pm_runtime_resume_and_get()
media: i2c: dw9807-vcm: use pm_runtime_resume_and_get()
media: i2c: hi556: use pm_runtime_resume_and_get()
media: i2c: imx214: use pm_runtime_resume_and_get()
media: i2c: imx219: use pm_runtime_resume_and_get()
media: i2c: imx258: use pm_runtime_resume_and_get()
media: i2c: imx274: use pm_runtime_resume_and_get()
media: i2c: imx290: use pm_runtime_resume_and_get()
media: i2c: imx319: use pm_runtime_resume_and_get()
media: i2c: imx334: use pm_runtime_resume_and_get()
media: i2c: imx355: use pm_runtime_resume_and_get()
media: i2c: mt9m001: use pm_runtime_resume_and_get()
media: i2c: ov02a10: use pm_runtime_resume_and_get()
media: i2c: ov13858: use pm_runtime_resume_and_get()
media: i2c: ov2659: use pm_runtime_resume_and_get()
media: i2c: ov2685: use pm_runtime_resume_and_get()
media: i2c: ov2740: use pm_runtime_resume_and_get()
media: i2c: ov5647: use pm_runtime_resume_and_get()
media: i2c: ov5648: use pm_runtime_resume_and_get()
media: i2c: ov5670: use pm_runtime_resume_and_get()
media: i2c: ov5675: use pm_runtime_resume_and_get()
media: i2c: ov5695: use pm_runtime_resume_and_get()
media: i2c: ov7740: use pm_runtime_resume_and_get()
media: i2c: ov8856: use pm_runtime_resume_and_get()
media: i2c: ov8865: use pm_runtime_resume_and_get()
media: i2c: ov9734: use pm_runtime_resume_and_get()
media: i2c: tvp5150: use pm_runtime_resume_and_get()
media: i2c: video-i2c: use pm_runtime_resume_and_get()
media: sti/hva: use pm_runtime_resume_and_get()
media: ipu3: use pm_runtime_resume_and_get()
media: coda: use pm_runtime_resume_and_get()
media: exynos4-is: use pm_runtime_resume_and_get()
media: exynos-gsc: use pm_runtime_resume_and_get()
media: mtk-jpeg: use pm_runtime_resume_and_get()
media: camss: use pm_runtime_resume_and_get()
media: venus: use pm_runtime_resume_and_get()
media: venus: vdec: use pm_runtime_resume_and_get()
media: venus: venc: use pm_runtime_resume_and_get()
media: rcar-fcp: use pm_runtime_resume_and_get()
media: rkisp1: use pm_runtime_resume_and_get()
media: s3c-camif: use pm_runtime_resume_and_get()
media: s5p-mfc: use pm_runtime_resume_and_get()
media: bdisp-v4l2: use pm_runtime_resume_and_get()
media: stm32: use pm_runtime_resume_and_get()
media: sunxi: use pm_runtime_resume_and_get()
media: ti-vpe: use pm_runtime_resume_and_get()
media: vsp1: use pm_runtime_resume_and_get()
media: rcar-vin: use pm_runtime_resume_and_get()
media: hantro: document the usage of pm_runtime_get_sync()
drivers/media/cec/platform/s5p/s5p_cec.c | 5 +++-
drivers/media/i2c/ak7375.c | 10 +------
drivers/media/i2c/ccs/ccs-core.c | 18 +++++-------
drivers/media/i2c/dw9714.c | 10 +------
drivers/media/i2c/dw9768.c | 10 +------
drivers/media/i2c/dw9807-vcm.c | 10 +------
drivers/media/i2c/hi556.c | 3 +-
drivers/media/i2c/imx214.c | 6 ++--
drivers/media/i2c/imx219.c | 6 ++--
drivers/media/i2c/imx258.c | 6 ++--
drivers/media/i2c/imx274.c | 3 +-
drivers/media/i2c/imx290.c | 6 ++--
drivers/media/i2c/imx319.c | 6 ++--
drivers/media/i2c/imx334.c | 5 ++--
drivers/media/i2c/imx355.c | 6 ++--
drivers/media/i2c/mt9m001.c | 8 ++----
drivers/media/i2c/ov02a10.c | 6 ++--
drivers/media/i2c/ov13858.c | 6 ++--
drivers/media/i2c/ov2659.c | 6 ++--
drivers/media/i2c/ov2685.c | 7 ++---
drivers/media/i2c/ov2740.c | 6 ++--
drivers/media/i2c/ov5647.c | 9 +++---
drivers/media/i2c/ov5648.c | 6 ++--
drivers/media/i2c/ov5670.c | 6 ++--
drivers/media/i2c/ov5675.c | 3 +-
drivers/media/i2c/ov5695.c | 6 ++--
drivers/media/i2c/ov7740.c | 8 ++----
drivers/media/i2c/ov8856.c | 3 +-
drivers/media/i2c/ov8865.c | 6 ++--
drivers/media/i2c/ov9734.c | 3 +-
drivers/media/i2c/tvp5150.c | 16 ++---------
drivers/media/i2c/video-i2c.c | 14 +++-------
drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 3 +-
drivers/media/platform/am437x/am437x-vpfe.c | 22 +++++++++++----
drivers/media/platform/atmel/atmel-isc-base.c | 27 +++++++++++++-----
drivers/media/platform/atmel/atmel-isi.c | 19 ++++++++++---
drivers/media/platform/coda/coda-common.c | 5 ++--
drivers/media/platform/exynos-gsc/gsc-core.c | 3 --
drivers/media/platform/exynos-gsc/gsc-m2m.c | 2 +-
.../media/platform/exynos4-is/fimc-capture.c | 6 ++--
drivers/media/platform/exynos4-is/fimc-is.c | 4 +--
.../platform/exynos4-is/fimc-isp-video.c | 3 +-
drivers/media/platform/exynos4-is/fimc-isp.c | 7 ++---
drivers/media/platform/exynos4-is/fimc-lite.c | 5 ++--
drivers/media/platform/exynos4-is/fimc-m2m.c | 2 +-
drivers/media/platform/exynos4-is/media-dev.c | 8 ++----
drivers/media/platform/exynos4-is/mipi-csis.c | 8 ++----
.../media/platform/marvell-ccic/mcam-core.c | 9 ++++--
.../media/platform/mtk-jpeg/mtk_jpeg_core.c | 4 +--
drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 6 ++--
.../platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 4 +--
.../media/platform/qcom/camss/camss-csid.c | 6 ++--
.../media/platform/qcom/camss/camss-csiphy.c | 6 ++--
.../media/platform/qcom/camss/camss-ispif.c | 6 ++--
drivers/media/platform/qcom/camss/camss-vfe.c | 5 ++--
drivers/media/platform/qcom/venus/core.c | 28 +++++++++++--------
.../media/platform/qcom/venus/pm_helpers.c | 10 +++----
drivers/media/platform/qcom/venus/vdec.c | 4 +--
drivers/media/platform/qcom/venus/venc.c | 5 ++--
drivers/media/platform/rcar-fcp.c | 6 ++--
drivers/media/platform/rcar-vin/rcar-csi2.c | 6 ++++
drivers/media/platform/rcar-vin/rcar-dma.c | 6 ++--
drivers/media/platform/rcar-vin/rcar-v4l2.c | 6 ++--
drivers/media/platform/rcar_fdp1.c | 12 ++++++--
drivers/media/platform/renesas-ceu.c | 4 +--
drivers/media/platform/rockchip/rga/rga-buf.c | 3 +-
drivers/media/platform/rockchip/rga/rga.c | 4 ++-
.../platform/rockchip/rkisp1/rkisp1-capture.c | 3 +-
.../media/platform/s3c-camif/camif-capture.c | 2 +-
drivers/media/platform/s3c-camif/camif-core.c | 5 ++--
drivers/media/platform/s5p-jpeg/jpeg-core.c | 2 +-
drivers/media/platform/s5p-mfc/s5p_mfc_pm.c | 6 ++--
drivers/media/platform/sh_vou.c | 6 +++-
drivers/media/platform/sti/bdisp/bdisp-v4l2.c | 7 +++--
drivers/media/platform/sti/delta/delta-v4l2.c | 4 +--
drivers/media/platform/sti/hva/hva-hw.c | 17 +++++------
drivers/media/platform/stm32/stm32-dcmi.c | 5 ++--
.../platform/sunxi/sun4i-csi/sun4i_v4l2.c | 6 ++--
.../sunxi/sun8i-rotate/sun8i_rotate.c | 2 +-
drivers/media/platform/ti-vpe/cal-video.c | 4 ++-
drivers/media/platform/ti-vpe/cal.c | 8 ++++--
drivers/media/platform/ti-vpe/vpe.c | 4 +--
drivers/media/platform/vsp1/vsp1_drv.c | 6 ++--
.../staging/media/atomisp/pci/atomisp_fops.c | 6 ++--
drivers/staging/media/hantro/hantro_drv.c | 7 +++++
drivers/staging/media/imx/imx7-mipi-csis.c | 7 ++---
drivers/staging/media/ipu3/ipu3.c | 3 +-
drivers/staging/media/rkvdec/rkvdec.c | 2 +-
.../staging/media/sunxi/cedrus/cedrus_video.c | 6 ++--
drivers/staging/media/tegra-vde/vde.c | 16 +++++++----
drivers/staging/media/tegra-video/csi.c | 3 +-
drivers/staging/media/tegra-video/vi.c | 3 +-
92 files changed, 298 insertions(+), 335 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 25/79] staging: media: vde: use pm_runtime_resume_and_get()
2021-04-27 10:25 [PATCH v3 00/79] Address some issues with PM runtime at media subsystem Mauro Carvalho Chehab
@ 2021-04-27 10:26 ` Mauro Carvalho Chehab
2021-04-27 11:47 ` Dmitry Osipenko
2021-04-27 10:26 ` [PATCH v3 26/79] staging: media: csi: " Mauro Carvalho Chehab
2021-04-27 10:26 ` [PATCH v3 27/79] staging: media: vi: " Mauro Carvalho Chehab
2 siblings, 1 reply; 7+ messages in thread
From: Mauro Carvalho Chehab @ 2021-04-27 10:26 UTC (permalink / raw)
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Dmitry Osipenko,
Greg Kroah-Hartman, Jonathan Hunter, Mauro Carvalho Chehab,
Thierry Reding, devel, linux-kernel, linux-media, linux-tegra
Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
added pm_runtime_resume_and_get() in order to automatically handle
dev->power.usage_count decrement on errors.
Use the new API, in order to cleanup the error check logic.
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
drivers/staging/media/tegra-vde/vde.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/media/tegra-vde/vde.c b/drivers/staging/media/tegra-vde/vde.c
index 28845b5bafaf..8936f140a246 100644
--- a/drivers/staging/media/tegra-vde/vde.c
+++ b/drivers/staging/media/tegra-vde/vde.c
@@ -775,9 +775,9 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde *vde,
if (ret)
goto release_dpb_frames;
- ret = pm_runtime_get_sync(dev);
+ ret = pm_runtime_resume_and_get(dev);
if (ret < 0)
- goto put_runtime_pm;
+ goto unlock;
/*
* We rely on the VDE registers reset value, otherwise VDE
@@ -843,6 +843,8 @@ static int tegra_vde_ioctl_decode_h264(struct tegra_vde *vde,
put_runtime_pm:
pm_runtime_mark_last_busy(dev);
pm_runtime_put_autosuspend(dev);
+
+unlock:
mutex_unlock(&vde->lock);
release_dpb_frames:
@@ -1069,8 +1071,8 @@ static int tegra_vde_probe(struct platform_device *pdev)
* power-cycle it in order to put hardware into a predictable lower
* power state.
*/
- pm_runtime_get_sync(dev);
- pm_runtime_put(dev);
+ if (pm_runtime_resume_and_get(dev) >= 0)
+ pm_runtime_put(dev);
return 0;
@@ -1088,8 +1090,9 @@ static int tegra_vde_remove(struct platform_device *pdev)
{
struct tegra_vde *vde = platform_get_drvdata(pdev);
struct device *dev = &pdev->dev;
+ int ret;
- pm_runtime_get_sync(dev);
+ ret = pm_runtime_resume_and_get(dev);
pm_runtime_dont_use_autosuspend(dev);
pm_runtime_disable(dev);
@@ -1097,7 +1100,8 @@ static int tegra_vde_remove(struct platform_device *pdev)
* Balance RPM state, the VDE power domain is left ON and hardware
* is clock-gated. It's safe to reboot machine now.
*/
- pm_runtime_put_noidle(dev);
+ if (ret >= 0)
+ pm_runtime_put_noidle(dev);
clk_disable_unprepare(vde->clk);
misc_deregister(&vde->miscdev);
--
2.30.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v3 26/79] staging: media: csi: use pm_runtime_resume_and_get()
2021-04-27 10:25 [PATCH v3 00/79] Address some issues with PM runtime at media subsystem Mauro Carvalho Chehab
2021-04-27 10:26 ` [PATCH v3 25/79] staging: media: vde: use pm_runtime_resume_and_get() Mauro Carvalho Chehab
@ 2021-04-27 10:26 ` Mauro Carvalho Chehab
2021-04-27 10:26 ` [PATCH v3 27/79] staging: media: vi: " Mauro Carvalho Chehab
2 siblings, 0 replies; 7+ messages in thread
From: Mauro Carvalho Chehab @ 2021-04-27 10:26 UTC (permalink / raw)
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Greg Kroah-Hartman,
Jonathan Hunter, Mauro Carvalho Chehab, Sowjanya Komatineni,
Thierry Reding, devel, linux-kernel, linux-media, linux-tegra
Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
added pm_runtime_resume_and_get() in order to automatically handle
dev->power.usage_count decrement on errors.
Use the new API, in order to cleanup the error check logic.
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
drivers/staging/media/tegra-video/csi.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/media/tegra-video/csi.c b/drivers/staging/media/tegra-video/csi.c
index 033a6935c26d..e938bf4c48b6 100644
--- a/drivers/staging/media/tegra-video/csi.c
+++ b/drivers/staging/media/tegra-video/csi.c
@@ -298,10 +298,9 @@ static int tegra_csi_enable_stream(struct v4l2_subdev *subdev)
struct tegra_csi *csi = csi_chan->csi;
int ret, err;
- ret = pm_runtime_get_sync(csi->dev);
+ ret = pm_runtime_resume_and_get(csi->dev);
if (ret < 0) {
dev_err(csi->dev, "failed to get runtime PM: %d\n", ret);
- pm_runtime_put_noidle(csi->dev);
return ret;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v3 27/79] staging: media: vi: use pm_runtime_resume_and_get()
2021-04-27 10:25 [PATCH v3 00/79] Address some issues with PM runtime at media subsystem Mauro Carvalho Chehab
2021-04-27 10:26 ` [PATCH v3 25/79] staging: media: vde: use pm_runtime_resume_and_get() Mauro Carvalho Chehab
2021-04-27 10:26 ` [PATCH v3 26/79] staging: media: csi: " Mauro Carvalho Chehab
@ 2021-04-27 10:26 ` Mauro Carvalho Chehab
2 siblings, 0 replies; 7+ messages in thread
From: Mauro Carvalho Chehab @ 2021-04-27 10:26 UTC (permalink / raw)
Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Greg Kroah-Hartman,
Jonathan Hunter, Mauro Carvalho Chehab, Sowjanya Komatineni,
Thierry Reding, devel, linux-kernel, linux-media, linux-tegra
Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
added pm_runtime_resume_and_get() in order to automatically handle
dev->power.usage_count decrement on errors.
Use the new API, in order to cleanup the error check logic.
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
drivers/staging/media/tegra-video/vi.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
index 7a09061cda57..1298740a9c6c 100644
--- a/drivers/staging/media/tegra-video/vi.c
+++ b/drivers/staging/media/tegra-video/vi.c
@@ -297,10 +297,9 @@ static int tegra_channel_start_streaming(struct vb2_queue *vq, u32 count)
struct tegra_vi_channel *chan = vb2_get_drv_priv(vq);
int ret;
- ret = pm_runtime_get_sync(chan->vi->dev);
+ ret = pm_runtime_resume_and_get(chan->vi->dev);
if (ret < 0) {
dev_err(chan->vi->dev, "failed to get runtime PM: %d\n", ret);
- pm_runtime_put_noidle(chan->vi->dev);
return ret;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 25/79] staging: media: vde: use pm_runtime_resume_and_get()
2021-04-27 10:26 ` [PATCH v3 25/79] staging: media: vde: use pm_runtime_resume_and_get() Mauro Carvalho Chehab
@ 2021-04-27 11:47 ` Dmitry Osipenko
2021-04-28 7:20 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 7+ messages in thread
From: Dmitry Osipenko @ 2021-04-27 11:47 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: linuxarm, mauro.chehab, Greg Kroah-Hartman, Jonathan Hunter,
Mauro Carvalho Chehab, Thierry Reding, devel, linux-kernel,
linux-media, linux-tegra
27.04.2021 13:26, Mauro Carvalho Chehab пишет:
> @@ -1088,8 +1090,9 @@ static int tegra_vde_remove(struct platform_device *pdev)
> {
> struct tegra_vde *vde = platform_get_drvdata(pdev);
> struct device *dev = &pdev->dev;
> + int ret;
>
> - pm_runtime_get_sync(dev);
> + ret = pm_runtime_resume_and_get(dev);
Should be cleaner to return error directly here, IMO.
> pm_runtime_dont_use_autosuspend(dev);
> pm_runtime_disable(dev);
>
> @@ -1097,7 +1100,8 @@ static int tegra_vde_remove(struct platform_device *pdev)
> * Balance RPM state, the VDE power domain is left ON and hardware
> * is clock-gated. It's safe to reboot machine now.
> */
> - pm_runtime_put_noidle(dev);
> + if (ret >= 0)
> + pm_runtime_put_noidle(dev);
> clk_disable_unprepare(vde->clk);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 25/79] staging: media: vde: use pm_runtime_resume_and_get()
2021-04-27 11:47 ` Dmitry Osipenko
@ 2021-04-28 7:20 ` Mauro Carvalho Chehab
2021-04-28 8:05 ` Dmitry Osipenko
0 siblings, 1 reply; 7+ messages in thread
From: Mauro Carvalho Chehab @ 2021-04-28 7:20 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: linuxarm, mauro.chehab, Greg Kroah-Hartman, Jonathan Hunter,
Mauro Carvalho Chehab, Thierry Reding, devel, linux-kernel,
linux-media, linux-tegra
Em Tue, 27 Apr 2021 14:47:01 +0300
Dmitry Osipenko <digetx@gmail.com> escreveu:
> 27.04.2021 13:26, Mauro Carvalho Chehab пишет:
> > @@ -1088,8 +1090,9 @@ static int tegra_vde_remove(struct platform_device *pdev)
> > {
> > struct tegra_vde *vde = platform_get_drvdata(pdev);
> > struct device *dev = &pdev->dev;
> > + int ret;
> >
> > - pm_runtime_get_sync(dev);
> > + ret = pm_runtime_resume_and_get(dev);
>
> Should be cleaner to return error directly here, IMO.
I double-checked how drivers/base/platform.c deals with non-zero
returns at the .remove method:
static int platform_remove(struct device *_dev)
{
struct platform_driver *drv = to_platform_driver(_dev->driver);
struct platform_device *dev = to_platform_device(_dev);
if (drv->remove) {
int ret = drv->remove(dev);
if (ret)
dev_warn(_dev, "remove callback returned a non-zero value. This will be ignored.\n");
}
dev_pm_domain_detach(_dev, true);
return 0;
}
Basically, it will print a message but will ignore whatever happens
afterwards.
So, if the driver is changed to return an error there, it will leak
resources.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 25/79] staging: media: vde: use pm_runtime_resume_and_get()
2021-04-28 7:20 ` Mauro Carvalho Chehab
@ 2021-04-28 8:05 ` Dmitry Osipenko
0 siblings, 0 replies; 7+ messages in thread
From: Dmitry Osipenko @ 2021-04-28 8:05 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: linuxarm, mauro.chehab, Greg Kroah-Hartman, Jonathan Hunter,
Mauro Carvalho Chehab, Thierry Reding, devel, linux-kernel,
linux-media, linux-tegra
28.04.2021 10:20, Mauro Carvalho Chehab пишет:
> Em Tue, 27 Apr 2021 14:47:01 +0300
> Dmitry Osipenko <digetx@gmail.com> escreveu:
>
>> 27.04.2021 13:26, Mauro Carvalho Chehab пишет:
>>> @@ -1088,8 +1090,9 @@ static int tegra_vde_remove(struct platform_device *pdev)
>>> {
>>> struct tegra_vde *vde = platform_get_drvdata(pdev);
>>> struct device *dev = &pdev->dev;
>>> + int ret;
>>>
>>> - pm_runtime_get_sync(dev);
>>> + ret = pm_runtime_resume_and_get(dev);
>>
>> Should be cleaner to return error directly here, IMO.
>
> I double-checked how drivers/base/platform.c deals with non-zero
> returns at the .remove method:
>
> static int platform_remove(struct device *_dev)
> {
> struct platform_driver *drv = to_platform_driver(_dev->driver);
> struct platform_device *dev = to_platform_device(_dev);
>
> if (drv->remove) {
> int ret = drv->remove(dev);
>
> if (ret)
> dev_warn(_dev, "remove callback returned a non-zero value. This will be ignored.\n");
> }
> dev_pm_domain_detach(_dev, true);
>
> return 0;
> }
>
> Basically, it will print a message but will ignore whatever happens
> afterwards.
>
> So, if the driver is changed to return an error there, it will leak
> resources.
Indeed, thank you. But then the pm_runtime_get_sync() should be more
appropriate since this function is specifically made for such cases
where returned value is ignored.
A better option could be better to add a clarifying comment to the code
rather than to change it to a variant which introduces confusion, IMO.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-04-28 8:05 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-04-27 10:25 [PATCH v3 00/79] Address some issues with PM runtime at media subsystem Mauro Carvalho Chehab
2021-04-27 10:26 ` [PATCH v3 25/79] staging: media: vde: use pm_runtime_resume_and_get() Mauro Carvalho Chehab
2021-04-27 11:47 ` Dmitry Osipenko
2021-04-28 7:20 ` Mauro Carvalho Chehab
2021-04-28 8:05 ` Dmitry Osipenko
2021-04-27 10:26 ` [PATCH v3 26/79] staging: media: csi: " Mauro Carvalho Chehab
2021-04-27 10:26 ` [PATCH v3 27/79] staging: media: vi: " Mauro Carvalho Chehab
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox