arm64: dts: renesas: r8a7796: Add CMT device nodes

Commit Message

Biju Das Oct. 26, 2018, 8:25 a.m. UTC

This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.

Signed-off-by: Biju Das <[e-mail]>
---
This patch is tested against renesas-dev

I have executed on inconsistency-check, nanosleep and clocksource_switch
selftests on this arm64 SoC. The inconsistency-check and nanosleep tests
are working fine.The clocksource_switch asynchronous test is failing due
to inconsistency-check failure on "arch_sys_counter".

But if i skip the clocksource_switching of "arch_sys_counter", the
asynchronous test is passing for CMT0/1/2/3 timer.

Has any one noticed this issue?
---
 arch/arm64/boot/dts/renesas/r8a7796.dtsi | 70 ++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

Comments

Fabrizio Castro Oct. 26, 2018, 9:48 a.m. UTC | #1

> Subject: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device nodes
>
> This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
>
> Signed-off-by: Biju Das <[e-mail]>

Reviewed-by: Fabrizio Castro <[e-mail]>

> ---
> This patch is tested against renesas-dev
>
> I have executed on inconsistency-check, nanosleep and clocksource_switch
> selftests on this arm64 SoC. The inconsistency-check and nanosleep tests
> are working fine.The clocksource_switch asynchronous test is failing due
> to inconsistency-check failure on "arch_sys_counter".
>
> But if i skip the clocksource_switching of "arch_sys_counter", the
> asynchronous test is passing for CMT0/1/2/3 timer.
>
> Has any one noticed this issue?
> ---
>  arch/arm64/boot/dts/renesas/r8a7796.dtsi | 70 ++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> index 1ec6aaa..d62febd0 100644
> --- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> @@ -401,6 +401,76 @@
>  reg = <0 0xe6060000 0 0x50c>;
>  };
>
> +cmt0: timer@e60f0000 {
> +compatible = "renesas,r8a7796-cmt0",
> +     "renesas,rcar-gen3-cmt0";
> +reg = <0 0xe60f0000 0 0x1004>;
> +interrupts = <GIC_SPI 142 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 143 IRQ_TYPE_LEVEL_HIGH>;
> +clocks = <&cpg CPG_MOD 303>;
> +clock-names = "fck";
> +power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> +resets = <&cpg 303>;
> +status = "disabled";
> +};
> +
> +cmt1: timer@e6130000 {
> +compatible = "renesas,r8a7796-cmt1",
> +     "renesas,rcar-gen3-cmt1";
> +reg = <0 0xe6130000 0 0x1004>;
> +interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 127 IRQ_TYPE_LEVEL_HIGH>;
> +clocks = <&cpg CPG_MOD 302>;
> +clock-names = "fck";
> +power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> +resets = <&cpg 302>;
> +status = "disabled";
> +};
> +
> +cmt2: timer@e6140000 {
> +compatible = "renesas,r8a7796-cmt1",
> +     "renesas,rcar-gen3-cmt1";
> +reg = <0 0xe6140000 0 0x1004>;
> +interrupts = <GIC_SPI 398 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 399 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 400 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 401 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 402 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 403 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 404 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 405 IRQ_TYPE_LEVEL_HIGH>;
> +clocks = <&cpg CPG_MOD 301>;
> +clock-names = "fck";
> +power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> +resets = <&cpg 301>;
> +status = "disabled";
> +};
> +
> +cmt3: timer@e6148000 {
> +compatible = "renesas,r8a7796-cmt1",
> +     "renesas,rcar-gen3-cmt1";
> +reg = <0 0xe6148000 0 0x1004>;
> +interrupts = <GIC_SPI 470 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 471 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 474 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 475 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 476 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>;
> +clocks = <&cpg CPG_MOD 300>;
> +clock-names = "fck";
> +power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> +resets = <&cpg 300>;
> +status = "disabled";
> +};
> +
>  cpg: clock-controller@e6150000 {
>  compatible = "renesas,r8a7796-cpg-mssr";
>  reg = <0 0xe6150000 0 0x1000>;
> --
> 2.7.4




Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Biju Das Nov. 19, 2018, 8:27 a.m. UTC | #2

Hi All,

Gentle reminder, Is this patch looks ok to you? Or any changes needed.

Regards,
Biju

> -----Original Message-----
> From: Biju Das <[e-mail]>
> Sent: 26 October 2018 09:25
> To: Rob Herring <[e-mail]>; Mark Rutland
> <[e-mail]>
> Cc: Biju Das <[e-mail]>; Simon Horman
> <[e-mail]>; Magnus Damm <[e-mail]>;
> [e-mail]; [e-mail]; Geert
> Uytterhoeven <[e-mail]>; Chris Paterson
> <[e-mail]>; Daniel Lezcano
> <[e-mail]>; Thomas Gleixner <[e-mail]>; John
> Stultz <[e-mail]>; Fabrizio Castro
> <[e-mail]>
> Subject: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device nodes
>
> This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
>
> Signed-off-by: Biju Das <[e-mail]>
> ---
> This patch is tested against renesas-dev
>
> I have executed on inconsistency-check, nanosleep and clocksource_switch
> selftests on this arm64 SoC. The inconsistency-check and nanosleep tests are
> working fine.The clocksource_switch asynchronous test is failing due to
> inconsistency-check failure on "arch_sys_counter".
>
> But if i skip the clocksource_switching of "arch_sys_counter", the
> asynchronous test is passing for CMT0/1/2/3 timer.
>
> Has any one noticed this issue?
> ---
>  arch/arm64/boot/dts/renesas/r8a7796.dtsi | 70
> ++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> index 1ec6aaa..d62febd0 100644
> --- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> @@ -401,6 +401,76 @@
>  reg = <0 0xe6060000 0 0x50c>;
>  };
>
> +cmt0: timer@e60f0000 {
> +compatible = "renesas,r8a7796-cmt0",
> +     "renesas,rcar-gen3-cmt0";
> +reg = <0 0xe60f0000 0 0x1004>;
> +interrupts = <GIC_SPI 142 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 143 IRQ_TYPE_LEVEL_HIGH>;
> +clocks = <&cpg CPG_MOD 303>;
> +clock-names = "fck";
> +power-domains = <&sysc
> R8A7796_PD_ALWAYS_ON>;
> +resets = <&cpg 303>;
> +status = "disabled";
> +};
> +
> +cmt1: timer@e6130000 {
> +compatible = "renesas,r8a7796-cmt1",
> +     "renesas,rcar-gen3-cmt1";
> +reg = <0 0xe6130000 0 0x1004>;
> +interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 127 IRQ_TYPE_LEVEL_HIGH>;
> +clocks = <&cpg CPG_MOD 302>;
> +clock-names = "fck";
> +power-domains = <&sysc
> R8A7796_PD_ALWAYS_ON>;
> +resets = <&cpg 302>;
> +status = "disabled";
> +};
> +
> +cmt2: timer@e6140000 {
> +compatible = "renesas,r8a7796-cmt1",
> +     "renesas,rcar-gen3-cmt1";
> +reg = <0 0xe6140000 0 0x1004>;
> +interrupts = <GIC_SPI 398 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 399 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 400 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 401 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 402 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 403 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 404 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 405 IRQ_TYPE_LEVEL_HIGH>;
> +clocks = <&cpg CPG_MOD 301>;
> +clock-names = "fck";
> +power-domains = <&sysc
> R8A7796_PD_ALWAYS_ON>;
> +resets = <&cpg 301>;
> +status = "disabled";
> +};
> +
> +cmt3: timer@e6148000 {
> +compatible = "renesas,r8a7796-cmt1",
> +     "renesas,rcar-gen3-cmt1";
> +reg = <0 0xe6148000 0 0x1004>;
> +interrupts = <GIC_SPI 470 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 471 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 474 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 475 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 476 IRQ_TYPE_LEVEL_HIGH>,
> +     <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>;
> +clocks = <&cpg CPG_MOD 300>;
> +clock-names = "fck";
> +power-domains = <&sysc
> R8A7796_PD_ALWAYS_ON>;
> +resets = <&cpg 300>;
> +status = "disabled";
> +};
> +
>  cpg: clock-controller@e6150000 {
>  compatible = "renesas,r8a7796-cpg-mssr";
>  reg = <0 0xe6150000 0 0x1000>;
> --
> 2.7.4




Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Daniel Lezcano Nov. 19, 2018, 9:36 a.m. UTC | #3

On 26/10/2018 10:25, Biju Das wrote:
> This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
> 
> Signed-off-by: Biju Das <[e-mail]>
> ---
> This patch is tested against renesas-dev
> 
> I have executed on inconsistency-check, nanosleep and clocksource_switch
> selftests on this arm64 SoC. The inconsistency-check and nanosleep tests
> are working fine.The clocksource_switch asynchronous test is failing due
> to inconsistency-check failure on "arch_sys_counter".
> 
> But if i skip the clocksource_switching of "arch_sys_counter", the
> asynchronous test is passing for CMT0/1/2/3 timer.
> 
> Has any one noticed this issue?

Were you able to narrow down the issue?

> ---
>  arch/arm64/boot/dts/renesas/r8a7796.dtsi | 70 ++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> index 1ec6aaa..d62febd0 100644
> --- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> @@ -401,6 +401,76 @@
>  			reg = <0 0xe6060000 0 0x50c>;
>  		};
>  
> +		cmt0: timer@e60f0000 {
> +			compatible = "renesas,r8a7796-cmt0",
> +				     "renesas,rcar-gen3-cmt0";
> +			reg = <0 0xe60f0000 0 0x1004>;
> +			interrupts = <GIC_SPI 142 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 143 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cpg CPG_MOD 303>;
> +			clock-names = "fck";
> +			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> +			resets = <&cpg 303>;
> +			status = "disabled";
> +		};
> +
> +		cmt1: timer@e6130000 {
> +			compatible = "renesas,r8a7796-cmt1",
> +				     "renesas,rcar-gen3-cmt1";
> +			reg = <0 0xe6130000 0 0x1004>;
> +			interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 127 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cpg CPG_MOD 302>;
> +			clock-names = "fck";
> +			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> +			resets = <&cpg 302>;
> +			status = "disabled";
> +		};
> +
> +		cmt2: timer@e6140000 {
> +			compatible = "renesas,r8a7796-cmt1",
> +				     "renesas,rcar-gen3-cmt1";
> +			reg = <0 0xe6140000 0 0x1004>;
> +			interrupts = <GIC_SPI 398 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 399 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 400 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 401 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 402 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 403 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 404 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 405 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cpg CPG_MOD 301>;
> +			clock-names = "fck";
> +			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> +			resets = <&cpg 301>;
> +			status = "disabled";
> +		};
> +
> +		cmt3: timer@e6148000 {
> +			compatible = "renesas,r8a7796-cmt1",
> +				     "renesas,rcar-gen3-cmt1";
> +			reg = <0 0xe6148000 0 0x1004>;
> +			interrupts = <GIC_SPI 470 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 471 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 474 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 475 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 476 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cpg CPG_MOD 300>;
> +			clock-names = "fck";
> +			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> +			resets = <&cpg 300>;
> +			status = "disabled";
> +		};
> +
>  		cpg: clock-controller@e6150000 {
>  			compatible = "renesas,r8a7796-cpg-mssr";
>  			reg = <0 0xe6150000 0 0x1000>;
>

Daniel Lezcano Nov. 19, 2018, 10:26 a.m. UTC | #4

On 26/10/2018 10:25, Biju Das wrote:
> This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
> 
> Signed-off-by: Biju Das <[e-mail]>
> ---
> This patch is tested against renesas-dev
> 
> I have executed on inconsistency-check, nanosleep and clocksource_switch
> selftests on this arm64 SoC. The inconsistency-check and nanosleep tests
> are working fine.The clocksource_switch asynchronous test is failing due
> to inconsistency-check failure on "arch_sys_counter".
> 
> But if i skip the clocksource_switching of "arch_sys_counter", the
> asynchronous test is passing for CMT0/1/2/3 timer.
> 
> Has any one noticed this issue?

So now that you mention that, I've been through the clocksource_switch
on another ARM64 platform (hikey960) and disabled the
ARM64_ERRATUM_858921 config option. I can see the same issue.

Is this option set on your config ?

Geert Uytterhoeven Nov. 19, 2018, 10:32 a.m. UTC | #5

Hi Daniel,

On Mon, Nov 19, 2018 at 11:26 AM Daniel Lezcano
<[e-mail]> wrote:
> On 26/10/2018 10:25, Biju Das wrote:
> > This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
> >
> > Signed-off-by: Biju Das <[e-mail]>
> > ---
> > This patch is tested against renesas-dev
> >
> > I have executed on inconsistency-check, nanosleep and clocksource_switch
> > selftests on this arm64 SoC. The inconsistency-check and nanosleep tests
> > are working fine.The clocksource_switch asynchronous test is failing due
> > to inconsistency-check failure on "arch_sys_counter".
> >
> > But if i skip the clocksource_switching of "arch_sys_counter", the
> > asynchronous test is passing for CMT0/1/2/3 timer.
> >
> > Has any one noticed this issue?
>
> So now that you mention that, I've been through the clocksource_switch
> on another ARM64 platform (hikey960) and disabled the
> ARM64_ERRATUM_858921 config option. I can see the same issue.
>
> Is this option set on your config ?

Note that r8a7796 does not have Cortex-A73, but A57/A53.
But let's wait for Biju's answer, to see if it makes a difference...

Gr{oetje,eeting}s,

                        Geert

Biju Das Nov. 19, 2018, 10:35 a.m. UTC | #6

Hi Daniel,

Thanks for the feedback.

> -----Original Message-----
> From: Daniel Lezcano <[e-mail]>
> Sent: 19 November 2018 10:26
> To: Biju Das <[e-mail]>; Rob Herring
> <[e-mail]>; Mark Rutland <[e-mail]>
> Cc: Simon Horman <[e-mail]>; Magnus Damm
> <[e-mail]>; [e-mail];
> [e-mail]; Geert Uytterhoeven
> <[e-mail]>; Chris Paterson
> <[e-mail]>; Thomas Gleixner <[e-mail]>;
> John Stultz <[e-mail]>; Fabrizio Castro
> <[e-mail]>
> Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device nodes
>
> On 26/10/2018 10:25, Biju Das wrote:
> > This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
> >
> > Signed-off-by: Biju Das <[e-mail]>
> > ---
> > This patch is tested against renesas-dev
> >
> > I have executed on inconsistency-check, nanosleep and
> > clocksource_switch selftests on this arm64 SoC. The
> > inconsistency-check and nanosleep tests are working fine.The
> > clocksource_switch asynchronous test is failing due to inconsistency-check
> failure on "arch_sys_counter".
> >
> > But if i skip the clocksource_switching of "arch_sys_counter", the
> > asynchronous test is passing for CMT0/1/2/3 timer.
> >
> > Has any one noticed this issue?
>
> So now that you mention that, I've been through the clocksource_switch on
> another ARM64 platform (hikey960) and disabled the
> ARM64_ERRATUM_858921 config option. I can see the same issue.
>
> Is this option set on your config ?

No.  As per  " config ARM64_ERRATUM_858921", it is "Workaround for Cortex-A73 erratum 858921"

Our SoC is 2xCA-57 + 4 x CA-53.  Does  it impact CA-57 + CA_53?

Any way I will enable this config option and will provide you the results.

The following errata is set in our kernel config.

CONFIG_ARM64_ERRATUM_826319=y
CONFIG_ARM64_ERRATUM_827319=y
CONFIG_ARM64_ERRATUM_824069=y
CONFIG_ARM64_ERRATUM_819472=y
CONFIG_ARM64_ERRATUM_832075=y
CONFIG_ARM64_ERRATUM_834220=y
CONFIG_ARM64_ERRATUM_845719=y
CONFIG_ARM64_ERRATUM_843419=y
CONFIG_ARM64_ERRATUM_1024718=y
CONFIG_ARM64_ERRATUM_1188873=y

Regards.
Biju



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Daniel Lezcano Nov. 19, 2018, 11:01 a.m. UTC | #7

On 19/11/2018 11:35, Biju Das wrote:
> Hi Daniel,
> 
> Thanks for the feedback.
> 
>> -----Original Message-----
>> From: Daniel Lezcano <[e-mail]>
>> Sent: 19 November 2018 10:26
>> To: Biju Das <[e-mail]>; Rob Herring
>> <[e-mail]>; Mark Rutland <[e-mail]>
>> Cc: Simon Horman <[e-mail]>; Magnus Damm
>> <[e-mail]>; [e-mail];
>> [e-mail]; Geert Uytterhoeven
>> <[e-mail]>; Chris Paterson
>> <[e-mail]>; Thomas Gleixner <[e-mail]>;
>> John Stultz <[e-mail]>; Fabrizio Castro
>> <[e-mail]>
>> Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device nodes
>>
>> On 26/10/2018 10:25, Biju Das wrote:
>>> This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
>>>
>>> Signed-off-by: Biju Das <[e-mail]>
>>> ---
>>> This patch is tested against renesas-dev
>>>
>>> I have executed on inconsistency-check, nanosleep and
>>> clocksource_switch selftests on this arm64 SoC. The
>>> inconsistency-check and nanosleep tests are working fine.The
>>> clocksource_switch asynchronous test is failing due to inconsistency-check
>> failure on "arch_sys_counter".
>>>
>>> But if i skip the clocksource_switching of "arch_sys_counter", the
>>> asynchronous test is passing for CMT0/1/2/3 timer.
>>>
>>> Has any one noticed this issue?
>>
>> So now that you mention that, I've been through the clocksource_switch on
>> another ARM64 platform (hikey960) and disabled the
>> ARM64_ERRATUM_858921 config option. I can see the same issue.
>>
>> Is this option set on your config ?
> 
> No.  As per  " config ARM64_ERRATUM_858921", it is "Workaround for Cortex-A73 erratum 858921"
> 
> Our SoC is 2xCA-57 + 4 x CA-53.  Does  it impact CA-57 + CA_53?

Dunno :/

> Any way I will enable this config option and will provide you the results.

Ok, thanks!

> The following errata is set in our kernel config.
> 
> CONFIG_ARM64_ERRATUM_826319=y
> CONFIG_ARM64_ERRATUM_827319=y
> CONFIG_ARM64_ERRATUM_824069=y
> CONFIG_ARM64_ERRATUM_819472=y
> CONFIG_ARM64_ERRATUM_832075=y
> CONFIG_ARM64_ERRATUM_834220=y
> CONFIG_ARM64_ERRATUM_845719=y
> CONFIG_ARM64_ERRATUM_843419=y
> CONFIG_ARM64_ERRATUM_1024718=y
> CONFIG_ARM64_ERRATUM_1188873=y
> 
> Regards.
> Biju
> 
> 
> 
> Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
>

Biju Das Nov. 19, 2018, 3:50 p.m. UTC | #8

Hi Daniel,

Thanks for the feedback.

> >> Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
> >> nodes
> >>
> >> On 26/10/2018 10:25, Biju Das wrote:
> >>> This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
> >>>
> >>> Signed-off-by: Biju Das <[e-mail]>
> >>> ---
> >>> This patch is tested against renesas-dev
> >>>
> >>> I have executed on inconsistency-check, nanosleep and
> >>> clocksource_switch selftests on this arm64 SoC. The
> >>> inconsistency-check and nanosleep tests are working fine.The
> >>> clocksource_switch asynchronous test is failing due to
> >>> inconsistency-check
> >> failure on "arch_sys_counter".
> >>>
> >>> But if i skip the clocksource_switching of "arch_sys_counter", the
> >>> asynchronous test is passing for CMT0/1/2/3 timer.
> >>>
> >>> Has any one noticed this issue?
> >>
> >> So now that you mention that, I've been through the
> >> clocksource_switch on another ARM64 platform (hikey960) and disabled
> >> the
> >> ARM64_ERRATUM_858921 config option. I can see the same issue.
> >>
> >> Is this option set on your config ?
> >
> > No.  As per  " config ARM64_ERRATUM_858921", it is "Workaround for
> Cortex-A73 erratum 858921"
> >
> > Our SoC is 2xCA-57 + 4 x CA-53.  Does  it impact CA-57 + CA_53?
>
> Dunno :/
>
> > Any way I will enable this config option and will provide you the results.
>
> Ok, thanks!

The following config is enabled by default on upstream kernel(4.20-rc3)
CONFIG_ARM_ARCH_TIMER_EVTSTREAM=y
CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND=y
CONFIG_FSL_ERRATUM_A008585=y
CONFIG_HISILICON_ERRATUM_161010101=y
CONFIG_ARM64_ERRATUM_858921=y

For a quick testing,  I have activated the erratum using the property "fsl,erratum-a008585" on device tree.
With this I confirm the issue is fixed.

I have  some questions on this.
1) Based  on the test result ,do you think renesas soc also  impacted by the ARM64_ERRATUM_858921?
2) Is there any way to find, is this Erratum actually causing the asynchronous test to fail?

timer {
                compatible = "arm,armv8-timer";
                interrupts-extended = <&gic GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(6) | IRQ_TYPE_LEVEL_LOW)>,
                                      <&gic GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(6) | IRQ_TYPE_LEVEL_LOW)>,
                                      <&gic GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(6) | IRQ_TYPE_LEVEL_LOW)>,
                                      <&gic GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(6) | IRQ_TYPE_LEVEL_LOW)>;
+                fsl,erratum-a008585;
}

Regards,
Biju



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Daniel Lezcano Nov. 19, 2018, 5:14 p.m. UTC | #9

On 19/11/2018 16:50, Biju Das wrote:
> Hi Daniel,
> 
> Thanks for the feedback.
> 
>>>> Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
>>>> nodes
>>>>
>>>> On 26/10/2018 10:25, Biju Das wrote:
>>>>> This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
>>>>>
>>>>> Signed-off-by: Biju Das <[e-mail]>
>>>>> ---
>>>>> This patch is tested against renesas-dev
>>>>>
>>>>> I have executed on inconsistency-check, nanosleep and
>>>>> clocksource_switch selftests on this arm64 SoC. The
>>>>> inconsistency-check and nanosleep tests are working fine.The
>>>>> clocksource_switch asynchronous test is failing due to
>>>>> inconsistency-check
>>>> failure on "arch_sys_counter".
>>>>>
>>>>> But if i skip the clocksource_switching of "arch_sys_counter", the
>>>>> asynchronous test is passing for CMT0/1/2/3 timer.
>>>>>
>>>>> Has any one noticed this issue?
>>>>
>>>> So now that you mention that, I've been through the
>>>> clocksource_switch on another ARM64 platform (hikey960) and disabled
>>>> the
>>>> ARM64_ERRATUM_858921 config option. I can see the same issue.
>>>>
>>>> Is this option set on your config ?
>>>
>>> No.  As per  " config ARM64_ERRATUM_858921", it is "Workaround for
>> Cortex-A73 erratum 858921"
>>>
>>> Our SoC is 2xCA-57 + 4 x CA-53.  Does  it impact CA-57 + CA_53?
>>
>> Dunno :/
>>
>>> Any way I will enable this config option and will provide you the results.
>>
>> Ok, thanks!
> 
> The following config is enabled by default on upstream kernel(4.20-rc3)
> CONFIG_ARM_ARCH_TIMER_EVTSTREAM=y
> CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND=y
> CONFIG_FSL_ERRATUM_A008585=y
> CONFIG_HISILICON_ERRATUM_161010101=y
> CONFIG_ARM64_ERRATUM_858921=y
> 
> For a quick testing,  I have activated the erratum using the property "fsl,erratum-a008585" on device tree.
> With this I confirm the issue is fixed.
> 
> I have  some questions on this.
> 1) Based  on the test result ,do you think renesas soc also  impacted by the ARM64_ERRATUM_858921?
> 2) Is there any way to find, is this Erratum actually causing the asynchronous test to fail?

I guess, you can hack the __fsl_a008585_read_reg macro and check if the
invalid condition is reached.

This thread https://lkml.org/lkml/2018/5/10/773 will give you all the
answers you are looking for (well very likely).

Let me know if it helped.


> timer {
>                 compatible = "arm,armv8-timer";
>                 interrupts-extended = <&gic GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(6) | IRQ_TYPE_LEVEL_LOW)>,
>                                       <&gic GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(6) | IRQ_TYPE_LEVEL_LOW)>,
>                                       <&gic GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(6) | IRQ_TYPE_LEVEL_LOW)>,
>                                       <&gic GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(6) | IRQ_TYPE_LEVEL_LOW)>;
> +                fsl,erratum-a008585;
> }

Simon Horman Nov. 21, 2018, 10:24 a.m. UTC | #10

