Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1391786 > unrolled thread

[PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register()

Started byMasahiro Yamada <yamada.masahiro@socionext.com>
First post2016-05-01 12:40 +0200
Last post2016-05-03 13:50 +0200
Articles 8 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register() Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-05-01 12:40 +0200
    Re: [PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register() Joachim Eastwood <manabian@gmail.com> - 2016-05-01 23:10 +0200
    Re: [PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register() Philipp Zabel <p.zabel@pengutronix.de> - 2016-05-02 10:30 +0200
      Re: [PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register() Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-05-02 18:00 +0200
        Re: [PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register() Philipp Zabel <p.zabel@pengutronix.de> - 2016-05-03 11:10 +0200
          Re: [PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register() Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-05-03 12:30 +0200
            Re: [PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register() Philipp Zabel <p.zabel@pengutronix.de> - 2016-05-03 13:10 +0200
              Re: [PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register() Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-05-03 13:50 +0200

#1391786 — [PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register()

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2016-05-01 12:40 +0200
Subject[PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register()
Message-ID<rtWX8-83x-1@gated-at.bofh.it>
Use devm_reset_controller_register() for the reset controller
registration and remove the unregister call from the .remove callback.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/reset/reset-lpc18xx.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/reset/reset-lpc18xx.c b/drivers/reset/reset-lpc18xx.c
index 3b8a4f5..dd4f27e 100644
--- a/drivers/reset/reset-lpc18xx.c
+++ b/drivers/reset/reset-lpc18xx.c
@@ -199,7 +199,7 @@ static int lpc18xx_rgu_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, rc);
 
-	ret = reset_controller_register(&rc->rcdev);
+	ret = devm_reset_controller_register(&pdev->dev, &rc->rcdev);
 	if (ret) {
 		dev_err(&pdev->dev, "unable to register device\n");
 		goto dis_clks;
@@ -229,8 +229,6 @@ static int lpc18xx_rgu_remove(struct platform_device *pdev)
 	if (ret)
 		dev_warn(&pdev->dev, "failed to unregister restart handler\n");
 
-	reset_controller_unregister(&rc->rcdev);
-
 	clk_disable_unprepare(rc->clk_delay);
 	clk_disable_unprepare(rc->clk_reg);
 
-- 
1.9.1

[toc] | [next] | [standalone]

#1391885

FromJoachim Eastwood <manabian@gmail.com>
Date2016-05-01 23:10 +0200
Message-ID<ru6MO-7W7-17@gated-at.bofh.it>
In reply to#1391786
On 1 May 2016 at 12:36, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> Use devm_reset_controller_register() for the reset controller
> registration and remove the unregister call from the .remove callback.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---

Acked-by: Joachim Eastwood <manabian@gmail.com>


regards,
Joachim Eastwood

[toc] | [prev] | [next] | [standalone]

#1392058

FromPhilipp Zabel <p.zabel@pengutronix.de>
Date2016-05-02 10:30 +0200
Message-ID<ruhoS-8sl-17@gated-at.bofh.it>
In reply to#1391786
Am Sonntag, den 01.05.2016, 19:36 +0900 schrieb Masahiro Yamada:
> Use devm_reset_controller_register() for the reset controller
> registration and remove the unregister call from the .remove callback.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
>  drivers/reset/reset-lpc18xx.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/reset/reset-lpc18xx.c b/drivers/reset/reset-lpc18xx.c
> index 3b8a4f5..dd4f27e 100644
> --- a/drivers/reset/reset-lpc18xx.c
> +++ b/drivers/reset/reset-lpc18xx.c
> @@ -199,7 +199,7 @@ static int lpc18xx_rgu_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, rc);
>  
> -	ret = reset_controller_register(&rc->rcdev);
> +	ret = devm_reset_controller_register(&pdev->dev, &rc->rcdev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "unable to register device\n");
>  		goto dis_clks;
> @@ -229,8 +229,6 @@ static int lpc18xx_rgu_remove(struct platform_device *pdev)
>  	if (ret)
>  		dev_warn(&pdev->dev, "failed to unregister restart handler\n");
>  
> -	reset_controller_unregister(&rc->rcdev);
> -
>  	clk_disable_unprepare(rc->clk_delay);
>  	clk_disable_unprepare(rc->clk_reg);
>  

Hmm, would this patch theoretically allow a window between the calls to
clk_disable_unprepare(clk_reg) and devm_reset_controller_release() where
reset_control_get() + reset_control_(de)assert() would access unclocked
registers?

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1392335

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2016-05-02 18:00 +0200
Message-ID<ruoqm-6cc-15@gated-at.bofh.it>
In reply to#1392058
2016-05-02 17:26 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> Am Sonntag, den 01.05.2016, 19:36 +0900 schrieb Masahiro Yamada:
>> Use devm_reset_controller_register() for the reset controller
>> registration and remove the unregister call from the .remove callback.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>>  drivers/reset/reset-lpc18xx.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/reset/reset-lpc18xx.c b/drivers/reset/reset-lpc18xx.c
>> index 3b8a4f5..dd4f27e 100644
>> --- a/drivers/reset/reset-lpc18xx.c
>> +++ b/drivers/reset/reset-lpc18xx.c
>> @@ -199,7 +199,7 @@ static int lpc18xx_rgu_probe(struct platform_device *pdev)
>>
>>       platform_set_drvdata(pdev, rc);
>>
>> -     ret = reset_controller_register(&rc->rcdev);
>> +     ret = devm_reset_controller_register(&pdev->dev, &rc->rcdev);
>>       if (ret) {
>>               dev_err(&pdev->dev, "unable to register device\n");
>>               goto dis_clks;
>> @@ -229,8 +229,6 @@ static int lpc18xx_rgu_remove(struct platform_device *pdev)
>>       if (ret)
>>               dev_warn(&pdev->dev, "failed to unregister restart handler\n");
>>
>> -     reset_controller_unregister(&rc->rcdev);
>> -
>>       clk_disable_unprepare(rc->clk_delay);
>>       clk_disable_unprepare(rc->clk_reg);
>>
>
> Hmm, would this patch theoretically allow a window between the calls to
> clk_disable_unprepare(clk_reg) and devm_reset_controller_release() where
> reset_control_get() + reset_control_(de)assert() would access unclocked
> registers?

This is not clear to me.

Why reset_control_get() + reset_control_(de)assert() would happen here?


devm_reset_controller_release() just calls reset_controller_unregister().

It is just a manipulation of a linked list.


void reset_controller_unregister(struct reset_controller_dev *rcdev)
{
      mutex_lock(&reset_controller_list_mutex);
      list_del(&rcdev->list);
      mutex_unlock(&reset_controller_list_mutex);
}




-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [next] | [standalone]

#1393213

FromPhilipp Zabel <p.zabel@pengutronix.de>
Date2016-05-03 11:10 +0200
Message-ID<ruEv8-5nI-21@gated-at.bofh.it>
In reply to#1392335
Am Dienstag, den 03.05.2016, 00:52 +0900 schrieb Masahiro Yamada:
> 2016-05-02 17:26 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> > Am Sonntag, den 01.05.2016, 19:36 +0900 schrieb Masahiro Yamada:
> >> Use devm_reset_controller_register() for the reset controller
> >> registration and remove the unregister call from the .remove callback.
> >>
> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> >> ---
> >>
> >>  drivers/reset/reset-lpc18xx.c | 4 +---
> >>  1 file changed, 1 insertion(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/reset/reset-lpc18xx.c b/drivers/reset/reset-lpc18xx.c
> >> index 3b8a4f5..dd4f27e 100644
> >> --- a/drivers/reset/reset-lpc18xx.c
> >> +++ b/drivers/reset/reset-lpc18xx.c
> >> @@ -199,7 +199,7 @@ static int lpc18xx_rgu_probe(struct platform_device *pdev)
> >>
> >>       platform_set_drvdata(pdev, rc);
> >>
> >> -     ret = reset_controller_register(&rc->rcdev);
> >> +     ret = devm_reset_controller_register(&pdev->dev, &rc->rcdev);
> >>       if (ret) {
> >>               dev_err(&pdev->dev, "unable to register device\n");
> >>               goto dis_clks;
> >> @@ -229,8 +229,6 @@ static int lpc18xx_rgu_remove(struct platform_device *pdev)
> >>       if (ret)
> >>               dev_warn(&pdev->dev, "failed to unregister restart handler\n");
> >>
> >> -     reset_controller_unregister(&rc->rcdev);
> >> -
> >>       clk_disable_unprepare(rc->clk_delay);
> >>       clk_disable_unprepare(rc->clk_reg);
> >>
> >
> > Hmm, would this patch theoretically allow a window between the calls to
> > clk_disable_unprepare(clk_reg) and devm_reset_controller_release() where
> > reset_control_get() + reset_control_(de)assert() would access unclocked
> > registers?
> 
> This is not clear to me.
> 
> Why reset_control_get() + reset_control_(de)assert() would happen here?

I suppose on a non-SMP device, without parallel probing this can't
really happen in practice.
It still seems weird that suddenly we disable the clocks before
unregistering the reset controller instead of afterwards.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1393308

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2016-05-03 12:30 +0200
Message-ID<ruFKy-6sc-17@gated-at.bofh.it>
In reply to#1393213
Hi Philipp,

2016-05-03 18:05 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> Am Dienstag, den 03.05.2016, 00:52 +0900 schrieb Masahiro Yamada:
>> 2016-05-02 17:26 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
>> > Am Sonntag, den 01.05.2016, 19:36 +0900 schrieb Masahiro Yamada:
>> >> Use devm_reset_controller_register() for the reset controller
>> >> registration and remove the unregister call from the .remove callback.
>> >>
>> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> >> ---
>> >>
>> >>  drivers/reset/reset-lpc18xx.c | 4 +---
>> >>  1 file changed, 1 insertion(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/reset/reset-lpc18xx.c b/drivers/reset/reset-lpc18xx.c
>> >> index 3b8a4f5..dd4f27e 100644
>> >> --- a/drivers/reset/reset-lpc18xx.c
>> >> +++ b/drivers/reset/reset-lpc18xx.c
>> >> @@ -199,7 +199,7 @@ static int lpc18xx_rgu_probe(struct platform_device *pdev)
>> >>
>> >>       platform_set_drvdata(pdev, rc);
>> >>
>> >> -     ret = reset_controller_register(&rc->rcdev);
>> >> +     ret = devm_reset_controller_register(&pdev->dev, &rc->rcdev);
>> >>       if (ret) {
>> >>               dev_err(&pdev->dev, "unable to register device\n");
>> >>               goto dis_clks;
>> >> @@ -229,8 +229,6 @@ static int lpc18xx_rgu_remove(struct platform_device *pdev)
>> >>       if (ret)
>> >>               dev_warn(&pdev->dev, "failed to unregister restart handler\n");
>> >>
>> >> -     reset_controller_unregister(&rc->rcdev);
>> >> -
>> >>       clk_disable_unprepare(rc->clk_delay);
>> >>       clk_disable_unprepare(rc->clk_reg);
>> >>
>> >
>> > Hmm, would this patch theoretically allow a window between the calls to
>> > clk_disable_unprepare(clk_reg) and devm_reset_controller_release() where
>> > reset_control_get() + reset_control_(de)assert() would access unclocked
>> > registers?
>>
>> This is not clear to me.
>>
>> Why reset_control_get() + reset_control_(de)assert() would happen here?
>
> I suppose on a non-SMP device, without parallel probing this can't
> really happen in practice.
> It still seems weird that suddenly we disable the clocks before
> unregistering the reset controller instead of afterwards.
>

I still do not understand what you mean.

This patch moves the reset_controller_unregister() call
after clk_disable_unprepare().


But, reset_controller_unregister() is just a manipulation of a liked list.
It does not trigger any hardware access.

Am I wrong?

void reset_controller_unregister(struct reset_controller_dev *rcdev)
{
        mutex_lock(&reset_controller_list_mutex);
        list_del(&rcdev->list);
        mutex_unlock(&reset_controller_list_mutex);
}



-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [next] | [standalone]

#1393333

FromPhilipp Zabel <p.zabel@pengutronix.de>
Date2016-05-03 13:10 +0200
Message-ID<ruGng-77Y-5@gated-at.bofh.it>
In reply to#1393308
Am Dienstag, den 03.05.2016, 19:25 +0900 schrieb Masahiro Yamada:
> Hi Philipp,
> 
> 2016-05-03 18:05 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> > Am Dienstag, den 03.05.2016, 00:52 +0900 schrieb Masahiro Yamada:
> >> 2016-05-02 17:26 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> >> > Am Sonntag, den 01.05.2016, 19:36 +0900 schrieb Masahiro Yamada:
> >> >> Use devm_reset_controller_register() for the reset controller
> >> >> registration and remove the unregister call from the .remove callback.
> >> >>
> >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> >> >> ---
> >> >>
> >> >>  drivers/reset/reset-lpc18xx.c | 4 +---
> >> >>  1 file changed, 1 insertion(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/drivers/reset/reset-lpc18xx.c b/drivers/reset/reset-lpc18xx.c
> >> >> index 3b8a4f5..dd4f27e 100644
> >> >> --- a/drivers/reset/reset-lpc18xx.c
> >> >> +++ b/drivers/reset/reset-lpc18xx.c
> >> >> @@ -199,7 +199,7 @@ static int lpc18xx_rgu_probe(struct platform_device *pdev)
> >> >>
> >> >>       platform_set_drvdata(pdev, rc);
> >> >>
> >> >> -     ret = reset_controller_register(&rc->rcdev);
> >> >> +     ret = devm_reset_controller_register(&pdev->dev, &rc->rcdev);
> >> >>       if (ret) {
> >> >>               dev_err(&pdev->dev, "unable to register device\n");
> >> >>               goto dis_clks;
> >> >> @@ -229,8 +229,6 @@ static int lpc18xx_rgu_remove(struct platform_device *pdev)
> >> >>       if (ret)
> >> >>               dev_warn(&pdev->dev, "failed to unregister restart handler\n");
> >> >>
> >> >> -     reset_controller_unregister(&rc->rcdev);
> >> >> -
> >> >>       clk_disable_unprepare(rc->clk_delay);
> >> >>       clk_disable_unprepare(rc->clk_reg);
> >> >>
> >> >
> >> > Hmm, would this patch theoretically allow a window between the calls to
> >> > clk_disable_unprepare(clk_reg) and devm_reset_controller_release() where
> >> > reset_control_get() + reset_control_(de)assert() would access unclocked
> >> > registers?
> >>
> >> This is not clear to me.
> >>
> >> Why reset_control_get() + reset_control_(de)assert() would happen here?
> >
> > I suppose on a non-SMP device, without parallel probing this can't
> > really happen in practice.
> > It still seems weird that suddenly we disable the clocks before
> > unregistering the reset controller instead of afterwards.
> >
> 
> I still do not understand what you mean.
> 
> This patch moves the reset_controller_unregister() call
> after clk_disable_unprepare().

And so the register access is made impossible before the reset
controller device actually vanishes from the publicly visible list.

> But, reset_controller_unregister() is just a manipulation of a liked list.
> It does not trigger any hardware access.
> 
> Am I wrong?

No, you are perfectly right. I don't see how this can be a real problem
unless at the same time another driver could try to request the still
available reset control.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1393354

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2016-05-03 13:50 +0200
Message-ID<ruGZZ-7ro-15@gated-at.bofh.it>
In reply to#1393333
2016-05-03 20:08 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> Am Dienstag, den 03.05.2016, 19:25 +0900 schrieb Masahiro Yamada:
>> Hi Philipp,
>>
>> 2016-05-03 18:05 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
>> > Am Dienstag, den 03.05.2016, 00:52 +0900 schrieb Masahiro Yamada:
>> >> 2016-05-02 17:26 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
>> >> > Am Sonntag, den 01.05.2016, 19:36 +0900 schrieb Masahiro Yamada:
>> >> >> Use devm_reset_controller_register() for the reset controller
>> >> >> registration and remove the unregister call from the .remove callback.
>> >> >>
>> >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> >> >> ---
>> >> >>
>> >> >>  drivers/reset/reset-lpc18xx.c | 4 +---
>> >> >>  1 file changed, 1 insertion(+), 3 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/reset/reset-lpc18xx.c b/drivers/reset/reset-lpc18xx.c
>> >> >> index 3b8a4f5..dd4f27e 100644
>> >> >> --- a/drivers/reset/reset-lpc18xx.c
>> >> >> +++ b/drivers/reset/reset-lpc18xx.c
>> >> >> @@ -199,7 +199,7 @@ static int lpc18xx_rgu_probe(struct platform_device *pdev)
>> >> >>
>> >> >>       platform_set_drvdata(pdev, rc);
>> >> >>
>> >> >> -     ret = reset_controller_register(&rc->rcdev);
>> >> >> +     ret = devm_reset_controller_register(&pdev->dev, &rc->rcdev);
>> >> >>       if (ret) {
>> >> >>               dev_err(&pdev->dev, "unable to register device\n");
>> >> >>               goto dis_clks;
>> >> >> @@ -229,8 +229,6 @@ static int lpc18xx_rgu_remove(struct platform_device *pdev)
>> >> >>       if (ret)
>> >> >>               dev_warn(&pdev->dev, "failed to unregister restart handler\n");
>> >> >>
>> >> >> -     reset_controller_unregister(&rc->rcdev);
>> >> >> -
>> >> >>       clk_disable_unprepare(rc->clk_delay);
>> >> >>       clk_disable_unprepare(rc->clk_reg);
>> >> >>
>> >> >
>> >> > Hmm, would this patch theoretically allow a window between the calls to
>> >> > clk_disable_unprepare(clk_reg) and devm_reset_controller_release() where
>> >> > reset_control_get() + reset_control_(de)assert() would access unclocked
>> >> > registers?
>> >>
>> >> This is not clear to me.
>> >>
>> >> Why reset_control_get() + reset_control_(de)assert() would happen here?
>> >
>> > I suppose on a non-SMP device, without parallel probing this can't
>> > really happen in practice.
>> > It still seems weird that suddenly we disable the clocks before
>> > unregistering the reset controller instead of afterwards.
>> >
>>
>> I still do not understand what you mean.
>>
>> This patch moves the reset_controller_unregister() call
>> after clk_disable_unprepare().
>
> And so the register access is made impossible before the reset
> controller device actually vanishes from the publicly visible list.
>
>> But, reset_controller_unregister() is just a manipulation of a liked list.
>> It does not trigger any hardware access.
>>
>> Am I wrong?
>
> No, you are perfectly right. I don't see how this can be a real problem
> unless at the same time another driver could try to request the still
> available reset control.


Ah, now I understood.
Thanks!



-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web