Re: [PATCH] hwrng: omap - balance runtime PM and clocks on probe-defer paths

[Herbert Xu]

William Theesfeld <william@xxxxxxxxxxxxx> wrote:
> omap_rng_probe() calls pm_runtime_enable() and pm_runtime_resume_and_get()
> to bring the device up.  If either devm_clk_get() call subsequently
> returns -EPROBE_DEFER, the function returns -EPROBE_DEFER directly,
> leaking the runtime PM usage counter taken by resume_and_get() and
> leaving pm_runtime enabled.
> 
> Convert both early returns to set ret and jump to err_register, which
> already performs the matching pm_runtime_put_sync() + pm_runtime_disable()
> unwind.  Because devm_clk_get() returns ERR_PTR on failure (not NULL)
> and err_register calls clk_disable_unprepare() unconditionally, also
> NULL out the failed clk pointers before the goto so that
> clk_disable_unprepare() (which only handles NULL safely, not ERR_PTR)
> does not deref an error pointer.
> 
> While here, NULL out priv->clk and priv->clk_reg in the existing
> "optional clock not present" else branches.  In that pre-existing case
> the pointer was left as ERR_PTR, and the unconditional
> clk_disable_unprepare() in omap_rng_remove() would have dereferenced
> it on driver unbind.  No functional change for systems where both
> clocks are present.
> 
> Found by smatch ("missing unwind goto?").
> 
> Signed-off-by: William Theesfeld <william@xxxxxxxxxxxxx>
> ---
> drivers/char/hw_random/omap-rng.c | 24 ++++++++++++++++++++----
> 1 file changed, 20 insertions(+), 4 deletions(-)

Looks alright to me.

But please add a Fixes header.  I think the bug was added by:

commit 43ec540e6f9b8e795dc9000114636ff72afc5b01
Author: Thomas Petazzoni <thomas.petazzoni@xxxxxxxxxxxxxxxxxx>
Date:   Tue Mar 7 15:14:49 2017 +0100

    hwrng: omap - move clock related code to omap_rng_probe()

as prior to that the probe function did the right thing.

Thanks,
-- 
Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt