rtc: ac100: Fix ac100 determine rate bug
PATCH v2 1/1 rtc: ac100: Fix ac100 determine rate bug
This patch fixes a bug, that prevents the Allwinner A83T and the A80 from a successful boot. You can find the shortend trace below:
Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = (ptrval) [00000000] *pgd=00000000 Internal error: Oops: 5 [#1] SMP ARM Modules linked in: CPU: 0 PID: 49 Comm: kworker/0:1 Not tainted 4.15.0-10190-gb89e32ccd1be #2 Hardware name: Allwinner sun8i Family Workqueue: events deferred_probe_work_func PC is at clk_hw_get_rate+0x0/0x34 LR is at ac100_clkout_determine_rate+0x48/0x19c
[ ... ]
(clk_hw_get_rate) from (ac100_clkout_determine_rate+0x48/0x19c) (ac100_clkout_determine_rate) from (clk_core_set_rate_nolock+0x3c/0x1a0) (clk_core_set_rate_nolock) from (clk_set_rate+0x30/0x88) (clk_set_rate) from (of_clk_set_defaults+0x200/0x364) (of_clk_set_defaults) from (platform_drv_probe+0x18/0xb0)
To fix that bug, we first check if the return of the clk_hw_get_parent_by_index is non zero. If it is zero we skip that clock parent.
The BUG report could be found here: https://lkml.org/lkml/2018/2/10/198
---
Changes in v2:
* add tag Fixes: ... to commit message
* add comment to if statement why we are doing this check
drivers/rtc/rtc-ac100.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
drivers/rtc/rtc-ac100.c +11 -1
@@ -183,7 +183,17 @@ static int ac100_clkout_determine_rate(struct clk_hw *hw,for (i =0 ; i < num_parents; i++) {struct clk_hw *parent =clk_hw_get_parent_by_index (hw, i); -unsigned long tmp, prate =clk_hw_get_rate (parent); +unsigned long tmp, prate; + +/* +* We purposefully left open the possibility to use the clock +* from the codec side but it is not implemented right now. +* Thus we need to check if the parent exists. +*/ +if (!parent) +continue ; + + prate =clk_hw_get_rate (parent); tmp =ac100_clkout_round_rate (hw, req->rate, prate);
Signature · 1 line
2.11.0
Quoted 2 lines · Philipp Rossak
This patch fixes a bug, that prevents the Allwinner A83T and the A80 from a successful boot. You can find the shortend trace below:
Since when is it there?
Quoted 20 lines
Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = (ptrval) [00000000] *pgd=00000000 Internal error: Oops: 5 [#1] SMP ARM Modules linked in: CPU: 0 PID: 49 Comm: kworker/0:1 Not tainted 4.15.0-10190-gb89e32ccd1be #2 Hardware name: Allwinner sun8i Family Workqueue: events deferred_probe_work_func PC is at clk_hw_get_rate+0x0/0x34 LR is at ac100_clkout_determine_rate+0x48/0x19c[ ... ]
(
clk_hw_get_rate) from (ac100_clkout_determine_rate+0x48/0x19c) (ac100_clkout_determine_rate) from (clk_core_set_rate_nolock+0x3c/0x1a0) (clk_core_set_rate_nolock) from (clk_set_rate+0x30/0x88) (clk_set_rate) from (of_clk_set_defaults+0x200/0x364) (of_clk_set_defaults) from (platform_drv_probe+0x18/0xb0)
Quoted 7 lines
To fix that bug, we first check if the return of the
clk_hw_get_parent_by_indexis non zero. If it is zero we skip that clock parent.The BUG report could be found here: https://lkml.org/lkml/2018/2/10/198
Fixes 04940631b8d2 ("rtc: ac100: Add clk output support")
Should it be sent to stable?
Quoted 20 lines
Signed-off-by Philipp Rossak <[email protected]>---
Changes in v2:
* add tag Fixes: ... to commit message
* add comment to if statement why we are doing this checkdrivers/rtc/rtc-ac100.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)drivers/rtc/rtc-ac100.c +1
-1@@ -183,7 +183,17 @@ static int ac100_clkout_determine_rate(struct clk_hw *hw,for (i =0 ; i < num_parents; i++) {struct clk_hw *parent =clk_hw_get_parent_by_index (hw, i); -unsigned long tmp, prate =clk_hw_get_rate (parent); +unsigned long tmp, prate;
drivers/rtc/rtc-ac100.c static int ac100_clkout_determine_rate(struct clk_hw *hw, · line 196
Quoted 10 lines
drivers/rtc/rtc-ac100.c +10
-0+ +/* +* We purposefully left open the possibility to use the clock +* from the codec side but it is not implemented right now. +* Thus we need to check if the parent exists. +*/ +if (!parent) +continue ; + + prate =clk_hw_get_rate (parent);
clk_hw_get_num_parents should return the exact number of parents, which is going to be 1 if you only have one parent, like all DTS seems to have.
If not, then it should be explained in the comment and / or fixed properly.
Maxime
Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com
Quoted 71 lines · Maxime Ripard
Quoted 2 lines · Philipp Rossak
This patch fixes a bug, that prevents the Allwinner A83T and the A80 from a successful boot. You can find the shortend trace below:
Since when is it there?
Quoted 20 lines
Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = (ptrval) [00000000] *pgd=00000000 Internal error: Oops: 5 [#1] SMP ARM Modules linked in: CPU: 0 PID: 49 Comm: kworker/0:1 Not tainted 4.15.0-10190-gb89e32ccd1be #2 Hardware name: Allwinner sun8i Family Workqueue: events deferred_probe_work_func PC is at clk_hw_get_rate+0x0/0x34 LR is at ac100_clkout_determine_rate+0x48/0x19c[ ... ]
(
clk_hw_get_rate) from (ac100_clkout_determine_rate+0x48/0x19c) (ac100_clkout_determine_rate) from (clk_core_set_rate_nolock+0x3c/0x1a0) (clk_core_set_rate_nolock) from (clk_set_rate+0x30/0x88) (clk_set_rate) from (of_clk_set_defaults+0x200/0x364) (of_clk_set_defaults) from (platform_drv_probe+0x18/0xb0)Quoted 7 lines
To fix that bug, we first check if the return of the
clk_hw_get_parent_by_indexis non zero. If it is zero we skip that clock parent.The BUG report could be found here: https://lkml.org/lkml/2018/2/10/198
Fixes 04940631b8d2 ("rtc: ac100: Add clk output support")Should it be sent to stable?
Quoted 20 lines
Signed-off-by Philipp Rossak <[email protected]>---
Changes in v2:
* add tag Fixes: ... to commit message
* add comment to if statement why we are doing this checkdrivers/rtc/rtc-ac100.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)drivers/rtc/rtc-ac100.c +1
-1@@ -183,7 +183,17 @@ static int ac100_clkout_determine_rate(struct clk_hw *hw,for (i =0 ; i < num_parents; i++) {struct clk_hw *parent =clk_hw_get_parent_by_index (hw, i); -unsigned long tmp, prate =clk_hw_get_rate (parent); +unsigned long tmp, prate;drivers/rtc/rtc-ac100.c static int ac100_clkout_determine_rate(struct clk_hw *hw, · line 196
Quoted 10 lines
drivers/rtc/rtc-ac100.c +10
-0+ +/* +* We purposefully left open the possibility to use the clock +* from the codec side but it is not implemented right now. +* Thus we need to check if the parent exists. +*/ +if (!parent) +continue ; + + prate =clk_hw_get_rate (parent);
clk_hw_get_num_parentsshould return the exact number of parents, which is going to be 1 if you only have one parent, like all DTS seems to have.
Quoted 2 lines
If not, then it should be explained in the comment and / or fixed properly.
The clock has two parents. One is a fixed clock internally registered by the driver. This is actually an external crystal, and we should probably add a device node and the works for it. The other parent is a clock from the codec side, which we properly declare and reference in the device tree. This clock, though defined, is not implemented in any driver (because we don't have any ATM).
This second missing clock is what's causing issues here. The clk core looks for the parent by name, can't find one that is registered, and returns NULL.
I guess the comment above is still not clear enough?
ChenYu
Quoted 6 lines
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
Quoted 8 lines · Chen-Yu Tsai
Quoted 6 lines · Maxime Ripard
Quoted 2 lines · Philipp Rossak
This patch fixes a bug, that prevents the Allwinner A83T and the A80 from a successful boot. You can find the shortend trace below:
Since when is it there?
The bug is there since v4.16-rc1 and appeared after the clk branch was merged.
^^ Should I add this info also in the commit message?
Quoted 75 lines
Quoted 65 lines
Quoted 20 lines
Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = (ptrval) [00000000] *pgd=00000000 Internal error: Oops: 5 [#1] SMP ARM Modules linked in: CPU: 0 PID: 49 Comm: kworker/0:1 Not tainted 4.15.0-10190-gb89e32ccd1be #2 Hardware name: Allwinner sun8i Family Workqueue: events deferred_probe_work_func PC is at clk_hw_get_rate+0x0/0x34 LR is at ac100_clkout_determine_rate+0x48/0x19c[ ... ]
(
clk_hw_get_rate) from (ac100_clkout_determine_rate+0x48/0x19c) (ac100_clkout_determine_rate) from (clk_core_set_rate_nolock+0x3c/0x1a0) (clk_core_set_rate_nolock) from (clk_set_rate+0x30/0x88) (clk_set_rate) from (of_clk_set_defaults+0x200/0x364) (of_clk_set_defaults) from (platform_drv_probe+0x18/0xb0)Quoted 7 lines
To fix that bug, we first check if the return of the
clk_hw_get_parent_by_indexis non zero. If it is zero we skip that clock parent.The BUG report could be found here: https://lkml.org/lkml/2018/2/10/198
Fixes 04940631b8d2 ("rtc: ac100: Add clk output support")Should it be sent to stable?
Quoted 20 lines
Signed-off-by Philipp Rossak <[email protected]>---
Changes in v2:
* add tag Fixes: ... to commit message
* add comment to if statement why we are doing this checkdrivers/rtc/rtc-ac100.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)drivers/rtc/rtc-ac100.c +1
-1@@ -183,7 +183,17 @@ static int ac100_clkout_determine_rate(struct clk_hw *hw,for (i =0 ; i < num_parents; i++) {struct clk_hw *parent =clk_hw_get_parent_by_index (hw, i); -unsigned long tmp, prate =clk_hw_get_rate (parent); +unsigned long tmp, prate;drivers/rtc/rtc-ac100.c static int ac100_clkout_determine_rate(struct clk_hw *hw, · line 196
Quoted 10 lines
drivers/rtc/rtc-ac100.c +10
-0+ +/* +* We purposefully left open the possibility to use the clock +* from the codec side but it is not implemented right now. +* Thus we need to check if the parent exists. +*/ +if (!parent) +continue ; + + prate =clk_hw_get_rate (parent);
clk_hw_get_num_parentsshould return the exact number of parents, which is going to be 1 if you only have one parent, like all DTS seems to have.Quoted 2 lines
If not, then it should be explained in the comment and / or fixed properly.
The clock has two parents. One is a fixed clock internally registered by the driver. This is actually an external crystal, and we should probably add a device node and the works for it. The other parent is a clock from the codec side, which we properly declare and reference in the device tree. This clock, though defined, is not implemented in any driver (because we don't have any ATM).
Quoted 5 lines
This second missing clock is what's causing issues here. The clk core looks for the parent by name, can't find one that is registered, and returns NULL.
I guess the comment above is still not clear enough?
I can get more detailed in the comment. I thought about this:
The clock has two parents, one is a fixed clock which is internally registered by the ac100 driver. The other parent is a clock from the codec side of the chip, which we properly declare and reference in the devicetree and is not implemented in any driver right now. If the clock core looks for the parent of that second missing clock, it can't one that is registered and returns NULL.
Thus we need to check if the parent exists before we get the parent rate.
Is that ok for you?
Philipp
Quoted 9 lines
ChenYu
Quoted 6 lines
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
Quoted 9 lines · Philipp Rossak
Quoted 6 lines · Chen-Yu Tsai
Quoted 4 lines · Maxime Ripard
Quoted 2 lines · Philipp Rossak
This patch fixes a bug, that prevents the Allwinner A83T and the A80 from a successful boot. You can find the shortend trace below:
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
Since when is it there?
The bug is there since v4.16-rc1 and appeared after the clk branch was merged.
^^ Should I add this info also in the commit message?
Yes
Quoted 91 lines
Quoted 75 lines
Quoted 65 lines
Quoted 20 lines
Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = (ptrval) [00000000] *pgd=00000000 Internal error: Oops: 5 [#1] SMP ARM Modules linked in: CPU: 0 PID: 49 Comm: kworker/0:1 Not tainted 4.15.0-10190-gb89e32ccd1be #2 Hardware name: Allwinner sun8i Family Workqueue: events deferred_probe_work_func PC is at clk_hw_get_rate+0x0/0x34 LR is at ac100_clkout_determine_rate+0x48/0x19c[ ... ]
(
clk_hw_get_rate) from (ac100_clkout_determine_rate+0x48/0x19c) (ac100_clkout_determine_rate) from (clk_core_set_rate_nolock+0x3c/0x1a0) (clk_core_set_rate_nolock) from (clk_set_rate+0x30/0x88) (clk_set_rate) from (of_clk_set_defaults+0x200/0x364) (of_clk_set_defaults) from (platform_drv_probe+0x18/0xb0)Quoted 7 lines
To fix that bug, we first check if the return of the
clk_hw_get_parent_by_indexis non zero. If it is zero we skip that clock parent.The BUG report could be found here: https://lkml.org/lkml/2018/2/10/198
Fixes 04940631b8d2 ("rtc: ac100: Add clk output support")Should it be sent to stable?
Quoted 20 lines
Signed-off-by Philipp Rossak <[email protected]>---
Changes in v2:
* add tag Fixes: ... to commit message
* add comment to if statement why we are doing this checkdrivers/rtc/rtc-ac100.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)drivers/rtc/rtc-ac100.c +1
-1@@ -183,7 +183,17 @@ static int ac100_clkout_determine_rate(struct clk_hw *hw,for (i =0 ; i < num_parents; i++) {struct clk_hw *parent =clk_hw_get_parent_by_index (hw, i); -unsigned long tmp, prate =clk_hw_get_rate (parent); +unsigned long tmp, prate;drivers/rtc/rtc-ac100.c static int ac100_clkout_determine_rate(struct clk_hw *hw, · line 196
Quoted 10 lines
drivers/rtc/rtc-ac100.c +10
-0+ +/* +* We purposefully left open the possibility to use the clock +* from the codec side but it is not implemented right now. +* Thus we need to check if the parent exists. +*/ +if (!parent) +continue ; + + prate =clk_hw_get_rate (parent);
clk_hw_get_num_parentsshould return the exact number of parents, which is going to be 1 if you only have one parent, like all DTS seems to have.Quoted 2 lines
If not, then it should be explained in the comment and / or fixed properly.
The clock has two parents. One is a fixed clock internally registered by the driver. This is actually an external crystal, and we should probably add a device node and the works for it. The other parent is a clock from the codec side, which we properly declare and reference in the device tree. This clock, though defined, is not implemented in any driver (because we don't have any ATM).
Quoted 5 lines
This second missing clock is what's causing issues here. The clk core looks for the parent by name, can't find one that is registered, and returns NULL.
I guess the comment above is still not clear enough?
I can get more detailed in the comment. I thought about this:
The clock has two parents, one is a fixed clock which is internally registered by the ac100 driver. The other parent is a clock from the codec side of the chip, which we properly declare and reference in the devicetree and is not implemented in any driver right now.
If the clock core looks for the parent of that second missing clock, it can't one that is registered and returns NULL.
Thus we need to check if the parent exists before we get the parent rate.
Quoted 1 line
Is that ok for you?
Yep, much better thanks!
Maxime
Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com