On Fri, Oct 26, 2018 at 09:25:07AM +0100, Biju Das wrote:
> This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
> 
> Signed-off-by: Biju Das <[e-mail]>
> ---
> This patch is tested against renesas-dev
> 
> I have executed on inconsistency-check, nanosleep and clocksource_switch
> selftests on this arm64 SoC. The inconsistency-check and nanosleep tests
> are working fine.The clocksource_switch asynchronous test is failing due
> to inconsistency-check failure on "arch_sys_counter".
> 
> But if i skip the clocksource_switching of "arch_sys_counter", the
> asynchronous test is passing for CMT0/1/2/3 timer.
> 
> Has any one noticed this issue?

I am reluctant to apply this patch until there is a better understanding
of the issue above.

Biju Das Nov. 21, 2018, 10:27 a.m. UTC | #11

Hi Simon,

Thanks for the feedback.

> -----Original Message-----
> From: Simon Horman <[e-mail]>
> Sent: 21 November 2018 10:24
> To: Biju Das <[e-mail]>
> Cc: Rob Herring <[e-mail]>; Mark Rutland
> <[e-mail]>; Magnus Damm <[e-mail]>;
> [e-mail]; [e-mail]; Geert
> Uytterhoeven <[e-mail]>; Chris Paterson
> <[e-mail]>; Daniel Lezcano
> <[e-mail]>; Thomas Gleixner <[e-mail]>; John
> Stultz <[e-mail]>; Fabrizio Castro
> <[e-mail]>
> Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device nodes
>
> On Fri, Oct 26, 2018 at 09:25:07AM +0100, Biju Das wrote:
> > This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
> >
> > Signed-off-by: Biju Das <[e-mail]>
> > ---
> > This patch is tested against renesas-dev
> >
> > I have executed on inconsistency-check, nanosleep and
> > clocksource_switch selftests on this arm64 SoC. The
> > inconsistency-check and nanosleep tests are working fine.The
> > clocksource_switch asynchronous test is failing due to inconsistency-check
> failure on "arch_sys_counter".
> >
> > But if i skip the clocksource_switching of "arch_sys_counter", the
> > asynchronous test is passing for CMT0/1/2/3 timer.
> >
> > Has any one noticed this issue?
>
> I am reluctant to apply this patch until there is a better understanding of the
> issue above.

If I understand correctly this patch has no issue at all. The problem is related to ARM architecture  timer.
Please correct me if you think otherwise.

Regards,
Biju



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Biju Das Nov. 22, 2018, 9:46 a.m. UTC | #12

Hello Daniel,

Thanks for the feedback.

> -----Original Message-----
> From: Daniel Lezcano <[e-mail]>
> Sent: 19 November 2018 17:15
> To: Biju Das <[e-mail]>; Rob Herring
> <[e-mail]>; Mark Rutland <[e-mail]>
> Cc: Simon Horman <[e-mail]>; Magnus Damm
> <[e-mail]>; [e-mail];
> [e-mail]; Geert Uytterhoeven
> <[e-mail]>; Chris Paterson
> <[e-mail]>; Thomas Gleixner <[e-mail]>;
> John Stultz <[e-mail]>; Fabrizio Castro
> <[e-mail]>
> Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device nodes
>
> On 19/11/2018 16:50, Biju Das wrote:
> > Hi Daniel,
> >
> > Thanks for the feedback.
> >
> >>>> Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
> >>>> nodes
> >>>>
> >>>> On 26/10/2018 10:25, Biju Das wrote:
> >>>>> This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
> >>>>>
> >>>>> Signed-off-by: Biju Das <[e-mail]>
> >>>>> ---
> >>>>> This patch is tested against renesas-dev
> >>>>>
> >>>>> I have executed on inconsistency-check, nanosleep and
> >>>>> clocksource_switch selftests on this arm64 SoC. The
> >>>>> inconsistency-check and nanosleep tests are working fine.The
> >>>>> clocksource_switch asynchronous test is failing due to
> >>>>> inconsistency-check
> >>>> failure on "arch_sys_counter".
> >>>>>
> >>>>> But if i skip the clocksource_switching of "arch_sys_counter", the
> >>>>> asynchronous test is passing for CMT0/1/2/3 timer.
> >>>>>
> >>>>> Has any one noticed this issue?
> >>>>
> >>>> So now that you mention that, I've been through the
> >>>> clocksource_switch on another ARM64 platform (hikey960) and
> >>>> disabled the
> >>>> ARM64_ERRATUM_858921 config option. I can see the same issue.
> >>>>
> >>>> Is this option set on your config ?
> >>>
> >>> No.  As per  " config ARM64_ERRATUM_858921", it is "Workaround for
> >> Cortex-A73 erratum 858921"
> >>>
> >>> Our SoC is 2xCA-57 + 4 x CA-53.  Does  it impact CA-57 + CA_53?
> >>
> >> Dunno :/
> >>
> >>> Any way I will enable this config option and will provide you the results.
> >>
> >> Ok, thanks!
> >
> > The following config is enabled by default on upstream
> > kernel(4.20-rc3) CONFIG_ARM_ARCH_TIMER_EVTSTREAM=y
> > CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND=y
> > CONFIG_FSL_ERRATUM_A008585=y
> > CONFIG_HISILICON_ERRATUM_161010101=y
> > CONFIG_ARM64_ERRATUM_858921=y
> >
> > For a quick testing,  I have activated the erratum using the property
> "fsl,erratum-a008585" on device tree.
> > With this I confirm the issue is fixed.
> >
> > I have  some questions on this.
> > 1) Based  on the test result ,do you think renesas soc also  impacted by the
> ARM64_ERRATUM_858921?
> > 2) Is there any way to find, is this Erratum actually causing the
> asynchronous test to fail?
>
> I guess, you can hack the __fsl_a008585_read_reg macro and check if the
> invalid condition is reached.
>
> This thread https://lkml.org/lkml/2018/5/10/773 will give you all the answers
> you are looking for (well very likely).
>
> Let me know if it helped.

In our case , Delta: 174760 ns

1530553351:205762284
1530553351:205762404
--------------------
1530553351:205951226
1530553351:205776466
--------------------

I have tried the workaround for ARM64_ERRATUM_858921, that also fixes the issue.

But all the workaround disables ARM64 VDSO. How do we conclude that is it VDSO issue or ARM64_ERRATUM issue?

Please share your ideas.

Regards,
Biju

> > timer {
> >                 compatible = "arm,armv8-timer";
> >                 interrupts-extended = <&gic GIC_PPI 13
> (GIC_CPU_MASK_SIMPLE(6) | IRQ_TYPE_LEVEL_LOW)>,
> >                                       <&gic GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(6) |
> IRQ_TYPE_LEVEL_LOW)>,
> >                                       <&gic GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(6) |
> IRQ_TYPE_LEVEL_LOW)>,
> >                                       <&gic GIC_PPI 10
> > (GIC_CPU_MASK_SIMPLE(6) | IRQ_TYPE_LEVEL_LOW)>;
> > +                fsl,erratum-a008585;
> > }
>
>
>
>
> --
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-
> blog/> Blog




Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Daniel Lezcano Nov. 22, 2018, 1:47 p.m. UTC | #13

On 22/11/2018 10:46, Biju Das wrote:
> Hello Daniel,
> 
> Thanks for the feedback.
> 
>> -----Original Message-----
>> From: Daniel Lezcano <[e-mail]>
>> Sent: 19 November 2018 17:15
>> To: Biju Das <[e-mail]>; Rob Herring
>> <[e-mail]>; Mark Rutland <[e-mail]>
>> Cc: Simon Horman <[e-mail]>; Magnus Damm
>> <[e-mail]>; [e-mail];
>> [e-mail]; Geert Uytterhoeven
>> <[e-mail]>; Chris Paterson
>> <[e-mail]>; Thomas Gleixner <[e-mail]>;
>> John Stultz <[e-mail]>; Fabrizio Castro
>> <[e-mail]>
>> Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device nodes
>>
>> On 19/11/2018 16:50, Biju Das wrote:
>>> Hi Daniel,
>>>
>>> Thanks for the feedback.
>>>
>>>>>> Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
>>>>>> nodes
>>>>>>
>>>>>> On 26/10/2018 10:25, Biju Das wrote:
>>>>>>> This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
>>>>>>>
>>>>>>> Signed-off-by: Biju Das <[e-mail]>
>>>>>>> ---
>>>>>>> This patch is tested against renesas-dev
>>>>>>>
>>>>>>> I have executed on inconsistency-check, nanosleep and
>>>>>>> clocksource_switch selftests on this arm64 SoC. The
>>>>>>> inconsistency-check and nanosleep tests are working fine.The
>>>>>>> clocksource_switch asynchronous test is failing due to
>>>>>>> inconsistency-check
>>>>>> failure on "arch_sys_counter".
>>>>>>>
>>>>>>> But if i skip the clocksource_switching of "arch_sys_counter", the
>>>>>>> asynchronous test is passing for CMT0/1/2/3 timer.
>>>>>>>
>>>>>>> Has any one noticed this issue?
>>>>>>
>>>>>> So now that you mention that, I've been through the
>>>>>> clocksource_switch on another ARM64 platform (hikey960) and
>>>>>> disabled the
>>>>>> ARM64_ERRATUM_858921 config option. I can see the same issue.
>>>>>>
>>>>>> Is this option set on your config ?
>>>>>
>>>>> No.  As per  " config ARM64_ERRATUM_858921", it is "Workaround for
>>>> Cortex-A73 erratum 858921"
>>>>>
>>>>> Our SoC is 2xCA-57 + 4 x CA-53.  Does  it impact CA-57 + CA_53?
>>>>
>>>> Dunno :/
>>>>
>>>>> Any way I will enable this config option and will provide you the results.
>>>>
>>>> Ok, thanks!
>>>
>>> The following config is enabled by default on upstream
>>> kernel(4.20-rc3) CONFIG_ARM_ARCH_TIMER_EVTSTREAM=y
>>> CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND=y
>>> CONFIG_FSL_ERRATUM_A008585=y
>>> CONFIG_HISILICON_ERRATUM_161010101=y
>>> CONFIG_ARM64_ERRATUM_858921=y
>>>
>>> For a quick testing,  I have activated the erratum using the property
>> "fsl,erratum-a008585" on device tree.
>>> With this I confirm the issue is fixed.
>>>
>>> I have  some questions on this.
>>> 1) Based  on the test result ,do you think renesas soc also  impacted by the
>> ARM64_ERRATUM_858921?
>>> 2) Is there any way to find, is this Erratum actually causing the
>> asynchronous test to fail?
>>
>> I guess, you can hack the __fsl_a008585_read_reg macro and check if the
>> invalid condition is reached.
>>
>> This thread https://lkml.org/lkml/2018/5/10/773 will give you all the answers
>> you are looking for (well very likely).
>>
>> Let me know if it helped.
> 
> In our case , Delta: 174760 ns
> 
> 1530553351:205762284
> 1530553351:205762404
> --------------------
> 1530553351:205951226
> 1530553351:205776466
> --------------------
> 
> I have tried the workaround for ARM64_ERRATUM_858921, that also fixes the issue.
> 
> But all the workaround disables ARM64 VDSO. How do we conclude that is it VDSO issue or ARM64_ERRATUM issue?

May be disable all errata and set vdso_default to false?

[ ... ]

-static bool vdso_default = true;
+static bool vdso_default = false;

[ ... ]

>>> timer {
>>>                 compatible = "arm,armv8-timer";
>>>                 interrupts-extended = <&gic GIC_PPI 13
>> (GIC_CPU_MASK_SIMPLE(6) | IRQ_TYPE_LEVEL_LOW)>,
>>>                                       <&gic GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(6) |
>> IRQ_TYPE_LEVEL_LOW)>,
>>>                                       <&gic GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(6) |
>> IRQ_TYPE_LEVEL_LOW)>,
>>>                                       <&gic GIC_PPI 10
>>> (GIC_CPU_MASK_SIMPLE(6) | IRQ_TYPE_LEVEL_LOW)>;
>>> +                fsl,erratum-a008585;
>>> }
>>
>>
>>
>>
>> --
>>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>>
>> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
>> <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-
>> blog/> Blog
> 
> 
> 
> 
> Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
>

Biju Das Nov. 22, 2018, 3:16 p.m. UTC | #14

Hello Daniel,

Thanks for the feedback.

> >> Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
> >> nodes
> >>
> >> On 19/11/2018 16:50, Biju Das wrote:
> >>> Hi Daniel,
> >>>
> >>> Thanks for the feedback.
> >>>
> >>>>>> Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
> >>>>>> nodes
> >>>>>>
> >>>>>> On 26/10/2018 10:25, Biju Das wrote:
> >>>>>>> This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
> >>>>>>>
> >>>>>>> Signed-off-by: Biju Das <[e-mail]>
> >>>>>>> ---
> >>>>>>> This patch is tested against renesas-dev
> >>>>>>>
> >>>>>>> I have executed on inconsistency-check, nanosleep and
> >>>>>>> clocksource_switch selftests on this arm64 SoC. The
> >>>>>>> inconsistency-check and nanosleep tests are working fine.The
> >>>>>>> clocksource_switch asynchronous test is failing due to
> >>>>>>> inconsistency-check
> >>>>>> failure on "arch_sys_counter".
> >>>>>>>
> >>>>>>> But if i skip the clocksource_switching of "arch_sys_counter",
> >>>>>>> the asynchronous test is passing for CMT0/1/2/3 timer.
> >>>>>>>
> >>>>>>> Has any one noticed this issue?
> >>>>>>
> >>>>>> So now that you mention that, I've been through the
> >>>>>> clocksource_switch on another ARM64 platform (hikey960) and
> >>>>>> disabled the
> >>>>>> ARM64_ERRATUM_858921 config option. I can see the same issue.
> >>>>>>
> >>>>>> Is this option set on your config ?
> >>>>>
> >>>>> No.  As per  " config ARM64_ERRATUM_858921", it is "Workaround for
> >>>> Cortex-A73 erratum 858921"
> >>>>>
> >>>>> Our SoC is 2xCA-57 + 4 x CA-53.  Does  it impact CA-57 + CA_53?
> >>>>
> >>>> Dunno :/
> >>>>
> >>>>> Any way I will enable this config option and will provide you the
> results.
> >>>>
> >>>> Ok, thanks!
> >>>
> >>> The following config is enabled by default on upstream
> >>> kernel(4.20-rc3) CONFIG_ARM_ARCH_TIMER_EVTSTREAM=y
> >>> CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND=y
> >>> CONFIG_FSL_ERRATUM_A008585=y
> >>> CONFIG_HISILICON_ERRATUM_161010101=y
> >>> CONFIG_ARM64_ERRATUM_858921=y
> >>>
> >>> For a quick testing,  I have activated the erratum using the
> >>> property
> >> "fsl,erratum-a008585" on device tree.
> >>> With this I confirm the issue is fixed.
> >>>
> >>> I have  some questions on this.
> >>> 1) Based  on the test result ,do you think renesas soc also
> >>> impacted by the
> >> ARM64_ERRATUM_858921?
> >>> 2) Is there any way to find, is this Erratum actually causing the
> >> asynchronous test to fail?
> >>
> >> I guess, you can hack the __fsl_a008585_read_reg macro and check if
> >> the invalid condition is reached.
> >>
> >> This thread https://lkml.org/lkml/2018/5/10/773 will give you all the
> >> answers you are looking for (well very likely).
> >>
> >> Let me know if it helped.
> >
> > In our case , Delta: 174760 ns
> >
> > 1530553351:205762284
> > 1530553351:205762404
> > --------------------
> > 1530553351:205951226
> > 1530553351:205776466
> > --------------------
> >
> > I have tried the workaround for ARM64_ERRATUM_858921, that also fixes
> the issue.
> >
> > But all the workaround disables ARM64 VDSO. How do we conclude that is
> it VDSO issue or ARM64_ERRATUM issue?
>
> May be disable all errata and set vdso_default to false?
>
> [ ... ]
>
> -static bool vdso_default = true;
> +static bool vdso_default = false;
>
> [ ... ]

I have disabled the  activation of errata from device tree and set vdso_default=false.
With this also it works fine. So looks like arm64 vdso is the issue in our case.

Do you agree with the conclusion?

[    0.000000] rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=6
[    0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
[    0.000000] arch_timer: cp15 timer(s) running at 8.32MHz (virt).
[    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff max_cycles: 0x1eb398c07, max_idle_ns: 440795202503 ns
[    0.000004] sched_clock: 56 bits at 8MHz, resolution 120ns, wraps every 2199023255503ns

regards,
 Biju



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Daniel Lezcano Nov. 22, 2018, 3:30 p.m. UTC | #15

Added Marc Zyngier in Cc.

On 22/11/2018 16:16, Biju Das wrote:
> Hello Daniel,
> 
> Thanks for the feedback.
> 
>>>> Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
>>>> nodes
>>>>
>>>> On 19/11/2018 16:50, Biju Das wrote:
>>>>> Hi Daniel,
>>>>>
>>>>> Thanks for the feedback.
>>>>>
>>>>>>>> Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
>>>>>>>> nodes
>>>>>>>>
>>>>>>>> On 26/10/2018 10:25, Biju Das wrote:
>>>>>>>>> This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Biju Das <[e-mail]>
>>>>>>>>> ---
>>>>>>>>> This patch is tested against renesas-dev
>>>>>>>>>
>>>>>>>>> I have executed on inconsistency-check, nanosleep and
>>>>>>>>> clocksource_switch selftests on this arm64 SoC. The
>>>>>>>>> inconsistency-check and nanosleep tests are working fine.The
>>>>>>>>> clocksource_switch asynchronous test is failing due to
>>>>>>>>> inconsistency-check
>>>>>>>> failure on "arch_sys_counter".
>>>>>>>>>
>>>>>>>>> But if i skip the clocksource_switching of "arch_sys_counter",
>>>>>>>>> the asynchronous test is passing for CMT0/1/2/3 timer.
>>>>>>>>>
>>>>>>>>> Has any one noticed this issue?
>>>>>>>>
>>>>>>>> So now that you mention that, I've been through the
>>>>>>>> clocksource_switch on another ARM64 platform (hikey960) and
>>>>>>>> disabled the
>>>>>>>> ARM64_ERRATUM_858921 config option. I can see the same issue.
>>>>>>>>
>>>>>>>> Is this option set on your config ?
>>>>>>>
>>>>>>> No.  As per  " config ARM64_ERRATUM_858921", it is "Workaround for
>>>>>> Cortex-A73 erratum 858921"
>>>>>>>
>>>>>>> Our SoC is 2xCA-57 + 4 x CA-53.  Does  it impact CA-57 + CA_53?
>>>>>>
>>>>>> Dunno :/
>>>>>>
>>>>>>> Any way I will enable this config option and will provide you the
>> results.
>>>>>>
>>>>>> Ok, thanks!
>>>>>
>>>>> The following config is enabled by default on upstream
>>>>> kernel(4.20-rc3) CONFIG_ARM_ARCH_TIMER_EVTSTREAM=y
>>>>> CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND=y
>>>>> CONFIG_FSL_ERRATUM_A008585=y
>>>>> CONFIG_HISILICON_ERRATUM_161010101=y
>>>>> CONFIG_ARM64_ERRATUM_858921=y
>>>>>
>>>>> For a quick testing,  I have activated the erratum using the
>>>>> property
>>>> "fsl,erratum-a008585" on device tree.
>>>>> With this I confirm the issue is fixed.
>>>>>
>>>>> I have  some questions on this.
>>>>> 1) Based  on the test result ,do you think renesas soc also
>>>>> impacted by the
>>>> ARM64_ERRATUM_858921?
>>>>> 2) Is there any way to find, is this Erratum actually causing the
>>>> asynchronous test to fail?
>>>>
>>>> I guess, you can hack the __fsl_a008585_read_reg macro and check if
>>>> the invalid condition is reached.
>>>>
>>>> This thread https://lkml.org/lkml/2018/5/10/773 will give you all the
>>>> answers you are looking for (well very likely).
>>>>
>>>> Let me know if it helped.
>>>
>>> In our case , Delta: 174760 ns
>>>
>>> 1530553351:205762284
>>> 1530553351:205762404
>>> --------------------
>>> 1530553351:205951226
>>> 1530553351:205776466
>>> --------------------
>>>
>>> I have tried the workaround for ARM64_ERRATUM_858921, that also fixes
>> the issue.
>>>
>>> But all the workaround disables ARM64 VDSO. How do we conclude that is
>> it VDSO issue or ARM64_ERRATUM issue?
>>
>> May be disable all errata and set vdso_default to false?
>>
>> [ ... ]
>>
>> -static bool vdso_default = true;
>> +static bool vdso_default = false;
>>
>> [ ... ]
> 
> I have disabled the  activation of errata from device tree and set vdso_default=false.
> With this also it works fine. So looks like arm64 vdso is the issue in our case.
> 
> Do you agree with the conclusion?

I agree we have an element to investigate the issue. That is very
specific to this timer and a good knowledge of the internals is required.

Certainly Marc Zyngier or Mark Rutland can help here.

> [    0.000000] rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=6
> [    0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
> [    0.000000] arch_timer: cp15 timer(s) running at 8.32MHz (virt).
> [    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff max_cycles: 0x1eb398c07, max_idle_ns: 440795202503 ns
> [    0.000004] sched_clock: 56 bits at 8MHz, resolution 120ns, wraps every 2199023255503ns

Marc Zyngier Nov. 22, 2018, 3:55 p.m. UTC | #16

On 22/11/2018 15:30, Daniel Lezcano wrote:
> 
> Added Marc Zyngier in Cc.

Thanks for looping me in.

> 
> On 22/11/2018 16:16, Biju Das wrote:
>> Hello Daniel,
>>
>> Thanks for the feedback.
>>
>>>>> Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
>>>>> nodes
>>>>>
>>>>> On 19/11/2018 16:50, Biju Das wrote:
>>>>>> Hi Daniel,
>>>>>>
>>>>>> Thanks for the feedback.
>>>>>>
>>>>>>>>> Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
>>>>>>>>> nodes
>>>>>>>>>
>>>>>>>>> On 26/10/2018 10:25, Biju Das wrote:
>>>>>>>>>> This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Biju Das <[e-mail]>
>>>>>>>>>> ---
>>>>>>>>>> This patch is tested against renesas-dev
>>>>>>>>>>
>>>>>>>>>> I have executed on inconsistency-check, nanosleep and
>>>>>>>>>> clocksource_switch selftests on this arm64 SoC. The
>>>>>>>>>> inconsistency-check and nanosleep tests are working fine.The
>>>>>>>>>> clocksource_switch asynchronous test is failing due to
>>>>>>>>>> inconsistency-check
>>>>>>>>> failure on "arch_sys_counter".
>>>>>>>>>>
>>>>>>>>>> But if i skip the clocksource_switching of "arch_sys_counter",
>>>>>>>>>> the asynchronous test is passing for CMT0/1/2/3 timer.
>>>>>>>>>>
>>>>>>>>>> Has any one noticed this issue?
>>>>>>>>>
>>>>>>>>> So now that you mention that, I've been through the
>>>>>>>>> clocksource_switch on another ARM64 platform (hikey960) and
>>>>>>>>> disabled the
>>>>>>>>> ARM64_ERRATUM_858921 config option. I can see the same issue.
>>>>>>>>>
>>>>>>>>> Is this option set on your config ?
>>>>>>>>
>>>>>>>> No.  As per  " config ARM64_ERRATUM_858921", it is "Workaround for
>>>>>>> Cortex-A73 erratum 858921"
>>>>>>>>
>>>>>>>> Our SoC is 2xCA-57 + 4 x CA-53.  Does  it impact CA-57 + CA_53?

Erratum 858921 is strictly an A73 thing, and neither A57 nor A53 suffer
from this issue. If you're seeing something that looks similar, that's
probably because the integration of the global counter is less than
perfect (insert massive understatement here), similar to the way it is
broken with FSL's A008585 erratum.

>>>>>>>
>>>>>>> Dunno :/
>>>>>>>
>>>>>>>> Any way I will enable this config option and will provide you the
>>> results.
>>>>>>>
>>>>>>> Ok, thanks!
>>>>>>
>>>>>> The following config is enabled by default on upstream
>>>>>> kernel(4.20-rc3) CONFIG_ARM_ARCH_TIMER_EVTSTREAM=y
>>>>>> CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND=y
>>>>>> CONFIG_FSL_ERRATUM_A008585=y
>>>>>> CONFIG_HISILICON_ERRATUM_161010101=y
>>>>>> CONFIG_ARM64_ERRATUM_858921=y
>>>>>>
>>>>>> For a quick testing,  I have activated the erratum using the
>>>>>> property
>>>>> "fsl,erratum-a008585" on device tree.
>>>>>> With this I confirm the issue is fixed.
>>>>>>
>>>>>> I have  some questions on this.
>>>>>> 1) Based  on the test result ,do you think renesas soc also
>>>>>> impacted by the
>>>>> ARM64_ERRATUM_858921?
>>>>>> 2) Is there any way to find, is this Erratum actually causing the
>>>>> asynchronous test to fail?
>>>>>
>>>>> I guess, you can hack the __fsl_a008585_read_reg macro and check if
>>>>> the invalid condition is reached.
>>>>>
>>>>> This thread https://lkml.org/lkml/2018/5/10/773 will give you all the
>>>>> answers you are looking for (well very likely).
>>>>>
>>>>> Let me know if it helped.
>>>>
>>>> In our case , Delta: 174760 ns
>>>>
>>>> 1530553351:205762284
>>>> 1530553351:205762404
>>>> --------------------
>>>> 1530553351:205951226
>>>> 1530553351:205776466
>>>> --------------------
>>>>
>>>> I have tried the workaround for ARM64_ERRATUM_858921, that also fixes
>>> the issue.
>>>>
>>>> But all the workaround disables ARM64 VDSO. How do we conclude that is
>>> it VDSO issue or ARM64_ERRATUM issue?

