| Message ID | 83ec24acb15f17e2ce589575c2f5eb7bdd1daf28.1620207353.git.mchehab+huawei@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix some PM runtime issues at the media subsystem | expand |
Hi Mauro, Thanks for the patch. On Wed, May 05, 2021 at 11:42:15AM +0200, Mauro Carvalho Chehab wrote: > The pm_runtime_get_sync() internally increments the > dev->power.usage_count without decrementing it, even on errors. > > There is a bug at ccs_pm_get_init(): when this function returns > an error, the stream is not started, and RPM usage_count > should not be incremented. However, if the calls to > v4l2_ctrl_handler_setup() return errors, it will be kept > incremented. > > At ccs_suspend() the best is to 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 automatically, > in the case of errors. > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> Could you add Fixes: line and Cc: stable? The patch that breaks this is 96e3a6b92f23a . It would be better to fix the bug first so the patch to the stable trees doesn't need special handling. > --- > drivers/media/i2c/ccs/ccs-core.c | 39 ++++++++++++++++++++------------ > 1 file changed, 24 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c > index b05f409014b2..04c3ab9e37b4 100644 > --- a/drivers/media/i2c/ccs/ccs-core.c > +++ b/drivers/media/i2c/ccs/ccs-core.c > @@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor) > struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd); > int rval; > > + /* > + * It can't use pm_runtime_resume_and_get() here, as the driver > + * relies at the returned value to detect if the device was already > + * active or not. > + */ > rval = pm_runtime_get_sync(&client->dev); > - if (rval < 0) { > - pm_runtime_put_noidle(&client->dev); > + if (rval < 0) > + goto error; > > - return rval; > - } else if (!rval) { > - rval = v4l2_ctrl_handler_setup(&sensor->pixel_array-> > - ctrl_handler); > - if (rval) > - return rval; > + /* Device was already active, so don't set controls */ > + if (rval == 1) > + return 0; > > - return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); > - } > + /* Restore V4L2 controls to the suspended device */ > + rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->ctrl_handler); > + if (rval) > + goto error; > > + rval = v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); > + if (rval) > + goto error; > + > + /* Keep PM runtime usage_count incremented on success */ > return 0; > +error: > + pm_runtime_put_noidle(&client->dev); This needs to be pm_runtime_put() as the device has been successfully. > + return rval; > } > > static int ccs_set_stream(struct v4l2_subdev *subdev, int enable) > @@ -3089,12 +3101,9 @@ static int __maybe_unused ccs_suspend(struct device *dev) > bool streaming = sensor->streaming; > int rval; > > - rval = pm_runtime_get_sync(dev); > - if (rval < 0) { > - pm_runtime_put_noidle(dev); > - > + rval = pm_runtime_resume_and_get(dev); > + if (rval < 0) > return rval; > - } > > if (sensor->streaming) > ccs_stop_streaming(sensor);
Em Wed, 5 May 2021 13:34:09 +0300 Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > Hi Mauro, > > Thanks for the patch. > > On Wed, May 05, 2021 at 11:42:15AM +0200, Mauro Carvalho Chehab wrote: > > The pm_runtime_get_sync() internally increments the > > dev->power.usage_count without decrementing it, even on errors. > > > > There is a bug at ccs_pm_get_init(): when this function returns > > an error, the stream is not started, and RPM usage_count > > should not be incremented. However, if the calls to > > v4l2_ctrl_handler_setup() return errors, it will be kept > > incremented. > > > > At ccs_suspend() the best is to 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 automatically, > > in the case of errors. > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > Could you add Fixes: line and Cc: stable? Sure. See the fixes one enclosed. > The patch that breaks this is 96e3a6b92f23a . > > It would be better to fix the bug first so the patch to the stable trees > doesn't need special handling. > > > --- > > drivers/media/i2c/ccs/ccs-core.c | 39 ++++++++++++++++++++------------ > > 1 file changed, 24 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c > > index b05f409014b2..04c3ab9e37b4 100644 > > --- a/drivers/media/i2c/ccs/ccs-core.c > > +++ b/drivers/media/i2c/ccs/ccs-core.c > > @@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor) > > struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd); > > int rval; > > > > + /* > > + * It can't use pm_runtime_resume_and_get() here, as the driver > > + * relies at the returned value to detect if the device was already > > + * active or not. > > + */ > > rval = pm_runtime_get_sync(&client->dev); > > - if (rval < 0) { > > - pm_runtime_put_noidle(&client->dev); > > + if (rval < 0) > > + goto error; > > > > - return rval; > > - } else if (!rval) { > > - rval = v4l2_ctrl_handler_setup(&sensor->pixel_array-> > > - ctrl_handler); > > - if (rval) > > - return rval; > > + /* Device was already active, so don't set controls */ > > + if (rval == 1) > > + return 0; > > > > - return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); > > - } > > + /* Restore V4L2 controls to the suspended device */ > > + rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->ctrl_handler); > > + if (rval) > > + goto error; > > > > + rval = v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); > > + if (rval) > > + goto error; > > + > > + /* Keep PM runtime usage_count incremented on success */ > > return 0; > > +error: > > + pm_runtime_put_noidle(&client->dev); > > This needs to be pm_runtime_put() as the device has been successfully. Ok. > > > + return rval; > > } > > > > static int ccs_set_stream(struct v4l2_subdev *subdev, int enable) > > @@ -3089,12 +3101,9 @@ static int __maybe_unused ccs_suspend(struct device *dev) > > bool streaming = sensor->streaming; > > int rval; > > > > - rval = pm_runtime_get_sync(dev); > > - if (rval < 0) { > > - pm_runtime_put_noidle(dev); > > - > > + rval = pm_runtime_resume_and_get(dev); > > + if (rval < 0) > > return rval; > > - } > > > > if (sensor->streaming) > > ccs_stop_streaming(sensor); > Thanks, Mauro --- [PATCH] media: i2c: ccs-core: 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. There is a bug at ccs_pm_get_init(): when this function returns an error, the stream is not started, and RPM usage_count should not be incremented. However, if the calls to v4l2_ctrl_handler_setup() return errors, it will be kept incremented. At ccs_suspend() the best is to 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 automatically, in the case of errors. Fixes: 96e3a6b92f23 ("media: smiapp: Avoid maintaining power state information") Cc: stable@vger.kernel.org Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c index b05f409014b2..5ea471fefa3a 100644 --- a/drivers/media/i2c/ccs/ccs-core.c +++ b/drivers/media/i2c/ccs/ccs-core.c @@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor) struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd); int rval; + /* + * It can't use pm_runtime_resume_and_get() here, as the driver + * relies at the returned value to detect if the device was already + * active or not. + */ rval = pm_runtime_get_sync(&client->dev); - if (rval < 0) { - pm_runtime_put_noidle(&client->dev); + if (rval < 0) + goto error; - return rval; - } else if (!rval) { - rval = v4l2_ctrl_handler_setup(&sensor->pixel_array-> - ctrl_handler); - if (rval) - return rval; + /* Device was already active, so don't set controls */ + if (rval == 1) + return 0; - return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); - } + /* Restore V4L2 controls to the suspended device */ + rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->ctrl_handler); + if (rval) + goto error; + rval = v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); + if (rval) + goto error; + + /* Keep PM runtime usage_count incremented on success */ return 0; +error: + pm_runtime_put(&client->dev); + return rval; } static int ccs_set_stream(struct v4l2_subdev *subdev, int enable)
Em Wed, 5 May 2021 12:57:00 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu: > Em Wed, 5 May 2021 13:34:09 +0300 > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > > > Hi Mauro, > > > > Thanks for the patch. > > > > On Wed, May 05, 2021 at 11:42:15AM +0200, Mauro Carvalho Chehab wrote: > > > The pm_runtime_get_sync() internally increments the > > > dev->power.usage_count without decrementing it, even on errors. > > > > > > There is a bug at ccs_pm_get_init(): when this function returns > > > an error, the stream is not started, and RPM usage_count > > > should not be incremented. However, if the calls to > > > v4l2_ctrl_handler_setup() return errors, it will be kept > > > incremented. > > > > > > At ccs_suspend() the best is to 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 automatically, > > > in the case of errors. > > > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > > > Could you add Fixes: line and Cc: stable? > > Sure. See the fixes one enclosed. > > > The patch that breaks this is 96e3a6b92f23a . > > > > It would be better to fix the bug first so the patch to the stable trees > > doesn't need special handling. > > > > > --- > > > drivers/media/i2c/ccs/ccs-core.c | 39 ++++++++++++++++++++------------ > > > 1 file changed, 24 insertions(+), 15 deletions(-) > > > > > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c > > > index b05f409014b2..04c3ab9e37b4 100644 > > > --- a/drivers/media/i2c/ccs/ccs-core.c > > > +++ b/drivers/media/i2c/ccs/ccs-core.c > > > @@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor) > > > struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd); > > > int rval; > > > > > > + /* > > > + * It can't use pm_runtime_resume_and_get() here, as the driver > > > + * relies at the returned value to detect if the device was already > > > + * active or not. > > > + */ > > > rval = pm_runtime_get_sync(&client->dev); > > > - if (rval < 0) { > > > - pm_runtime_put_noidle(&client->dev); > > > + if (rval < 0) > > > + goto error; > > > > > > - return rval; > > > - } else if (!rval) { > > > - rval = v4l2_ctrl_handler_setup(&sensor->pixel_array-> > > > - ctrl_handler); > > > - if (rval) > > > - return rval; > > > + /* Device was already active, so don't set controls */ > > > + if (rval == 1) > > > + return 0; > > > > > > - return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); > > > - } > > > + /* Restore V4L2 controls to the suspended device */ > > > + rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->ctrl_handler); > > > + if (rval) > > > + goto error; > > > > > > + rval = v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); > > > + if (rval) > > > + goto error; > > > + > > > + /* Keep PM runtime usage_count incremented on success */ > > > return 0; > > > +error: > > > + pm_runtime_put_noidle(&client->dev); > > > > This needs to be pm_runtime_put() as the device has been successfully. > > Ok. > > > > > > + return rval; > > > } > > > > > > static int ccs_set_stream(struct v4l2_subdev *subdev, int enable) > > > @@ -3089,12 +3101,9 @@ static int __maybe_unused ccs_suspend(struct device *dev) > > > bool streaming = sensor->streaming; > > > int rval; > > > > > > - rval = pm_runtime_get_sync(dev); > > > - if (rval < 0) { > > > - pm_runtime_put_noidle(dev); > > > - > > > + rval = pm_runtime_resume_and_get(dev); > > > + if (rval < 0) > > > return rval; > > > - } > > > > > > if (sensor->streaming) > > > ccs_stop_streaming(sensor); > > > > Thanks, > Mauro > > --- > > [PATCH] media: i2c: ccs-core: 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. > > There is a bug at ccs_pm_get_init(): when this function returns > an error, the stream is not started, and RPM usage_count > should not be incremented. However, if the calls to > v4l2_ctrl_handler_setup() return errors, it will be kept > incremented. > > At ccs_suspend() the best is to 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 automatically, > in the case of errors. > > Fixes: 96e3a6b92f23 ("media: smiapp: Avoid maintaining power state information") > Cc: stable@vger.kernel.org > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c > index b05f409014b2..5ea471fefa3a 100644 > --- a/drivers/media/i2c/ccs/ccs-core.c > +++ b/drivers/media/i2c/ccs/ccs-core.c > @@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor) > struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd); > int rval; > > + /* > + * It can't use pm_runtime_resume_and_get() here, as the driver > + * relies at the returned value to detect if the device was already > + * active or not. > + */ > rval = pm_runtime_get_sync(&client->dev); > - if (rval < 0) { > - pm_runtime_put_noidle(&client->dev); > + if (rval < 0) > + goto error; > > - return rval; > - } else if (!rval) { > - rval = v4l2_ctrl_handler_setup(&sensor->pixel_array-> > - ctrl_handler); > - if (rval) > - return rval; > + /* Device was already active, so don't set controls */ > + if (rval == 1) > + return 0; > > - return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); > - } > + /* Restore V4L2 controls to the suspended device */ In time: I'll fold this at the patch: diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c index 5ea471fefa3a..4a848ac2d2cd 100644 --- a/drivers/media/i2c/ccs/ccs-core.c +++ b/drivers/media/i2c/ccs/ccs-core.c @@ -1896 +1896 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor) - /* Restore V4L2 controls to the suspended device */ + /* Restore V4L2 controls to the previously suspended device */ Regards, Mauro
Hi Mauro, On Wed, May 05, 2021 at 12:57:00PM +0200, Mauro Carvalho Chehab wrote: > Em Wed, 5 May 2021 13:34:09 +0300 > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > > > Hi Mauro, > > > > Thanks for the patch. > > > > On Wed, May 05, 2021 at 11:42:15AM +0200, Mauro Carvalho Chehab wrote: > > > The pm_runtime_get_sync() internally increments the > > > dev->power.usage_count without decrementing it, even on errors. > > > > > > There is a bug at ccs_pm_get_init(): when this function returns > > > an error, the stream is not started, and RPM usage_count > > > should not be incremented. However, if the calls to > > > v4l2_ctrl_handler_setup() return errors, it will be kept > > > incremented. > > > > > > At ccs_suspend() the best is to 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 automatically, > > > in the case of errors. > > > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > > > Could you add Fixes: line and Cc: stable? > > Sure. See the fixes one enclosed. > > > The patch that breaks this is 96e3a6b92f23a . > > > > It would be better to fix the bug first so the patch to the stable trees > > doesn't need special handling. Please ignore this comment. > > > > > --- > > > drivers/media/i2c/ccs/ccs-core.c | 39 ++++++++++++++++++++------------ > > > 1 file changed, 24 insertions(+), 15 deletions(-) > > > > > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c > > > index b05f409014b2..04c3ab9e37b4 100644 > > > --- a/drivers/media/i2c/ccs/ccs-core.c > > > +++ b/drivers/media/i2c/ccs/ccs-core.c > > > @@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor) > > > struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd); > > > int rval; > > > > > > + /* > > > + * It can't use pm_runtime_resume_and_get() here, as the driver > > > + * relies at the returned value to detect if the device was already > > > + * active or not. > > > + */ > > > rval = pm_runtime_get_sync(&client->dev); > > > - if (rval < 0) { > > > - pm_runtime_put_noidle(&client->dev); > > > + if (rval < 0) > > > + goto error; > > > > > > - return rval; > > > - } else if (!rval) { > > > - rval = v4l2_ctrl_handler_setup(&sensor->pixel_array-> > > > - ctrl_handler); > > > - if (rval) > > > - return rval; > > > + /* Device was already active, so don't set controls */ > > > + if (rval == 1) > > > + return 0; > > > > > > - return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); > > > - } > > > + /* Restore V4L2 controls to the suspended device */ > > > + rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->ctrl_handler); > > > + if (rval) > > > + goto error; > > > > > > + rval = v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); > > > + if (rval) > > > + goto error; > > > + > > > + /* Keep PM runtime usage_count incremented on success */ > > > return 0; > > > +error: > > > + pm_runtime_put_noidle(&client->dev); > > > > This needs to be pm_runtime_put() as the device has been successfully. > > Ok. > > > > > > + return rval; > > > } > > > > > > static int ccs_set_stream(struct v4l2_subdev *subdev, int enable) > > > @@ -3089,12 +3101,9 @@ static int __maybe_unused ccs_suspend(struct device *dev) > > > bool streaming = sensor->streaming; > > > int rval; > > > > > > - rval = pm_runtime_get_sync(dev); > > > - if (rval < 0) { > > > - pm_runtime_put_noidle(dev); > > > - > > > + rval = pm_runtime_resume_and_get(dev); > > > + if (rval < 0) > > > return rval; > > > - } > > > > > > if (sensor->streaming) > > > ccs_stop_streaming(sensor); > > > > Thanks, > Mauro > > --- > > [PATCH] media: i2c: ccs-core: 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. > > There is a bug at ccs_pm_get_init(): when this function returns > an error, the stream is not started, and RPM usage_count > should not be incremented. However, if the calls to > v4l2_ctrl_handler_setup() return errors, it will be kept > incremented. > > At ccs_suspend() the best is to 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 automatically, > in the case of errors. > > Fixes: 96e3a6b92f23 ("media: smiapp: Avoid maintaining power state information") > Cc: stable@vger.kernel.org > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c > index b05f409014b2..5ea471fefa3a 100644 > --- a/drivers/media/i2c/ccs/ccs-core.c > +++ b/drivers/media/i2c/ccs/ccs-core.c > @@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor) > struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd); > int rval; > > + /* > + * It can't use pm_runtime_resume_and_get() here, as the driver > + * relies at the returned value to detect if the device was already > + * active or not. > + */ > rval = pm_runtime_get_sync(&client->dev); > - if (rval < 0) { > - pm_runtime_put_noidle(&client->dev); > + if (rval < 0) > + goto error; > > - return rval; > - } else if (!rval) { > - rval = v4l2_ctrl_handler_setup(&sensor->pixel_array-> > - ctrl_handler); > - if (rval) > - return rval; > + /* Device was already active, so don't set controls */ > + if (rval == 1) > + return 0; > > - return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); > - } > + /* Restore V4L2 controls to the suspended device */ > + rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->ctrl_handler); > + if (rval) > + goto error; > > + rval = v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); > + if (rval) > + goto error; > + > + /* Keep PM runtime usage_count incremented on success */ > return 0; > +error: > + pm_runtime_put(&client->dev); > + return rval; > } > > static int ccs_set_stream(struct v4l2_subdev *subdev, int enable) > > > >
On Wed, 5 May 2021 12:58:57 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > Em Wed, 5 May 2021 12:57:00 +0200 > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu: > > > Em Wed, 5 May 2021 13:34:09 +0300 > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > > > > > Hi Mauro, > > > > > > Thanks for the patch. > > > > > > On Wed, May 05, 2021 at 11:42:15AM +0200, Mauro Carvalho Chehab wrote: > > > > The pm_runtime_get_sync() internally increments the > > > > dev->power.usage_count without decrementing it, even on errors. > > > > > > > > There is a bug at ccs_pm_get_init(): when this function returns > > > > an error, the stream is not started, and RPM usage_count > > > > should not be incremented. However, if the calls to > > > > v4l2_ctrl_handler_setup() return errors, it will be kept > > > > incremented. > > > > > > > > At ccs_suspend() the best is to 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 automatically, > > > > in the case of errors. > > > > > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > > > > > Could you add Fixes: line and Cc: stable? > > > > Sure. See the fixes one enclosed. > > > > > The patch that breaks this is 96e3a6b92f23a . > > > > > > It would be better to fix the bug first so the patch to the stable trees > > > doesn't need special handling. > > > > > > > --- > > > > drivers/media/i2c/ccs/ccs-core.c | 39 ++++++++++++++++++++------------ > > > > 1 file changed, 24 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c > > > > index b05f409014b2..04c3ab9e37b4 100644 > > > > --- a/drivers/media/i2c/ccs/ccs-core.c > > > > +++ b/drivers/media/i2c/ccs/ccs-core.c > > > > @@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor) > > > > struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd); > > > > int rval; > > > > > > > > + /* > > > > + * It can't use pm_runtime_resume_and_get() here, as the driver > > > > + * relies at the returned value to detect if the device was already > > > > + * active or not. > > > > + */ > > > > rval = pm_runtime_get_sync(&client->dev); > > > > - if (rval < 0) { > > > > - pm_runtime_put_noidle(&client->dev); > > > > + if (rval < 0) > > > > + goto error; > > > > > > > > - return rval; > > > > - } else if (!rval) { > > > > - rval = v4l2_ctrl_handler_setup(&sensor->pixel_array-> > > > > - ctrl_handler); > > > > - if (rval) > > > > - return rval; > > > > + /* Device was already active, so don't set controls */ > > > > + if (rval == 1) > > > > + return 0; > > > > > > > > - return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); > > > > - } > > > > + /* Restore V4L2 controls to the suspended device */ > > > > + rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->ctrl_handler); > > > > + if (rval) > > > > + goto error; > > > > > > > > + rval = v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); > > > > + if (rval) > > > > + goto error; > > > > + > > > > + /* Keep PM runtime usage_count incremented on success */ > > > > return 0; > > > > +error: > > > > + pm_runtime_put_noidle(&client->dev); > > > > > > This needs to be pm_runtime_put() as the device has been successfully. > > > > Ok. > > > > > > > > > + return rval; > > > > } > > > > > > > > static int ccs_set_stream(struct v4l2_subdev *subdev, int enable) > > > > @@ -3089,12 +3101,9 @@ static int __maybe_unused ccs_suspend(struct device *dev) > > > > bool streaming = sensor->streaming; > > > > int rval; > > > > > > > > - rval = pm_runtime_get_sync(dev); > > > > - if (rval < 0) { > > > > - pm_runtime_put_noidle(dev); > > > > - > > > > + rval = pm_runtime_resume_and_get(dev); > > > > + if (rval < 0) > > > > return rval; > > > > - } > > > > > > > > if (sensor->streaming) > > > > ccs_stop_streaming(sensor); > > > > > > > Thanks, > > Mauro > > > > --- > > > > [PATCH] media: i2c: ccs-core: 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. > > > > There is a bug at ccs_pm_get_init(): when this function returns > > an error, the stream is not started, and RPM usage_count > > should not be incremented. However, if the calls to > > v4l2_ctrl_handler_setup() return errors, it will be kept > > incremented. > > > > At ccs_suspend() the best is to 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 automatically, > > in the case of errors. > > > > Fixes: 96e3a6b92f23 ("media: smiapp: Avoid maintaining power state information") > > Cc: stable@vger.kernel.org > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c > > index b05f409014b2..5ea471fefa3a 100644 > > --- a/drivers/media/i2c/ccs/ccs-core.c > > +++ b/drivers/media/i2c/ccs/ccs-core.c > > @@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor) > > struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd); > > int rval; > > > > + /* > > + * It can't use pm_runtime_resume_and_get() here, as the driver > > + * relies at the returned value to detect if the device was already > > + * active or not. > > + */ > > rval = pm_runtime_get_sync(&client->dev); > > - if (rval < 0) { > > - pm_runtime_put_noidle(&client->dev); > > + if (rval < 0) > > + goto error; > > > > - return rval; > > - } else if (!rval) { > > - rval = v4l2_ctrl_handler_setup(&sensor->pixel_array-> > > - ctrl_handler); > > - if (rval) > > - return rval; > > + /* Device was already active, so don't set controls */ > > + if (rval == 1) > > + return 0; > > > > - return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); > > - } > > + /* Restore V4L2 controls to the suspended device */ > > In time: I'll fold this at the patch: > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c > index 5ea471fefa3a..4a848ac2d2cd 100644 > --- a/drivers/media/i2c/ccs/ccs-core.c > +++ b/drivers/media/i2c/ccs/ccs-core.c > @@ -1896 +1896 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor) > - /* Restore V4L2 controls to the suspended device */ > + /* Restore V4L2 controls to the previously suspended device */ > > Regards, > Mauro
Hi Jonathan, Em Wed, 5 May 2021 13:35:48 +0100 Jonathan Cameron <Jonathan.Cameron@Huawei.com> escreveu: > > > [PATCH] media: i2c: ccs-core: 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. > > > > > > There is a bug at ccs_pm_get_init(): when this function returns > > > an error, the stream is not started, and RPM usage_count > > > should not be incremented. However, if the calls to > > > v4l2_ctrl_handler_setup() return errors, it will be kept > > > incremented. > > > > > > At ccs_suspend() the best is to 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 automatically, > > > in the case of errors. > > > > > > Fixes: 96e3a6b92f23 ("media: smiapp: Avoid maintaining power state information") > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Per Sakari's request (for practical reasons on backporting and c/c stable), this was split into two separate patches, one fixing the issues, and a separate trivial one with just the pm_runtime_resume_and_get(). I'm adding your RB on both. Thanks, Mauro
On Wed, 5 May 2021 16:06:45 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > Hi Jonathan, > > Em Wed, 5 May 2021 13:35:48 +0100 > Jonathan Cameron <Jonathan.Cameron@Huawei.com> escreveu: > > > > > [PATCH] media: i2c: ccs-core: 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. > > > > > > > > There is a bug at ccs_pm_get_init(): when this function returns > > > > an error, the stream is not started, and RPM usage_count > > > > should not be incremented. However, if the calls to > > > > v4l2_ctrl_handler_setup() return errors, it will be kept > > > > incremented. > > > > > > > > At ccs_suspend() the best is to 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 automatically, > > > > in the case of errors. > > > > > > > > Fixes: 96e3a6b92f23 ("media: smiapp: Avoid maintaining power state information") > > > > Cc: stable@vger.kernel.org > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Per Sakari's request (for practical reasons on backporting and > c/c stable), this was split into two separate patches, one > fixing the issues, and a separate trivial one with just the > pm_runtime_resume_and_get(). I'm adding your RB on both. Makes sense. Jonathan > > Thanks, > Mauro
diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c index b05f409014b2..04c3ab9e37b4 100644 --- a/drivers/media/i2c/ccs/ccs-core.c +++ b/drivers/media/i2c/ccs/ccs-core.c @@ -1880,21 +1880,33 @@ static int ccs_pm_get_init(struct ccs_sensor *sensor) struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd); int rval; + /* + * It can't use pm_runtime_resume_and_get() here, as the driver + * relies at the returned value to detect if the device was already + * active or not. + */ rval = pm_runtime_get_sync(&client->dev); - if (rval < 0) { - pm_runtime_put_noidle(&client->dev); + if (rval < 0) + goto error; - return rval; - } else if (!rval) { - rval = v4l2_ctrl_handler_setup(&sensor->pixel_array-> - ctrl_handler); - if (rval) - return rval; + /* Device was already active, so don't set controls */ + if (rval == 1) + return 0; - return v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); - } + /* Restore V4L2 controls to the suspended device */ + rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->ctrl_handler); + if (rval) + goto error; + rval = v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); + if (rval) + goto error; + + /* Keep PM runtime usage_count incremented on success */ return 0; +error: + pm_runtime_put_noidle(&client->dev); + return rval; } static int ccs_set_stream(struct v4l2_subdev *subdev, int enable) @@ -3089,12 +3101,9 @@ static int __maybe_unused ccs_suspend(struct device *dev) bool streaming = sensor->streaming; int rval; - rval = pm_runtime_get_sync(dev); - if (rval < 0) { - pm_runtime_put_noidle(dev); - + rval = pm_runtime_resume_and_get(dev); + if (rval < 0) return rval; - } if (sensor->streaming) ccs_stop_streaming(sensor);
The pm_runtime_get_sync() internally increments the dev->power.usage_count without decrementing it, even on errors. There is a bug at ccs_pm_get_init(): when this function returns an error, the stream is not started, and RPM usage_count should not be incremented. However, if the calls to v4l2_ctrl_handler_setup() return errors, it will be kept incremented. At ccs_suspend() the best is to 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 automatically, in the case of errors. Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> --- drivers/media/i2c/ccs/ccs-core.c | 39 ++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 15 deletions(-)