| Message ID | 20191002143551.32288-1-dinguyen@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCHv3] ARM: drivers/amba: release and cleanup the resource to allow for deferred probe | expand |
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; } /*
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
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
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);
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(-)