| Message ID | 20220301071859.24285-1-linmq006@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] media: atmel: atmel-isc: Fix PM disable depth imbalance in atmel_isc_probe | expand |
On 3/1/22 9:18 AM, Miaoqian Lin wrote: > The pm_runtime_enable will increase power disable depth. > If the probe fails, we should use pm_runtime_disable() to balance > pm_runtime_enable(). > > Fixes: 0a0e265 ("media: atmel: atmel-isc: split driver into driver base and isc") > Signed-off-by: Miaoqian Lin <linmq006@gmail.com> > --- > changes in v2: > - remove unused label. > --- > drivers/media/platform/atmel/atmel-sama5d2-isc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c > index 1b2063cce0f7..7f1ebbb25437 100644 > --- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c > +++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c > @@ -559,6 +559,8 @@ static int atmel_isc_probe(struct platform_device *pdev) > cleanup_subdev: > isc_subdev_cleanup(isc); > > + pm_runtime_disable(dev); > + Hello Miaoqian Lin , Could you please perform the same change (or similar) in atmel-sama7g5-isc , as the sama7g5 ISC will perform the same and has the same bug. Thank you for the patch, Eugen > unregister_v4l2_device: > v4l2_device_unregister(&isc->v4l2_dev); > > -- > 2.17.1 >
On Tue, Mar 01, 2022 at 01:51:02PM +0000, Eugen.Hristev@microchip.com wrote: > > --- > > changes in v2: > > - remove unused label. > > --- > > drivers/media/platform/atmel/atmel-sama5d2-isc.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c > > index 1b2063cce0f7..7f1ebbb25437 100644 > > --- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c > > +++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c > > @@ -559,6 +559,8 @@ static int atmel_isc_probe(struct platform_device *pdev) > > cleanup_subdev: > > isc_subdev_cleanup(isc); > > > > + pm_runtime_disable(dev); > > + > > Hello Miaoqian Lin , > > Could you please perform the same change (or similar) in > atmel-sama7g5-isc , as the sama7g5 ISC will perform the same and has the > same bug. > Hi, Eugen Hristev: I think you are referring to microchip_xisc_probe() function in atmel-sama7g5-isc, and I have look into it. After it calls pm_runtime_enable(), it only have a regular path whichs return 0 and indicates the probe is successful. It doesn't have error handling path. regmap_read() function returns a negative errno in error cases, but it is used to get Microchip XISC version. I am not sure if failure means the probe fails. > > unregister_v4l2_device: > > v4l2_device_unregister(&isc->v4l2_dev); > > > > -- > > 2.17.1 > > >
On 3/2/22 7:44 AM, Miaoqian Lin wrote: > On Tue, Mar 01, 2022 at 01:51:02PM +0000, Eugen.Hristev@microchip.com wrote: >>> --- >>> changes in v2: >>> - remove unused label. >>> --- >>> drivers/media/platform/atmel/atmel-sama5d2-isc.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c >>> index 1b2063cce0f7..7f1ebbb25437 100644 >>> --- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c >>> +++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c >>> @@ -559,6 +559,8 @@ static int atmel_isc_probe(struct platform_device *pdev) >>> cleanup_subdev: >>> isc_subdev_cleanup(isc); >>> >>> + pm_runtime_disable(dev); >>> + >> >> Hello Miaoqian Lin , >> >> Could you please perform the same change (or similar) in >> atmel-sama7g5-isc , as the sama7g5 ISC will perform the same and has the >> same bug. >> > Hi, Eugen Hristev: > > I think you are referring to microchip_xisc_probe() function in > atmel-sama7g5-isc, and I have look into it. After it calls > pm_runtime_enable(), it only have a regular path whichs return 0 and > indicates > the probe is successful. It doesn't have error handling path. Hi, You are right. It will always have a success path. Your patch is fine for me : Reviewed-by: Eugen Hristev <eugen.hristev@microchip.com> Thanks ! > > regmap_read() function returns a negative errno in error cases, but it > is used to get Microchip XISC version. I am not sure if failure means > the probe fails. >>> unregister_v4l2_device: >>> v4l2_device_unregister(&isc->v4l2_dev); >>> >>> -- >>> 2.17.1 >>> >>
On 3/1/22 08:18, Miaoqian Lin wrote: > The pm_runtime_enable will increase power disable depth. > If the probe fails, we should use pm_runtime_disable() to balance > pm_runtime_enable(). > > Fixes: 0a0e265 ("media: atmel: atmel-isc: split driver into driver base and isc") > Signed-off-by: Miaoqian Lin <linmq006@gmail.com> > --- > changes in v2: > - remove unused label. > --- > drivers/media/platform/atmel/atmel-sama5d2-isc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c > index 1b2063cce0f7..7f1ebbb25437 100644 > --- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c > +++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c > @@ -559,6 +559,8 @@ static int atmel_isc_probe(struct platform_device *pdev) > cleanup_subdev: > isc_subdev_cleanup(isc); > > + pm_runtime_disable(dev); Same issue as with the st-delta patch: there is one 'goto cleanup_subdev' that is called before the pm_runtime_enable, so this patch just introduces another imbalance. You need an additional goto label here and rework it a little bit to get this right. Regards, Hans > + > unregister_v4l2_device: > v4l2_device_unregister(&isc->v4l2_dev); >
diff --git a/drivers/media/platform/atmel/atmel-sama5d2-isc.c b/drivers/media/platform/atmel/atmel-sama5d2-isc.c index 1b2063cce0f7..7f1ebbb25437 100644 --- a/drivers/media/platform/atmel/atmel-sama5d2-isc.c +++ b/drivers/media/platform/atmel/atmel-sama5d2-isc.c @@ -559,6 +559,8 @@ static int atmel_isc_probe(struct platform_device *pdev) cleanup_subdev: isc_subdev_cleanup(isc); + pm_runtime_disable(dev); + unregister_v4l2_device: v4l2_device_unregister(&isc->v4l2_dev);
The pm_runtime_enable will increase power disable depth. If the probe fails, we should use pm_runtime_disable() to balance pm_runtime_enable(). Fixes: 0a0e265 ("media: atmel: atmel-isc: split driver into driver base and isc") Signed-off-by: Miaoqian Lin <linmq006@gmail.com> --- changes in v2: - remove unused label. --- drivers/media/platform/atmel/atmel-sama5d2-isc.c | 2 ++ 1 file changed, 2 insertions(+)