Re: [PATCH] ARM: drivers/amba: release the resource to allow for deferred probe

[Russell King - ARM Linux admin]

On Tue, Oct 01, 2019 at 04:40:26PM -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 than the amba bus. 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 releases the resource when amba_device_try_add() returns
> -EPROBE_DEFER. This allows the deferred probe to continue.
> 
> Fixes: 79bdcb202a35 ("ARM: 8906/1: drivers/amba: add reset control to
> amba bus probe")
> Signed-off-by: Dinh Nguyen <dinguyen@xxxxxxxxxx>
> ---
>  drivers/amba/bus.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index f39f075abff9..f246b847c991 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -535,6 +535,7 @@ int amba_device_add(struct amba_device *dev, struct resource *parent)
>  
>  	if (ret == -EPROBE_DEFER) {
>  		struct deferred_device *ddev;
> +		release_resource(&dev->res);

This is in the wrong place, and misses more serious leaks.

>  		ddev = kmalloc(sizeof(*ddev), GFP_KERNEL);
>  		if (!ddev)

What we have is bad error cleanup code in amba_device_try_add().
Consider what would happen if dev_pm_domain_attach() inside that
function were to return with -EPROBE_DEFER with your patch in
place - we would call release_resource() twice on the same
resource.  Clearly, that's incorrect.

The problem is that an error from
of_reset_control_array_get_optional_shared() just returns, leaving
everything that amba_device_try_add() already did still in place.
So, for example, a subsequent call to amba_device_try_add() will
remap the resource, leaking the previous mapping.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up