[PATCHv3] ARM: drivers/amba: release and cleanup the resource to allow for deferred probe
diff mbox series

Message ID 20191002143551.32288-1-dinguyen@kernel.org
State New
Headers show
Series
  • [PATCHv3] ARM: drivers/amba: release and cleanup the resource to allow for deferred probe
Related show

Commit Message

Dinh Nguyen Oct. 2, 2019, 2:35 p.m. UTC
With commit "79bdcb202a35 ARM: 8906/1: drivers/amba: add reset control to
amba bus probe", the amba bus driver needs to be deferred probe because the
reset driver is probed later. However with a deferred probe, the call to
request_resource() in the driver returns -EBUSY. The reason is the driver
has not released the resource from the previous probe attempt.

This patch fixes how we handle the condition of EPROBE_DEFER that is returned
from getting the reset controls. For this condition, the patch will jump
to defer_probe, which will iounmap, dev_pm_domain_detach, and release the
resource.

Fixes: 79bdcb202a35 ("ARM: 8906/1: drivers/amba: add reset control to
amba bus probe")
Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
---
v3: jump to defer_probe where the driver will unmap and pm_detach the
    driver resource for the next probe attempt
v2: release the resource when of_reset_control_array_get_optional_shared()
    returns EPROBE_DEFER
---
 drivers/amba/bus.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Russell King - ARM Linux admin Oct. 2, 2019, 5:32 p.m. UTC | #1
On Wed, Oct 02, 2019 at 09:35:51AM -0500, Dinh Nguyen wrote:
> With commit "79bdcb202a35 ARM: 8906/1: drivers/amba: add reset control to
> amba bus probe", the amba bus driver needs to be deferred probe because the
> reset driver is probed later. However with a deferred probe, the call to
> request_resource() in the driver returns -EBUSY. The reason is the driver
> has not released the resource from the previous probe attempt.
> 
> This patch fixes how we handle the condition of EPROBE_DEFER that is returned
> from getting the reset controls. For this condition, the patch will jump
> to defer_probe, which will iounmap, dev_pm_domain_detach, and release the
> resource.
> 
> Fixes: 79bdcb202a35 ("ARM: 8906/1: drivers/amba: add reset control to
> amba bus probe")
> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
> ---
> v3: jump to defer_probe where the driver will unmap and pm_detach the
>     driver resource for the next probe attempt
> v2: release the resource when of_reset_control_array_get_optional_shared()
>     returns EPROBE_DEFER
> ---
>  drivers/amba/bus.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index f39f075abff9..4a021b1dab3d 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -409,9 +409,12 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
>  		 */
>  		rstc = of_reset_control_array_get_optional_shared(dev->dev.of_node);
>  		if (IS_ERR(rstc)) {
> -			if (PTR_ERR(rstc) != -EPROBE_DEFER)
> +			ret = PTR_ERR(rstc);
> +			if (ret == -EPROBE_DEFER)
> +				goto defer_probe;
> +			else
>  				dev_err(&dev->dev, "Can't get amba reset!\n");
> -			return PTR_ERR(rstc);
> +			return ret;

So, if of_reset_control_array_get_optional_shared() returns an error,
we end up leaking the ioremap(), the resource claim, the pclk enable
and pm domain?  If it returns -EPROBE_DEFER, we end up leaking the
pclk enable?

I think this is going to be quicker if I write the patch - I haven't
build-tested this yet though.  Please check whether this works for
you.

Thanks.

8<=====
From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] drivers/amba: fix reset control error handling

With commit 79bdcb202a35 ("ARM: 8906/1: drivers/amba: add reset control
to amba bus probe") it is possible for the the amba bus driver to defer
probing the device for its IDs because the reset driver may be probed
later.

However when a subsequent probe occurs, the call to request_resource()
in the driver returns -EBUSY as the driver has not released the resource
from the initial probe attempt - or cleaned up any of the preceding
actions.

Fix this both for the deferred probe case as well as a failure to get
the reset.

Fixes: 79bdcb202a35 ("ARM: 8906/1: drivers/amba: add reset control to amba bus probe")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/amba/bus.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index f39f075abff9..fe1523664816 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -409,9 +409,11 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
 		 */
 		rstc = of_reset_control_array_get_optional_shared(dev->dev.of_node);
 		if (IS_ERR(rstc)) {
-			if (PTR_ERR(rstc) != -EPROBE_DEFER)
-				dev_err(&dev->dev, "Can't get amba reset!\n");
-			return PTR_ERR(rstc);
+			ret = PTR_ERR(rstc);
+			if (ret != -EPROBE_DEFER)
+				dev_err(&dev->dev, "can't get reset: %d\n",
+					ret);
+			goto err_reset;
 		}
 		reset_control_deassert(rstc);
 		reset_control_put(rstc);
@@ -472,6 +474,12 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
 	release_resource(&dev->res);
  err_out:
 	return ret;
+
+ err_reset:
+	amba_put_disable_pclk(dev);
+	iounmap(tmp);
+	dev_pm_domain_detach(&dev->dev, true);
+	goto err_release;
 }
 
 /*
Dinh Nguyen Oct. 2, 2019, 8:45 p.m. UTC | #2
On 10/2/19 12:32 PM, Russell King - ARM Linux admin wrote:
> On Wed, Oct 02, 2019 at 09:35:51AM -0500, Dinh Nguyen wrote:
>> With commit "79bdcb202a35 ARM: 8906/1: drivers/amba: add reset control to
>> amba bus probe", the amba bus driver needs to be deferred probe because the
>> reset driver is probed later. However with a deferred probe, the call to
>> request_resource() in the driver returns -EBUSY. The reason is the driver
>> has not released the resource from the previous probe attempt.
>>
>> This patch fixes how we handle the condition of EPROBE_DEFER that is returned
>> from getting the reset controls. For this condition, the patch will jump
>> to defer_probe, which will iounmap, dev_pm_domain_detach, and release the
>> resource.
>>
>> Fixes: 79bdcb202a35 ("ARM: 8906/1: drivers/amba: add reset control to
>> amba bus probe")
>> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
>> ---
>> v3: jump to defer_probe where the driver will unmap and pm_detach the
>>     driver resource for the next probe attempt
>> v2: release the resource when of_reset_control_array_get_optional_shared()
>>     returns EPROBE_DEFER
>> ---
>>  drivers/amba/bus.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
>> index f39f075abff9..4a021b1dab3d 100644
>> --- a/drivers/amba/bus.c
>> +++ b/drivers/amba/bus.c
>> @@ -409,9 +409,12 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
>>  		 */
>>  		rstc = of_reset_control_array_get_optional_shared(dev->dev.of_node);
>>  		if (IS_ERR(rstc)) {
>> -			if (PTR_ERR(rstc) != -EPROBE_DEFER)
>> +			ret = PTR_ERR(rstc);
>> +			if (ret == -EPROBE_DEFER)
>> +				goto defer_probe;
>> +			else
>>  				dev_err(&dev->dev, "Can't get amba reset!\n");
>> -			return PTR_ERR(rstc);
>> +			return ret;
> 
> So, if of_reset_control_array_get_optional_shared() returns an error,
> we end up leaking the ioremap(), the resource claim, the pclk enable
> and pm domain?  If it returns -EPROBE_DEFER, we end up leaking the
> pclk enable?
> 
> I think this is going to be quicker if I write the patch - I haven't
> build-tested this yet though.  Please check whether this works for
> you.
> 
> Thanks.
> 
> 8<=====
> From: Russell King <rmk+kernel@armlinux.org.uk>
> Subject: [PATCH] drivers/amba: fix reset control error handling
> 
> With commit 79bdcb202a35 ("ARM: 8906/1: drivers/amba: add reset control
> to amba bus probe") it is possible for the the amba bus driver to defer
> probing the device for its IDs because the reset driver may be probed
> later.
> 
> However when a subsequent probe occurs, the call to request_resource()
> in the driver returns -EBUSY as the driver has not released the resource
> from the initial probe attempt - or cleaned up any of the preceding
> actions.
> 
> Fix this both for the deferred probe case as well as a failure to get
> the reset.
> 
> Fixes: 79bdcb202a35 ("ARM: 8906/1: drivers/amba: add reset control to amba bus probe")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/amba/bus.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index f39f075abff9..fe1523664816 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -409,9 +409,11 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
>  		 */
>  		rstc = of_reset_control_array_get_optional_shared(dev->dev.of_node);
>  		if (IS_ERR(rstc)) {
> -			if (PTR_ERR(rstc) != -EPROBE_DEFER)
> -				dev_err(&dev->dev, "Can't get amba reset!\n");
> -			return PTR_ERR(rstc);
> +			ret = PTR_ERR(rstc);
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(&dev->dev, "can't get reset: %d\n",
> +					ret);
> +			goto err_reset;
>  		}
>  		reset_control_deassert(rstc);
>  		reset_control_put(rstc);
> @@ -472,6 +474,12 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
>  	release_resource(&dev->res);
>   err_out:
>  	return ret;
> +
> + err_reset:
> +	amba_put_disable_pclk(dev);
> +	iounmap(tmp);
> +	dev_pm_domain_detach(&dev->dev, true);
> +	goto err_release;
>  }
>  
>  /*
> 

Tested-by: Dinh Nguyen <dinguyen@kernel.org>

Thanks,
Dinh
Dinh Nguyen Oct. 2, 2019, 8:45 p.m. UTC | #3
On 10/2/19 12:32 PM, Russell King - ARM Linux admin wrote:
> On Wed, Oct 02, 2019 at 09:35:51AM -0500, Dinh Nguyen wrote:
>> With commit "79bdcb202a35 ARM: 8906/1: drivers/amba: add reset control to
>> amba bus probe", the amba bus driver needs to be deferred probe because the
>> reset driver is probed later. However with a deferred probe, the call to
>> request_resource() in the driver returns -EBUSY. The reason is the driver
>> has not released the resource from the previous probe attempt.
>>
>> This patch fixes how we handle the condition of EPROBE_DEFER that is returned
>> from getting the reset controls. For this condition, the patch will jump
>> to defer_probe, which will iounmap, dev_pm_domain_detach, and release the
>> resource.
>>
>> Fixes: 79bdcb202a35 ("ARM: 8906/1: drivers/amba: add reset control to
>> amba bus probe")
>> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
>> ---
>> v3: jump to defer_probe where the driver will unmap and pm_detach the
>>     driver resource for the next probe attempt
>> v2: release the resource when of_reset_control_array_get_optional_shared()
>>     returns EPROBE_DEFER
>> ---
>>  drivers/amba/bus.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
>> index f39f075abff9..4a021b1dab3d 100644
>> --- a/drivers/amba/bus.c
>> +++ b/drivers/amba/bus.c
>> @@ -409,9 +409,12 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
>>  		 */
>>  		rstc = of_reset_control_array_get_optional_shared(dev->dev.of_node);
>>  		if (IS_ERR(rstc)) {
>> -			if (PTR_ERR(rstc) != -EPROBE_DEFER)
>> +			ret = PTR_ERR(rstc);
>> +			if (ret == -EPROBE_DEFER)
>> +				goto defer_probe;
>> +			else
>>  				dev_err(&dev->dev, "Can't get amba reset!\n");
>> -			return PTR_ERR(rstc);
>> +			return ret;
> 
> So, if of_reset_control_array_get_optional_shared() returns an error,
> we end up leaking the ioremap(), the resource claim, the pclk enable
> and pm domain?  If it returns -EPROBE_DEFER, we end up leaking the
> pclk enable?
> 
> I think this is going to be quicker if I write the patch - I haven't
> build-tested this yet though.  Please check whether this works for
> you.
> 
> Thanks.
> 
> 8<=====
> From: Russell King <rmk+kernel@armlinux.org.uk>
> Subject: [PATCH] drivers/amba: fix reset control error handling
> 
> With commit 79bdcb202a35 ("ARM: 8906/1: drivers/amba: add reset control
> to amba bus probe") it is possible for the the amba bus driver to defer
> probing the device for its IDs because the reset driver may be probed
> later.
> 
> However when a subsequent probe occurs, the call to request_resource()
> in the driver returns -EBUSY as the driver has not released the resource
> from the initial probe attempt - or cleaned up any of the preceding
> actions.
> 
> Fix this both for the deferred probe case as well as a failure to get
> the reset.
> 
> Fixes: 79bdcb202a35 ("ARM: 8906/1: drivers/amba: add reset control to amba bus probe")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/amba/bus.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index f39f075abff9..fe1523664816 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -409,9 +409,11 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
>  		 */
>  		rstc = of_reset_control_array_get_optional_shared(dev->dev.of_node);
>  		if (IS_ERR(rstc)) {
> -			if (PTR_ERR(rstc) != -EPROBE_DEFER)
> -				dev_err(&dev->dev, "Can't get amba reset!\n");
> -			return PTR_ERR(rstc);
> +			ret = PTR_ERR(rstc);
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(&dev->dev, "can't get reset: %d\n",
> +					ret);
> +			goto err_reset;
>  		}
>  		reset_control_deassert(rstc);
>  		reset_control_put(rstc);
> @@ -472,6 +474,12 @@ static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
>  	release_resource(&dev->res);
>   err_out:
>  	return ret;
> +
> + err_reset:
> +	amba_put_disable_pclk(dev);
> +	iounmap(tmp);
> +	dev_pm_domain_detach(&dev->dev, true);
> +	goto err_release;
>  }
>  
>  /*
> 

Tested-by: Dinh Nguyen <dinguyen@kernel.org>

Thanks,
Dinh

Patch
diff mbox series

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index f39f075abff9..4a021b1dab3d 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -409,9 +409,12 @@  static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
 		 */
 		rstc = of_reset_control_array_get_optional_shared(dev->dev.of_node);
 		if (IS_ERR(rstc)) {
-			if (PTR_ERR(rstc) != -EPROBE_DEFER)
+			ret = PTR_ERR(rstc);
+			if (ret == -EPROBE_DEFER)
+				goto defer_probe;
+			else
 				dev_err(&dev->dev, "Can't get amba reset!\n");
-			return PTR_ERR(rstc);
+			return ret;
 		}
 		reset_control_deassert(rstc);
 		reset_control_put(rstc);
@@ -448,6 +451,7 @@  static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
 			ret = -ENODEV;
 	}
 
+ defer_probe:
 	iounmap(tmp);
 	dev_pm_domain_detach(&dev->dev, true);