| Message ID | 20190807084635.24173-3-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | clocksource/drivers/ostm: Miscellaneous improvements | expand |
On 07/08/2019 10:46, Geert Uytterhoeven wrote: > Fix various issues in the error path of ostm_init(): > 1. Drop error message printing on of_iomap() failure, as the memory > allocation core already takes of that, > 2. Handle irq_of_parse_and_map() failures correctly: it returns > unsigned int, hence make irq unsigned int, and zero is an error, > 3. Propagate/return appropriate error codes instead of -EFAULT. > 4. Add a missing clk_put(), > 5. Split the single cleanup block in separate phases, to avoid > clk_put() calling WARN() when passed an error pointer. Does it make sense to convert to timer-of API? > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > v3: > - New. > --- > drivers/clocksource/renesas-ostm.c | 54 ++++++++++++++++-------------- > 1 file changed, 29 insertions(+), 25 deletions(-) > > diff --git a/drivers/clocksource/renesas-ostm.c b/drivers/clocksource/renesas-ostm.c > index 37c39b901bb12b38..1e22e54d7b0df40d 100644 > --- a/drivers/clocksource/renesas-ostm.c > +++ b/drivers/clocksource/renesas-ostm.c > @@ -155,7 +155,7 @@ static irqreturn_t ostm_timer_interrupt(int irq, void *dev_id) > return IRQ_HANDLED; > } > > -static int __init ostm_init_clkevt(struct ostm_device *ostm, int irq, > +static int __init ostm_init_clkevt(struct ostm_device *ostm, unsigned int irq, > unsigned long rate) > { > struct clock_event_device *ced = &ostm->ced; > @@ -185,11 +185,11 @@ static int __init ostm_init_clkevt(struct ostm_device *ostm, int irq, > > static int __init ostm_init(struct device_node *np) > { > - struct ostm_device *ostm; > - int ret = -EFAULT; > struct clk *ostm_clk = NULL; > - int irq; > + struct ostm_device *ostm; > unsigned long rate; > + unsigned int irq; > + int ret; > > ostm = kzalloc(sizeof(*ostm), GFP_KERNEL); > if (!ostm) > @@ -197,27 +197,29 @@ static int __init ostm_init(struct device_node *np) > > ostm->base = of_iomap(np, 0); > if (!ostm->base) { > - pr_err("ostm: failed to remap I/O memory\n"); > - goto err; > + ret = -ENOMEM; > + goto err_free; > } > > irq = irq_of_parse_and_map(np, 0); > - if (irq < 0) { > + if (!irq) { > pr_err("ostm: Failed to get irq\n"); > - goto err; > + ret = -EINVAL; > + goto err_unmap; > } > > ostm_clk = of_clk_get(np, 0); > if (IS_ERR(ostm_clk)) { > pr_err("ostm: Failed to get clock\n"); > ostm_clk = NULL; > - goto err; > + ret = PTR_ERR(ostm_clk); > + goto err_unmap; > } > > ret = clk_prepare_enable(ostm_clk); > if (ret) { > pr_err("ostm: Failed to enable clock\n"); > - goto err; > + goto err_clk_put; > } > > rate = clk_get_rate(ostm_clk); > @@ -229,28 +231,30 @@ static int __init ostm_init(struct device_node *np) > */ > if (!system_clock) { > ret = ostm_init_clksrc(ostm, rate); > + if (ret) > + goto err_clk_disable; > > - if (!ret) { > - ostm_init_sched_clock(ostm, rate); > - pr_info("ostm: used for clocksource\n"); > - } > - > + ostm_init_sched_clock(ostm, rate); > + pr_info("ostm: used for clocksource\n"); > } else { > ret = ostm_init_clkevt(ostm, irq, rate); > + if (ret) > + goto err_clk_disable; > > - if (!ret) > - pr_info("ostm: used for clock events\n"); > - } > - > -err: > - if (ret) { > - clk_disable_unprepare(ostm_clk); > - iounmap(ostm->base); > - kfree(ostm); > - return ret; > + pr_info("ostm: used for clock events\n"); > } > > return 0; > + > +err_clk_disable: > + clk_disable_unprepare(ostm_clk); > +err_clk_put: > + clk_put(ostm_clk); > +err_unmap: > + iounmap(ostm->base); > +err_free: > + kfree(ostm); > + return ret; > } > > TIMER_OF_DECLARE(ostm, "renesas,ostm", ostm_init); >
Hi Daniel, On Wed, Aug 7, 2019 at 8:13 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > On 07/08/2019 10:46, Geert Uytterhoeven wrote: > > Fix various issues in the error path of ostm_init(): > > 1. Drop error message printing on of_iomap() failure, as the memory > > allocation core already takes of that, > > 2. Handle irq_of_parse_and_map() failures correctly: it returns > > unsigned int, hence make irq unsigned int, and zero is an error, > > 3. Propagate/return appropriate error codes instead of -EFAULT. > > 4. Add a missing clk_put(), > > 5. Split the single cleanup block in separate phases, to avoid > > clk_put() calling WARN() when passed an error pointer. > > Does it make sense to convert to timer-of API? I don't know. Will have a look... Gr{oetje,eeting}s, Geert
On Wed, Aug 07, 2019 at 08:13:27PM +0200, Daniel Lezcano wrote: > On 07/08/2019 10:46, Geert Uytterhoeven wrote: > > Fix various issues in the error path of ostm_init(): > > 1. Drop error message printing on of_iomap() failure, as the memory > > allocation core already takes of that, > > 2. Handle irq_of_parse_and_map() failures correctly: it returns > > unsigned int, hence make irq unsigned int, and zero is an error, > > 3. Propagate/return appropriate error codes instead of -EFAULT. > > 4. Add a missing clk_put(), > > 5. Split the single cleanup block in separate phases, to avoid > > clk_put() calling WARN() when passed an error pointer. > > Does it make sense to convert to timer-of API? I don't have an opinion on that at this time, but as an improvement this patch looks good to me. Reviewed-by: Simon Horman <horms+renesas@verge.net.au> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
diff --git a/drivers/clocksource/renesas-ostm.c b/drivers/clocksource/renesas-ostm.c index 37c39b901bb12b38..1e22e54d7b0df40d 100644 --- a/drivers/clocksource/renesas-ostm.c +++ b/drivers/clocksource/renesas-ostm.c @@ -155,7 +155,7 @@ static irqreturn_t ostm_timer_interrupt(int irq, void *dev_id) return IRQ_HANDLED; } -static int __init ostm_init_clkevt(struct ostm_device *ostm, int irq, +static int __init ostm_init_clkevt(struct ostm_device *ostm, unsigned int irq, unsigned long rate) { struct clock_event_device *ced = &ostm->ced; @@ -185,11 +185,11 @@ static int __init ostm_init_clkevt(struct ostm_device *ostm, int irq, static int __init ostm_init(struct device_node *np) { - struct ostm_device *ostm; - int ret = -EFAULT; struct clk *ostm_clk = NULL; - int irq; + struct ostm_device *ostm; unsigned long rate; + unsigned int irq; + int ret; ostm = kzalloc(sizeof(*ostm), GFP_KERNEL); if (!ostm) @@ -197,27 +197,29 @@ static int __init ostm_init(struct device_node *np) ostm->base = of_iomap(np, 0); if (!ostm->base) { - pr_err("ostm: failed to remap I/O memory\n"); - goto err; + ret = -ENOMEM; + goto err_free; } irq = irq_of_parse_and_map(np, 0); - if (irq < 0) { + if (!irq) { pr_err("ostm: Failed to get irq\n"); - goto err; + ret = -EINVAL; + goto err_unmap; } ostm_clk = of_clk_get(np, 0); if (IS_ERR(ostm_clk)) { pr_err("ostm: Failed to get clock\n"); ostm_clk = NULL; - goto err; + ret = PTR_ERR(ostm_clk); + goto err_unmap; } ret = clk_prepare_enable(ostm_clk); if (ret) { pr_err("ostm: Failed to enable clock\n"); - goto err; + goto err_clk_put; } rate = clk_get_rate(ostm_clk); @@ -229,28 +231,30 @@ static int __init ostm_init(struct device_node *np) */ if (!system_clock) { ret = ostm_init_clksrc(ostm, rate); + if (ret) + goto err_clk_disable; - if (!ret) { - ostm_init_sched_clock(ostm, rate); - pr_info("ostm: used for clocksource\n"); - } - + ostm_init_sched_clock(ostm, rate); + pr_info("ostm: used for clocksource\n"); } else { ret = ostm_init_clkevt(ostm, irq, rate); + if (ret) + goto err_clk_disable; - if (!ret) - pr_info("ostm: used for clock events\n"); - } - -err: - if (ret) { - clk_disable_unprepare(ostm_clk); - iounmap(ostm->base); - kfree(ostm); - return ret; + pr_info("ostm: used for clock events\n"); } return 0; + +err_clk_disable: + clk_disable_unprepare(ostm_clk); +err_clk_put: + clk_put(ostm_clk); +err_unmap: + iounmap(ostm->base); +err_free: + kfree(ostm); + return ret; } TIMER_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
Fix various issues in the error path of ostm_init(): 1. Drop error message printing on of_iomap() failure, as the memory allocation core already takes of that, 2. Handle irq_of_parse_and_map() failures correctly: it returns unsigned int, hence make irq unsigned int, and zero is an error, 3. Propagate/return appropriate error codes instead of -EFAULT. 4. Add a missing clk_put(), 5. Split the single cleanup block in separate phases, to avoid clk_put() calling WARN() when passed an error pointer. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- v3: - New. --- drivers/clocksource/renesas-ostm.c | 54 ++++++++++++++++-------------- 1 file changed, 29 insertions(+), 25 deletions(-)