On 5/5/21 3:08 PM, Jonathan Cameron wrote: > On Wed, 5 May 2021 11:41:58 +0200 > Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote: > >> There are several issues in the way the atmel driver handles >> pm_runtime_get_sync(): >> >> - it doesn't check return codes; >> - it doesn't properly decrement the usage_count on all places; >> - it starts streaming even if pm_runtime_get_sync() fails. >> - while it tries to get pm_runtime at the clock enable logic, >> it doesn't check if the operation was suceeded. >> >> Replace all occurrences of it to use the new kAPI: >> pm_runtime_resume_and_get(), which ensures that, if the >> return code is not negative, the usage_count was incremented. >> >> With that, add additional checks when this is called, in order >> to ensure that errors will be properly addressed. >> >> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> > > Hi Mauro, I don't know media enough to know what is the right answer > but in some of this series, a failure in > pm_runtime_resume_and_get() leads to a bunch of buffer cleanup > (patch 22 being an example) and in others return happens without doing > that cleanup. > > It might be both are safe, or I'm missing something else, but I'm > certainly not confident enough to give any tags on this one as a result > of that mismatch. > >> --- >> drivers/media/platform/atmel/atmel-isc-base.c | 30 ++++++++++++++----- >> drivers/media/platform/atmel/atmel-isi.c | 19 +++++++++--- >> 2 files changed, 38 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c >> index fe3ec8d0eaee..ce8e1351fa53 100644 >> --- a/drivers/media/platform/atmel/atmel-isc-base.c >> +++ b/drivers/media/platform/atmel/atmel-isc-base.c >> @@ -294,9 +294,13 @@ static int isc_wait_clk_stable(struct clk_hw *hw) >> static int isc_clk_prepare(struct clk_hw *hw) >> { >> struct isc_clk *isc_clk = to_isc_clk(hw); >> + int ret; >> >> - if (isc_clk->id == ISC_ISPCK) >> - pm_runtime_get_sync(isc_clk->dev); >> + if (isc_clk->id == ISC_ISPCK) { >> + ret = pm_runtime_resume_and_get(isc_clk->dev); >> + if (ret < 0) >> + return ret; >> + } Hi Mauro, With this patch, the ISC is broken on latest media tree. It looks like pm_runtime_resume_and_get for the ISC_ISPCK clock returns -ENOACCESS and thus, the probe of the driver fails: atmel-sama5d2-isc f0008000.isc: failed to enable ispck: -13 atmel-sama5d2-isc: probe of f0008000.isc failed with error -13 Could you point out how I could fix this ? Maybe the isc_clk->dev is not properly handled/initialized in some other part of the code ? Thanks ! Eugen >> >> return isc_wait_clk_stable(hw); >> } >> @@ -353,9 +357,13 @@ static int isc_clk_is_enabled(struct clk_hw *hw) >> { >> struct isc_clk *isc_clk = to_isc_clk(hw); >> u32 status; >> + int ret; >> >> - if (isc_clk->id == ISC_ISPCK) >> - pm_runtime_get_sync(isc_clk->dev); >> + if (isc_clk->id == ISC_ISPCK) { >> + ret = pm_runtime_resume_and_get(isc_clk->dev); >> + if (ret < 0) >> + return 0; >> + } >> >> regmap_read(isc_clk->regmap, ISC_CLKSR, &status); >> >> @@ -807,7 +815,12 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count) >> goto err_start_stream; >> } >> >> - pm_runtime_get_sync(isc->dev); >> + ret = pm_runtime_resume_and_get(isc->dev); >> + if (ret < 0) { >> + v4l2_err(&isc->v4l2_dev, "RPM resume failed in subdev %d\n", >> + ret); >> + goto err_pm_get; >> + } >> >> ret = isc_configure(isc); >> if (unlikely(ret)) >> @@ -838,7 +851,7 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count) >> >> err_configure: >> pm_runtime_put_sync(isc->dev); >> - >> +err_pm_get: >> v4l2_subdev_call(isc->current_subdev->sd, video, s_stream, 0); >> >> err_start_stream: >> @@ -1809,6 +1822,7 @@ static void isc_awb_work(struct work_struct *w) >> u32 baysel; >> unsigned long flags; >> u32 min, max; >> + int ret; >> >> /* streaming is not active anymore */ >> if (isc->stop) >> @@ -1831,7 +1845,9 @@ static void isc_awb_work(struct work_struct *w) >> ctrls->hist_id = hist_id; >> baysel = isc->config.sd_format->cfa_baycfg << ISC_HIS_CFG_BAYSEL_SHIFT; >> >> - pm_runtime_get_sync(isc->dev); >> + ret = pm_runtime_resume_and_get(isc->dev); >> + if (ret < 0) >> + return; >> >> /* >> * only update if we have all the required histograms and controls >> diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c >> index e392b3efe363..5b1dd358f2e6 100644 >> --- a/drivers/media/platform/atmel/atmel-isi.c >> +++ b/drivers/media/platform/atmel/atmel-isi.c >> @@ -422,7 +422,9 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count) >> struct frame_buffer *buf, *node; >> int ret; >> >> - pm_runtime_get_sync(isi->dev); >> + ret = pm_runtime_resume_and_get(isi->dev); >> + if (ret < 0) >> + return ret; > This is the case I'm referring to above. > >> >> /* Enable stream on the sub device */ >> ret = v4l2_subdev_call(isi->entity.subdev, video, s_stream, 1); >> @@ -782,9 +784,10 @@ static int isi_enum_frameintervals(struct file *file, void *fh, >> return 0; >> } >> >> -static void isi_camera_set_bus_param(struct atmel_isi *isi) >> +static int isi_camera_set_bus_param(struct atmel_isi *isi) >> { >> u32 cfg1 = 0; >> + int ret; >> >> /* set bus param for ISI */ >> if (isi->pdata.hsync_act_low) >> @@ -801,12 +804,16 @@ static void isi_camera_set_bus_param(struct atmel_isi *isi) >> cfg1 |= ISI_CFG1_THMASK_BEATS_16; >> >> /* Enable PM and peripheral clock before operate isi registers */ >> - pm_runtime_get_sync(isi->dev); >> + ret = pm_runtime_resume_and_get(isi->dev); >> + if (ret < 0) >> + return ret; >> >> isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS); >> isi_writel(isi, ISI_CFG1, cfg1); >> >> pm_runtime_put(isi->dev); >> + >> + return 0; >> } >> >> /* -----------------------------------------------------------------------*/ >> @@ -1085,7 +1092,11 @@ static int isi_graph_notify_complete(struct v4l2_async_notifier *notifier) >> dev_err(isi->dev, "No supported mediabus format found\n"); >> return ret; >> } >> - isi_camera_set_bus_param(isi); >> + ret = isi_camera_set_bus_param(isi); >> + if (ret) { >> + dev_err(isi->dev, "Can't wake up device\n"); >> + return ret; >> + } >> >> ret = isi_set_default_fmt(isi); >> if (ret) { >