The VDSO is just a userspace proxy for the counter, and nothing else. If
the counter is broken, we have no choice but to disable the VDSO and get
everyone on the slow path.

>>>
>>> May be disable all errata and set vdso_default to false?
>>>
>>> [ ... ]
>>>
>>> -static bool vdso_default = true;
>>> +static bool vdso_default = false;
>>>
>>> [ ... ]
>>
>> I have disabled the  activation of errata from device tree and set vdso_default=false.
>> With this also it works fine. So looks like arm64 vdso is the issue in our case.

That doesn't make much sense, as all it means is that you take an extra
trap to access the exact same data.

>>
>> Do you agree with the conclusion?
> 
> I agree we have an element to investigate the issue. That is very
> specific to this timer and a good knowledge of the internals is required.
> 
> Certainly Marc Zyngier or Mark Rutland can help here.
> 
>> [    0.000000] rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=6
>> [    0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
>> [    0.000000] arch_timer: cp15 timer(s) running at 8.32MHz (virt).
>> [    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff max_cycles: 0x1eb398c07, max_idle_ns: 440795202503 ns
>> [    0.000004] sched_clock: 56 bits at 8MHz, resolution 120ns, wraps every 2199023255503ns

Now that is a bit more interesting: How is this kernel booted? The fact
that is uses the virtual timer and not the physical one makes me think
that it is booted at EL1 instead of EL2. The £100 question is whether
CNTVOFF_EL2 is consistently set to the same value across CPUs.

Thanks,

	M.

Simon Horman Nov. 23, 2018, 12:37 p.m. UTC | #17

On Wed, Nov 21, 2018 at 10:27:41AM +0000, Biju Das wrote:
> Hi Simon,
> 
> Thanks for the feedback.
> 
> > -----Original Message-----
> > From: Simon Horman <[e-mail]>
> > Sent: 21 November 2018 10:24
> > To: Biju Das <[e-mail]>
> > Cc: Rob Herring <[e-mail]>; Mark Rutland
> > <[e-mail]>; Magnus Damm <[e-mail]>;
> > [e-mail]; [e-mail]; Geert
> > Uytterhoeven <[e-mail]>; Chris Paterson
> > <[e-mail]>; Daniel Lezcano
> > <[e-mail]>; Thomas Gleixner <[e-mail]>; John
> > Stultz <[e-mail]>; Fabrizio Castro
> > <[e-mail]>
> > Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device nodes
> >
> > On Fri, Oct 26, 2018 at 09:25:07AM +0100, Biju Das wrote:
> > > This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
> > >
> > > Signed-off-by: Biju Das <[e-mail]>
> > > ---
> > > This patch is tested against renesas-dev
> > >
> > > I have executed on inconsistency-check, nanosleep and
> > > clocksource_switch selftests on this arm64 SoC. The
> > > inconsistency-check and nanosleep tests are working fine.The
> > > clocksource_switch asynchronous test is failing due to inconsistency-check
> > failure on "arch_sys_counter".
> > >
> > > But if i skip the clocksource_switching of "arch_sys_counter", the
> > > asynchronous test is passing for CMT0/1/2/3 timer.
> > >
> > > Has any one noticed this issue?
> >
> > I am reluctant to apply this patch until there is a better understanding of the
> > issue above.
> 
> If I understand correctly this patch has no issue at all. The problem is related to ARM architecture  timer.
> Please correct me if you think otherwise.

Thanks for the clarification, I have applied this patch for v4.21.

Geert Uytterhoeven Jan. 24, 2019, 10:15 a.m. UTC | #18

Hi Biju,

On Fri, Oct 26, 2018 at 10:32 AM Biju Das <[e-mail]> wrote:
> This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
>
> Signed-off-by: Biju Das <[e-mail]>
> ---
> This patch is tested against renesas-dev
>
> I have executed on inconsistency-check, nanosleep and clocksource_switch
> selftests on this arm64 SoC. The inconsistency-check and nanosleep tests
> are working fine.The clocksource_switch asynchronous test is failing due
> to inconsistency-check failure on "arch_sys_counter".
>
> But if i skip the clocksource_switching of "arch_sys_counter", the
> asynchronous test is passing for CMT0/1/2/3 timer.
>
> Has any one noticed this issue?

clockevents/next now has commit 7cd6dca3600d8d71
("clocksource/drivers/arch_timer: Workaround for Allwinner A64 timer
instability").  Perhaps this is related, and the same test program may
indicate similar issues?

See also https://lore.kernel.org/lkml/[e-mail]/

Gr{oetje,eeting}s,

                        Geert

Biju Das Jan. 25, 2019, 11:30 a.m. UTC | #19

Hi Geert,

Thanks for the feedback. I started testing CMT on latest kernel(Linux-next-20190125 and also renesas-dev) and found that it is broken on R-Car M3-W device.

On further investigation the patch (" 8234f6734c5d74ac794e5517437f51c57d65f865"   PM-runtime: Switch autosuspend over to using hrtimers)  is causing the issue.

During clock source switching, It calls the function "sh_cmt_enable"  which calls " pm_runtime_get_sync(&ch->cmt->pdev->dev);"  and after that console freezes.

Sergei: Have you noticed this issue  on R-Car V3M and V3H boards with latest kernel?

Regards,
Biju

> -----Original Message-----
> From: [e-mail] <linux-renesas-soc-
> [e-mail]> On Behalf Of Geert Uytterhoeven
> Sent: 24 January 2019 10:16
> To: Biju Das <[e-mail]>
> Cc: Rob Herring <[e-mail]>; Mark Rutland
> <[e-mail]>; Simon Horman <[e-mail]>; Magnus
> Damm <[e-mail]>; Linux-Renesas <linux-renesas-
> [e-mail]>; open list:OPEN FIRMWARE AND FLATTENED DEVICE
> TREE BINDINGS <[e-mail]>; Geert Uytterhoeven
> <[e-mail]>; Chris Paterson
> <[e-mail]>; Daniel Lezcano
> <[e-mail]>; Thomas Gleixner <[e-mail]>; John
> Stultz <[e-mail]>; Fabrizio Castro
> <[e-mail]>; Samuel Holland <[e-mail]>
> Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device nodes
>
> Hi Biju,
>
> On Fri, Oct 26, 2018 at 10:32 AM Biju Das <[e-mail]> wrote:
> > This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
> >
> > Signed-off-by: Biju Das <[e-mail]>
> > ---
> > This patch is tested against renesas-dev
> >
> > I have executed on inconsistency-check, nanosleep and
> > clocksource_switch selftests on this arm64 SoC. The
> > inconsistency-check and nanosleep tests are working fine.The
> > clocksource_switch asynchronous test is failing due to inconsistency-check
> failure on "arch_sys_counter".
> >
> > But if i skip the clocksource_switching of "arch_sys_counter", the
> > asynchronous test is passing for CMT0/1/2/3 timer.
> >
> > Has any one noticed this issue?
>
> clockevents/next now has commit 7cd6dca3600d8d71
> ("clocksource/drivers/arch_timer: Workaround for Allwinner A64 timer
> instability").  Perhaps this is related, and the same test program may indicate
> similar issues?
>
> See also https://lore.kernel.org/lkml/20190113021719.46457-2-
> [e-mail]/
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Biju Das Jan. 25, 2019, 12:26 p.m. UTC | #20

Hi All,

I have enabled lock debugging  and it is showing "possible recursive locking detected "with the patch ("8234f6734c5d74ac794e5517437f51c57d65f865"   PM-runtime: Switch autosuspend over to using hrtimers)

root@salvator-x:~# echo  e60f0000.timer > /sys/devices/system/clocksource/clocksource0/current_clocksource
[   67.995854]
[   67.997354] ============================================
[   68.002663] WARNING: possible recursive locking detected
[   68.007974] 5.0.0-rc3-next-20190125-00005-g382822b #31 Not tainted
[   68.014150] --------------------------------------------
[   68.019459] migration/0/11 is trying to acquire lock:
[   68.024509] 0000000065de5fdf (tk_core.seq){----}, at: rpm_resume+0x5f0/0x7a8
[   68.031570]
[   68.031570] but task is already holding lock:
[   68.037399] 0000000065de5fdf (tk_core.seq){----}, at: multi_cpu_stop+0x8c/0x140
[   68.044714]
[   68.044714] other info that might help us debug this:
[   68.051237]  Possible unsafe locking scenario:
[   68.051237]
[   68.057153]        CPU0
[   68.059594]        ----
[   68.062034]   lock(tk_core.seq);
[   68.065258]   lock(tk_core.seq);
[   68.068482]
[   68.068482]  *** DEADLOCK ***
[   68.068482]
[   68.074402]  May be due to missing lock nesting notation
[   68.074402]
[   68.081188] 4 locks held by migration/0/11:
[   68.085366]  #0: 00000000463b6446 (timekeeper_lock){-.-.}, at: change_clocksource+0x2c/0x118
[   68.093812]  #1: 0000000065de5fdf (tk_core.seq){----}, at: multi_cpu_stop+0x8c/0x140
[   68.101558]  #2: 0000000098b45550 (&ch->lock){....}, at: sh_cmt_start+0x28/0x200
[   68.108961]  #3: 00000000da0f4e80 (&(&dev->power.lock)->rlock){-...}, at: __rpm_callback+0xbc/0x1e8
[   68.118011]
[   68.118011] stack backtrace:
[   68.122368] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.0.0-rc3-next-20190125-00005-g382822b #31
[   68.131412] Hardware name: Renesas Salvator-X 2nd version board based on r8a7796 (DT)
[   68.139240] Call trace:
[   68.141687]  dump_backtrace+0x0/0x178
[   68.145346]  show_stack+0x14/0x20
[   68.148661]  dump_stack+0xb0/0xec
[   68.151975]  __lock_acquire+0xfb4/0x1c08
[   68.155894]  lock_acquire+0xd0/0x268
[   68.159467]  ktime_get+0x5c/0x108
[   68.162779]  rpm_resume+0x5f0/0x7a8
[   68.166265]  __pm_runtime_resume+0x50/0x98
[   68.170358]  sh_cmt_start+0x84/0x200
[   68.173931]  sh_cmt_clocksource_enable+0x28/0x48
[   68.178546]  change_clocksource+0x84/0x118
[   68.182639]  multi_cpu_stop+0x8c/0x140
[   68.186385]  cpu_stopper_thread+0xac/0x120
[   68.190481]  smpboot_thread_fn+0x1ac/0x2c8
[   68.194574]  kthread+0x12c/0x130
[   68.197800]  ret_from_fork+0x10/0x18

Regards,
Biju

> -----Original Message-----
> From: Biju Das
> Sent: 25 January 2019 11:27
> To: 'Geert Uytterhoeven' <[e-mail]>; Vincent Guittot
> <[e-mail]>; Sergei Shtylyov
> <[e-mail]>
> Cc: Rob Herring <[e-mail]>; Mark Rutland
> <[e-mail]>; Simon Horman <[e-mail]>; Magnus
> Damm <[e-mail]>; Linux-Renesas <linux-renesas-
> [e-mail]>; open list:OPEN FIRMWARE AND FLATTENED DEVICE
> TREE BINDINGS <[e-mail]>; Geert Uytterhoeven
> <[e-mail]>; Chris Paterson
> <[e-mail]>; Daniel Lezcano
> <[e-mail]>; Thomas Gleixner <[e-mail]>; John
> Stultz <[e-mail]>; Fabrizio Castro
> <[e-mail]>; Samuel Holland <[e-mail]>
> Subject: RE: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device nodes
>
> Hi Geert,
>
> Thanks for the feedback. I started testing CMT on latest kernel(Linux-next-
> 20190125 and also renesas-dev) and found that it is broken on R-Car M3-W
> device.
>
> On further investigation the patch ("
> 8234f6734c5d74ac794e5517437f51c57d65f865"   PM-runtime: Switch
> autosuspend over to using hrtimers)  is causing the issue.
>
> During clock source switching, It calls the function "sh_cmt_enable"  which
> calls " pm_runtime_get_sync(&ch->cmt->pdev->dev);"  and after that
> console freezes.
>
> Sergei: Have you noticed this issue  on R-Car V3M and V3H boards with latest
> kernel?
>
> Regards,
> Biju
>
> > -----Original Message-----
> > From: [e-mail] <linux-renesas-soc-
> > [e-mail]> On Behalf Of Geert Uytterhoeven
> > Sent: 24 January 2019 10:16
> > To: Biju Das <[e-mail]>
> > Cc: Rob Herring <[e-mail]>; Mark Rutland
> > <[e-mail]>; Simon Horman <[e-mail]>;
> Magnus Damm
> > <[e-mail]>; Linux-Renesas <linux-renesas-
> > [e-mail]>; open list:OPEN FIRMWARE AND FLATTENED DEVICE
> > TREE BINDINGS <[e-mail]>; Geert Uytterhoeven
> > <[e-mail]>; Chris Paterson
> > <[e-mail]>; Daniel Lezcano
> > <[e-mail]>; Thomas Gleixner <[e-mail]>;
> > John Stultz <[e-mail]>; Fabrizio Castro
> > <[e-mail]>; Samuel Holland
> <[e-mail]>
> > Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
> > nodes
> >
> > Hi Biju,
> >
> > On Fri, Oct 26, 2018 at 10:32 AM Biju Das <[e-mail]>
> wrote:
> > > This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
> > >
> > > Signed-off-by: Biju Das <[e-mail]>
> > > ---
> > > This patch is tested against renesas-dev
> > >
> > > I have executed on inconsistency-check, nanosleep and
> > > clocksource_switch selftests on this arm64 SoC. The
> > > inconsistency-check and nanosleep tests are working fine.The
> > > clocksource_switch asynchronous test is failing due to
> > > inconsistency-check
> > failure on "arch_sys_counter".
> > >
> > > But if i skip the clocksource_switching of "arch_sys_counter", the
> > > asynchronous test is passing for CMT0/1/2/3 timer.
> > >
> > > Has any one noticed this issue?
> >
> > clockevents/next now has commit 7cd6dca3600d8d71
> > ("clocksource/drivers/arch_timer: Workaround for Allwinner A64 timer
> > instability").  Perhaps this is related, and the same test program may
> > indicate similar issues?
> >
> > See also https://lore.kernel.org/lkml/20190113021719.46457-2-
> > [e-mail]/
> >
> > Gr{oetje,eeting}s,
> >
> >                         Geert
> >
> > --
> > Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> > geert@linux- m68k.org
> >
> > In personal conversations with technical people, I call myself a
> > hacker. But when I'm talking to journalists I just say "programmer" or
> something like that.
> >                                 -- Linus Torvalds



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Vincent Guittot Jan. 25, 2019, 2:06 p.m. UTC | #21

Hi Biju,

It seems that the call of mark_last_busy in rpm_resume raises this problem. We have the sequence:
change_clocksource

    write_seqcount_begin
    ...
    timekeeping_update
        ...
        sh_cmt_clocksource_enable
            ...
            rpm_resume
                pm_runtime_mark_last_busy
                    ktime_get
                        do
                            read_seqcount_begin
                        while read_seqcount_retry
    ....
    write_seqcount_end

In fact, we should be safe because we haven't yet changed the clocksource as
we are enabling the clocksource that will be used for the switch.

Can you try the patch below ?
It uses ktime_get_mono_fast_ns instead which is lock safe for such case

---
 drivers/base/power/runtime.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 457be03..708a13f 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -130,7 +130,7 @@ u64 pm_runtime_autosuspend_expiration(struct device *dev)
 {
 	int autosuspend_delay;
 	u64 last_busy, expires = 0;
-	u64 now = ktime_to_ns(ktime_get());
+	u64 now = ktime_get_mono_fast_ns();
 
 	if (!dev->power.use_autosuspend)
 		goto out;
@@ -909,7 +909,7 @@ static enum hrtimer_restart  pm_suspend_timer_fn(struct hrtimer *timer)
 	 * If 'expires' is after the current time, we've been called
 	 * too early.
 	 */
-	if (expires > 0 && expires < ktime_to_ns(ktime_get())) {
+	if (expires > 0 && expires < ktime_get_mono_fast_ns()) {
 		dev->power.timer_expires = 0;
 		rpm_suspend(dev, dev->power.timer_autosuspends ?
 		    (RPM_ASYNC | RPM_AUTO) : RPM_ASYNC);
@@ -928,7 +928,7 @@ static enum hrtimer_restart  pm_suspend_timer_fn(struct hrtimer *timer)
 int pm_schedule_suspend(struct device *dev, unsigned int delay)
 {
 	unsigned long flags;
-	ktime_t expires;
+	u64 expires;
 	int retval;
 
 	spin_lock_irqsave(&dev->power.lock, flags);
@@ -945,8 +945,8 @@ int pm_schedule_suspend(struct device *dev, unsigned int delay)
 	/* Other scheduled or pending requests need to be canceled. */
 	pm_runtime_cancel_pending(dev);
 
-	expires = ktime_add(ktime_get(), ms_to_ktime(delay));
-	dev->power.timer_expires = ktime_to_ns(expires);
+	expires = ktime_get_mono_fast_ns() + (u64)delay * NSEC_PER_MSEC);
+	dev->power.timer_expires = expires;
 	dev->power.timer_autosuspends = 0;
 	hrtimer_start(&dev->power.suspend_timer, expires, HRTIMER_MODE_ABS);

Biju Das Jan. 25, 2019, 2:29 p.m. UTC | #22

Hi Vincent,

Thanks for the update. I am still seeing the same deadlock.

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index fb5e2b6..a96641a 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -155,7 +155,7 @@ u64 pm_runtime_autosuspend_expiration(struct device *dev)

        expires  = READ_ONCE(dev->power.last_busy);
        expires += (u64)autosuspend_delay * NSEC_PER_MSEC;
-       if (expires > ktime_to_ns(ktime_get()))
+       if (expires > ktime_get_mono_fast_ns())
                return expires; /* Expires in the future */

        return 0;
@@ -921,7 +921,7 @@ static enum hrtimer_restart  pm_suspend_timer_fn(struct hrtimer *timer)
         * If 'expires' is after the current time, we've been called
         * too early.
         */
-       if (expires > 0 && expires < ktime_to_ns(ktime_get())) {
+       if (expires > 0 && expires < ktime_get_mono_fast_ns()) {
                dev->power.timer_expires = 0;
                rpm_suspend(dev, dev->power.timer_autosuspends ?
                    (RPM_ASYNC | RPM_AUTO) : RPM_ASYNC);
@@ -940,7 +940,7 @@ static enum hrtimer_restart  pm_suspend_timer_fn(struct hrtimer *timer)
 int pm_schedule_suspend(struct device *dev, unsigned int delay)
 {
        unsigned long flags;
-       ktime_t expires;
+       u64 expires;
        int retval;

        spin_lock_irqsave(&dev->power.lock, flags);
@@ -957,8 +957,8 @@ int pm_schedule_suspend(struct device *dev, unsigned int delay)
        /* Other scheduled or pending requests need to be canceled. */
        pm_runtime_cancel_pending(dev);

-       expires = ktime_add(ktime_get(), ms_to_ktime(delay));
-       dev->power.timer_expires = ktime_to_ns(expires);
+       expires = ktime_get_mono_fast_ns() + ((u64)delay * NSEC_PER_MSEC);
+       dev->power.timer_expires = expires;
        dev->power.timer_autosuspends = 0;
        hrtimer_start(&dev->power.suspend_timer, expires, HRTIMER_MODE_ABS);


please find the logs.

root@salvator-x:~# echo  e60f0000.timer > /sys/devices/system/clocksource/clocksource0/current_clocksource
[   42.769260]
[   42.770760] ============================================
[   42.776068] WARNING: possible recursive locking detected
[   42.781378] 5.0.0-rc3-next-20190125-00001-gfa6709c-dirty #33 Not tainted
[   42.788076] --------------------------------------------
[   42.793385] migration/0/11 is trying to acquire lock:
[   42.798435] 00000000b43b7183 (tk_core.seq){----}, at: rpm_resume+0x5f0/0x7a8
[   42.805497]
[   42.805497] but task is already holding lock:
[   42.811326] 00000000b43b7183 (tk_core.seq){----}, at: multi_cpu_stop+0x8c/0x140
[   42.818641]
[   42.818641] other info that might help us debug this:
[   42.825165]  Possible unsafe locking scenario:
[   42.825165]
[   42.831080]        CPU0
[   42.833521]        ----
[   42.835962]   lock(tk_core.seq);
[   42.839185]   lock(tk_core.seq);
[   42.842409]
[   42.842409]  *** DEADLOCK ***
[   42.842409]
[   42.848328]  May be due to missing lock nesting notation
[   42.848328]
[   42.855115] 4 locks held by migration/0/11:
[   42.859293]  #0: 00000000a7511731 (timekeeper_lock){-.-.}, at: change_clocksource+0x2c/0x118
[   42.867738]  #1: 00000000b43b7183 (tk_core.seq){----}, at: multi_cpu_stop+0x8c/0x140
[   42.875484]  #2: 00000000b54c38a2 (&ch->lock){....}, at: sh_cmt_start+0x28/0x200
[   42.882887]  #3: 0000000010d14eec (&(&dev->power.lock)->rlock){-...}, at: __rpm_callback+0xbc/0x1e8
[   42.891937]
[   42.891937] stack backtrace:
[   42.896294] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.0.0-rc3-next-20190125-00001-gfa6709c-dirty #33
[   42.905860] Hardware name: Renesas Salvator-X 2nd version board based on r8a7796 (DT)
[   42.913689] Call trace:
[   42.916137]  dump_backtrace+0x0/0x178
[   42.919796]  show_stack+0x14/0x20
[   42.923111]  dump_stack+0xb0/0xec
[   42.926425]  __lock_acquire+0xfb4/0x1c08
[   42.930345]  lock_acquire+0xd0/0x268
[   42.933917]  ktime_get+0x5c/0x108
[   42.937229]  rpm_resume+0x5f0/0x7a8
[   42.940714]  __pm_runtime_resume+0x50/0x98
[   42.944807]  sh_cmt_start+0x84/0x200
[   42.948380]  sh_cmt_clocksource_enable+0x28/0x48
[   42.952995]  change_clocksource+0x84/0x118
[   42.957087]  multi_cpu_stop+0x8c/0x140
[   42.960834]  cpu_stopper_thread+0xac/0x120
[   42.964931]  smpboot_thread_fn+0x1ac/0x2c8
[   42.969024]  kthread+0x12c/0x130
[   42.972249]  ret_from_fork+0x10/0x18

Regards,
Biju

> -----Original Message-----
> From: Vincent Guittot <[e-mail]>
> Sent: 25 January 2019 14:06
> Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device nodes
>
> Hi Biju,
>
> It seems that the call of mark_last_busy in rpm_resume raises this problem.
> We have the sequence:
> change_clocksource
>
>     write_seqcount_begin
>     ...
>     timekeeping_update
>         ...
>         sh_cmt_clocksource_enable
>             ...
>             rpm_resume
>                 pm_runtime_mark_last_busy
>                     ktime_get
>                         do
>                             read_seqcount_begin
>                         while read_seqcount_retry
>     ....
>     write_seqcount_end
>
> In fact, we should be safe because we haven't yet changed the clocksource
> as we are enabling the clocksource that will be used for the switch.
>
> Can you try the patch below ?
> It uses ktime_get_mono_fast_ns instead which is lock safe for such case
>
> ---
>  drivers/base/power/runtime.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 457be03..708a13f 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -130,7 +130,7 @@ u64 pm_runtime_autosuspend_expiration(struct
> device *dev)  {
>  int autosuspend_delay;
>  u64 last_busy, expires = 0;
> -u64 now = ktime_to_ns(ktime_get());
> +u64 now = ktime_get_mono_fast_ns();
>
>  if (!dev->power.use_autosuspend)
>  goto out;
> @@ -909,7 +909,7 @@ static enum hrtimer_restart
> pm_suspend_timer_fn(struct hrtimer *timer)
>   * If 'expires' is after the current time, we've been called
>   * too early.
>   */
> -if (expires > 0 && expires < ktime_to_ns(ktime_get())) {
> +if (expires > 0 && expires < ktime_get_mono_fast_ns()) {
>  dev->power.timer_expires = 0;
>  rpm_suspend(dev, dev->power.timer_autosuspends ?
>      (RPM_ASYNC | RPM_AUTO) : RPM_ASYNC); @@ -928,7
> +928,7 @@ static enum hrtimer_restart  pm_suspend_timer_fn(struct
> hrtimer *timer)  int pm_schedule_suspend(struct device *dev, unsigned int
> delay)  {
>  unsigned long flags;
> -ktime_t expires;
> +u64 expires;
>  int retval;
>
>  spin_lock_irqsave(&dev->power.lock, flags); @@ -945,8 +945,8 @@
> int pm_schedule_suspend(struct device *dev, unsigned int delay)
>  /* Other scheduled or pending requests need to be canceled. */
>  pm_runtime_cancel_pending(dev);
>
> -expires = ktime_add(ktime_get(), ms_to_ktime(delay));
> -dev->power.timer_expires = ktime_to_ns(expires);
> +expires = ktime_get_mono_fast_ns() + (u64)delay *
> NSEC_PER_MSEC);
> +dev->power.timer_expires = expires;
>  dev->power.timer_autosuspends = 0;
>  hrtimer_start(&dev->power.suspend_timer, expires,
> HRTIMER_MODE_ABS);
>
> --
> 2.7.4
>
> Le Friday 25 Jan 2019 à 12:26:54 (+0000), Biju Das a écrit :
> > Hi All,
> >
> > I have enabled lock debugging  and it is showing "possible recursive locking
> detected "with the patch ("8234f6734c5d74ac794e5517437f51c57d65f865"
> PM-runtime: Switch autosuspend over to using hrtimers)
> >
> > root@salvator-x:~# echo  e60f0000.timer >
> /sys/devices/system/clocksource/clocksource0/current_clocksource
> > [   67.995854]
> > [   67.997354] ============================================
> > [   68.002663] WARNING: possible recursive locking detected
> > [   68.007974] 5.0.0-rc3-next-20190125-00005-g382822b #31 Not tainted
> > [   68.014150] --------------------------------------------
> > [   68.019459] migration/0/11 is trying to acquire lock:
> > [   68.024509] 0000000065de5fdf (tk_core.seq){----}, at:
> rpm_resume+0x5f0/0x7a8
> > [   68.031570]
> > [   68.031570] but task is already holding lock:
> > [   68.037399] 0000000065de5fdf (tk_core.seq){----}, at:
> multi_cpu_stop+0x8c/0x140
> > [   68.044714]
> > [   68.044714] other info that might help us debug this:
> > [   68.051237]  Possible unsafe locking scenario:
> > [   68.051237]
> > [   68.057153]        CPU0
> > [   68.059594]        ----
> > [   68.062034]   lock(tk_core.seq);
> > [   68.065258]   lock(tk_core.seq);
> > [   68.068482]
> > [   68.068482]  *** DEADLOCK ***
> > [   68.068482]
> > [   68.074402]  May be due to missing lock nesting notation
> > [   68.074402]
> > [   68.081188] 4 locks held by migration/0/11:
> > [   68.085366]  #0: 00000000463b6446 (timekeeper_lock){-.-.}, at:
> change_clocksource+0x2c/0x118
> > [   68.093812]  #1: 0000000065de5fdf (tk_core.seq){----}, at:
> multi_cpu_stop+0x8c/0x140
> > [   68.101558]  #2: 0000000098b45550 (&ch->lock){....}, at:
> sh_cmt_start+0x28/0x200
> > [   68.108961]  #3: 00000000da0f4e80 (&(&dev->power.lock)->rlock){-...}, at:
> __rpm_callback+0xbc/0x1e8
> > [   68.118011]
> > [   68.118011] stack backtrace:
> > [   68.122368] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.0.0-rc3-next-
> 20190125-00005-g382822b #31
> > [   68.131412] Hardware name: Renesas Salvator-X 2nd version board based
> on r8a7796 (DT)
> > [   68.139240] Call trace:
> > [   68.141687]  dump_backtrace+0x0/0x178
> > [   68.145346]  show_stack+0x14/0x20
> > [   68.148661]  dump_stack+0xb0/0xec
> > [   68.151975]  __lock_acquire+0xfb4/0x1c08
> > [   68.155894]  lock_acquire+0xd0/0x268
> > [   68.159467]  ktime_get+0x5c/0x108
> > [   68.162779]  rpm_resume+0x5f0/0x7a8
> > [   68.166265]  __pm_runtime_resume+0x50/0x98
> > [   68.170358]  sh_cmt_start+0x84/0x200
> > [   68.173931]  sh_cmt_clocksource_enable+0x28/0x48
> > [   68.178546]  change_clocksource+0x84/0x118
> > [   68.182639]  multi_cpu_stop+0x8c/0x140
> > [   68.186385]  cpu_stopper_thread+0xac/0x120
> > [   68.190481]  smpboot_thread_fn+0x1ac/0x2c8
> > [   68.194574]  kthread+0x12c/0x130
> > [   68.197800]  ret_from_fork+0x10/0x18
> >
> > Regards,
> > Biju
> >
> > > -----Original Message-----
> > > From: Biju Das
> > > Sent: 25 January 2019 11:27
> > > To: 'Geert Uytterhoeven' <[e-mail]>; Vincent Guittot
> > > <[e-mail]>; Sergei Shtylyov
> > > <[e-mail]>
> > > Cc: Rob Herring <[e-mail]>; Mark Rutland
> > > <[e-mail]>; Simon Horman <[e-mail]>;
> Magnus
> > > Damm <[e-mail]>; Linux-Renesas <linux-renesas-
> > > [e-mail]>; open list:OPEN FIRMWARE AND FLATTENED
> DEVICE
> > > TREE BINDINGS <[e-mail]>; Geert Uytterhoeven
> > > <[e-mail]>; Chris Paterson
> > > <[e-mail]>; Daniel Lezcano
> > > <[e-mail]>; Thomas Gleixner <[e-mail]>;
> > > John Stultz <[e-mail]>; Fabrizio Castro
> > > <[e-mail]>; Samuel Holland
> > > <[e-mail]>
> > > Subject: RE: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
> > > nodes
> > >
> > > Hi Geert,
> > >
> > > Thanks for the feedback. I started testing CMT on latest
> > > kernel(Linux-next-
> > > 20190125 and also renesas-dev) and found that it is broken on R-Car
> > > M3-W device.
> > >
> > > On further investigation the patch ("
> > > 8234f6734c5d74ac794e5517437f51c57d65f865"   PM-runtime: Switch
> > > autosuspend over to using hrtimers)  is causing the issue.
> > >
> > > During clock source switching, It calls the function "sh_cmt_enable"
> > > which calls " pm_runtime_get_sync(&ch->cmt->pdev->dev);"  and after
> > > that console freezes.
> > >
> > > Sergei: Have you noticed this issue  on R-Car V3M and V3H boards
> > > with latest kernel?
> > >
> > > Regards,
> > > Biju
> > >
> > > > -----Original Message-----
> > > > From: [e-mail] <linux-renesas-soc-
> > > > [e-mail]> On Behalf Of Geert Uytterhoeven
> > > > Sent: 24 January 2019 10:16
> > > > To: Biju Das <[e-mail]>
> > > > Cc: Rob Herring <[e-mail]>; Mark Rutland
> > > > <[e-mail]>; Simon Horman <[e-mail]>;
> > > Magnus Damm
> > > > <[e-mail]>; Linux-Renesas <linux-renesas-
> > > > [e-mail]>; open list:OPEN FIRMWARE AND FLATTENED
> DEVICE
> > > > TREE BINDINGS <[e-mail]>; Geert Uytterhoeven
> > > > <[e-mail]>; Chris Paterson
> > > > <[e-mail]>; Daniel Lezcano
> > > > <[e-mail]>; Thomas Gleixner <[e-mail]>;
> > > > John Stultz <[e-mail]>; Fabrizio Castro
> > > > <[e-mail]>; Samuel Holland
> > > <[e-mail]>
> > > > Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
> > > > nodes
> > > >
> > > > Hi Biju,
> > > >
> > > > On Fri, Oct 26, 2018 at 10:32 AM Biju Das
> > > > <[e-mail]>
> > > wrote:
> > > > > This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
> > > > >
> > > > > Signed-off-by: Biju Das <[e-mail]>
> > > > > ---
> > > > > This patch is tested against renesas-dev
> > > > >
> > > > > I have executed on inconsistency-check, nanosleep and
> > > > > clocksource_switch selftests on this arm64 SoC. The
> > > > > inconsistency-check and nanosleep tests are working fine.The
> > > > > clocksource_switch asynchronous test is failing due to
> > > > > inconsistency-check
> > > > failure on "arch_sys_counter".
> > > > >
> > > > > But if i skip the clocksource_switching of "arch_sys_counter",
> > > > > the asynchronous test is passing for CMT0/1/2/3 timer.
> > > > >
> > > > > Has any one noticed this issue?
> > > >
> > > > clockevents/next now has commit 7cd6dca3600d8d71
> > > > ("clocksource/drivers/arch_timer: Workaround for Allwinner A64
> > > > timer instability").  Perhaps this is related, and the same test
> > > > program may indicate similar issues?
> > > >
> > > > See also https://lore.kernel.org/lkml/20190113021719.46457-2-
> > > > [e-mail]/
> > > >
> > > > Gr{oetje,eeting}s,
> > > >
> > > >                         Geert
> > > >
> > > > --
> > > > Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> > > > geert@linux- m68k.org
> > > >
> > > > In personal conversations with technical people, I call myself a
> > > > hacker. But when I'm talking to journalists I just say
> > > > "programmer" or
> > > something like that.
> > > >                                 -- Linus Torvalds
> >
> >
> >
> > Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne
> End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under
> Registered No. 04586709.



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Biju Das Jan. 25, 2019, 2:44 p.m. UTC | #23

Hi Vincent,

The below patch fixed deadlock.

--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -105,7 +105,7 @@ static inline bool pm_runtime_callbacks_present(struct device *dev)

 static inline void pm_runtime_mark_last_busy(struct device *dev)
 {
-       WRITE_ONCE(dev->power.last_busy, ktime_to_ns(ktime_get()));
+       WRITE_ONCE(dev->power.last_busy, ktime_get_mono_fast_ns());
 }

Regards,
Biju
> -----Original Message-----
> From: Biju Das
> Sent: 25 January 2019 14:26
> To: 'Vincent Guittot' <[e-mail]>
> Cc: Geert Uytterhoeven <[e-mail]>; Sergei Shtylyov
> <[e-mail]>; Rob Herring
> <[e-mail]>; Mark Rutland <[e-mail]>; Simon
> Horman <[e-mail]>; Magnus Damm
> <[e-mail]>; Linux-Renesas <linux-renesas-
> [e-mail]>; open list:OPEN FIRMWARE AND FLATTENED DEVICE
> TREE BINDINGS <[e-mail]>; Geert Uytterhoeven
> <[e-mail]>; Chris Paterson
> <[e-mail]>; Daniel Lezcano
> <[e-mail]>; Thomas Gleixner <[e-mail]>; John
> Stultz <[e-mail]>; Fabrizio Castro
> <[e-mail]>; Samuel Holland <[e-mail]>
> Subject: RE: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device nodes
>
> Hi Vincent,
>
> Thanks for the update. I am still seeing the same deadlock.
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index fb5e2b6..a96641a 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -155,7 +155,7 @@ u64 pm_runtime_autosuspend_expiration(struct
> device *dev)
>
>         expires  = READ_ONCE(dev->power.last_busy);
>         expires += (u64)autosuspend_delay * NSEC_PER_MSEC;
> -       if (expires > ktime_to_ns(ktime_get()))
> +       if (expires > ktime_get_mono_fast_ns())
>                 return expires; /* Expires in the future */
>
>         return 0;
> @@ -921,7 +921,7 @@ static enum hrtimer_restart
> pm_suspend_timer_fn(struct hrtimer *timer)
>          * If 'expires' is after the current time, we've been called
>          * too early.
>          */
> -       if (expires > 0 && expires < ktime_to_ns(ktime_get())) {
> +       if (expires > 0 && expires < ktime_get_mono_fast_ns()) {
>                 dev->power.timer_expires = 0;
>                 rpm_suspend(dev, dev->power.timer_autosuspends ?
>                     (RPM_ASYNC | RPM_AUTO) : RPM_ASYNC); @@ -940,7 +940,7
> @@ static enum hrtimer_restart  pm_suspend_timer_fn(struct hrtimer
> *timer)  int pm_schedule_suspend(struct device *dev, unsigned int delay)  {
>         unsigned long flags;
> -       ktime_t expires;
> +       u64 expires;
>         int retval;
>
>         spin_lock_irqsave(&dev->power.lock, flags); @@ -957,8 +957,8 @@ int
> pm_schedule_suspend(struct device *dev, unsigned int delay)
>         /* Other scheduled or pending requests need to be canceled. */
>         pm_runtime_cancel_pending(dev);
>
> -       expires = ktime_add(ktime_get(), ms_to_ktime(delay));
> -       dev->power.timer_expires = ktime_to_ns(expires);
> +       expires = ktime_get_mono_fast_ns() + ((u64)delay *
> NSEC_PER_MSEC);
> +       dev->power.timer_expires = expires;
>         dev->power.timer_autosuspends = 0;
>         hrtimer_start(&dev->power.suspend_timer, expires,
> HRTIMER_MODE_ABS);
>
>
> please find the logs.
>
> root@salvator-x:~# echo  e60f0000.timer >
> /sys/devices/system/clocksource/clocksource0/current_clocksource
> [   42.769260]
> [   42.770760] ============================================
> [   42.776068] WARNING: possible recursive locking detected
> [   42.781378] 5.0.0-rc3-next-20190125-00001-gfa6709c-dirty #33 Not tainted
> [   42.788076] --------------------------------------------
> [   42.793385] migration/0/11 is trying to acquire lock:
> [   42.798435] 00000000b43b7183 (tk_core.seq){----}, at:
> rpm_resume+0x5f0/0x7a8
> [   42.805497]
> [   42.805497] but task is already holding lock:
> [   42.811326] 00000000b43b7183 (tk_core.seq){----}, at:
> multi_cpu_stop+0x8c/0x140
> [   42.818641]
> [   42.818641] other info that might help us debug this:
> [   42.825165]  Possible unsafe locking scenario:
> [   42.825165]
> [   42.831080]        CPU0
> [   42.833521]        ----
> [   42.835962]   lock(tk_core.seq);
> [   42.839185]   lock(tk_core.seq);
> [   42.842409]
> [   42.842409]  *** DEADLOCK ***
> [   42.842409]
> [   42.848328]  May be due to missing lock nesting notation
> [   42.848328]
> [   42.855115] 4 locks held by migration/0/11:
> [   42.859293]  #0: 00000000a7511731 (timekeeper_lock){-.-.}, at:
> change_clocksource+0x2c/0x118
> [   42.867738]  #1: 00000000b43b7183 (tk_core.seq){----}, at:
> multi_cpu_stop+0x8c/0x140
> [   42.875484]  #2: 00000000b54c38a2 (&ch->lock){....}, at:
> sh_cmt_start+0x28/0x200
> [   42.882887]  #3: 0000000010d14eec (&(&dev->power.lock)->rlock){-...}, at:
> __rpm_callback+0xbc/0x1e8
> [   42.891937]
> [   42.891937] stack backtrace:
> [   42.896294] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.0.0-rc3-next-
> 20190125-00001-gfa6709c-dirty #33
> [   42.905860] Hardware name: Renesas Salvator-X 2nd version board based
> on r8a7796 (DT)
> [   42.913689] Call trace:
> [   42.916137]  dump_backtrace+0x0/0x178
> [   42.919796]  show_stack+0x14/0x20
> [   42.923111]  dump_stack+0xb0/0xec
> [   42.926425]  __lock_acquire+0xfb4/0x1c08
> [   42.930345]  lock_acquire+0xd0/0x268
> [   42.933917]  ktime_get+0x5c/0x108
> [   42.937229]  rpm_resume+0x5f0/0x7a8
> [   42.940714]  __pm_runtime_resume+0x50/0x98
> [   42.944807]  sh_cmt_start+0x84/0x200
> [   42.948380]  sh_cmt_clocksource_enable+0x28/0x48
> [   42.952995]  change_clocksource+0x84/0x118
> [   42.957087]  multi_cpu_stop+0x8c/0x140
> [   42.960834]  cpu_stopper_thread+0xac/0x120
> [   42.964931]  smpboot_thread_fn+0x1ac/0x2c8
> [   42.969024]  kthread+0x12c/0x130
> [   42.972249]  ret_from_fork+0x10/0x18
>
> Regards,
> Biju
>
> > -----Original Message-----
> > From: Vincent Guittot <[e-mail]>
> > Sent: 25 January 2019 14:06
> > Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
> > nodes
> >
> > Hi Biju,
> >
> > It seems that the call of mark_last_busy in rpm_resume raises this
> problem.
> > We have the sequence:
> > change_clocksource
> >
> >     write_seqcount_begin
> >     ...
> >     timekeeping_update
> >         ...
> >         sh_cmt_clocksource_enable
> >             ...
> >             rpm_resume
> >                 pm_runtime_mark_last_busy
> >                     ktime_get
> >                         do
> >                             read_seqcount_begin
> >                         while read_seqcount_retry
> >     ....
> >     write_seqcount_end
> >
> > In fact, we should be safe because we haven't yet changed the
> > clocksource as we are enabling the clocksource that will be used for the
> switch.
> >
> > Can you try the patch below ?
> > It uses ktime_get_mono_fast_ns instead which is lock safe for such
> > case
> >
> > ---
> >  drivers/base/power/runtime.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/base/power/runtime.c
> > b/drivers/base/power/runtime.c index 457be03..708a13f 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -130,7 +130,7 @@ u64 pm_runtime_autosuspend_expiration(struct
> > device *dev)  {
> >  int autosuspend_delay;
> >  u64 last_busy, expires = 0;
> > -u64 now = ktime_to_ns(ktime_get());
> > +u64 now = ktime_get_mono_fast_ns();
> >
> >  if (!dev->power.use_autosuspend)
> >  goto out;
> > @@ -909,7 +909,7 @@ static enum hrtimer_restart
> > pm_suspend_timer_fn(struct hrtimer *timer)
> >   * If 'expires' is after the current time, we've been called
> >   * too early.
> >   */
> > -if (expires > 0 && expires < ktime_to_ns(ktime_get())) {
> > +if (expires > 0 && expires < ktime_get_mono_fast_ns()) {
> >  dev->power.timer_expires = 0;
> >  rpm_suspend(dev, dev->power.timer_autosuspends ?
> >      (RPM_ASYNC | RPM_AUTO) : RPM_ASYNC); @@ -928,7
> > +928,7 @@ static enum hrtimer_restart  pm_suspend_timer_fn(struct
> > hrtimer *timer)  int pm_schedule_suspend(struct device *dev, unsigned
> > int
> > delay)  {
> >  unsigned long flags;
> > -ktime_t expires;
> > +u64 expires;
> >  int retval;
> >
> >  spin_lock_irqsave(&dev->power.lock, flags); @@ -945,8 +945,8 @@
> int
> > pm_schedule_suspend(struct device *dev, unsigned int delay)
> >  /* Other scheduled or pending requests need to be canceled. */
> >  pm_runtime_cancel_pending(dev);
> >
> > -expires = ktime_add(ktime_get(), ms_to_ktime(delay));
> > -dev->power.timer_expires = ktime_to_ns(expires);
> > +expires = ktime_get_mono_fast_ns() + (u64)delay *
> > NSEC_PER_MSEC);
> > +dev->power.timer_expires = expires;
> >  dev->power.timer_autosuspends = 0;
> >  hrtimer_start(&dev->power.suspend_timer, expires,
> HRTIMER_MODE_ABS);
> >
> > --
> > 2.7.4
> >
> > Le Friday 25 Jan 2019 à 12:26:54 (+0000), Biju Das a écrit :
> > > Hi All,
> > >
> > > I have enabled lock debugging  and it is showing "possible recursive
> > > locking
> > detected "with the patch ("8234f6734c5d74ac794e5517437f51c57d65f865"
> > PM-runtime: Switch autosuspend over to using hrtimers)
> > >
> > > root@salvator-x:~# echo  e60f0000.timer >
> > /sys/devices/system/clocksource/clocksource0/current_clocksource
> > > [   67.995854]
> > > [   67.997354] ============================================
> > > [   68.002663] WARNING: possible recursive locking detected
> > > [   68.007974] 5.0.0-rc3-next-20190125-00005-g382822b #31 Not tainted
> > > [   68.014150] --------------------------------------------
> > > [   68.019459] migration/0/11 is trying to acquire lock:
> > > [   68.024509] 0000000065de5fdf (tk_core.seq){----}, at:
> > rpm_resume+0x5f0/0x7a8
> > > [   68.031570]
> > > [   68.031570] but task is already holding lock:
> > > [   68.037399] 0000000065de5fdf (tk_core.seq){----}, at:
> > multi_cpu_stop+0x8c/0x140
> > > [   68.044714]
> > > [   68.044714] other info that might help us debug this:
> > > [   68.051237]  Possible unsafe locking scenario:
> > > [   68.051237]
> > > [   68.057153]        CPU0
> > > [   68.059594]        ----
> > > [   68.062034]   lock(tk_core.seq);
> > > [   68.065258]   lock(tk_core.seq);
> > > [   68.068482]
> > > [   68.068482]  *** DEADLOCK ***
> > > [   68.068482]
> > > [   68.074402]  May be due to missing lock nesting notation
> > > [   68.074402]
> > > [   68.081188] 4 locks held by migration/0/11:
> > > [   68.085366]  #0: 00000000463b6446 (timekeeper_lock){-.-.}, at:
> > change_clocksource+0x2c/0x118
> > > [   68.093812]  #1: 0000000065de5fdf (tk_core.seq){----}, at:
> > multi_cpu_stop+0x8c/0x140
> > > [   68.101558]  #2: 0000000098b45550 (&ch->lock){....}, at:
> > sh_cmt_start+0x28/0x200
> > > [   68.108961]  #3: 00000000da0f4e80 (&(&dev->power.lock)->rlock){-...},
> at:
> > __rpm_callback+0xbc/0x1e8
> > > [   68.118011]
> > > [   68.118011] stack backtrace:
> > > [   68.122368] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.0.0-rc3-
> next-
> > 20190125-00005-g382822b #31
> > > [   68.131412] Hardware name: Renesas Salvator-X 2nd version board
> based
> > on r8a7796 (DT)
> > > [   68.139240] Call trace:
> > > [   68.141687]  dump_backtrace+0x0/0x178
> > > [   68.145346]  show_stack+0x14/0x20
> > > [   68.148661]  dump_stack+0xb0/0xec
> > > [   68.151975]  __lock_acquire+0xfb4/0x1c08
> > > [   68.155894]  lock_acquire+0xd0/0x268
> > > [   68.159467]  ktime_get+0x5c/0x108
> > > [   68.162779]  rpm_resume+0x5f0/0x7a8
> > > [   68.166265]  __pm_runtime_resume+0x50/0x98
> > > [   68.170358]  sh_cmt_start+0x84/0x200
> > > [   68.173931]  sh_cmt_clocksource_enable+0x28/0x48
> > > [   68.178546]  change_clocksource+0x84/0x118
> > > [   68.182639]  multi_cpu_stop+0x8c/0x140
> > > [   68.186385]  cpu_stopper_thread+0xac/0x120
> > > [   68.190481]  smpboot_thread_fn+0x1ac/0x2c8
> > > [   68.194574]  kthread+0x12c/0x130
> > > [   68.197800]  ret_from_fork+0x10/0x18
> > >
> > > Regards,
> > > Biju
> > >
> > > > -----Original Message-----
> > > > From: Biju Das
> > > > Sent: 25 January 2019 11:27
> > > > To: 'Geert Uytterhoeven' <[e-mail]>; Vincent Guittot
> > > > <[e-mail]>; Sergei Shtylyov
> > > > <[e-mail]>
> > > > Cc: Rob Herring <[e-mail]>; Mark Rutland
> > > > <[e-mail]>; Simon Horman <[e-mail]>;
> > Magnus
> > > > Damm <[e-mail]>; Linux-Renesas <linux-renesas-
> > > > [e-mail]>; open list:OPEN FIRMWARE AND FLATTENED
> > DEVICE
> > > > TREE BINDINGS <[e-mail]>; Geert Uytterhoeven
> > > > <[e-mail]>; Chris Paterson
> > > > <[e-mail]>; Daniel Lezcano
> > > > <[e-mail]>; Thomas Gleixner <[e-mail]>;
> > > > John Stultz <[e-mail]>; Fabrizio Castro
> > > > <[e-mail]>; Samuel Holland
> > > > <[e-mail]>
> > > > Subject: RE: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
> > > > nodes
> > > >
> > > > Hi Geert,
> > > >
> > > > Thanks for the feedback. I started testing CMT on latest
> > > > kernel(Linux-next-
> > > > 20190125 and also renesas-dev) and found that it is broken on
> > > > R-Car M3-W device.
> > > >
> > > > On further investigation the patch ("
> > > > 8234f6734c5d74ac794e5517437f51c57d65f865"   PM-runtime: Switch
> > > > autosuspend over to using hrtimers)  is causing the issue.
> > > >
> > > > During clock source switching, It calls the function "sh_cmt_enable"
> > > > which calls " pm_runtime_get_sync(&ch->cmt->pdev->dev);"  and
> > > > after that console freezes.
> > > >
> > > > Sergei: Have you noticed this issue  on R-Car V3M and V3H boards
> > > > with latest kernel?
> > > >
> > > > Regards,
> > > > Biju
> > > >
> > > > > -----Original Message-----
> > > > > From: [e-mail]
> > > > > <linux-renesas-soc- [e-mail]> On Behalf Of Geert
> > > > > Uytterhoeven
> > > > > Sent: 24 January 2019 10:16
> > > > > To: Biju Das <[e-mail]>
> > > > > Cc: Rob Herring <[e-mail]>; Mark Rutland
> > > > > <[e-mail]>; Simon Horman <[e-mail]>;
> > > > Magnus Damm
> > > > > <[e-mail]>; Linux-Renesas <linux-renesas-
> > > > > [e-mail]>; open list:OPEN FIRMWARE AND FLATTENED
> > DEVICE
> > > > > TREE BINDINGS <[e-mail]>; Geert Uytterhoeven
> > > > > <[e-mail]>; Chris Paterson
> > > > > <[e-mail]>; Daniel Lezcano
> > > > > <[e-mail]>; Thomas Gleixner
> > > > > <[e-mail]>; John Stultz <[e-mail]>;
> > > > > Fabrizio Castro <[e-mail]>; Samuel Holland
> > > > <[e-mail]>
> > > > > Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT
> > > > > device nodes
> > > > >
> > > > > Hi Biju,
> > > > >
> > > > > On Fri, Oct 26, 2018 at 10:32 AM Biju Das
> > > > > <[e-mail]>
> > > > wrote:
> > > > > > This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
> > > > > >
> > > > > > Signed-off-by: Biju Das <[e-mail]>
> > > > > > ---
> > > > > > This patch is tested against renesas-dev
> > > > > >
> > > > > > I have executed on inconsistency-check, nanosleep and
> > > > > > clocksource_switch selftests on this arm64 SoC. The
> > > > > > inconsistency-check and nanosleep tests are working fine.The
> > > > > > clocksource_switch asynchronous test is failing due to
> > > > > > inconsistency-check
> > > > > failure on "arch_sys_counter".
> > > > > >
> > > > > > But if i skip the clocksource_switching of "arch_sys_counter",
> > > > > > the asynchronous test is passing for CMT0/1/2/3 timer.
> > > > > >
> > > > > > Has any one noticed this issue?
> > > > >
> > > > > clockevents/next now has commit 7cd6dca3600d8d71
> > > > > ("clocksource/drivers/arch_timer: Workaround for Allwinner A64
> > > > > timer instability").  Perhaps this is related, and the same test
> > > > > program may indicate similar issues?
> > > > >
> > > > > See also https://lore.kernel.org/lkml/20190113021719.46457-2-
> > > > > [e-mail]/
> > > > >
> > > > > Gr{oetje,eeting}s,
> > > > >
> > > > >                         Geert
> > > > >
> > > > > --
> > > > > Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> > > > > geert@linux- m68k.org
> > > > >
> > > > > In personal conversations with technical people, I call myself a
> > > > > hacker. But when I'm talking to journalists I just say
> > > > > "programmer" or
> > > > something like that.
> > > > >                                 -- Linus Torvalds
> > >
> > >
> > >
> > > Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne
> > End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under
> > Registered No. 04586709.



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Vincent Guittot Jan. 25, 2019, 2:48 p.m. UTC | #24

On Fri, 25 Jan 2019 at 15:45, Biju Das <[e-mail]> wrote:
>
> Hi Vincent,
>
> The below patch fixed deadlock.
>
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -105,7 +105,7 @@ static inline bool pm_runtime_callbacks_present(struct device *dev)
>
>  static inline void pm_runtime_mark_last_busy(struct device *dev)
>  {
> -       WRITE_ONCE(dev->power.last_busy, ktime_to_ns(ktime_get()));
> +       WRITE_ONCE(dev->power.last_busy, ktime_get_mono_fast_ns());

Ah yes. I have searched in runtime.c but  forgot to looked in pm_runtime.h

>  }
>
> Regards,
> Biju
> > -----Original Message-----
> > From: Biju Das
> > Sent: 25 January 2019 14:26
> > To: 'Vincent Guittot' <[e-mail]>
> > Cc: Geert Uytterhoeven <[e-mail]>; Sergei Shtylyov
> > <[e-mail]>; Rob Herring
> > <[e-mail]>; Mark Rutland <[e-mail]>; Simon
> > Horman <[e-mail]>; Magnus Damm
> > <[e-mail]>; Linux-Renesas <linux-renesas-
> > [e-mail]>; open list:OPEN FIRMWARE AND FLATTENED DEVICE
> > TREE BINDINGS <[e-mail]>; Geert Uytterhoeven
> > <[e-mail]>; Chris Paterson
> > <[e-mail]>; Daniel Lezcano
> > <[e-mail]>; Thomas Gleixner <[e-mail]>; John
> > Stultz <[e-mail]>; Fabrizio Castro
> > <[e-mail]>; Samuel Holland <[e-mail]>
> > Subject: RE: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device nodes
> >
> > Hi Vincent,
> >
> > Thanks for the update. I am still seeing the same deadlock.
> >
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index fb5e2b6..a96641a 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -155,7 +155,7 @@ u64 pm_runtime_autosuspend_expiration(struct
> > device *dev)
> >
> >         expires  = READ_ONCE(dev->power.last_busy);
> >         expires += (u64)autosuspend_delay * NSEC_PER_MSEC;
> > -       if (expires > ktime_to_ns(ktime_get()))
> > +       if (expires > ktime_get_mono_fast_ns())
> >                 return expires; /* Expires in the future */
> >
> >         return 0;
> > @@ -921,7 +921,7 @@ static enum hrtimer_restart
> > pm_suspend_timer_fn(struct hrtimer *timer)
> >          * If 'expires' is after the current time, we've been called
> >          * too early.
> >          */
> > -       if (expires > 0 && expires < ktime_to_ns(ktime_get())) {
> > +       if (expires > 0 && expires < ktime_get_mono_fast_ns()) {
> >                 dev->power.timer_expires = 0;
> >                 rpm_suspend(dev, dev->power.timer_autosuspends ?
> >                     (RPM_ASYNC | RPM_AUTO) : RPM_ASYNC); @@ -940,7 +940,7
> > @@ static enum hrtimer_restart  pm_suspend_timer_fn(struct hrtimer
> > *timer)  int pm_schedule_suspend(struct device *dev, unsigned int delay)  {
> >         unsigned long flags;
> > -       ktime_t expires;
> > +       u64 expires;
> >         int retval;
> >
> >         spin_lock_irqsave(&dev->power.lock, flags); @@ -957,8 +957,8 @@ int
> > pm_schedule_suspend(struct device *dev, unsigned int delay)
> >         /* Other scheduled or pending requests need to be canceled. */
> >         pm_runtime_cancel_pending(dev);
> >
> > -       expires = ktime_add(ktime_get(), ms_to_ktime(delay));
> > -       dev->power.timer_expires = ktime_to_ns(expires);
> > +       expires = ktime_get_mono_fast_ns() + ((u64)delay *
> > NSEC_PER_MSEC);
> > +       dev->power.timer_expires = expires;
> >         dev->power.timer_autosuspends = 0;
> >         hrtimer_start(&dev->power.suspend_timer, expires,
> > HRTIMER_MODE_ABS);
> >
> >
> > please find the logs.
> >
> > root@salvator-x:~# echo  e60f0000.timer >
> > /sys/devices/system/clocksource/clocksource0/current_clocksource
> > [   42.769260]
> > [   42.770760] ============================================
> > [   42.776068] WARNING: possible recursive locking detected
> > [   42.781378] 5.0.0-rc3-next-20190125-00001-gfa6709c-dirty #33 Not tainted
> > [   42.788076] --------------------------------------------
> > [   42.793385] migration/0/11 is trying to acquire lock:
> > [   42.798435] 00000000b43b7183 (tk_core.seq){----}, at:
> > rpm_resume+0x5f0/0x7a8
> > [   42.805497]
> > [   42.805497] but task is already holding lock:
> > [   42.811326] 00000000b43b7183 (tk_core.seq){----}, at:
> > multi_cpu_stop+0x8c/0x140
> > [   42.818641]
> > [   42.818641] other info that might help us debug this:
> > [   42.825165]  Possible unsafe locking scenario:
> > [   42.825165]
> > [   42.831080]        CPU0
> > [   42.833521]        ----
> > [   42.835962]   lock(tk_core.seq);
> > [   42.839185]   lock(tk_core.seq);
> > [   42.842409]
> > [   42.842409]  *** DEADLOCK ***
> > [   42.842409]
> > [   42.848328]  May be due to missing lock nesting notation
> > [   42.848328]
> > [   42.855115] 4 locks held by migration/0/11:
> > [   42.859293]  #0: 00000000a7511731 (timekeeper_lock){-.-.}, at:
> > change_clocksource+0x2c/0x118
> > [   42.867738]  #1: 00000000b43b7183 (tk_core.seq){----}, at:
> > multi_cpu_stop+0x8c/0x140
> > [   42.875484]  #2: 00000000b54c38a2 (&ch->lock){....}, at:
> > sh_cmt_start+0x28/0x200
> > [   42.882887]  #3: 0000000010d14eec (&(&dev->power.lock)->rlock){-...}, at:
> > __rpm_callback+0xbc/0x1e8
> > [   42.891937]
> > [   42.891937] stack backtrace:
> > [   42.896294] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.0.0-rc3-next-
> > 20190125-00001-gfa6709c-dirty #33
> > [   42.905860] Hardware name: Renesas Salvator-X 2nd version board based
> > on r8a7796 (DT)
> > [   42.913689] Call trace:
> > [   42.916137]  dump_backtrace+0x0/0x178
> > [   42.919796]  show_stack+0x14/0x20
> > [   42.923111]  dump_stack+0xb0/0xec
> > [   42.926425]  __lock_acquire+0xfb4/0x1c08
> > [   42.930345]  lock_acquire+0xd0/0x268
> > [   42.933917]  ktime_get+0x5c/0x108
> > [   42.937229]  rpm_resume+0x5f0/0x7a8
> > [   42.940714]  __pm_runtime_resume+0x50/0x98
> > [   42.944807]  sh_cmt_start+0x84/0x200
> > [   42.948380]  sh_cmt_clocksource_enable+0x28/0x48
> > [   42.952995]  change_clocksource+0x84/0x118
> > [   42.957087]  multi_cpu_stop+0x8c/0x140
> > [   42.960834]  cpu_stopper_thread+0xac/0x120
> > [   42.964931]  smpboot_thread_fn+0x1ac/0x2c8
> > [   42.969024]  kthread+0x12c/0x130
> > [   42.972249]  ret_from_fork+0x10/0x18
> >
> > Regards,
> > Biju
> >
> > > -----Original Message-----
> > > From: Vincent Guittot <[e-mail]>
> > > Sent: 25 January 2019 14:06
> > > Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
> > > nodes
> > >
> > > Hi Biju,
> > >
> > > It seems that the call of mark_last_busy in rpm_resume raises this
> > problem.
> > > We have the sequence:
> > > change_clocksource
> > >
> > >     write_seqcount_begin
> > >     ...
> > >     timekeeping_update
> > >         ...
> > >         sh_cmt_clocksource_enable
> > >             ...
> > >             rpm_resume
> > >                 pm_runtime_mark_last_busy
> > >                     ktime_get
> > >                         do
> > >                             read_seqcount_begin
> > >                         while read_seqcount_retry
> > >     ....
> > >     write_seqcount_end
> > >
> > > In fact, we should be safe because we haven't yet changed the
> > > clocksource as we are enabling the clocksource that will be used for the
> > switch.
> > >
> > > Can you try the patch below ?
> > > It uses ktime_get_mono_fast_ns instead which is lock safe for such
> > > case
> > >
> > > ---
> > >  drivers/base/power/runtime.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/base/power/runtime.c
> > > b/drivers/base/power/runtime.c index 457be03..708a13f 100644
> > > --- a/drivers/base/power/runtime.c
> > > +++ b/drivers/base/power/runtime.c
> > > @@ -130,7 +130,7 @@ u64 pm_runtime_autosuspend_expiration(struct
> > > device *dev)  {
> > >  int autosuspend_delay;
> > >  u64 last_busy, expires = 0;
> > > -u64 now = ktime_to_ns(ktime_get());
> > > +u64 now = ktime_get_mono_fast_ns();
> > >
> > >  if (!dev->power.use_autosuspend)
> > >  goto out;
> > > @@ -909,7 +909,7 @@ static enum hrtimer_restart
> > > pm_suspend_timer_fn(struct hrtimer *timer)
> > >   * If 'expires' is after the current time, we've been called
> > >   * too early.
> > >   */
> > > -if (expires > 0 && expires < ktime_to_ns(ktime_get())) {
> > > +if (expires > 0 && expires < ktime_get_mono_fast_ns()) {
> > >  dev->power.timer_expires = 0;
> > >  rpm_suspend(dev, dev->power.timer_autosuspends ?
> > >      (RPM_ASYNC | RPM_AUTO) : RPM_ASYNC); @@ -928,7
> > > +928,7 @@ static enum hrtimer_restart  pm_suspend_timer_fn(struct
> > > hrtimer *timer)  int pm_schedule_suspend(struct device *dev, unsigned
> > > int
> > > delay)  {
> > >  unsigned long flags;
> > > -ktime_t expires;
> > > +u64 expires;
> > >  int retval;
> > >
> > >  spin_lock_irqsave(&dev->power.lock, flags); @@ -945,8 +945,8 @@
> > int
> > > pm_schedule_suspend(struct device *dev, unsigned int delay)
> > >  /* Other scheduled or pending requests need to be canceled. */
> > >  pm_runtime_cancel_pending(dev);
> > >
> > > -expires = ktime_add(ktime_get(), ms_to_ktime(delay));
> > > -dev->power.timer_expires = ktime_to_ns(expires);
> > > +expires = ktime_get_mono_fast_ns() + (u64)delay *
> > > NSEC_PER_MSEC);
> > > +dev->power.timer_expires = expires;
> > >  dev->power.timer_autosuspends = 0;
> > >  hrtimer_start(&dev->power.suspend_timer, expires,
> > HRTIMER_MODE_ABS);
> > >
> > > --
> > > 2.7.4
> > >
> > > Le Friday 25 Jan 2019 à 12:26:54 (+0000), Biju Das a écrit :
> > > > Hi All,
> > > >
> > > > I have enabled lock debugging  and it is showing "possible recursive
> > > > locking
> > > detected "with the patch ("8234f6734c5d74ac794e5517437f51c57d65f865"
> > > PM-runtime: Switch autosuspend over to using hrtimers)
> > > >
> > > > root@salvator-x:~# echo  e60f0000.timer >
> > > /sys/devices/system/clocksource/clocksource0/current_clocksource
> > > > [   67.995854]
> > > > [   67.997354] ============================================
> > > > [   68.002663] WARNING: possible recursive locking detected
> > > > [   68.007974] 5.0.0-rc3-next-20190125-00005-g382822b #31 Not tainted
> > > > [   68.014150] --------------------------------------------
> > > > [   68.019459] migration/0/11 is trying to acquire lock:
> > > > [   68.024509] 0000000065de5fdf (tk_core.seq){----}, at:
> > > rpm_resume+0x5f0/0x7a8
> > > > [   68.031570]
> > > > [   68.031570] but task is already holding lock:
> > > > [   68.037399] 0000000065de5fdf (tk_core.seq){----}, at:
> > > multi_cpu_stop+0x8c/0x140
> > > > [   68.044714]
> > > > [   68.044714] other info that might help us debug this:
> > > > [   68.051237]  Possible unsafe locking scenario:
> > > > [   68.051237]
> > > > [   68.057153]        CPU0
> > > > [   68.059594]        ----
> > > > [   68.062034]   lock(tk_core.seq);
> > > > [   68.065258]   lock(tk_core.seq);
> > > > [   68.068482]
> > > > [   68.068482]  *** DEADLOCK ***
> > > > [   68.068482]
> > > > [   68.074402]  May be due to missing lock nesting notation
> > > > [   68.074402]
> > > > [   68.081188] 4 locks held by migration/0/11:
> > > > [   68.085366]  #0: 00000000463b6446 (timekeeper_lock){-.-.}, at:
> > > change_clocksource+0x2c/0x118
> > > > [   68.093812]  #1: 0000000065de5fdf (tk_core.seq){----}, at:
> > > multi_cpu_stop+0x8c/0x140
> > > > [   68.101558]  #2: 0000000098b45550 (&ch->lock){....}, at:
> > > sh_cmt_start+0x28/0x200
> > > > [   68.108961]  #3: 00000000da0f4e80 (&(&dev->power.lock)->rlock){-...},
> > at:
> > > __rpm_callback+0xbc/0x1e8
> > > > [   68.118011]
> > > > [   68.118011] stack backtrace:
> > > > [   68.122368] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.0.0-rc3-
> > next-
> > > 20190125-00005-g382822b #31
> > > > [   68.131412] Hardware name: Renesas Salvator-X 2nd version board
> > based
> > > on r8a7796 (DT)
> > > > [   68.139240] Call trace:
> > > > [   68.141687]  dump_backtrace+0x0/0x178
> > > > [   68.145346]  show_stack+0x14/0x20
> > > > [   68.148661]  dump_stack+0xb0/0xec
> > > > [   68.151975]  __lock_acquire+0xfb4/0x1c08
> > > > [   68.155894]  lock_acquire+0xd0/0x268
> > > > [   68.159467]  ktime_get+0x5c/0x108
> > > > [   68.162779]  rpm_resume+0x5f0/0x7a8
> > > > [   68.166265]  __pm_runtime_resume+0x50/0x98
> > > > [   68.170358]  sh_cmt_start+0x84/0x200
> > > > [   68.173931]  sh_cmt_clocksource_enable+0x28/0x48
> > > > [   68.178546]  change_clocksource+0x84/0x118
> > > > [   68.182639]  multi_cpu_stop+0x8c/0x140
> > > > [   68.186385]  cpu_stopper_thread+0xac/0x120
> > > > [   68.190481]  smpboot_thread_fn+0x1ac/0x2c8
> > > > [   68.194574]  kthread+0x12c/0x130
> > > > [   68.197800]  ret_from_fork+0x10/0x18
> > > >
> > > > Regards,
> > > > Biju
> > > >
> > > > > -----Original Message-----
> > > > > From: Biju Das
> > > > > Sent: 25 January 2019 11:27
> > > > > To: 'Geert Uytterhoeven' <[e-mail]>; Vincent Guittot
> > > > > <[e-mail]>; Sergei Shtylyov
> > > > > <[e-mail]>
> > > > > Cc: Rob Herring <[e-mail]>; Mark Rutland
> > > > > <[e-mail]>; Simon Horman <[e-mail]>;
> > > Magnus
> > > > > Damm <[e-mail]>; Linux-Renesas <linux-renesas-
> > > > > [e-mail]>; open list:OPEN FIRMWARE AND FLATTENED
> > > DEVICE
> > > > > TREE BINDINGS <[e-mail]>; Geert Uytterhoeven
> > > > > <[e-mail]>; Chris Paterson
> > > > > <[e-mail]>; Daniel Lezcano
> > > > > <[e-mail]>; Thomas Gleixner <[e-mail]>;
> > > > > John Stultz <[e-mail]>; Fabrizio Castro
> > > > > <[e-mail]>; Samuel Holland
> > > > > <[e-mail]>
> > > > > Subject: RE: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
> > > > > nodes
> > > > >
> > > > > Hi Geert,
> > > > >
> > > > > Thanks for the feedback. I started testing CMT on latest
> > > > > kernel(Linux-next-
> > > > > 20190125 and also renesas-dev) and found that it is broken on
> > > > > R-Car M3-W device.
> > > > >
> > > > > On further investigation the patch ("
> > > > > 8234f6734c5d74ac794e5517437f51c57d65f865"   PM-runtime: Switch
> > > > > autosuspend over to using hrtimers)  is causing the issue.
> > > > >
> > > > > During clock source switching, It calls the function "sh_cmt_enable"
> > > > > which calls " pm_runtime_get_sync(&ch->cmt->pdev->dev);"  and
> > > > > after that console freezes.
> > > > >
> > > > > Sergei: Have you noticed this issue  on R-Car V3M and V3H boards
> > > > > with latest kernel?
> > > > >
> > > > > Regards,
> > > > > Biju
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: [e-mail]
> > > > > > <linux-renesas-soc- [e-mail]> On Behalf Of Geert
> > > > > > Uytterhoeven
> > > > > > Sent: 24 January 2019 10:16
> > > > > > To: Biju Das <[e-mail]>
> > > > > > Cc: Rob Herring <[e-mail]>; Mark Rutland
> > > > > > <[e-mail]>; Simon Horman <[e-mail]>;
> > > > > Magnus Damm
> > > > > > <[e-mail]>; Linux-Renesas <linux-renesas-
> > > > > > [e-mail]>; open list:OPEN FIRMWARE AND FLATTENED
> > > DEVICE
> > > > > > TREE BINDINGS <[e-mail]>; Geert Uytterhoeven
> > > > > > <[e-mail]>; Chris Paterson
> > > > > > <[e-mail]>; Daniel Lezcano
> > > > > > <[e-mail]>; Thomas Gleixner
> > > > > > <[e-mail]>; John Stultz <[e-mail]>;
> > > > > > Fabrizio Castro <[e-mail]>; Samuel Holland
> > > > > <[e-mail]>
> > > > > > Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT
> > > > > > device nodes
> > > > > >
> > > > > > Hi Biju,
> > > > > >
> > > > > > On Fri, Oct 26, 2018 at 10:32 AM Biju Das
> > > > > > <[e-mail]>
> > > > > wrote:
> > > > > > > This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
> > > > > > >
> > > > > > > Signed-off-by: Biju Das <[e-mail]>
> > > > > > > ---
> > > > > > > This patch is tested against renesas-dev
> > > > > > >
> > > > > > > I have executed on inconsistency-check, nanosleep and
> > > > > > > clocksource_switch selftests on this arm64 SoC. The
> > > > > > > inconsistency-check and nanosleep tests are working fine.The
> > > > > > > clocksource_switch asynchronous test is failing due to
> > > > > > > inconsistency-check
> > > > > > failure on "arch_sys_counter".
> > > > > > >
> > > > > > > But if i skip the clocksource_switching of "arch_sys_counter",
> > > > > > > the asynchronous test is passing for CMT0/1/2/3 timer.
> > > > > > >
> > > > > > > Has any one noticed this issue?
> > > > > >
> > > > > > clockevents/next now has commit 7cd6dca3600d8d71
> > > > > > ("clocksource/drivers/arch_timer: Workaround for Allwinner A64
> > > > > > timer instability").  Perhaps this is related, and the same test
> > > > > > program may indicate similar issues?
> > > > > >
> > > > > > See also https://lore.kernel.org/lkml/20190113021719.46457-2-
> > > > > > [e-mail]/
> > > > > >
> > > > > > Gr{oetje,eeting}s,
> > > > > >
> > > > > >                         Geert
> > > > > >
> > > > > > --
> > > > > > Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> > > > > > geert@linux- m68k.org
> > > > > >
> > > > > > In personal conversations with technical people, I call myself a
> > > > > > hacker. But when I'm talking to journalists I just say
> > > > > > "programmer" or
> > > > > something like that.
> > > > > >                                 -- Linus Torvalds
> > > >
> > > >
> > > >
> > > > Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne
> > > End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under
> > > Registered No. 04586709.
>
>
>
> Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Vincent Guittot Jan. 25, 2019, 2:53 p.m. UTC | #25

Adding Rafael

On Fri, 25 Jan 2019 at 15:48, Vincent Guittot
<[e-mail]> wrote:
>
> On Fri, 25 Jan 2019 at 15:45, Biju Das <[e-mail]> wrote:
> >
> > Hi Vincent,
> >
> > The below patch fixed deadlock.
> >
> > --- a/include/linux/pm_runtime.h
> > +++ b/include/linux/pm_runtime.h
> > @@ -105,7 +105,7 @@ static inline bool pm_runtime_callbacks_present(struct device *dev)
> >
> >  static inline void pm_runtime_mark_last_busy(struct device *dev)
> >  {
> > -       WRITE_ONCE(dev->power.last_busy, ktime_to_ns(ktime_get()));
> > +       WRITE_ONCE(dev->power.last_busy, ktime_get_mono_fast_ns());
>
> Ah yes. I have searched in runtime.c but  forgot to looked in pm_runtime.h

Rafael,

When pm-runtime is used for a clocksource, we get a deadlock because
we call pm_runtime_mark_last_busy() when ebaling the clocksource
whereas we are in the write_seqcount sequence.
Using ktime_get_mono_fast_ns fixes the problem because it doesn't use
tk_core.seq

Regards,
Vincent

>
> >  }
> >
> > Regards,
> > Biju
> > > -----Original Message-----
> > > From: Biju Das
> > > Sent: 25 January 2019 14:26
> > > To: 'Vincent Guittot' <[e-mail]>
> > > Cc: Geert Uytterhoeven <[e-mail]>; Sergei Shtylyov
> > > <[e-mail]>; Rob Herring
> > > <[e-mail]>; Mark Rutland <[e-mail]>; Simon
> > > Horman <[e-mail]>; Magnus Damm
> > > <[e-mail]>; Linux-Renesas <linux-renesas-
> > > [e-mail]>; open list:OPEN FIRMWARE AND FLATTENED DEVICE
> > > TREE BINDINGS <[e-mail]>; Geert Uytterhoeven
> > > <[e-mail]>; Chris Paterson
> > > <[e-mail]>; Daniel Lezcano
> > > <[e-mail]>; Thomas Gleixner <[e-mail]>; John
> > > Stultz <[e-mail]>; Fabrizio Castro
> > > <[e-mail]>; Samuel Holland <[e-mail]>
> > > Subject: RE: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device nodes
> > >
> > > Hi Vincent,
> > >
> > > Thanks for the update. I am still seeing the same deadlock.
> > >
> > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > index fb5e2b6..a96641a 100644
> > > --- a/drivers/base/power/runtime.c
> > > +++ b/drivers/base/power/runtime.c
> > > @@ -155,7 +155,7 @@ u64 pm_runtime_autosuspend_expiration(struct
> > > device *dev)
> > >
> > >         expires  = READ_ONCE(dev->power.last_busy);
> > >         expires += (u64)autosuspend_delay * NSEC_PER_MSEC;
> > > -       if (expires > ktime_to_ns(ktime_get()))
> > > +       if (expires > ktime_get_mono_fast_ns())
> > >                 return expires; /* Expires in the future */
> > >
> > >         return 0;
> > > @@ -921,7 +921,7 @@ static enum hrtimer_restart
> > > pm_suspend_timer_fn(struct hrtimer *timer)
> > >          * If 'expires' is after the current time, we've been called
> > >          * too early.
> > >          */
> > > -       if (expires > 0 && expires < ktime_to_ns(ktime_get())) {
> > > +       if (expires > 0 && expires < ktime_get_mono_fast_ns()) {
> > >                 dev->power.timer_expires = 0;
> > >                 rpm_suspend(dev, dev->power.timer_autosuspends ?
> > >                     (RPM_ASYNC | RPM_AUTO) : RPM_ASYNC); @@ -940,7 +940,7
> > > @@ static enum hrtimer_restart  pm_suspend_timer_fn(struct hrtimer
> > > *timer)  int pm_schedule_suspend(struct device *dev, unsigned int delay)  {
> > >         unsigned long flags;
> > > -       ktime_t expires;
> > > +       u64 expires;
> > >         int retval;
> > >
> > >         spin_lock_irqsave(&dev->power.lock, flags); @@ -957,8 +957,8 @@ int
> > > pm_schedule_suspend(struct device *dev, unsigned int delay)
> > >         /* Other scheduled or pending requests need to be canceled. */
> > >         pm_runtime_cancel_pending(dev);
> > >
> > > -       expires = ktime_add(ktime_get(), ms_to_ktime(delay));
> > > -       dev->power.timer_expires = ktime_to_ns(expires);
> > > +       expires = ktime_get_mono_fast_ns() + ((u64)delay *
> > > NSEC_PER_MSEC);
> > > +       dev->power.timer_expires = expires;
> > >         dev->power.timer_autosuspends = 0;
> > >         hrtimer_start(&dev->power.suspend_timer, expires,
> > > HRTIMER_MODE_ABS);
> > >
> > >
> > > please find the logs.
> > >
> > > root@salvator-x:~# echo  e60f0000.timer >
> > > /sys/devices/system/clocksource/clocksource0/current_clocksource
> > > [   42.769260]
> > > [   42.770760] ============================================
> > > [   42.776068] WARNING: possible recursive locking detected
> > > [   42.781378] 5.0.0-rc3-next-20190125-00001-gfa6709c-dirty #33 Not tainted
> > > [   42.788076] --------------------------------------------
> > > [   42.793385] migration/0/11 is trying to acquire lock:
> > > [   42.798435] 00000000b43b7183 (tk_core.seq){----}, at:
> > > rpm_resume+0x5f0/0x7a8
> > > [   42.805497]
> > > [   42.805497] but task is already holding lock:
> > > [   42.811326] 00000000b43b7183 (tk_core.seq){----}, at:
> > > multi_cpu_stop+0x8c/0x140
> > > [   42.818641]
> > > [   42.818641] other info that might help us debug this:
> > > [   42.825165]  Possible unsafe locking scenario:
> > > [   42.825165]
> > > [   42.831080]        CPU0
> > > [   42.833521]        ----
> > > [   42.835962]   lock(tk_core.seq);
> > > [   42.839185]   lock(tk_core.seq);
> > > [   42.842409]
> > > [   42.842409]  *** DEADLOCK ***
> > > [   42.842409]
> > > [   42.848328]  May be due to missing lock nesting notation
> > > [   42.848328]
> > > [   42.855115] 4 locks held by migration/0/11:
> > > [   42.859293]  #0: 00000000a7511731 (timekeeper_lock){-.-.}, at:
> > > change_clocksource+0x2c/0x118
> > > [   42.867738]  #1: 00000000b43b7183 (tk_core.seq){----}, at:
> > > multi_cpu_stop+0x8c/0x140
> > > [   42.875484]  #2: 00000000b54c38a2 (&ch->lock){....}, at:
> > > sh_cmt_start+0x28/0x200
> > > [   42.882887]  #3: 0000000010d14eec (&(&dev->power.lock)->rlock){-...}, at:
> > > __rpm_callback+0xbc/0x1e8
> > > [   42.891937]
> > > [   42.891937] stack backtrace:
> > > [   42.896294] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.0.0-rc3-next-
> > > 20190125-00001-gfa6709c-dirty #33
> > > [   42.905860] Hardware name: Renesas Salvator-X 2nd version board based
> > > on r8a7796 (DT)
> > > [   42.913689] Call trace:
> > > [   42.916137]  dump_backtrace+0x0/0x178
> > > [   42.919796]  show_stack+0x14/0x20
> > > [   42.923111]  dump_stack+0xb0/0xec
> > > [   42.926425]  __lock_acquire+0xfb4/0x1c08
> > > [   42.930345]  lock_acquire+0xd0/0x268
> > > [   42.933917]  ktime_get+0x5c/0x108
> > > [   42.937229]  rpm_resume+0x5f0/0x7a8
> > > [   42.940714]  __pm_runtime_resume+0x50/0x98
> > > [   42.944807]  sh_cmt_start+0x84/0x200
> > > [   42.948380]  sh_cmt_clocksource_enable+0x28/0x48
> > > [   42.952995]  change_clocksource+0x84/0x118
> > > [   42.957087]  multi_cpu_stop+0x8c/0x140
> > > [   42.960834]  cpu_stopper_thread+0xac/0x120
> > > [   42.964931]  smpboot_thread_fn+0x1ac/0x2c8
> > > [   42.969024]  kthread+0x12c/0x130
> > > [   42.972249]  ret_from_fork+0x10/0x18
> > >
> > > Regards,
> > > Biju
> > >
> > > > -----Original Message-----
> > > > From: Vincent Guittot <[e-mail]>
> > > > Sent: 25 January 2019 14:06
> > > > Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
> > > > nodes
> > > >
> > > > Hi Biju,
> > > >
> > > > It seems that the call of mark_last_busy in rpm_resume raises this
> > > problem.
> > > > We have the sequence:
> > > > change_clocksource
> > > >
> > > >     write_seqcount_begin
> > > >     ...
> > > >     timekeeping_update
> > > >         ...
> > > >         sh_cmt_clocksource_enable
> > > >             ...
> > > >             rpm_resume
> > > >                 pm_runtime_mark_last_busy
> > > >                     ktime_get
> > > >                         do
> > > >                             read_seqcount_begin
> > > >                         while read_seqcount_retry
> > > >     ....
> > > >     write_seqcount_end
> > > >
> > > > In fact, we should be safe because we haven't yet changed the
> > > > clocksource as we are enabling the clocksource that will be used for the
> > > switch.
> > > >
> > > > Can you try the patch below ?
> > > > It uses ktime_get_mono_fast_ns instead which is lock safe for such
> > > > case
> > > >
> > > > ---
> > > >  drivers/base/power/runtime.c | 10 +++++-----
> > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/base/power/runtime.c
> > > > b/drivers/base/power/runtime.c index 457be03..708a13f 100644
> > > > --- a/drivers/base/power/runtime.c
> > > > +++ b/drivers/base/power/runtime.c
> > > > @@ -130,7 +130,7 @@ u64 pm_runtime_autosuspend_expiration(struct
> > > > device *dev)  {
> > > >  int autosuspend_delay;
> > > >  u64 last_busy, expires = 0;
> > > > -u64 now = ktime_to_ns(ktime_get());
> > > > +u64 now = ktime_get_mono_fast_ns();
> > > >
> > > >  if (!dev->power.use_autosuspend)
> > > >  goto out;
> > > > @@ -909,7 +909,7 @@ static enum hrtimer_restart
> > > > pm_suspend_timer_fn(struct hrtimer *timer)
> > > >   * If 'expires' is after the current time, we've been called
> > > >   * too early.
> > > >   */
> > > > -if (expires > 0 && expires < ktime_to_ns(ktime_get())) {
> > > > +if (expires > 0 && expires < ktime_get_mono_fast_ns()) {
> > > >  dev->power.timer_expires = 0;
> > > >  rpm_suspend(dev, dev->power.timer_autosuspends ?
> > > >      (RPM_ASYNC | RPM_AUTO) : RPM_ASYNC); @@ -928,7
> > > > +928,7 @@ static enum hrtimer_restart  pm_suspend_timer_fn(struct
> > > > hrtimer *timer)  int pm_schedule_suspend(struct device *dev, unsigned
> > > > int
> > > > delay)  {
> > > >  unsigned long flags;
> > > > -ktime_t expires;
> > > > +u64 expires;
> > > >  int retval;
> > > >
> > > >  spin_lock_irqsave(&dev->power.lock, flags); @@ -945,8 +945,8 @@
> > > int
> > > > pm_schedule_suspend(struct device *dev, unsigned int delay)
> > > >  /* Other scheduled or pending requests need to be canceled. */
> > > >  pm_runtime_cancel_pending(dev);
> > > >
> > > > -expires = ktime_add(ktime_get(), ms_to_ktime(delay));
> > > > -dev->power.timer_expires = ktime_to_ns(expires);
> > > > +expires = ktime_get_mono_fast_ns() + (u64)delay *
> > > > NSEC_PER_MSEC);
> > > > +dev->power.timer_expires = expires;
> > > >  dev->power.timer_autosuspends = 0;
> > > >  hrtimer_start(&dev->power.suspend_timer, expires,
> > > HRTIMER_MODE_ABS);
> > > >
> > > > --
> > > > 2.7.4
> > > >
> > > > Le Friday 25 Jan 2019 à 12:26:54 (+0000), Biju Das a écrit :
> > > > > Hi All,
> > > > >
> > > > > I have enabled lock debugging  and it is showing "possible recursive
> > > > > locking
> > > > detected "with the patch ("8234f6734c5d74ac794e5517437f51c57d65f865"
> > > > PM-runtime: Switch autosuspend over to using hrtimers)
> > > > >
> > > > > root@salvator-x:~# echo  e60f0000.timer >
> > > > /sys/devices/system/clocksource/clocksource0/current_clocksource
> > > > > [   67.995854]
> > > > > [   67.997354] ============================================
> > > > > [   68.002663] WARNING: possible recursive locking detected
> > > > > [   68.007974] 5.0.0-rc3-next-20190125-00005-g382822b #31 Not tainted
> > > > > [   68.014150] --------------------------------------------
> > > > > [   68.019459] migration/0/11 is trying to acquire lock:
> > > > > [   68.024509] 0000000065de5fdf (tk_core.seq){----}, at:
> > > > rpm_resume+0x5f0/0x7a8
> > > > > [   68.031570]
> > > > > [   68.031570] but task is already holding lock:
> > > > > [   68.037399] 0000000065de5fdf (tk_core.seq){----}, at:
> > > > multi_cpu_stop+0x8c/0x140
> > > > > [   68.044714]
> > > > > [   68.044714] other info that might help us debug this:
> > > > > [   68.051237]  Possible unsafe locking scenario:
> > > > > [   68.051237]
> > > > > [   68.057153]        CPU0
> > > > > [   68.059594]        ----
> > > > > [   68.062034]   lock(tk_core.seq);
> > > > > [   68.065258]   lock(tk_core.seq);
> > > > > [   68.068482]
> > > > > [   68.068482]  *** DEADLOCK ***
> > > > > [   68.068482]
> > > > > [   68.074402]  May be due to missing lock nesting notation
> > > > > [   68.074402]
> > > > > [   68.081188] 4 locks held by migration/0/11:
> > > > > [   68.085366]  #0: 00000000463b6446 (timekeeper_lock){-.-.}, at:
> > > > change_clocksource+0x2c/0x118
> > > > > [   68.093812]  #1: 0000000065de5fdf (tk_core.seq){----}, at:
> > > > multi_cpu_stop+0x8c/0x140
> > > > > [   68.101558]  #2: 0000000098b45550 (&ch->lock){....}, at:
> > > > sh_cmt_start+0x28/0x200
> > > > > [   68.108961]  #3: 00000000da0f4e80 (&(&dev->power.lock)->rlock){-...},
> > > at:
> > > > __rpm_callback+0xbc/0x1e8
> > > > > [   68.118011]
> > > > > [   68.118011] stack backtrace:
> > > > > [   68.122368] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.0.0-rc3-
> > > next-
> > > > 20190125-00005-g382822b #31
> > > > > [   68.131412] Hardware name: Renesas Salvator-X 2nd version board
> > > based
> > > > on r8a7796 (DT)
> > > > > [   68.139240] Call trace:
> > > > > [   68.141687]  dump_backtrace+0x0/0x178
> > > > > [   68.145346]  show_stack+0x14/0x20
> > > > > [   68.148661]  dump_stack+0xb0/0xec
> > > > > [   68.151975]  __lock_acquire+0xfb4/0x1c08
> > > > > [   68.155894]  lock_acquire+0xd0/0x268
> > > > > [   68.159467]  ktime_get+0x5c/0x108
> > > > > [   68.162779]  rpm_resume+0x5f0/0x7a8
> > > > > [   68.166265]  __pm_runtime_resume+0x50/0x98
> > > > > [   68.170358]  sh_cmt_start+0x84/0x200
> > > > > [   68.173931]  sh_cmt_clocksource_enable+0x28/0x48
> > > > > [   68.178546]  change_clocksource+0x84/0x118
> > > > > [   68.182639]  multi_cpu_stop+0x8c/0x140
> > > > > [   68.186385]  cpu_stopper_thread+0xac/0x120
> > > > > [   68.190481]  smpboot_thread_fn+0x1ac/0x2c8
> > > > > [   68.194574]  kthread+0x12c/0x130
> > > > > [   68.197800]  ret_from_fork+0x10/0x18
> > > > >
> > > > > Regards,
> > > > > Biju
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Biju Das
> > > > > > Sent: 25 January 2019 11:27
> > > > > > To: 'Geert Uytterhoeven' <[e-mail]>; Vincent Guittot
> > > > > > <[e-mail]>; Sergei Shtylyov
> > > > > > <[e-mail]>
> > > > > > Cc: Rob Herring <[e-mail]>; Mark Rutland
> > > > > > <[e-mail]>; Simon Horman <[e-mail]>;
> > > > Magnus
> > > > > > Damm <[e-mail]>; Linux-Renesas <linux-renesas-
> > > > > > [e-mail]>; open list:OPEN FIRMWARE AND FLATTENED
> > > > DEVICE
> > > > > > TREE BINDINGS <[e-mail]>; Geert Uytterhoeven
> > > > > > <[e-mail]>; Chris Paterson
> > > > > > <[e-mail]>; Daniel Lezcano
> > > > > > <[e-mail]>; Thomas Gleixner <[e-mail]>;
> > > > > > John Stultz <[e-mail]>; Fabrizio Castro
> > > > > > <[e-mail]>; Samuel Holland
> > > > > > <[e-mail]>
> > > > > > Subject: RE: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
> > > > > > nodes
> > > > > >
> > > > > > Hi Geert,
> > > > > >
> > > > > > Thanks for the feedback. I started testing CMT on latest
> > > > > > kernel(Linux-next-
> > > > > > 20190125 and also renesas-dev) and found that it is broken on
> > > > > > R-Car M3-W device.
> > > > > >
> > > > > > On further investigation the patch ("
> > > > > > 8234f6734c5d74ac794e5517437f51c57d65f865"   PM-runtime: Switch
> > > > > > autosuspend over to using hrtimers)  is causing the issue.
> > > > > >
> > > > > > During clock source switching, It calls the function "sh_cmt_enable"
> > > > > > which calls " pm_runtime_get_sync(&ch->cmt->pdev->dev);"  and
> > > > > > after that console freezes.
> > > > > >
> > > > > > Sergei: Have you noticed this issue  on R-Car V3M and V3H boards
> > > > > > with latest kernel?
> > > > > >
> > > > > > Regards,
> > > > > > Biju
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: [e-mail]
> > > > > > > <linux-renesas-soc- [e-mail]> On Behalf Of Geert
> > > > > > > Uytterhoeven
> > > > > > > Sent: 24 January 2019 10:16
> > > > > > > To: Biju Das <[e-mail]>
> > > > > > > Cc: Rob Herring <[e-mail]>; Mark Rutland
> > > > > > > <[e-mail]>; Simon Horman <[e-mail]>;
> > > > > > Magnus Damm
> > > > > > > <[e-mail]>; Linux-Renesas <linux-renesas-
> > > > > > > [e-mail]>; open list:OPEN FIRMWARE AND FLATTENED
> > > > DEVICE
> > > > > > > TREE BINDINGS <[e-mail]>; Geert Uytterhoeven
> > > > > > > <[e-mail]>; Chris Paterson
> > > > > > > <[e-mail]>; Daniel Lezcano
> > > > > > > <[e-mail]>; Thomas Gleixner
> > > > > > > <[e-mail]>; John Stultz <[e-mail]>;
> > > > > > > Fabrizio Castro <[e-mail]>; Samuel Holland
> > > > > > <[e-mail]>
> > > > > > > Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT
> > > > > > > device nodes
> > > > > > >
> > > > > > > Hi Biju,
> > > > > > >
> > > > > > > On Fri, Oct 26, 2018 at 10:32 AM Biju Das
> > > > > > > <[e-mail]>
> > > > > > wrote:
> > > > > > > > This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
> > > > > > > >
> > > > > > > > Signed-off-by: Biju Das <[e-mail]>
> > > > > > > > ---
> > > > > > > > This patch is tested against renesas-dev
> > > > > > > >
> > > > > > > > I have executed on inconsistency-check, nanosleep and
> > > > > > > > clocksource_switch selftests on this arm64 SoC. The
> > > > > > > > inconsistency-check and nanosleep tests are working fine.The
> > > > > > > > clocksource_switch asynchronous test is failing due to
> > > > > > > > inconsistency-check
> > > > > > > failure on "arch_sys_counter".
> > > > > > > >
> > > > > > > > But if i skip the clocksource_switching of "arch_sys_counter",
> > > > > > > > the asynchronous test is passing for CMT0/1/2/3 timer.
> > > > > > > >
> > > > > > > > Has any one noticed this issue?
> > > > > > >
> > > > > > > clockevents/next now has commit 7cd6dca3600d8d71
> > > > > > > ("clocksource/drivers/arch_timer: Workaround for Allwinner A64
> > > > > > > timer instability").  Perhaps this is related, and the same test
> > > > > > > program may indicate similar issues?
> > > > > > >
> > > > > > > See also https://lore.kernel.org/lkml/20190113021719.46457-2-
> > > > > > > [e-mail]/
> > > > > > >
> > > > > > > Gr{oetje,eeting}s,
> > > > > > >
> > > > > > >                         Geert
> > > > > > >
> > > > > > > --
> > > > > > > Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> > > > > > > geert@linux- m68k.org
> > > > > > >
> > > > > > > In personal conversations with technical people, I call myself a
> > > > > > > hacker. But when I'm talking to journalists I just say
> > > > > > > "programmer" or
> > > > > > something like that.
> > > > > > >                                 -- Linus Torvalds
> > > > >
> > > > >
> > > > >
> > > > > Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne
> > > > End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under
> > > > Registered No. 04586709.
> >
> >
> >
> > Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Biju Das Jan. 25, 2019, 3:12 p.m. UTC | #26

Hi Vincent and Rafael,

Are you going to send the patch for the dead lock fix? Or Do you want me to send the patch?

Please let me know.

Regards,
Biju

> Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device nodes
>
> Adding Rafael
>
> On Fri, 25 Jan 2019 at 15:48, Vincent Guittot <[e-mail]>
> wrote:
> >
> > On Fri, 25 Jan 2019 at 15:45, Biju Das <[e-mail]> wrote:
> > >
> > > Hi Vincent,
> > >
> > > The below patch fixed deadlock.
> > >
> > > --- a/include/linux/pm_runtime.h
> > > +++ b/include/linux/pm_runtime.h
> > > @@ -105,7 +105,7 @@ static inline bool
> > > pm_runtime_callbacks_present(struct device *dev)
> > >
> > >  static inline void pm_runtime_mark_last_busy(struct device *dev)  {
> > > -       WRITE_ONCE(dev->power.last_busy, ktime_to_ns(ktime_get()));
> > > +       WRITE_ONCE(dev->power.last_busy, ktime_get_mono_fast_ns());
> >
> > Ah yes. I have searched in runtime.c but  forgot to looked in
> > pm_runtime.h
>
> Rafael,
>
> When pm-runtime is used for a clocksource, we get a deadlock because we
> call pm_runtime_mark_last_busy() when ebaling the clocksource whereas
> we are in the write_seqcount sequence.
> Using ktime_get_mono_fast_ns fixes the problem because it doesn't use
> tk_core.seq
>
> Regards,
> Vincent
>
> >
> > >  }
> > >
> > > Regards,
> > > Biju
> > > > -----Original Message-----
> > > > From: Biju Das
> > > > Sent: 25 January 2019 14:26
> > > > To: 'Vincent Guittot' <[e-mail]>
> > > > Cc: Geert Uytterhoeven <[e-mail]>; Sergei Shtylyov
> > > > <[e-mail]>; Rob Herring
> > > > <[e-mail]>; Mark Rutland <[e-mail]>;
> Simon
> > > > Horman <[e-mail]>; Magnus Damm
> <[e-mail]>;
> > > > Linux-Renesas <linux-renesas- [e-mail]>; open list:OPEN
> > > > FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
> > > > <[e-mail]>; Geert Uytterhoeven
> > > > <[e-mail]>; Chris Paterson
> > > > <[e-mail]>; Daniel Lezcano
> > > > <[e-mail]>; Thomas Gleixner <[e-mail]>;
> > > > John Stultz <[e-mail]>; Fabrizio Castro
> > > > <[e-mail]>; Samuel Holland
> > > > <[e-mail]>
> > > > Subject: RE: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
> > > > nodes
> > > >
> > > > Hi Vincent,
> > > >
> > > > Thanks for the update. I am still seeing the same deadlock.
> > > >
> > > > diff --git a/drivers/base/power/runtime.c
> > > > b/drivers/base/power/runtime.c index fb5e2b6..a96641a 100644
> > > > --- a/drivers/base/power/runtime.c
> > > > +++ b/drivers/base/power/runtime.c
> > > > @@ -155,7 +155,7 @@ u64
> pm_runtime_autosuspend_expiration(struct
> > > > device *dev)
> > > >
> > > >         expires  = READ_ONCE(dev->power.last_busy);
> > > >         expires += (u64)autosuspend_delay * NSEC_PER_MSEC;
> > > > -       if (expires > ktime_to_ns(ktime_get()))
> > > > +       if (expires > ktime_get_mono_fast_ns())
> > > >                 return expires; /* Expires in the future */
> > > >
> > > >         return 0;
> > > > @@ -921,7 +921,7 @@ static enum hrtimer_restart
> > > > pm_suspend_timer_fn(struct hrtimer *timer)
> > > >          * If 'expires' is after the current time, we've been called
> > > >          * too early.
> > > >          */
> > > > -       if (expires > 0 && expires < ktime_to_ns(ktime_get())) {
> > > > +       if (expires > 0 && expires < ktime_get_mono_fast_ns()) {
> > > >                 dev->power.timer_expires = 0;
> > > >                 rpm_suspend(dev, dev->power.timer_autosuspends ?
> > > >                     (RPM_ASYNC | RPM_AUTO) : RPM_ASYNC); @@ -940,7
> > > > +940,7 @@ static enum hrtimer_restart  pm_suspend_timer_fn(struct
> > > > hrtimer
> > > > *timer)  int pm_schedule_suspend(struct device *dev, unsigned int
> delay)  {
> > > >         unsigned long flags;
> > > > -       ktime_t expires;
> > > > +       u64 expires;
> > > >         int retval;
> > > >
> > > >         spin_lock_irqsave(&dev->power.lock, flags); @@ -957,8
> > > > +957,8 @@ int pm_schedule_suspend(struct device *dev, unsigned int
> delay)
> > > >         /* Other scheduled or pending requests need to be canceled. */
> > > >         pm_runtime_cancel_pending(dev);
> > > >
> > > > -       expires = ktime_add(ktime_get(), ms_to_ktime(delay));
> > > > -       dev->power.timer_expires = ktime_to_ns(expires);
> > > > +       expires = ktime_get_mono_fast_ns() + ((u64)delay *
> > > > NSEC_PER_MSEC);
> > > > +       dev->power.timer_expires = expires;
> > > >         dev->power.timer_autosuspends = 0;
> > > >         hrtimer_start(&dev->power.suspend_timer, expires,
> > > > HRTIMER_MODE_ABS);
> > > >
> > > >
> > > > please find the logs.
> > > >
> > > > root@salvator-x:~# echo  e60f0000.timer >
> > > > /sys/devices/system/clocksource/clocksource0/current_clocksource
> > > > [   42.769260]
> > > > [   42.770760] ============================================
> > > > [   42.776068] WARNING: possible recursive locking detected
> > > > [   42.781378] 5.0.0-rc3-next-20190125-00001-gfa6709c-dirty #33 Not
> tainted
> > > > [   42.788076] --------------------------------------------
> > > > [   42.793385] migration/0/11 is trying to acquire lock:
> > > > [   42.798435] 00000000b43b7183 (tk_core.seq){----}, at:
> > > > rpm_resume+0x5f0/0x7a8
> > > > [   42.805497]
> > > > [   42.805497] but task is already holding lock:
> > > > [   42.811326] 00000000b43b7183 (tk_core.seq){----}, at:
> > > > multi_cpu_stop+0x8c/0x140
> > > > [   42.818641]
> > > > [   42.818641] other info that might help us debug this:
> > > > [   42.825165]  Possible unsafe locking scenario:
> > > > [   42.825165]
> > > > [   42.831080]        CPU0
> > > > [   42.833521]        ----
> > > > [   42.835962]   lock(tk_core.seq);
> > > > [   42.839185]   lock(tk_core.seq);
> > > > [   42.842409]
> > > > [   42.842409]  *** DEADLOCK ***
> > > > [   42.842409]
> > > > [   42.848328]  May be due to missing lock nesting notation
> > > > [   42.848328]
> > > > [   42.855115] 4 locks held by migration/0/11:
> > > > [   42.859293]  #0: 00000000a7511731 (timekeeper_lock){-.-.}, at:
> > > > change_clocksource+0x2c/0x118
> > > > [   42.867738]  #1: 00000000b43b7183 (tk_core.seq){----}, at:
> > > > multi_cpu_stop+0x8c/0x140
> > > > [   42.875484]  #2: 00000000b54c38a2 (&ch->lock){....}, at:
> > > > sh_cmt_start+0x28/0x200
> > > > [   42.882887]  #3: 0000000010d14eec (&(&dev->power.lock)->rlock){-
> ...}, at:
> > > > __rpm_callback+0xbc/0x1e8
> > > > [   42.891937]
> > > > [   42.891937] stack backtrace:
> > > > [   42.896294] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.0.0-rc3-
> next-
> > > > 20190125-00001-gfa6709c-dirty #33
> > > > [   42.905860] Hardware name: Renesas Salvator-X 2nd version board
> based
> > > > on r8a7796 (DT)
> > > > [   42.913689] Call trace:
> > > > [   42.916137]  dump_backtrace+0x0/0x178
> > > > [   42.919796]  show_stack+0x14/0x20
> > > > [   42.923111]  dump_stack+0xb0/0xec
> > > > [   42.926425]  __lock_acquire+0xfb4/0x1c08
> > > > [   42.930345]  lock_acquire+0xd0/0x268
> > > > [   42.933917]  ktime_get+0x5c/0x108
> > > > [   42.937229]  rpm_resume+0x5f0/0x7a8
> > > > [   42.940714]  __pm_runtime_resume+0x50/0x98
> > > > [   42.944807]  sh_cmt_start+0x84/0x200
> > > > [   42.948380]  sh_cmt_clocksource_enable+0x28/0x48
> > > > [   42.952995]  change_clocksource+0x84/0x118
> > > > [   42.957087]  multi_cpu_stop+0x8c/0x140
> > > > [   42.960834]  cpu_stopper_thread+0xac/0x120
> > > > [   42.964931]  smpboot_thread_fn+0x1ac/0x2c8
> > > > [   42.969024]  kthread+0x12c/0x130
> > > > [   42.972249]  ret_from_fork+0x10/0x18
> > > >
> > > > Regards,
> > > > Biju
> > > >
> > > > > -----Original Message-----
> > > > > From: Vincent Guittot <[e-mail]>
> > > > > Sent: 25 January 2019 14:06
> > > > > Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT
> > > > > device nodes
> > > > >
> > > > > Hi Biju,
> > > > >
> > > > > It seems that the call of mark_last_busy in rpm_resume raises
> > > > > this
> > > > problem.
> > > > > We have the sequence:
> > > > > change_clocksource
> > > > >
> > > > >     write_seqcount_begin
> > > > >     ...
> > > > >     timekeeping_update
> > > > >         ...
> > > > >         sh_cmt_clocksource_enable
> > > > >             ...
> > > > >             rpm_resume
> > > > >                 pm_runtime_mark_last_busy
> > > > >                     ktime_get
> > > > >                         do
> > > > >                             read_seqcount_begin
> > > > >                         while read_seqcount_retry
> > > > >     ....
> > > > >     write_seqcount_end
> > > > >
> > > > > In fact, we should be safe because we haven't yet changed the
> > > > > clocksource as we are enabling the clocksource that will be used
> > > > > for the
> > > > switch.
> > > > >
> > > > > Can you try the patch below ?
> > > > > It uses ktime_get_mono_fast_ns instead which is lock safe for
> > > > > such case
> > > > >
> > > > > ---
> > > > >  drivers/base/power/runtime.c | 10 +++++-----
> > > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/base/power/runtime.c
> > > > > b/drivers/base/power/runtime.c index 457be03..708a13f 100644
> > > > > --- a/drivers/base/power/runtime.c
> > > > > +++ b/drivers/base/power/runtime.c
> > > > > @@ -130,7 +130,7 @@ u64
> pm_runtime_autosuspend_expiration(struct
> > > > > device *dev)  {
> > > > >  int autosuspend_delay;
> > > > >  u64 last_busy, expires = 0;
> > > > > -u64 now = ktime_to_ns(ktime_get());
> > > > > +u64 now = ktime_get_mono_fast_ns();
> > > > >
> > > > >  if (!dev->power.use_autosuspend)  goto out; @@ -909,7 +909,7 @@
> > > > > static enum hrtimer_restart pm_suspend_timer_fn(struct hrtimer
> > > > > *timer)
> > > > >   * If 'expires' is after the current time, we've been called
> > > > >   * too early.
> > > > >   */
> > > > > -if (expires > 0 && expires < ktime_to_ns(ktime_get())) {
> > > > > +if (expires > 0 && expires < ktime_get_mono_fast_ns()) {
> > > > >  dev->power.timer_expires = 0;
> > > > >  rpm_suspend(dev, dev->power.timer_autosuspends ?
> > > > >      (RPM_ASYNC | RPM_AUTO) : RPM_ASYNC); @@ -928,7
> > > > > +928,7 @@ static enum hrtimer_restart
> > > > > +pm_suspend_timer_fn(struct
> > > > > hrtimer *timer)  int pm_schedule_suspend(struct device *dev,
> > > > > unsigned int
> > > > > delay)  {
> > > > >  unsigned long flags;
> > > > > -ktime_t expires;
> > > > > +u64 expires;
> > > > >  int retval;
> > > > >
> > > > >  spin_lock_irqsave(&dev->power.lock, flags); @@ -945,8 +945,8 @@
> > > > int
> > > > > pm_schedule_suspend(struct device *dev, unsigned int delay)
> > > > >  /* Other scheduled or pending requests need to be canceled. */
> > > > > pm_runtime_cancel_pending(dev);
> > > > >
> > > > > -expires = ktime_add(ktime_get(), ms_to_ktime(delay));
> > > > > -dev->power.timer_expires = ktime_to_ns(expires);
> > > > > +expires = ktime_get_mono_fast_ns() + (u64)delay *
> > > > > NSEC_PER_MSEC);
> > > > > +dev->power.timer_expires = expires;
> > > > >  dev->power.timer_autosuspends = 0;
> > > > > hrtimer_start(&dev->power.suspend_timer, expires,
> > > > HRTIMER_MODE_ABS);
> > > > >
> > > > > --
> > > > > 2.7.4
> > > > >
> > > > > Le Friday 25 Jan 2019 à 12:26:54 (+0000), Biju Das a écrit :
> > > > > > Hi All,
> > > > > >
> > > > > > I have enabled lock debugging  and it is showing "possible
> > > > > > recursive locking
> > > > > detected "with the patch
> ("8234f6734c5d74ac794e5517437f51c57d65f865"
> > > > > PM-runtime: Switch autosuspend over to using hrtimers)
> > > > > >
> > > > > > root@salvator-x:~# echo  e60f0000.timer >
> > > > > /sys/devices/system/clocksource/clocksource0/current_clocksource
> > > > > > [   67.995854]
> > > > > > [   67.997354]
> ============================================
> > > > > > [   68.002663] WARNING: possible recursive locking detected
> > > > > > [   68.007974] 5.0.0-rc3-next-20190125-00005-g382822b #31 Not
> tainted
> > > > > > [   68.014150] --------------------------------------------
> > > > > > [   68.019459] migration/0/11 is trying to acquire lock:
> > > > > > [   68.024509] 0000000065de5fdf (tk_core.seq){----}, at:
> > > > > rpm_resume+0x5f0/0x7a8
> > > > > > [   68.031570]
> > > > > > [   68.031570] but task is already holding lock:
> > > > > > [   68.037399] 0000000065de5fdf (tk_core.seq){----}, at:
> > > > > multi_cpu_stop+0x8c/0x140
> > > > > > [   68.044714]
> > > > > > [   68.044714] other info that might help us debug this:
> > > > > > [   68.051237]  Possible unsafe locking scenario:
> > > > > > [   68.051237]
> > > > > > [   68.057153]        CPU0
> > > > > > [   68.059594]        ----
> > > > > > [   68.062034]   lock(tk_core.seq);
> > > > > > [   68.065258]   lock(tk_core.seq);
> > > > > > [   68.068482]
> > > > > > [   68.068482]  *** DEADLOCK ***
> > > > > > [   68.068482]
> > > > > > [   68.074402]  May be due to missing lock nesting notation
> > > > > > [   68.074402]
> > > > > > [   68.081188] 4 locks held by migration/0/11:
> > > > > > [   68.085366]  #0: 00000000463b6446 (timekeeper_lock){-.-.}, at:
> > > > > change_clocksource+0x2c/0x118
> > > > > > [   68.093812]  #1: 0000000065de5fdf (tk_core.seq){----}, at:
> > > > > multi_cpu_stop+0x8c/0x140
> > > > > > [   68.101558]  #2: 0000000098b45550 (&ch->lock){....}, at:
> > > > > sh_cmt_start+0x28/0x200
> > > > > > [   68.108961]  #3: 00000000da0f4e80 (&(&dev->power.lock)-
> >rlock){-...},
> > > > at:
> > > > > __rpm_callback+0xbc/0x1e8
> > > > > > [   68.118011]
> > > > > > [   68.118011] stack backtrace:
> > > > > > [   68.122368] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.0.0-
> rc3-
> > > > next-
> > > > > 20190125-00005-g382822b #31
> > > > > > [   68.131412] Hardware name: Renesas Salvator-X 2nd version
> board
> > > > based
> > > > > on r8a7796 (DT)
> > > > > > [   68.139240] Call trace:
> > > > > > [   68.141687]  dump_backtrace+0x0/0x178
> > > > > > [   68.145346]  show_stack+0x14/0x20
> > > > > > [   68.148661]  dump_stack+0xb0/0xec
> > > > > > [   68.151975]  __lock_acquire+0xfb4/0x1c08
> > > > > > [   68.155894]  lock_acquire+0xd0/0x268
> > > > > > [   68.159467]  ktime_get+0x5c/0x108
> > > > > > [   68.162779]  rpm_resume+0x5f0/0x7a8
> > > > > > [   68.166265]  __pm_runtime_resume+0x50/0x98
> > > > > > [   68.170358]  sh_cmt_start+0x84/0x200
> > > > > > [   68.173931]  sh_cmt_clocksource_enable+0x28/0x48
> > > > > > [   68.178546]  change_clocksource+0x84/0x118
> > > > > > [   68.182639]  multi_cpu_stop+0x8c/0x140
> > > > > > [   68.186385]  cpu_stopper_thread+0xac/0x120
> > > > > > [   68.190481]  smpboot_thread_fn+0x1ac/0x2c8
> > > > > > [   68.194574]  kthread+0x12c/0x130
> > > > > > [   68.197800]  ret_from_fork+0x10/0x18
> > > > > >
> > > > > > Regards,
> > > > > > Biju
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Biju Das
> > > > > > > Sent: 25 January 2019 11:27
> > > > > > > To: 'Geert Uytterhoeven' <[e-mail]>; Vincent
> > > > > > > Guittot <[e-mail]>; Sergei Shtylyov
> > > > > > > <[e-mail]>
> > > > > > > Cc: Rob Herring <[e-mail]>; Mark Rutland
> > > > > > > <[e-mail]>; Simon Horman
> <[e-mail]>;
> > > > > Magnus
> > > > > > > Damm <[e-mail]>; Linux-Renesas <linux-
> renesas-
> > > > > > > [e-mail]>; open list:OPEN FIRMWARE AND
> FLATTENED
> > > > > DEVICE
> > > > > > > TREE BINDINGS <[e-mail]>; Geert
> > > > > > > Uytterhoeven <[e-mail]>; Chris Paterson
> > > > > > > <[e-mail]>; Daniel Lezcano
> > > > > > > <[e-mail]>; Thomas Gleixner
> > > > > > > <[e-mail]>; John Stultz <[e-mail]>;
> > > > > > > Fabrizio Castro <[e-mail]>; Samuel
> > > > > > > Holland <[e-mail]>
> > > > > > > Subject: RE: [PATCH] arm64: dts: renesas: r8a7796: Add CMT
> > > > > > > device nodes
> > > > > > >
> > > > > > > Hi Geert,
> > > > > > >
> > > > > > > Thanks for the feedback. I started testing CMT on latest
> > > > > > > kernel(Linux-next-
> > > > > > > 20190125 and also renesas-dev) and found that it is broken
> > > > > > > on R-Car M3-W device.
> > > > > > >
> > > > > > > On further investigation the patch ("
> > > > > > > 8234f6734c5d74ac794e5517437f51c57d65f865"   PM-runtime:
> Switch
> > > > > > > autosuspend over to using hrtimers)  is causing the issue.
> > > > > > >
> > > > > > > During clock source switching, It calls the function
> "sh_cmt_enable"
> > > > > > > which calls " pm_runtime_get_sync(&ch->cmt->pdev->dev);"
> > > > > > > and after that console freezes.
> > > > > > >
> > > > > > > Sergei: Have you noticed this issue  on R-Car V3M and V3H
> > > > > > > boards with latest kernel?
> > > > > > >
> > > > > > > Regards,
> > > > > > > Biju
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: [e-mail]
> > > > > > > > <linux-renesas-soc- [e-mail]> On Behalf Of
> > > > > > > > Geert Uytterhoeven
> > > > > > > > Sent: 24 January 2019 10:16
> > > > > > > > To: Biju Das <[e-mail]>
> > > > > > > > Cc: Rob Herring <[e-mail]>; Mark Rutland
> > > > > > > > <[e-mail]>; Simon Horman
> <[e-mail]>;
> > > > > > > Magnus Damm
> > > > > > > > <[e-mail]>; Linux-Renesas <linux-renesas-
> > > > > > > > [e-mail]>; open list:OPEN FIRMWARE AND
> > > > > > > > FLATTENED
> > > > > DEVICE
> > > > > > > > TREE BINDINGS <[e-mail]>; Geert
> > > > > > > > Uytterhoeven <[e-mail]>; Chris Paterson
> > > > > > > > <[e-mail]>; Daniel Lezcano
> > > > > > > > <[e-mail]>; Thomas Gleixner
> > > > > > > > <[e-mail]>; John Stultz
> > > > > > > > <[e-mail]>; Fabrizio Castro
> > > > > > > > <[e-mail]>; Samuel Holland
> > > > > > > <[e-mail]>
> > > > > > > > Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT
> > > > > > > > device nodes
> > > > > > > >
> > > > > > > > Hi Biju,
> > > > > > > >
> > > > > > > > On Fri, Oct 26, 2018 at 10:32 AM Biju Das
> > > > > > > > <[e-mail]>
> > > > > > > wrote:
> > > > > > > > > This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Biju Das <[e-mail]>
> > > > > > > > > ---
> > > > > > > > > This patch is tested against renesas-dev
> > > > > > > > >
> > > > > > > > > I have executed on inconsistency-check, nanosleep and
> > > > > > > > > clocksource_switch selftests on this arm64 SoC. The
> > > > > > > > > inconsistency-check and nanosleep tests are working
> > > > > > > > > fine.The clocksource_switch asynchronous test is failing
> > > > > > > > > due to inconsistency-check
> > > > > > > > failure on "arch_sys_counter".
> > > > > > > > >
> > > > > > > > > But if i skip the clocksource_switching of
> > > > > > > > > "arch_sys_counter", the asynchronous test is passing for
> CMT0/1/2/3 timer.
> > > > > > > > >
> > > > > > > > > Has any one noticed this issue?
> > > > > > > >
> > > > > > > > clockevents/next now has commit 7cd6dca3600d8d71
> > > > > > > > ("clocksource/drivers/arch_timer: Workaround for Allwinner
> > > > > > > > A64 timer instability").  Perhaps this is related, and the
> > > > > > > > same test program may indicate similar issues?
> > > > > > > >
> > > > > > > > See also
> > > > > > > > https://lore.kernel.org/lkml/20190113021719.46457-2-
> > > > > > > > [e-mail]/
> > > > > > > >
> > > > > > > > Gr{oetje,eeting}s,
> > > > > > > >
> > > > > > > >                         Geert
> > > > > > > >
> > > > > > > > --
> > > > > > > > Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> > > > > > > > geert@linux- m68k.org
> > > > > > > >
> > > > > > > > In personal conversations with technical people, I call
> > > > > > > > myself a hacker. But when I'm talking to journalists I
> > > > > > > > just say "programmer" or
> > > > > > > something like that.
> > > > > > > >                                 -- Linus Torvalds
> > > > > >
> > > > > >
> > > > > >
> > > > > > Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road,
> > > > > > Bourne
> > > > > End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales
> > > > > under Registered No. 04586709.
> > >
> > >
> > >
> > > Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne
> End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under
> Registered No. 04586709.



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Vincent Guittot Jan. 25, 2019, 3:20 p.m. UTC | #27

On Fri, 25 Jan 2019 at 16:12, Biju Das <[e-mail]> wrote:
>
> Hi Vincent and Rafael,
>
> Are you going to send the patch for the dead lock fix? Or Do you want me to send the patch?

I will send it once agreed with Rafael that's the best solution.
Furthermore, linux next has few more patches that modify this part so
we have to decide how to apply it for v5.0-rcX

Thanks
Vincent
>
> Please let me know.
>
> Regards,
> Biju
>
> > Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device nodes
> >
> > Adding Rafael
> >
> > On Fri, 25 Jan 2019 at 15:48, Vincent Guittot <[e-mail]>
> > wrote:
> > >
> > > On Fri, 25 Jan 2019 at 15:45, Biju Das <[e-mail]> wrote:
> > > >
> > > > Hi Vincent,
> > > >
> > > > The below patch fixed deadlock.
> > > >
> > > > --- a/include/linux/pm_runtime.h
> > > > +++ b/include/linux/pm_runtime.h
> > > > @@ -105,7 +105,7 @@ static inline bool
> > > > pm_runtime_callbacks_present(struct device *dev)
> > > >
> > > >  static inline void pm_runtime_mark_last_busy(struct device *dev)  {
> > > > -       WRITE_ONCE(dev->power.last_busy, ktime_to_ns(ktime_get()));
> > > > +       WRITE_ONCE(dev->power.last_busy, ktime_get_mono_fast_ns());
> > >
> > > Ah yes. I have searched in runtime.c but  forgot to looked in
> > > pm_runtime.h
> >
> > Rafael,
> >
> > When pm-runtime is used for a clocksource, we get a deadlock because we
> > call pm_runtime_mark_last_busy() when ebaling the clocksource whereas
> > we are in the write_seqcount sequence.
> > Using ktime_get_mono_fast_ns fixes the problem because it doesn't use
> > tk_core.seq
> >
> > Regards,
> > Vincent
> >
> > >
> > > >  }
> > > >
> > > > Regards,
> > > > Biju
> > > > > -----Original Message-----
> > > > > From: Biju Das
> > > > > Sent: 25 January 2019 14:26
> > > > > To: 'Vincent Guittot' <[e-mail]>
> > > > > Cc: Geert Uytterhoeven <[e-mail]>; Sergei Shtylyov
> > > > > <[e-mail]>; Rob Herring
> > > > > <[e-mail]>; Mark Rutland <[e-mail]>;
> > Simon
> > > > > Horman <[e-mail]>; Magnus Damm
> > <[e-mail]>;
> > > > > Linux-Renesas <linux-renesas- [e-mail]>; open list:OPEN
> > > > > FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
> > > > > <[e-mail]>; Geert Uytterhoeven
> > > > > <[e-mail]>; Chris Paterson
> > > > > <[e-mail]>; Daniel Lezcano
> > > > > <[e-mail]>; Thomas Gleixner <[e-mail]>;
> > > > > John Stultz <[e-mail]>; Fabrizio Castro
> > > > > <[e-mail]>; Samuel Holland
> > > > > <[e-mail]>
> > > > > Subject: RE: [PATCH] arm64: dts: renesas: r8a7796: Add CMT device
> > > > > nodes
> > > > >
> > > > > Hi Vincent,
> > > > >
> > > > > Thanks for the update. I am still seeing the same deadlock.
> > > > >
> > > > > diff --git a/drivers/base/power/runtime.c
> > > > > b/drivers/base/power/runtime.c index fb5e2b6..a96641a 100644
> > > > > --- a/drivers/base/power/runtime.c
> > > > > +++ b/drivers/base/power/runtime.c
> > > > > @@ -155,7 +155,7 @@ u64
> > pm_runtime_autosuspend_expiration(struct
> > > > > device *dev)
> > > > >
> > > > >         expires  = READ_ONCE(dev->power.last_busy);
> > > > >         expires += (u64)autosuspend_delay * NSEC_PER_MSEC;
> > > > > -       if (expires > ktime_to_ns(ktime_get()))
> > > > > +       if (expires > ktime_get_mono_fast_ns())
> > > > >                 return expires; /* Expires in the future */
> > > > >
> > > > >         return 0;
> > > > > @@ -921,7 +921,7 @@ static enum hrtimer_restart
> > > > > pm_suspend_timer_fn(struct hrtimer *timer)
> > > > >          * If 'expires' is after the current time, we've been called
> > > > >          * too early.
> > > > >          */
> > > > > -       if (expires > 0 && expires < ktime_to_ns(ktime_get())) {
> > > > > +       if (expires > 0 && expires < ktime_get_mono_fast_ns()) {
> > > > >                 dev->power.timer_expires = 0;
> > > > >                 rpm_suspend(dev, dev->power.timer_autosuspends ?
> > > > >                     (RPM_ASYNC | RPM_AUTO) : RPM_ASYNC); @@ -940,7
> > > > > +940,7 @@ static enum hrtimer_restart  pm_suspend_timer_fn(struct
> > > > > hrtimer
> > > > > *timer)  int pm_schedule_suspend(struct device *dev, unsigned int
> > delay)  {
> > > > >         unsigned long flags;
> > > > > -       ktime_t expires;
> > > > > +       u64 expires;
> > > > >         int retval;
> > > > >
> > > > >         spin_lock_irqsave(&dev->power.lock, flags); @@ -957,8
> > > > > +957,8 @@ int pm_schedule_suspend(struct device *dev, unsigned int
> > delay)
> > > > >         /* Other scheduled or pending requests need to be canceled. */
> > > > >         pm_runtime_cancel_pending(dev);
> > > > >
> > > > > -       expires = ktime_add(ktime_get(), ms_to_ktime(delay));
> > > > > -       dev->power.timer_expires = ktime_to_ns(expires);
> > > > > +       expires = ktime_get_mono_fast_ns() + ((u64)delay *
> > > > > NSEC_PER_MSEC);
> > > > > +       dev->power.timer_expires = expires;
> > > > >         dev->power.timer_autosuspends = 0;
> > > > >         hrtimer_start(&dev->power.suspend_timer, expires,
> > > > > HRTIMER_MODE_ABS);
> > > > >
> > > > >
> > > > > please find the logs.
> > > > >
> > > > > root@salvator-x:~# echo  e60f0000.timer >
> > > > > /sys/devices/system/clocksource/clocksource0/current_clocksource
> > > > > [   42.769260]
> > > > > [   42.770760] ============================================
> > > > > [   42.776068] WARNING: possible recursive locking detected
> > > > > [   42.781378] 5.0.0-rc3-next-20190125-00001-gfa6709c-dirty #33 Not
> > tainted
> > > > > [   42.788076] --------------------------------------------
> > > > > [   42.793385] migration/0/11 is trying to acquire lock:
> > > > > [   42.798435] 00000000b43b7183 (tk_core.seq){----}, at:
> > > > > rpm_resume+0x5f0/0x7a8
> > > > > [   42.805497]
> > > > > [   42.805497] but task is already holding lock:
> > > > > [   42.811326] 00000000b43b7183 (tk_core.seq){----}, at:
> > > > > multi_cpu_stop+0x8c/0x140
> > > > > [   42.818641]
> > > > > [   42.818641] other info that might help us debug this:
> > > > > [   42.825165]  Possible unsafe locking scenario:
> > > > > [   42.825165]
> > > > > [   42.831080]        CPU0
> > > > > [   42.833521]        ----
> > > > > [   42.835962]   lock(tk_core.seq);
> > > > > [   42.839185]   lock(tk_core.seq);
> > > > > [   42.842409]
> > > > > [   42.842409]  *** DEADLOCK ***
> > > > > [   42.842409]
> > > > > [   42.848328]  May be due to missing lock nesting notation
> > > > > [   42.848328]
> > > > > [   42.855115] 4 locks held by migration/0/11:
> > > > > [   42.859293]  #0: 00000000a7511731 (timekeeper_lock){-.-.}, at:
> > > > > change_clocksource+0x2c/0x118
> > > > > [   42.867738]  #1: 00000000b43b7183 (tk_core.seq){----}, at:
> > > > > multi_cpu_stop+0x8c/0x140
> > > > > [   42.875484]  #2: 00000000b54c38a2 (&ch->lock){....}, at:
> > > > > sh_cmt_start+0x28/0x200
> > > > > [   42.882887]  #3: 0000000010d14eec (&(&dev->power.lock)->rlock){-
> > ...}, at:
> > > > > __rpm_callback+0xbc/0x1e8
> > > > > [   42.891937]
> > > > > [   42.891937] stack backtrace:
> > > > > [   42.896294] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.0.0-rc3-
> > next-
> > > > > 20190125-00001-gfa6709c-dirty #33
> > > > > [   42.905860] Hardware name: Renesas Salvator-X 2nd version board
> > based
> > > > > on r8a7796 (DT)
> > > > > [   42.913689] Call trace:
> > > > > [   42.916137]  dump_backtrace+0x0/0x178
> > > > > [   42.919796]  show_stack+0x14/0x20
> > > > > [   42.923111]  dump_stack+0xb0/0xec
> > > > > [   42.926425]  __lock_acquire+0xfb4/0x1c08
> > > > > [   42.930345]  lock_acquire+0xd0/0x268
> > > > > [   42.933917]  ktime_get+0x5c/0x108
> > > > > [   42.937229]  rpm_resume+0x5f0/0x7a8
> > > > > [   42.940714]  __pm_runtime_resume+0x50/0x98
> > > > > [   42.944807]  sh_cmt_start+0x84/0x200
> > > > > [   42.948380]  sh_cmt_clocksource_enable+0x28/0x48
> > > > > [   42.952995]  change_clocksource+0x84/0x118
> > > > > [   42.957087]  multi_cpu_stop+0x8c/0x140
> > > > > [   42.960834]  cpu_stopper_thread+0xac/0x120
> > > > > [   42.964931]  smpboot_thread_fn+0x1ac/0x2c8
> > > > > [   42.969024]  kthread+0x12c/0x130
> > > > > [   42.972249]  ret_from_fork+0x10/0x18
> > > > >
> > > > > Regards,
> > > > > Biju
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Vincent Guittot <[e-mail]>
> > > > > > Sent: 25 January 2019 14:06
> > > > > > Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT
> > > > > > device nodes
> > > > > >
> > > > > > Hi Biju,
> > > > > >
> > > > > > It seems that the call of mark_last_busy in rpm_resume raises
> > > > > > this
> > > > > problem.
> > > > > > We have the sequence:
> > > > > > change_clocksource
> > > > > >
> > > > > >     write_seqcount_begin
> > > > > >     ...
> > > > > >     timekeeping_update
> > > > > >         ...
> > > > > >         sh_cmt_clocksource_enable
> > > > > >             ...
> > > > > >             rpm_resume
> > > > > >                 pm_runtime_mark_last_busy
> > > > > >                     ktime_get
> > > > > >                         do
> > > > > >                             read_seqcount_begin
> > > > > >                         while read_seqcount_retry
> > > > > >     ....
> > > > > >     write_seqcount_end
> > > > > >
> > > > > > In fact, we should be safe because we haven't yet changed the
> > > > > > clocksource as we are enabling the clocksource that will be used
> > > > > > for the
> > > > > switch.
> > > > > >
> > > > > > Can you try the patch below ?
> > > > > > It uses ktime_get_mono_fast_ns instead which is lock safe for
> > > > > > such case
> > > > > >
> > > > > > ---
> > > > > >  drivers/base/power/runtime.c | 10 +++++-----
> > > > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/base/power/runtime.c
> > > > > > b/drivers/base/power/runtime.c index 457be03..708a13f 100644
> > > > > > --- a/drivers/base/power/runtime.c
> > > > > > +++ b/drivers/base/power/runtime.c
> > > > > > @@ -130,7 +130,7 @@ u64
> > pm_runtime_autosuspend_expiration(struct
> > > > > > device *dev)  {
> > > > > >  int autosuspend_delay;
> > > > > >  u64 last_busy, expires = 0;
> > > > > > -u64 now = ktime_to_ns(ktime_get());
> > > > > > +u64 now = ktime_get_mono_fast_ns();
> > > > > >
> > > > > >  if (!dev->power.use_autosuspend)  goto out; @@ -909,7 +909,7 @@
> > > > > > static enum hrtimer_restart pm_suspend_timer_fn(struct hrtimer
> > > > > > *timer)
> > > > > >   * If 'expires' is after the current time, we've been called
> > > > > >   * too early.
> > > > > >   */
> > > > > > -if (expires > 0 && expires < ktime_to_ns(ktime_get())) {
> > > > > > +if (expires > 0 && expires < ktime_get_mono_fast_ns()) {
> > > > > >  dev->power.timer_expires = 0;
> > > > > >  rpm_suspend(dev, dev->power.timer_autosuspends ?
> > > > > >      (RPM_ASYNC | RPM_AUTO) : RPM_ASYNC); @@ -928,7
> > > > > > +928,7 @@ static enum hrtimer_restart
> > > > > > +pm_suspend_timer_fn(struct
> > > > > > hrtimer *timer)  int pm_schedule_suspend(struct device *dev,
> > > > > > unsigned int
> > > > > > delay)  {
> > > > > >  unsigned long flags;
> > > > > > -ktime_t expires;
> > > > > > +u64 expires;
> > > > > >  int retval;
> > > > > >
> > > > > >  spin_lock_irqsave(&dev->power.lock, flags); @@ -945,8 +945,8 @@
> > > > > int
> > > > > > pm_schedule_suspend(struct device *dev, unsigned int delay)
> > > > > >  /* Other scheduled or pending requests need to be canceled. */
> > > > > > pm_runtime_cancel_pending(dev);
> > > > > >
> > > > > > -expires = ktime_add(ktime_get(), ms_to_ktime(delay));
> > > > > > -dev->power.timer_expires = ktime_to_ns(expires);
> > > > > > +expires = ktime_get_mono_fast_ns() + (u64)delay *
> > > > > > NSEC_PER_MSEC);
> > > > > > +dev->power.timer_expires = expires;
> > > > > >  dev->power.timer_autosuspends = 0;
> > > > > > hrtimer_start(&dev->power.suspend_timer, expires,
> > > > > HRTIMER_MODE_ABS);
> > > > > >
> > > > > > --
> > > > > > 2.7.4
> > > > > >
> > > > > > Le Friday 25 Jan 2019 à 12:26:54 (+0000), Biju Das a écrit :
> > > > > > > Hi All,
> > > > > > >
> > > > > > > I have enabled lock debugging  and it is showing "possible
> > > > > > > recursive locking
> > > > > > detected "with the patch
> > ("8234f6734c5d74ac794e5517437f51c57d65f865"
> > > > > > PM-runtime: Switch autosuspend over to using hrtimers)
> > > > > > >
> > > > > > > root@salvator-x:~# echo  e60f0000.timer >
> > > > > > /sys/devices/system/clocksource/clocksource0/current_clocksource
> > > > > > > [   67.995854]
> > > > > > > [   67.997354]
> > ============================================
> > > > > > > [   68.002663] WARNING: possible recursive locking detected
> > > > > > > [   68.007974] 5.0.0-rc3-next-20190125-00005-g382822b #31 Not
> > tainted
> > > > > > > [   68.014150] --------------------------------------------
> > > > > > > [   68.019459] migration/0/11 is trying to acquire lock:
> > > > > > > [   68.024509] 0000000065de5fdf (tk_core.seq){----}, at:
> > > > > > rpm_resume+0x5f0/0x7a8
> > > > > > > [   68.031570]
> > > > > > > [   68.031570] but task is already holding lock:
> > > > > > > [   68.037399] 0000000065de5fdf (tk_core.seq){----}, at:
> > > > > > multi_cpu_stop+0x8c/0x140
> > > > > > > [   68.044714]
> > > > > > > [   68.044714] other info that might help us debug this:
> > > > > > > [   68.051237]  Possible unsafe locking scenario:
> > > > > > > [   68.051237]
> > > > > > > [   68.057153]        CPU0
> > > > > > > [   68.059594]        ----
> > > > > > > [   68.062034]   lock(tk_core.seq);
> > > > > > > [   68.065258]   lock(tk_core.seq);
> > > > > > > [   68.068482]
> > > > > > > [   68.068482]  *** DEADLOCK ***
> > > > > > > [   68.068482]
> > > > > > > [   68.074402]  May be due to missing lock nesting notation
> > > > > > > [   68.074402]
> > > > > > > [   68.081188] 4 locks held by migration/0/11:
> > > > > > > [   68.085366]  #0: 00000000463b6446 (timekeeper_lock){-.-.}, at:
> > > > > > change_clocksource+0x2c/0x118
> > > > > > > [   68.093812]  #1: 0000000065de5fdf (tk_core.seq){----}, at:
> > > > > > multi_cpu_stop+0x8c/0x140
> > > > > > > [   68.101558]  #2: 0000000098b45550 (&ch->lock){....}, at:
> > > > > > sh_cmt_start+0x28/0x200
> > > > > > > [   68.108961]  #3: 00000000da0f4e80 (&(&dev->power.lock)-
> > >rlock){-...},
> > > > > at:
> > > > > > __rpm_callback+0xbc/0x1e8
> > > > > > > [   68.118011]
> > > > > > > [   68.118011] stack backtrace:
> > > > > > > [   68.122368] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.0.0-
> > rc3-
> > > > > next-
> > > > > > 20190125-00005-g382822b #31
> > > > > > > [   68.131412] Hardware name: Renesas Salvator-X 2nd version
> > board
> > > > > based
> > > > > > on r8a7796 (DT)
> > > > > > > [   68.139240] Call trace:
> > > > > > > [   68.141687]  dump_backtrace+0x0/0x178
> > > > > > > [   68.145346]  show_stack+0x14/0x20
> > > > > > > [   68.148661]  dump_stack+0xb0/0xec
> > > > > > > [   68.151975]  __lock_acquire+0xfb4/0x1c08
> > > > > > > [   68.155894]  lock_acquire+0xd0/0x268
> > > > > > > [   68.159467]  ktime_get+0x5c/0x108
> > > > > > > [   68.162779]  rpm_resume+0x5f0/0x7a8
> > > > > > > [   68.166265]  __pm_runtime_resume+0x50/0x98
> > > > > > > [   68.170358]  sh_cmt_start+0x84/0x200
> > > > > > > [   68.173931]  sh_cmt_clocksource_enable+0x28/0x48
> > > > > > > [   68.178546]  change_clocksource+0x84/0x118
> > > > > > > [   68.182639]  multi_cpu_stop+0x8c/0x140
> > > > > > > [   68.186385]  cpu_stopper_thread+0xac/0x120
> > > > > > > [   68.190481]  smpboot_thread_fn+0x1ac/0x2c8
> > > > > > > [   68.194574]  kthread+0x12c/0x130
> > > > > > > [   68.197800]  ret_from_fork+0x10/0x18
> > > > > > >
> > > > > > > Regards,
> > > > > > > Biju
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Biju Das
> > > > > > > > Sent: 25 January 2019 11:27
> > > > > > > > To: 'Geert Uytterhoeven' <[e-mail]>; Vincent
> > > > > > > > Guittot <[e-mail]>; Sergei Shtylyov
> > > > > > > > <[e-mail]>
> > > > > > > > Cc: Rob Herring <[e-mail]>; Mark Rutland
> > > > > > > > <[e-mail]>; Simon Horman
> > <[e-mail]>;
> > > > > > Magnus
> > > > > > > > Damm <[e-mail]>; Linux-Renesas <linux-
> > renesas-
> > > > > > > > [e-mail]>; open list:OPEN FIRMWARE AND
> > FLATTENED
> > > > > > DEVICE
> > > > > > > > TREE BINDINGS <[e-mail]>; Geert
> > > > > > > > Uytterhoeven <[e-mail]>; Chris Paterson
> > > > > > > > <[e-mail]>; Daniel Lezcano
> > > > > > > > <[e-mail]>; Thomas Gleixner
> > > > > > > > <[e-mail]>; John Stultz <[e-mail]>;
> > > > > > > > Fabrizio Castro <[e-mail]>; Samuel
> > > > > > > > Holland <[e-mail]>
> > > > > > > > Subject: RE: [PATCH] arm64: dts: renesas: r8a7796: Add CMT
> > > > > > > > device nodes
> > > > > > > >
> > > > > > > > Hi Geert,
> > > > > > > >
> > > > > > > > Thanks for the feedback. I started testing CMT on latest
> > > > > > > > kernel(Linux-next-
> > > > > > > > 20190125 and also renesas-dev) and found that it is broken
> > > > > > > > on R-Car M3-W device.
> > > > > > > >
> > > > > > > > On further investigation the patch ("
> > > > > > > > 8234f6734c5d74ac794e5517437f51c57d65f865"   PM-runtime:
> > Switch
> > > > > > > > autosuspend over to using hrtimers)  is causing the issue.
> > > > > > > >
> > > > > > > > During clock source switching, It calls the function
> > "sh_cmt_enable"
> > > > > > > > which calls " pm_runtime_get_sync(&ch->cmt->pdev->dev);"
> > > > > > > > and after that console freezes.
> > > > > > > >
> > > > > > > > Sergei: Have you noticed this issue  on R-Car V3M and V3H
> > > > > > > > boards with latest kernel?
> > > > > > > >
> > > > > > > > Regards,
> > > > > > > > Biju
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: [e-mail]
> > > > > > > > > <linux-renesas-soc- [e-mail]> On Behalf Of
> > > > > > > > > Geert Uytterhoeven
> > > > > > > > > Sent: 24 January 2019 10:16
> > > > > > > > > To: Biju Das <[e-mail]>
> > > > > > > > > Cc: Rob Herring <[e-mail]>; Mark Rutland
> > > > > > > > > <[e-mail]>; Simon Horman
> > <[e-mail]>;
> > > > > > > > Magnus Damm
> > > > > > > > > <[e-mail]>; Linux-Renesas <linux-renesas-
> > > > > > > > > [e-mail]>; open list:OPEN FIRMWARE AND
> > > > > > > > > FLATTENED
> > > > > > DEVICE
> > > > > > > > > TREE BINDINGS <[e-mail]>; Geert
> > > > > > > > > Uytterhoeven <[e-mail]>; Chris Paterson
> > > > > > > > > <[e-mail]>; Daniel Lezcano
> > > > > > > > > <[e-mail]>; Thomas Gleixner
> > > > > > > > > <[e-mail]>; John Stultz
> > > > > > > > > <[e-mail]>; Fabrizio Castro
> > > > > > > > > <[e-mail]>; Samuel Holland
> > > > > > > > <[e-mail]>
> > > > > > > > > Subject: Re: [PATCH] arm64: dts: renesas: r8a7796: Add CMT
> > > > > > > > > device nodes
> > > > > > > > >
> > > > > > > > > Hi Biju,
> > > > > > > > >
> > > > > > > > > On Fri, Oct 26, 2018 at 10:32 AM Biju Das
> > > > > > > > > <[e-mail]>
> > > > > > > > wrote:
> > > > > > > > > > This patch adds CMT{0|1|2|3} device nodes for r8a7796 SoC.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Biju Das <[e-mail]>
> > > > > > > > > > ---
> > > > > > > > > > This patch is tested against renesas-dev
> > > > > > > > > >
> > > > > > > > > > I have executed on inconsistency-check, nanosleep and
> > > > > > > > > > clocksource_switch selftests on this arm64 SoC. The
> > > > > > > > > > inconsistency-check and nanosleep tests are working
> > > > > > > > > > fine.The clocksource_switch asynchronous test is failing
> > > > > > > > > > due to inconsistency-check
> > > > > > > > > failure on "arch_sys_counter".
> > > > > > > > > >
> > > > > > > > > > But if i skip the clocksource_switching of
> > > > > > > > > > "arch_sys_counter", the asynchronous test is passing for
> > CMT0/1/2/3 timer.
> > > > > > > > > >
> > > > > > > > > > Has any one noticed this issue?
> > > > > > > > >
> > > > > > > > > clockevents/next now has commit 7cd6dca3600d8d71
> > > > > > > > > ("clocksource/drivers/arch_timer: Workaround for Allwinner
> > > > > > > > > A64 timer instability").  Perhaps this is related, and the
> > > > > > > > > same test program may indicate similar issues?
> > > > > > > > >
> > > > > > > > > See also
> > > > > > > > > https://lore.kernel.org/lkml/20190113021719.46457-2-
> > > > > > > > > [e-mail]/
> > > > > > > > >
> > > > > > > > > Gr{oetje,eeting}s,
> > > > > > > > >
> > > > > > > > >                         Geert
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> > > > > > > > > geert@linux- m68k.org
> > > > > > > > >
> > > > > > > > > In personal conversations with technical people, I call
> > > > > > > > > myself a hacker. But when I'm talking to journalists I
> > > > > > > > > just say "programmer" or
> > > > > > > > something like that.
> > > > > > > > >                                 -- Linus Torvalds
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road,
> > > > > > > Bourne
> > > > > > End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales
> > > > > > under Registered No. 04586709.
> > > >
> > > >
> > > >
> > > > Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne
> > End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under
> > Registered No. 04586709.
>
>
>
> Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Patch

diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
index 1ec6aaa..d62febd0 100644
--- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
@@ -401,6 +401,76 @@ 
 			reg = <0 0xe6060000 0 0x50c>;
 		};
 
+		cmt0: timer@e60f0000 {
+			compatible = "renesas,r8a7796-cmt0",
+				     "renesas,rcar-gen3-cmt0";
+			reg = <0 0xe60f0000 0 0x1004>;
+			interrupts = <GIC_SPI 142 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 143 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD 303>;
+			clock-names = "fck";
+			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
+			resets = <&cpg 303>;
+			status = "disabled";
+		};
+
+		cmt1: timer@e6130000 {
+			compatible = "renesas,r8a7796-cmt1",
+				     "renesas,rcar-gen3-cmt1";
+			reg = <0 0xe6130000 0 0x1004>;
+			interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 127 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD 302>;
+			clock-names = "fck";
+			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
+			resets = <&cpg 302>;
+			status = "disabled";
+		};
+
+		cmt2: timer@e6140000 {
+			compatible = "renesas,r8a7796-cmt1",
+				     "renesas,rcar-gen3-cmt1";
+			reg = <0 0xe6140000 0 0x1004>;
+			interrupts = <GIC_SPI 398 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 399 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 400 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 401 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 402 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 403 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 404 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 405 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD 301>;
+			clock-names = "fck";
+			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
+			resets = <&cpg 301>;
+			status = "disabled";
+		};
+
+		cmt3: timer@e6148000 {
+			compatible = "renesas,r8a7796-cmt1",
+				     "renesas,rcar-gen3-cmt1";
+			reg = <0 0xe6148000 0 0x1004>;
+			interrupts = <GIC_SPI 470 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 471 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 474 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 475 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 476 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD 300>;
+			clock-names = "fck";
+			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
+			resets = <&cpg 300>;
+			status = "disabled";
+		};
+
 		cpg: clock-controller@e6150000 {
 			compatible = "renesas,r8a7796-cpg-mssr";
 			reg = <0 0xe6150000 0 0x1000>;