[v2] media: atmel: atmel-isc: Fix PM disable depth imbalance in atmel_isc_probe

  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(+)
  

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);
>