Messages in this thread | | | Subject | Re: [PATCHv3] ARM: drivers/amba: release and cleanup the resource to allow for deferred probe | From | Dinh Nguyen <> | Date | Wed, 2 Oct 2019 15:45:42 -0500 |
| |
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
| |