[driver-core:umn.edu-reverts] BUILD SUCCESS 73d201b5c3bcd9af293eb8b5f8010f479b96a590
lyesconfig s390defconfig sparcallyesconfig sparc defconfig mips allyesconfig powerpc allyesconfig powerpc allmodconfig powerpc allnoconfig i386 randconfig-a005-20210428 i386 randconfig-a002-20210428 i386 randconfig-a001-20210428 i386 randconfig-a006-20210428 i386 randconfig-a003-20210428 i386 randconfig-a004-20210428 x86_64 randconfig-a015-20210428 x86_64 randconfig-a016-20210428 x86_64 randconfig-a011-20210428 x86_64 randconfig-a014-20210428 x86_64 randconfig-a013-20210428 x86_64 randconfig-a012-20210428 i386 randconfig-a012-20210428 i386 randconfig-a014-20210428 i386 randconfig-a013-20210428 i386 randconfig-a011-20210428 i386 randconfig-a015-20210428 i386 randconfig-a016-20210428 riscvnommu_k210_defconfig riscv allnoconfig riscv defconfig riscv rv32_defconfig um allmodconfig umallnoconfig um defconfig x86_64rhel-8.3-kselftests x86_64 defconfig x86_64 rhel-8.3 x86_64 rhel-8.3-kbuiltin x86_64 kexec clang tested configs: x86_64 randconfig-a004-20210428 x86_64 randconfig-a002-20210428 x86_64 randconfig-a005-20210428 x86_64 randconfig-a001-20210428 x86_64 randconfig-a003-20210428 x86_64 randconfig-a006-20210428 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[driver-core:readfile] BUILD SUCCESS eb9c2f4bdf48492684e41e3ebd1304e006db6492
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git readfile branch HEAD: eb9c2f4bdf48492684e41e3ebd1304e006db6492 readfile.2: new page describing readfile(2) elapsed time: 721m configs tested: 107 configs skipped: 2 The following configs have been built successfully. More configs may be tested in the coming days. gcc tested configs: arm defconfig arm64allyesconfig arm64 defconfig arm allyesconfig arm allmodconfig riscvallyesconfig arm hackkit_defconfig armshmobile_defconfig mips mtx1_defconfig arm netwinder_defconfig arm ep93xx_defconfig s390 debug_defconfig sh ap325rxa_defconfig mipsqi_lb60_defconfig mips allmodconfig alpha defconfig mips maltasmvp_eva_defconfig arc tb10x_defconfig arm jornada720_defconfig powerpc mpc837x_rdb_defconfig sparcalldefconfig powerpc katmai_defconfig powerpcgamecube_defconfig arm cns3420vb_defconfig powerpc bamboo_defconfig um allyesconfig arm multi_v4t_defconfig mips sb1250_swarm_defconfig arm nhk8815_defconfig sh kfr2r09-romimage_defconfig powerpc redwood_defconfig mipsomega2p_defconfig s390 alldefconfig openrisc simple_smp_defconfig ia64 tiger_defconfig arm omap1_defconfig riscvnommu_virt_defconfig powerpc wii_defconfig armspear3xx_defconfig ia64 allmodconfig ia64defconfig ia64 allyesconfig m68k allmodconfig m68kdefconfig m68k allyesconfig nds32 defconfig nios2allyesconfig cskydefconfig alphaallyesconfig xtensa allyesconfig h8300allyesconfig arc defconfig sh allmodconfig parisc defconfig s390 allyesconfig s390 allmodconfig parisc allyesconfig s390defconfig i386 allyesconfig sparcallyesconfig sparc defconfig i386defconfig nios2 defconfig arc allyesconfig nds32 allnoconfig mips allyesconfig powerpc allyesconfig powerpc allmodconfig powerpc allnoconfig i386 randconfig-a005-20210428 i386 randconfig-a002-20210428 i386 randconfig-a001-20210428 i386 randconfig-a006-20210428 i386 randconfig-a003-20210428 i386 randconfig-a004-20210428 x86_64 randconfig-a015-20210428 x86_64 randconfig-a016-20210428 x86_64 randconfig-a011-20210428 x86_64 randconfig-a014-20210428 x86_64 randconfig-a013-20210428 x86_64 randconfig-a012-20210428 i386 randconfig-a012-20210428 i386 randconfig-a014-20210428 i386 randconfig-a013-20210428 i386 randconfig-a011-20210428 i386 randconfig-a015-20210428 i386 randconfig-a016-20210428 riscvnommu_k210_defconfig riscv allnoconfig riscv defconfig riscv rv32_defconfig riscvallmodconfig um allmodconfig umallnoconfig um defconfig x86_64 allyesconfig x86_64rhel-8.3-kselftests x86_64 defconfig x86_64 rhel-8.3 x86_64 rhel-8.3-kbuiltin x86_64
Re: [PATCH v4 79/79] media: hantro: do a PM resume earlier
Hi Mauro, Thanks a lot for taking care of this. On Wed, 2021-04-28 at 16:52 +0200, Mauro Carvalho Chehab wrote: > The device_run() first enables the clock and then > tries to resume PM runtime, checking for errors. > > Well, if for some reason the pm_runtime can not resume, > it would be better to detect it beforehand. > > So, change the order inside device_run(). > > Signed-off-by: Mauro Carvalho Chehab Clocks could be behind power-domains, IIRC, so this change is fixing that. However, this has ever been a problem for this driver, so I don't think it makes sense to bother with Fixes tag. Reviewed-by: Ezequiel Garcia Thanks, Ezequiel > --- > drivers/staging/media/hantro/hantro_drv.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/media/hantro/hantro_drv.c > b/drivers/staging/media/hantro/hantro_drv.c > index 25fa36e7e773..67de6b15236d 100644 > --- a/drivers/staging/media/hantro/hantro_drv.c > +++ b/drivers/staging/media/hantro/hantro_drv.c > @@ -160,14 +160,14 @@ static void device_run(void *priv) > src = hantro_get_src_buf(ctx); > dst = hantro_get_dst_buf(ctx); > > - ret = clk_bulk_enable(ctx->dev->variant->num_clocks, > ctx->dev->clocks); > - if (ret) > - goto err_cancel_job; > - > ret = pm_runtime_resume_and_get(ctx->dev->dev); > if (ret < 0) > goto err_cancel_job; > > + ret = clk_bulk_enable(ctx->dev->variant->num_clocks, > ctx->dev->clocks); > + if (ret) > + goto err_cancel_job; > + > v4l2_m2m_buf_copy_metadata(src, dst, true); > > ctx->codec_ops->run(ctx); ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 78/79] media: hantro: use pm_runtime_resume_and_get()
On Wed, 2021-04-28 at 16:52 +0200, Mauro Carvalho Chehab wrote:
> 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.
>
> While there's nothing wrong with the current usage on this driver,
> as we're getting rid of the pm_runtime_get_sync() call all over
> the media subsystem, let's remove the last occurrence on this
> driver.
>
> Signed-off-by: Mauro Carvalho Chehab
Looks good.
Reviewed-by: Ezequiel Garcia
Thanks,
Ezequiel
> ---
> drivers/staging/media/hantro/hantro_drv.c | 23 ---
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/media/hantro/hantro_drv.c
> b/drivers/staging/media/hantro/hantro_drv.c
> index 595e82a82728..25fa36e7e773 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -56,14 +56,12 @@ dma_addr_t hantro_get_ref(struct hantro_ctx *ctx, u64 ts)
> return hantro_get_dec_buf_addr(ctx, buf);
> }
>
> -static void hantro_job_finish(struct hantro_dev *vpu,
> - struct hantro_ctx *ctx,
> - enum vb2_buffer_state result)
> +static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
> + struct hantro_ctx *ctx,
> + enum vb2_buffer_state result)
> {
> struct vb2_v4l2_buffer *src, *dst;
>
> - pm_runtime_mark_last_busy(vpu->dev);
> - pm_runtime_put_autosuspend(vpu->dev);
> clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
>
> src = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> @@ -81,6 +79,16 @@ static void hantro_job_finish(struct hantro_dev *vpu,
> result);
> }
>
> +static void hantro_job_finish(struct hantro_dev *vpu,
> + struct hantro_ctx *ctx,
> + enum vb2_buffer_state result)
> +{
> + pm_runtime_mark_last_busy(vpu->dev);
> + pm_runtime_put_autosuspend(vpu->dev);
> +
> + hantro_job_finish_no_pm(vpu, ctx, result);
> +}
> +
> void hantro_irq_done(struct hantro_dev *vpu,
> enum vb2_buffer_state result)
> {
> @@ -155,7 +163,8 @@ static void device_run(void *priv)
> ret = clk_bulk_enable(ctx->dev->variant->num_clocks,
> ctx->dev->clocks);
> if (ret)
> goto err_cancel_job;
> - ret = pm_runtime_get_sync(ctx->dev->dev);
> +
> + ret = pm_runtime_resume_and_get(ctx->dev->dev);
> if (ret < 0)
> goto err_cancel_job;
>
> @@ -165,7 +174,7 @@ static void device_run(void *priv)
> return;
>
> err_cancel_job:
> - hantro_job_finish(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
> + hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
> }
>
> static struct v4l2_m2m_ops vpu_m2m_ops = {
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 00/79] Address some issues with PM runtime at media subsystem
On Wed, Apr 28, 2021 at 04:51:21PM +0200, Mauro Carvalho Chehab wrote:
> 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(>dev);
> pm_runtime_disable(>dev);
> pm_runtime_set_suspended(>dev);
> pm_runtime_put_noidle(>dev);
>
>It makes no sense to resume PM just to suspend it again ;-)
This is perfectly alright. Take a look at ov7740_remove() for example:
pm_runtime_get_sync(>dev);
pm_runtime_disable(>dev);
pm_runtime_set_suspended(>dev);
pm_runtime_put_noidle(>dev);
ov7740_set_power(ov7740, 0);
There's an explicit power-off after balancing the PM count and this will
work regardless of the power state when entering this function.
So this has nothing to do with pm_runtime_get_sync() per se.
> 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;
Sure, but pm_runtime_get_async() also works this way. You just won't be
notified if the async resume 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;
It also introduced an inconsistency in the API and does not pair as well
with the pm_runtime_put variants.
> 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;
It's not known to be safe; there are ways to get also this interface
wrong as for example this series has shown.
> 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.
Cargo-cult programming always runs that risk.
> 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.
The only valid point about and use for pm_runtime_resume_and_get() is to
avoid leaking a PM usage count reference in the unlikely case that
resume fails (something which hardly any driver implements recovery
from anyway).
It's a convenience wrapper that saves you from writing one extra line in
some cases (depending on how you implement runtime-pm support) and not a
silver bullet against bugs.
> 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.
>
> ---
>
> v4:
> - Added a couple of additional fixes at existing PM runtime code;
> - Some patches are now more conservative in order to avoid causing
> regressions.
> v3:
> - fix a compilation error;
> v2:
> - addressed pointed issues and fixed a few other PM issues.
This really doesn't say much more than "changed stuff" so kinda hard to
track if review feedback has been taken into account for example.
Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 20/79] staging: media: rkvdec: fix pm_runtime_get_sync() usage count
On Wed, Apr 28, 2021 at 04:51:41PM +0200, Mauro Carvalho Chehab wrote:
> The pm_runtime_get_sync() internally increments the
> dev->power.usage_count without decrementing it, even on errors.
> Replace it by the new pm_runtime_resume_and_get(), introduced by:
> commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with
> usage counter")
> in order to properly decrement the usage counter and avoid memory
> leaks.
Again, there is no memory leak here either. Just a potential PM usage
counter leak.
> Reviewed-by: Ezequiel Garcia
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/staging/media/rkvdec/rkvdec.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c
> b/drivers/staging/media/rkvdec/rkvdec.c
> index d821661d30f3..8c17615f3a7a 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -658,7 +658,7 @@ static void rkvdec_device_run(void *priv)
> if (WARN_ON(!desc))
> return;
>
> - ret = pm_runtime_get_sync(rkvdec->dev);
> + ret = pm_runtime_resume_and_get(rkvdec->dev);
> if (ret < 0) {
> rkvdec_job_finish_no_pm(ctx, VB2_BUF_STATE_ERROR);
> return;
Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4 25/79] staging: media: tegra-vde: use pm_runtime_resume_and_get()
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
---
drivers/staging/media/tegra-vde/vde.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/media/tegra-vde/vde.c
b/drivers/staging/media/tegra-vde/vde.c
index 28845b5bafaf..1cdacb3f781c 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(>lock);
release_dpb_frames:
@@ -1069,11 +1071,17 @@ 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);
+ if (pm_runtime_resume_and_get(dev) < 0)
+ goto err_pm_runtime;
+
pm_runtime_put(dev);
return 0;
+err_pm_runtime:
+ pm_runtime_dont_use_autosuspend(dev);
+ pm_runtime_disable(dev);
+
err_deinit_iommu:
tegra_vde_iommu_deinit(vde);
@@ -1089,7 +1097,12 @@ static int tegra_vde_remove(struct platform_device *pdev)
struct tegra_vde *vde = platform_get_drvdata(pdev);
struct device *dev = >dev;
+ /*
+* As it increments RPM usage_count even on errors, we don't need to
+* check the returned code here.
+*/
pm_runtime_get_sync(dev);
+
pm_runtime_dont_use_autosuspend(dev);
pm_runtime_disable(dev);
--
2.30.2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4 20/79] staging: media: rkvdec: fix pm_runtime_get_sync() usage count
The pm_runtime_get_sync() internally increments the
dev->power.usage_count without decrementing it, even on errors.
Replace it by the new pm_runtime_resume_and_get(), introduced by:
commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with
usage counter")
in order to properly decrement the usage counter and avoid memory
leaks.
Reviewed-by: Ezequiel Garcia
Signed-off-by: Mauro Carvalho Chehab
---
drivers/staging/media/rkvdec/rkvdec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/media/rkvdec/rkvdec.c
b/drivers/staging/media/rkvdec/rkvdec.c
index d821661d30f3..8c17615f3a7a 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -658,7 +658,7 @@ static void rkvdec_device_run(void *priv)
if (WARN_ON(!desc))
return;
- ret = pm_runtime_get_sync(rkvdec->dev);
+ ret = pm_runtime_resume_and_get(rkvdec->dev);
if (ret < 0) {
rkvdec_job_finish_no_pm(ctx, VB2_BUF_STATE_ERROR);
return;
--
2.30.2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4 78/79] media: hantro: use pm_runtime_resume_and_get()
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.
While there's nothing wrong with the current usage on this driver,
as we're getting rid of the pm_runtime_get_sync() call all over
the media subsystem, let's remove the last occurrence on this
driver.
Signed-off-by: Mauro Carvalho Chehab
---
drivers/staging/media/hantro/hantro_drv.c | 23 ---
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/media/hantro/hantro_drv.c
b/drivers/staging/media/hantro/hantro_drv.c
index 595e82a82728..25fa36e7e773 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -56,14 +56,12 @@ dma_addr_t hantro_get_ref(struct hantro_ctx *ctx, u64 ts)
return hantro_get_dec_buf_addr(ctx, buf);
}
-static void hantro_job_finish(struct hantro_dev *vpu,
- struct hantro_ctx *ctx,
- enum vb2_buffer_state result)
+static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
+ struct hantro_ctx *ctx,
+ enum vb2_buffer_state result)
{
struct vb2_v4l2_buffer *src, *dst;
- pm_runtime_mark_last_busy(vpu->dev);
- pm_runtime_put_autosuspend(vpu->dev);
clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
src = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
@@ -81,6 +79,16 @@ static void hantro_job_finish(struct hantro_dev *vpu,
result);
}
+static void hantro_job_finish(struct hantro_dev *vpu,
+ struct hantro_ctx *ctx,
+ enum vb2_buffer_state result)
+{
+ pm_runtime_mark_last_busy(vpu->dev);
+ pm_runtime_put_autosuspend(vpu->dev);
+
+ hantro_job_finish_no_pm(vpu, ctx, result);
+}
+
void hantro_irq_done(struct hantro_dev *vpu,
enum vb2_buffer_state result)
{
@@ -155,7 +163,8 @@ static void device_run(void *priv)
ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
if (ret)
goto err_cancel_job;
- ret = pm_runtime_get_sync(ctx->dev->dev);
+
+ ret = pm_runtime_resume_and_get(ctx->dev->dev);
if (ret < 0)
goto err_cancel_job;
@@ -165,7 +174,7 @@ static void device_run(void *priv)
return;
err_cancel_job:
- hantro_job_finish(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
+ hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
}
static struct v4l2_m2m_ops vpu_m2m_ops = {
--
2.30.2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4 26/79] staging: media: tegra-video: use pm_runtime_resume_and_get()
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
---
drivers/staging/media/tegra-video/csi.c | 3 +--
drivers/staging/media/tegra-video/vi.c | 3 +--
2 files changed, 2 insertions(+), 4 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;
}
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4 79/79] media: hantro: do a PM resume earlier
The device_run() first enables the clock and then tries to resume PM runtime, checking for errors. Well, if for some reason the pm_runtime can not resume, it would be better to detect it beforehand. So, change the order inside device_run(). Signed-off-by: Mauro Carvalho Chehab --- drivers/staging/media/hantro/hantro_drv.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c index 25fa36e7e773..67de6b15236d 100644 --- a/drivers/staging/media/hantro/hantro_drv.c +++ b/drivers/staging/media/hantro/hantro_drv.c @@ -160,14 +160,14 @@ static void device_run(void *priv) src = hantro_get_src_buf(ctx); dst = hantro_get_dst_buf(ctx); - ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks); - if (ret) - goto err_cancel_job; - ret = pm_runtime_resume_and_get(ctx->dev->dev); if (ret < 0) goto err_cancel_job; + ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks); + if (ret) + goto err_cancel_job; + v4l2_m2m_buf_copy_metadata(src, dst, true); ctx->codec_ops->run(ctx); -- 2.30.2 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4 00/79] Address some issues with PM runtime at media subsystem
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(>dev);
pm_runtime_disable(>dev);
pm_runtime_set_suspended(>dev);
pm_runtime_put_noidle(>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.
---
v4:
- Added a couple of additional fixes at existing PM runtime code;
- Some patches are now more conservative in order to avoid causing
regressions.
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: s6p_cec: decrement usage count if disabled
media: i2c: ccs-core: return the right error code at suspend
media: i2c: ov7740: don't resume at remove time
media: i2c: video-i2c: don't resume at remove time
media: i2c: imx334: fix the pm runtime get logic
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: 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: sti/delta: fix pm_runtime_get_sync() usage count
media: sunxi: fix pm_runtime_get_sync() usage count
staging: media: rkvdec: fix pm_runtime_get_sync() usage count
staging: media: atomisp: 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: tegra-vde: use pm_runtime_resume_and_get()
staging: media: tegra-video: 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: imx355: use pm_runtime_resume_and_get()
[PATCH v4 24/79] staging: media: cedrus_video: use pm_runtime_resume_and_get()
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
---
drivers/staging/media/sunxi/cedrus/cedrus_video.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index b62eb8e84057..9ddd789d0b1f 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -490,11 +490,9 @@ static int cedrus_start_streaming(struct vb2_queue *vq,
unsigned int count)
}
if (V4L2_TYPE_IS_OUTPUT(vq->type)) {
- ret = pm_runtime_get_sync(dev->dev);
- if (ret < 0) {
- pm_runtime_put_noidle(dev->dev);
+ ret = pm_runtime_resume_and_get(dev->dev);
+ if (ret < 0)
goto err_cleanup;
- }
if (dev->dec_ops[ctx->current_codec]->start) {
ret = dev->dec_ops[ctx->current_codec]->start(ctx);
--
2.30.2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4 22/79] staging: media: imx7-mipi-csis: use pm_runtime_resume_and_get()
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.
Acked-by: Rui Miguel Silva
Signed-off-by: Mauro Carvalho Chehab
---
drivers/staging/media/imx/imx7-mipi-csis.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c
b/drivers/staging/media/imx/imx7-mipi-csis.c
index 025fdc488bd6..1dc680d94a46 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -695,11 +695,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev
*mipi_sd, int enable)
mipi_csis_clear_counters(state);
- ret = pm_runtime_get_sync(>pdev->dev);
- if (ret < 0) {
- pm_runtime_put_noidle(>pdev->dev);
+ ret = pm_runtime_resume_and_get(>pdev->dev);
+ if (ret < 0)
return ret;
- }
+
ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
if (ret < 0 && ret != -ENOIOCTLCMD)
goto done;
--
2.30.2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4 21/79] staging: media: atomisp: use pm_runtime_resume_and_get()
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
---
drivers/staging/media/atomisp/pci/atomisp_fops.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c
b/drivers/staging/media/atomisp/pci/atomisp_fops.c
index f1e6b2597853..26d05474a035 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
@@ -837,7 +837,7 @@ static int atomisp_open(struct file *file)
}
/* runtime power management, turn on ISP */
- ret = pm_runtime_get_sync(vdev->v4l2_dev->dev);
+ ret = pm_runtime_resume_and_get(vdev->v4l2_dev->dev);
if (ret < 0) {
dev_err(isp->dev, "Failed to power on device\n");
goto error;
@@ -881,9 +881,9 @@ static int atomisp_open(struct file *file)
css_error:
atomisp_css_uninit(isp);
-error:
- hmm_pool_unregister(HMM_POOL_TYPE_DYNAMIC);
pm_runtime_put(vdev->v4l2_dev->dev);
+error:
+ hmm_pool_unregister(HMM_POOL_TYPE_DYNAMIC);
rt_mutex_unlock(>mutex);
return ret;
}
--
2.30.2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4 23/79] staging: media: ipu3: use pm_runtime_resume_and_get()
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
---
drivers/staging/media/ipu3/ipu3.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/media/ipu3/ipu3.c
b/drivers/staging/media/ipu3/ipu3.c
index ee1bba6bdcac..8e1e9e46e604 100644
--- a/drivers/staging/media/ipu3/ipu3.c
+++ b/drivers/staging/media/ipu3/ipu3.c
@@ -392,10 +392,9 @@ int imgu_s_stream(struct imgu_device *imgu, int enable)
}
/* Set Power */
- r = pm_runtime_get_sync(dev);
+ r = pm_runtime_resume_and_get(dev);
if (r < 0) {
dev_err(dev, "failed to set imgu power\n");
- pm_runtime_put(dev);
return r;
}
--
2.30.2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 79/79] media: hantro: document the usage of pm_runtime_get_sync()
Hi Mauro,
On Wed, 2021-04-28 at 08:44 +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 28 Apr 2021 08:27:42 +0200
> Mauro Carvalho Chehab escreveu:
>
> > Em Tue, 27 Apr 2021 12:18:32 -0300
> > Ezequiel Garcia escreveu:
> >
> > > On Tue, 2021-04-27 at 16:08 +0100, Robin Murphy wrote:
> > > > On 2021-04-27 11:27, Mauro Carvalho Chehab wrote:
> > > > > Despite other *_get()/*_put() functions, where usage count is
> > > > > incremented only if not errors, the pm_runtime_get_sync() has
> > > > > a different behavior, incrementing the counter *even* on
> > > > > errors.
> > > > >
> > > > > That's an error prone behavior, as people often forget to
> > > > > decrement the usage counter.
> > > > >
> > > > > However, the hantro driver depends on this behavior, as it
> > > > > will decrement the usage_count unconditionally at the m2m
> > > > > job finish time, which makes sense.
> > > > >
> > > > > So, intead of using the pm_runtime_resume_and_get() that
> > > > > would decrement the counter on error, keep the current
> > > > > API, but add a documentation explaining the rationale for
> > > > > keep using pm_runtime_get_sync().
> > > > >
> > > > > Signed-off-by: Mauro Carvalho Chehab
> > > > > ---
> > > > > drivers/staging/media/hantro/hantro_drv.c | 7 +++
> > > > > 1 file changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/drivers/staging/media/hantro/hantro_drv.c
> > > > > b/drivers/staging/media/hantro/hantro_drv.c
> > > > > index 595e82a82728..96f940c1c85c 100644
> > > > > --- a/drivers/staging/media/hantro/hantro_drv.c
> > > > > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > > > > @@ -155,6 +155,13 @@ static void device_run(void *priv)
> > > > > ret = clk_bulk_enable(ctx->dev->variant->num_clocks,
> > > > > ctx->dev->clocks);
> > > > > if (ret)
> > > > > goto err_cancel_job;
> > > >
> > > > ..except this can also cause the same pm_runtime_put_autosuspend() call
> > > > without even reaching the "matching" get below, so rather than some
> > > > kind
> > > > of cleverness it seems more like it's just broken :/
> > > >
> > >
> > > Indeed, I was trying to find time to cook a quick patch, but kept
> > > getting preempted.
> > >
> > > Feel free to submit a fix for this, otherwise, I'll try to find
> > > time later this week.
> >
> > What about doing this instead:
> >
> > diff --git a/drivers/staging/media/hantro/hantro_drv.c
> > b/drivers/staging/media/hantro/hantro_drv.c
> > index 595e82a82728..67de6b15236d 100644
> > --- a/drivers/staging/media/hantro/hantro_drv.c
> > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > @@ -56,14 +56,12 @@ dma_addr_t hantro_get_ref(struct hantro_ctx *ctx, u64
> > ts)
> > return hantro_get_dec_buf_addr(ctx, buf);
> > }
> >
> > -static void hantro_job_finish(struct hantro_dev *vpu,
> > - struct hantro_ctx *ctx,
> > - enum vb2_buffer_state result)
> > +static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
> > + struct hantro_ctx *ctx,
> > + enum vb2_buffer_state result)
> > {
> > struct vb2_v4l2_buffer *src, *dst;
> >
> > - pm_runtime_mark_last_busy(vpu->dev);
> > - pm_runtime_put_autosuspend(vpu->dev);
> > clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> >
> > src = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> > @@ -81,6 +79,16 @@ static void hantro_job_finish(struct hantro_dev *vpu,
> > result);
> > }
> >
> > +static void hantro_job_finish(struct hantro_dev *vpu,
> > + struct hantro_ctx *ctx,
> > + enum vb2_buffer_state result)
> > +{
> > + pm_runtime_mark_last_busy(vpu->dev);
> > + pm_runtime_put_autosuspend(vpu->dev);
> > +
> > + hantro_job_finish_no_pm(vpu, ctx, result);
> > +}
> > +
> > void hantro_irq_done(struct hantro_dev *vpu,
> > enum vb2_buffer_state result)
> > {
> > @@ -152,12 +160,13 @@ static void device_run(void *priv)
> > src = hantro_get_src_buf(ctx);
> > dst = hantro_get_dst_buf(ctx);
> >
> > + ret = pm_runtime_resume_and_get(ctx->dev->dev);
> > + if (ret < 0)
> > + goto err_cancel_job;
> > +
> > ret = clk_bulk_enable(ctx->dev->variant->num_clocks,
> > ctx->dev->clocks);
> > if (ret)
> > goto err_cancel_job;
> > - ret = pm_runtime_get_sync(ctx->dev->dev);
> > - if (ret < 0)
> > - goto err_cancel_job;
> >
> > v4l2_m2m_buf_copy_metadata(src, dst, true);
> >
> > @@ -165,7 +174,7 @@ static void device_run(void *priv)
> > return;
> >
> > err_cancel_job:
> > - hantro_job_finish(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
> > + hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
> > }
> >
> > static struct v4l2_m2m_ops
Re: [PATCH 00/78] media: use pm_runtime_resume_and_get() instead of pm_runtime_get_sync()
There was a Smatch check for these bugs. This was a good source of recurring Reported-by tags for me. ;) Thanks for doing this. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 25/79] staging: media: vde: use pm_runtime_resume_and_get()
28.04.2021 10:20, Mauro Carvalho Chehab пишет:
> Em Tue, 27 Apr 2021 14:47:01 +0300
> Dmitry Osipenko 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 = >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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8192e: fix array of flexible structures
On Wed, Apr 28, 2021 at 08:01:21AM +0200, Greg KH wrote:
On Wed, Apr 28, 2021 at 12:28:44AM +0530, Jitendra wrote:
On Tue, Apr 27, 2021 at 08:10:20PM +0200, Greg KH wrote:
> On Tue, Apr 27, 2021 at 11:19:45PM +0530, Jitendra Khasdev wrote:
> > This patch fixes sparse warning "array of flexible structures"
> > for rtllib.h.
> >
> > eg. drivers/staging/rtl8192e/rtllib.h:832:48: warning: array of
> > flexible structures
> >
> > Signed-off-by: Jitendra Khasdev
> > ---
> > drivers/staging/rtl8192e/rtllib.h | 10 +-
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8192e/rtllib.h
b/drivers/staging/rtl8192e/rtllib.h
> > index 4cabaf2..c7cb318 100644
> > --- a/drivers/staging/rtl8192e/rtllib.h
> > +++ b/drivers/staging/rtl8192e/rtllib.h
> > @@ -802,7 +802,7 @@ struct rtllib_authentication {
> > __le16 transaction;
> > __le16 status;
> > /*challenge*/
> > - struct rtllib_info_element info_element[];
> > + struct rtllib_info_element *info_element;
>
> You just changed the definition of this structure, and the other
> structures here. Are you sure this is working properly?
>
I have compiled the driver and install it on my vm, but I don't this specific
hardware, so couldn't test it.
I fixed in context of sparse.
Please verify that this change is correct by looking at how the
structures are being created (i.e. is this being treated as a flexible
array or a pointer?)
I think we have been through this before and that sparse is not right,
but I can't remember...
Yes, it is getting used as flexible array in code. hence, simply we can drop
this patch.
Also, looks to me, there is no more sparse warnings to fix in staging.
---
Jitendra
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 25/79] staging: media: vde: use pm_runtime_resume_and_get()
Em Tue, 27 Apr 2021 14:47:01 +0300
Dmitry Osipenko 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 = >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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 79/79] media: hantro: document the usage of pm_runtime_get_sync()
Em Wed, 28 Apr 2021 08:27:42 +0200
Mauro Carvalho Chehab escreveu:
> Em Tue, 27 Apr 2021 12:18:32 -0300
> Ezequiel Garcia escreveu:
>
> > On Tue, 2021-04-27 at 16:08 +0100, Robin Murphy wrote:
> > > On 2021-04-27 11:27, Mauro Carvalho Chehab wrote:
> > > > Despite other *_get()/*_put() functions, where usage count is
> > > > incremented only if not errors, the pm_runtime_get_sync() has
> > > > a different behavior, incrementing the counter *even* on
> > > > errors.
> > > >
> > > > That's an error prone behavior, as people often forget to
> > > > decrement the usage counter.
> > > >
> > > > However, the hantro driver depends on this behavior, as it
> > > > will decrement the usage_count unconditionally at the m2m
> > > > job finish time, which makes sense.
> > > >
> > > > So, intead of using the pm_runtime_resume_and_get() that
> > > > would decrement the counter on error, keep the current
> > > > API, but add a documentation explaining the rationale for
> > > > keep using pm_runtime_get_sync().
> > > >
> > > > Signed-off-by: Mauro Carvalho Chehab
> > > > ---
> > > > drivers/staging/media/hantro/hantro_drv.c | 7 +++
> > > > 1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/drivers/staging/media/hantro/hantro_drv.c
> > > > b/drivers/staging/media/hantro/hantro_drv.c
> > > > index 595e82a82728..96f940c1c85c 100644
> > > > --- a/drivers/staging/media/hantro/hantro_drv.c
> > > > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > > > @@ -155,6 +155,13 @@ static void device_run(void *priv)
> > > > ret = clk_bulk_enable(ctx->dev->variant->num_clocks,
> > > > ctx->dev->clocks);
> > > > if (ret)
> > > > goto err_cancel_job;
> > >
> > > ..except this can also cause the same pm_runtime_put_autosuspend() call
> > > without even reaching the "matching" get below, so rather than some kind
> > > of cleverness it seems more like it's just broken :/
> > >
> >
> > Indeed, I was trying to find time to cook a quick patch, but kept
> > getting preempted.
> >
> > Feel free to submit a fix for this, otherwise, I'll try to find
> > time later this week.
>
> What about doing this instead:
>
> diff --git a/drivers/staging/media/hantro/hantro_drv.c
> b/drivers/staging/media/hantro/hantro_drv.c
> index 595e82a82728..67de6b15236d 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -56,14 +56,12 @@ dma_addr_t hantro_get_ref(struct hantro_ctx *ctx, u64 ts)
> return hantro_get_dec_buf_addr(ctx, buf);
> }
>
> -static void hantro_job_finish(struct hantro_dev *vpu,
> - struct hantro_ctx *ctx,
> - enum vb2_buffer_state result)
> +static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
> + struct hantro_ctx *ctx,
> + enum vb2_buffer_state result)
> {
> struct vb2_v4l2_buffer *src, *dst;
>
> - pm_runtime_mark_last_busy(vpu->dev);
> - pm_runtime_put_autosuspend(vpu->dev);
> clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
>
> src = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> @@ -81,6 +79,16 @@ static void hantro_job_finish(struct hantro_dev *vpu,
>result);
> }
>
> +static void hantro_job_finish(struct hantro_dev *vpu,
> + struct hantro_ctx *ctx,
> + enum vb2_buffer_state result)
> +{
> + pm_runtime_mark_last_busy(vpu->dev);
> + pm_runtime_put_autosuspend(vpu->dev);
> +
> + hantro_job_finish_no_pm(vpu, ctx, result);
> +}
> +
> void hantro_irq_done(struct hantro_dev *vpu,
>enum vb2_buffer_state result)
> {
> @@ -152,12 +160,13 @@ static void device_run(void *priv)
> src = hantro_get_src_buf(ctx);
> dst = hantro_get_dst_buf(ctx);
>
> + ret = pm_runtime_resume_and_get(ctx->dev->dev);
> + if (ret < 0)
> + goto err_cancel_job;
> +
> ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
> if (ret)
> goto err_cancel_job;
> - ret = pm_runtime_get_sync(ctx->dev->dev);
> - if (ret < 0)
> - goto err_cancel_job;
>
> v4l2_m2m_buf_copy_metadata(src, dst, true);
>
> @@ -165,7 +174,7 @@ static void device_run(void *priv)
> return;
>
> err_cancel_job:
> - hantro_job_finish(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
> + hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
> }
>
> static struct v4l2_m2m_ops vpu_m2m_ops = {
>
> Thanks,
> Mauro
Actually, the order at the finish logic should change as well.
Maybe like this:
static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
struct hantro_ctx *ctx,
enum vb2_buffer_state result)
{
struct vb2_v4l2_buffer *src, *dst;
src =
Re: [PATCH v3 79/79] media: hantro: document the usage of pm_runtime_get_sync()
Em Tue, 27 Apr 2021 12:18:32 -0300
Ezequiel Garcia escreveu:
> On Tue, 2021-04-27 at 16:08 +0100, Robin Murphy wrote:
> > On 2021-04-27 11:27, Mauro Carvalho Chehab wrote:
> > > Despite other *_get()/*_put() functions, where usage count is
> > > incremented only if not errors, the pm_runtime_get_sync() has
> > > a different behavior, incrementing the counter *even* on
> > > errors.
> > >
> > > That's an error prone behavior, as people often forget to
> > > decrement the usage counter.
> > >
> > > However, the hantro driver depends on this behavior, as it
> > > will decrement the usage_count unconditionally at the m2m
> > > job finish time, which makes sense.
> > >
> > > So, intead of using the pm_runtime_resume_and_get() that
> > > would decrement the counter on error, keep the current
> > > API, but add a documentation explaining the rationale for
> > > keep using pm_runtime_get_sync().
> > >
> > > Signed-off-by: Mauro Carvalho Chehab
> > > ---
> > > drivers/staging/media/hantro/hantro_drv.c | 7 +++
> > > 1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/staging/media/hantro/hantro_drv.c
> > > b/drivers/staging/media/hantro/hantro_drv.c
> > > index 595e82a82728..96f940c1c85c 100644
> > > --- a/drivers/staging/media/hantro/hantro_drv.c
> > > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > > @@ -155,6 +155,13 @@ static void device_run(void *priv)
> > > ret = clk_bulk_enable(ctx->dev->variant->num_clocks,
> > > ctx->dev->clocks);
> > > if (ret)
> > > goto err_cancel_job;
> >
> > ..except this can also cause the same pm_runtime_put_autosuspend() call
> > without even reaching the "matching" get below, so rather than some kind
> > of cleverness it seems more like it's just broken :/
> >
>
> Indeed, I was trying to find time to cook a quick patch, but kept
> getting preempted.
>
> Feel free to submit a fix for this, otherwise, I'll try to find
> time later this week.
What about doing this instead:
diff --git a/drivers/staging/media/hantro/hantro_drv.c
b/drivers/staging/media/hantro/hantro_drv.c
index 595e82a82728..67de6b15236d 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -56,14 +56,12 @@ dma_addr_t hantro_get_ref(struct hantro_ctx *ctx, u64 ts)
return hantro_get_dec_buf_addr(ctx, buf);
}
-static void hantro_job_finish(struct hantro_dev *vpu,
- struct hantro_ctx *ctx,
- enum vb2_buffer_state result)
+static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
+ struct hantro_ctx *ctx,
+ enum vb2_buffer_state result)
{
struct vb2_v4l2_buffer *src, *dst;
- pm_runtime_mark_last_busy(vpu->dev);
- pm_runtime_put_autosuspend(vpu->dev);
clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
src = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
@@ -81,6 +79,16 @@ static void hantro_job_finish(struct hantro_dev *vpu,
result);
}
+static void hantro_job_finish(struct hantro_dev *vpu,
+ struct hantro_ctx *ctx,
+ enum vb2_buffer_state result)
+{
+ pm_runtime_mark_last_busy(vpu->dev);
+ pm_runtime_put_autosuspend(vpu->dev);
+
+ hantro_job_finish_no_pm(vpu, ctx, result);
+}
+
void hantro_irq_done(struct hantro_dev *vpu,
enum vb2_buffer_state result)
{
@@ -152,12 +160,13 @@ static void device_run(void *priv)
src = hantro_get_src_buf(ctx);
dst = hantro_get_dst_buf(ctx);
+ ret = pm_runtime_resume_and_get(ctx->dev->dev);
+ if (ret < 0)
+ goto err_cancel_job;
+
ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
if (ret)
goto err_cancel_job;
- ret = pm_runtime_get_sync(ctx->dev->dev);
- if (ret < 0)
- goto err_cancel_job;
v4l2_m2m_buf_copy_metadata(src, dst, true);
@@ -165,7 +174,7 @@ static void device_run(void *priv)
return;
err_cancel_job:
- hantro_job_finish(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
+ hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
}
static struct v4l2_m2m_ops vpu_m2m_ops = {
Thanks,
Mauro
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8192e: fix array of flexible structures
On Wed, Apr 28, 2021 at 12:28:44AM +0530, Jitendra wrote:
> On Tue, Apr 27, 2021 at 08:10:20PM +0200, Greg KH wrote:
> > On Tue, Apr 27, 2021 at 11:19:45PM +0530, Jitendra Khasdev wrote:
> > > This patch fixes sparse warning "array of flexible structures"
> > > for rtllib.h.
> > >
> > > eg. drivers/staging/rtl8192e/rtllib.h:832:48: warning: array of
> > > flexible structures
> > >
> > > Signed-off-by: Jitendra Khasdev
> > > ---
> > > drivers/staging/rtl8192e/rtllib.h | 10 +-
> > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/staging/rtl8192e/rtllib.h
> > > b/drivers/staging/rtl8192e/rtllib.h
> > > index 4cabaf2..c7cb318 100644
> > > --- a/drivers/staging/rtl8192e/rtllib.h
> > > +++ b/drivers/staging/rtl8192e/rtllib.h
> > > @@ -802,7 +802,7 @@ struct rtllib_authentication {
> > > __le16 transaction;
> > > __le16 status;
> > > /*challenge*/
> > > - struct rtllib_info_element info_element[];
> > > + struct rtllib_info_element *info_element;
> >
> > You just changed the definition of this structure, and the other
> > structures here. Are you sure this is working properly?
> >
>
> I have compiled the driver and install it on my vm, but I don't this specific
> hardware, so couldn't test it.
>
> I fixed in context of sparse.
Please verify that this change is correct by looking at how the
structures are being created (i.e. is this being treated as a flexible
array or a pointer?)
I think we have been through this before and that sparse is not right,
but I can't remember...
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
