clk: zynqmp: fix compile testing without ZYNQMP_FIRMWARE
Subsystems (2)
PATCH 1/1 clk: zynqmp: fix compile testing without ZYNQMP_FIRMWARE
From: Arnd Bergmann <[email protected]>
When the firmware code is disabled, the incomplete error handling in the clk driver causes compile-time warnings:
drivers/clk/zynqmp/pll.c: In function 'zynqmp_pll_recalc_rate':drivers/clk/zynqmp/pll.c:147:29: error: 'fbdiv' is used uninitialized [-Werror=uninitialized]
147 | rate = parent_rate * fbdiv;
| ~~~~~~~~~~~~^~~~~~~
In function 'zynqmp_pll_get_mode',
inlined from 'zynqmp_pll_recalc_rate' at drivers/clk/zynqmp/pll.c:148:6:
drivers/clk/zynqmp/pll.c:61:27: error: 'ret_payload' is used uninitialized [-Werror=uninitialized]
61 | return ret_payload[1];
| ~~~~~~~~~~~^~~
drivers/clk/zynqmp/pll.c: In function 'zynqmp_pll_recalc_rate':
drivers/clk/zynqmp/pll.c:53:13: note: 'ret_payload' declared here
53 | u32 ret_payload[PAYLOAD_ARG_CNT];
| ^~~~~~~~~~~drivers/clk/zynqmp/clk-mux-zynqmp.c: In function 'zynqmp_clk_mux_get_parent':drivers/clk/zynqmp/clk-mux-zynqmp.c:57:16: error: 'val' is used uninitialized [-Werror=uninitialized]
57 | return val;
| ^~~
As it was apparently intentional to support this for compile testing purposes, change the code to have just enough error handling for the compiler to not notice the remaining bugs.
--- drivers/clk/zynqmp/clk-mux-zynqmp.c | 4 +++- drivers/clk/zynqmp/pll.c | 8 ++++++-- 2 files changed, 9 insertions(+), 3 deletions(-)
drivers/clk/zynqmp/clk-mux-zynqmp.c +3 -1
@@ -50,9 +50,11 @@ static u8 zynqmp_clk_mux_get_parent(struct clk_hw *hw) ret =zynqmp_pm_clock_getparent (clk_id, &val); -if (ret) +if (ret) {pr_warn_once ("%s() getparent failed for clock: %s, ret = %d\n" , __func__, clk_name, ret); +return ret; + }return val; }
drivers/clk/zynqmp/pll.c +6 -2
@@ -54,9 +54,11 @@ static inline enum pll_mode zynqmp_pll_get_mode(struct clk_hw *hw)int ret; ret =zynqmp_pm_get_pll_frac_mode (clk_id, ret_payload); -if (ret) +if (ret) {pr_warn_once ("%s() PLL get frac mode failed for %s, ret = %d\n" , __func__, clk_name, ret); +return ret; + }return ret_payload[1 ]; }
@@ -140,9 +142,11 @@ static unsigned long zynqmp_pll_recalc_rate(struct clk_hw *hw,int ret; ret =zynqmp_pm_clock_getdivider (clk_id, &fbdiv); -if (ret) +if (ret) {pr_warn_once ("%s() get divider failed for %s, ret = %d\n" , __func__, clk_name, ret); +return -1ul ; + } rate = parent_rate * fbdiv;if (zynqmp_pll_get_mode (hw) ==PLL_MODE_FRAC ) {
Signature · 1 line
2.29.2
Hi Arnd,
Thanks for the fix.
Quoted 18 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
When the firmware code is disabled, the incomplete error handling in the clk driver causes compile-time warnings:
drivers/clk/zynqmp/pll.c: In function 'zynqmp_pll_recalc_rate':drivers/clk/zynqmp/pll.c:147:29: error: 'fbdiv' is used uninitialized [-Werror=uninitialized]147 | rate = parent_rate * fbdiv; | ~~~~~~~~~~~~^~~~~~~ In function 'zynqmp_pll_get_mode', inlined from 'zynqmp_pll_recalc_rate' at drivers/clk/zynqmp/pll.c:148:6: drivers/clk/zynqmp/pll.c:61:27: error: 'ret_payload' is used uninitialized [-Werror=uninitialized] 61 | return ret_payload[1]; | ~~~~~~~~~~~^~~ drivers/clk/zynqmp/pll.c: In function 'zynqmp_pll_recalc_rate': drivers/clk/zynqmp/pll.c:53:13: note: 'ret_payload' declared here 53 | u32 ret_payload[PAYLOAD_ARG_CNT]; | ^~~~~~~~~~~
Quoted 4 lines
drivers/clk/zynqmp/clk-mux-zynqmp.c: In function 'zynqmp_clk_mux_get_parent':drivers/clk/zynqmp/clk-mux-zynqmp.c:57:16: error: 'val' is used uninitialized [-Werror=uninitialized]
57 | return val;
| ^~~
Not sure what I am missing but I couldn't reproduce these warnings. I tried a few different ways to toggle CONFIG_ZYNQMP_FIRMWARE.
Regardless...
Quoted 48 lines
As it was apparently intentional to support this for compile testing purposes, change the code to have just enough error handling for the compiler to not notice the remaining bugs.
Fixes 21f237534661 ("clk: zynqmp: Drop dependency on ARCH_ZYNQMP")Signed-off-by Arnd Bergmann <[email protected]>--- drivers/clk/zynqmp/clk-mux-zynqmp.c | 4 +++- drivers/clk/zynqmp/pll.c | 8 ++++++-- 2 files changed, 9 insertions(+), 3 deletions(-)drivers/clk/zynqmp/clk-mux-zynqmp.c +3
-1@@ -50,9 +50,11 @@ static u8 zynqmp_clk_mux_get_parent(struct clk_hw *hw) ret =zynqmp_pm_clock_getparent (clk_id, &val); -if (ret) +if (ret) {pr_warn_once ("%s() getparent failed for clock: %s, ret = %d\n" , __func__, clk_name, ret); +return ret; + }return val; }drivers/clk/zynqmp/pll.c +3
-1@@ -54,9 +54,11 @@ static inline enum pll_mode zynqmp_pll_get_mode(struct clk_hw *hw)int ret; ret =zynqmp_pm_get_pll_frac_mode (clk_id, ret_payload); -if (ret) +if (ret) {pr_warn_once ("%s() PLL get frac mode failed for %s, ret = %d\n" , __func__, clk_name, ret); +return ret; + }return ret_payload[1 ]; }@@ -140,9 +142,11 @@ static unsigned long zynqmp_pll_recalc_rate(struct clk_hw *hw,int ret;
drivers/clk/zynqmp/pll.c static unsigned long zynqmp_pll_recalc_rate(struct clk_hw *hw, · line 152
Quoted 10 lines
drivers/clk/zynqmp/pll.c +3
-1ret =zynqmp_pm_clock_getdivider (clk_id, &fbdiv); -if (ret) +if (ret) {pr_warn_once ("%s() get divider failed for %s, ret = %d\n" , __func__, clk_name, ret); +return -1ul ; + } rate = parent_rate * fbdiv;if (zynqmp_pll_get_mode (hw) ==PLL_MODE_FRAC ) {
The changes make sense in that the functions error out sensibly when the zynqmp firmware driver is not enabled.
Punit
Hi,
Quoted 30 lines · Punit Agrawal
Hi Arnd,
Thanks for the fix.
Quoted 18 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
When the firmware code is disabled, the incomplete error handling in the clk driver causes compile-time warnings:
drivers/clk/zynqmp/pll.c: In function 'zynqmp_pll_recalc_rate':drivers/clk/zynqmp/pll.c:147:29: error: 'fbdiv' is used uninitialized [-Werror=uninitialized]147 | rate = parent_rate * fbdiv; | ~~~~~~~~~~~~^~~~~~~ In function 'zynqmp_pll_get_mode', inlined from 'zynqmp_pll_recalc_rate' at drivers/clk/zynqmp/pll.c:148:6: drivers/clk/zynqmp/pll.c:61:27: error: 'ret_payload' is used uninitialized [-Werror=uninitialized] 61 | return ret_payload[1]; | ~~~~~~~~~~~^~~ drivers/clk/zynqmp/pll.c: In function 'zynqmp_pll_recalc_rate': drivers/clk/zynqmp/pll.c:53:13: note: 'ret_payload' declared here 53 | u32 ret_payload[PAYLOAD_ARG_CNT]; | ^~~~~~~~~~~Quoted 4 lines
drivers/clk/zynqmp/clk-mux-zynqmp.c: In function 'zynqmp_clk_mux_get_parent':drivers/clk/zynqmp/clk-mux-zynqmp.c:57:16: error: 'val' is used uninitialized [-Werror=uninitialized]
57 | return val;
| ^~~
Quoted 4 lines
Not sure what I am missing but I couldn't reproduce these warnings. I tried a few different ways to toggle
CONFIG_ZYNQMP_FIRMWARE.Regardless...
Me too. Can you share your .config file?
Quoted 18 lines
Quoted 18 lines
As it was apparently intentional to support this for compile testing purposes, change the code to have just enough error handling for the compiler to not notice the remaining bugs.
Fixes 21f237534661 ("clk: zynqmp: Drop dependency on ARCH_ZYNQMP")Signed-off-by Arnd Bergmann <[email protected]>--- drivers/clk/zynqmp/clk-mux-zynqmp.c | 4 +++- drivers/clk/zynqmp/pll.c | 8 ++++++-- 2 files changed, 9 insertions(+), 3 deletions(-)drivers/clk/zynqmp/clk-mux-zynqmp.c +0
-0@@ -50,9 +50,11 @@ static u8 zynqmp_clk_mux_get_parent(struct clk_hw *hw)
Quoted 7 lines
drivers/clk/zynqmp/clk-mux-zynqmp.c static u8 zynqmp_clk_mux_get_parent(struct clk_hw *hw) · line 56
Quoted 7 lines
drivers/clk/zynqmp/clk-mux-zynqmp.c +2
-1ret =zynqmp_pm_clock_getparent (clk_id, &val); -if (ret) +if (ret) {pr_warn_once ("%s() getparent failed for clock: %s, ret = %d\n" , __func__, clk_name, ret); +return ret;
return should be u8 and this can be negative value. That's why I think this should be fixed differently and all users should be checked that it is handled like that.
Quoted 11 lines
Quoted 11 lines
drivers/clk/zynqmp/clk-mux-zynqmp.c +1
-0+ }return val; }drivers/clk/zynqmp/pll.c +0
-0@@ -54,9 +54,11 @@ static inline enum pll_mode zynqmp_pll_get_mode(struct clk_hw *hw)int ret;
Quoted 7 lines
drivers/clk/zynqmp/pll.c static inline enum pll_mode zynqmp_pll_get_mode(struct clk_hw *hw) · line 61
Quoted 7 lines
drivers/clk/zynqmp/pll.c +3
-1ret =zynqmp_pm_get_pll_frac_mode (clk_id, ret_payload); -if (ret) +if (ret) {pr_warn_once ("%s() PLL get frac mode failed for %s, ret = %d\n" , __func__, clk_name, ret); +return ret; + }
Return should be enum pll_mode which is 0 or 1 which is likely not done here.
Quoted 6 lines
Quoted 6 lines
drivers/clk/zynqmp/pll.c +0
-0return ret_payload[1 ]; }@@ -140,9 +142,11 @@ static unsigned long zynqmp_pll_recalc_rate(struct clk_hw *hw,int ret;
Quoted 7 lines
drivers/clk/zynqmp/pll.c static unsigned long zynqmp_pll_recalc_rate(struct clk_hw *hw, · line 149
Quoted 7 lines
drivers/clk/zynqmp/pll.c +3
-1ret =zynqmp_pm_clock_getdivider (clk_id, &fbdiv); -if (ret) +if (ret) {pr_warn_once ("%s() get divider failed for %s, ret = %d\n" , __func__, clk_name, ret); +return -1ul ; + }
Same here.
Quoted 8 lines
drivers/clk/zynqmp/pll.c static unsigned long zynqmp_pll_recalc_rate(struct clk_hw *hw, · line 152
Quoted 3 lines
drivers/clk/zynqmp/pll.c +0
-0rate = parent_rate * fbdiv;if (zynqmp_pll_get_mode (hw) ==PLL_MODE_FRAC ) {The changes make sense in that the functions error out sensibly when the zynqmp firmware driver is not enabled.
Acked-by Punit Agrawal <[email protected]>
I think code should be checked that these error values are handled how they should be handled.
Thanks,
Michal
Quoted 114 lines · Michal Simek
Hi,
Quoted 30 lines · Punit Agrawal
Hi Arnd,
Thanks for the fix.
Quoted 18 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
When the firmware code is disabled, the incomplete error handling in the clk driver causes compile-time warnings:
drivers/clk/zynqmp/pll.c: In function 'zynqmp_pll_recalc_rate':drivers/clk/zynqmp/pll.c:147:29: error: 'fbdiv' is used uninitialized [-Werror=uninitialized]147 | rate = parent_rate * fbdiv; | ~~~~~~~~~~~~^~~~~~~ In function 'zynqmp_pll_get_mode', inlined from 'zynqmp_pll_recalc_rate' at drivers/clk/zynqmp/pll.c:148:6: drivers/clk/zynqmp/pll.c:61:27: error: 'ret_payload' is used uninitialized [-Werror=uninitialized] 61 | return ret_payload[1]; | ~~~~~~~~~~~^~~ drivers/clk/zynqmp/pll.c: In function 'zynqmp_pll_recalc_rate': drivers/clk/zynqmp/pll.c:53:13: note: 'ret_payload' declared here 53 | u32 ret_payload[PAYLOAD_ARG_CNT]; | ^~~~~~~~~~~Quoted 4 lines
drivers/clk/zynqmp/clk-mux-zynqmp.c: In function 'zynqmp_clk_mux_get_parent':drivers/clk/zynqmp/clk-mux-zynqmp.c:57:16: error: 'val' is used uninitialized [-Werror=uninitialized]
57 | return val;
| ^~~Quoted 4 lines
Not sure what I am missing but I couldn't reproduce these warnings. I tried a few different ways to toggle
CONFIG_ZYNQMP_FIRMWARE.Regardless...
Me too. Can you share your .config file?
Quoted 18 lines
Quoted 18 lines
As it was apparently intentional to support this for compile testing purposes, change the code to have just enough error handling for the compiler to not notice the remaining bugs.
Fixes 21f237534661 ("clk: zynqmp: Drop dependency on ARCH_ZYNQMP")Signed-off-by Arnd Bergmann <[email protected]>--- drivers/clk/zynqmp/clk-mux-zynqmp.c | 4 +++- drivers/clk/zynqmp/pll.c | 8 ++++++-- 2 files changed, 9 insertions(+), 3 deletions(-)drivers/clk/zynqmp/clk-mux-zynqmp.c +0
-0@@ -50,9 +50,11 @@ static u8 zynqmp_clk_mux_get_parent(struct clk_hw *hw)Quoted 7 lines
drivers/clk/zynqmp/clk-mux-zynqmp.c static u8 zynqmp_clk_mux_get_parent(struct clk_hw *hw) · line 56
Quoted 7 lines
drivers/clk/zynqmp/clk-mux-zynqmp.c +2
-1ret =zynqmp_pm_clock_getparent (clk_id, &val); -if (ret) +if (ret) {pr_warn_once ("%s() getparent failed for clock: %s, ret = %d\n" , __func__, clk_name, ret); +return ret;return should be u8 and this can be negative value. That's why I think this should be fixed differently and all users should be checked that it is handled like that.
Quoted 11 lines
Quoted 11 lines
drivers/clk/zynqmp/clk-mux-zynqmp.c +1
-0+ }return val; }drivers/clk/zynqmp/pll.c +0
-0@@ -54,9 +54,11 @@ static inline enum pll_mode zynqmp_pll_get_mode(struct clk_hw *hw)int ret;Quoted 7 lines
drivers/clk/zynqmp/pll.c static inline enum pll_mode zynqmp_pll_get_mode(struct clk_hw *hw) · line 61
Quoted 7 lines
drivers/clk/zynqmp/pll.c +3
-1ret =zynqmp_pm_get_pll_frac_mode (clk_id, ret_payload); -if (ret) +if (ret) {pr_warn_once ("%s() PLL get frac mode failed for %s, ret = %d\n" , __func__, clk_name, ret); +return ret; + }Return should be enum
pll_modewhich is 0 or 1 which is likely not done here.Quoted 6 lines
Quoted 6 lines
drivers/clk/zynqmp/pll.c +0
-0return ret_payload[1 ]; }@@ -140,9 +142,11 @@ static unsigned long zynqmp_pll_recalc_rate(struct clk_hw *hw,int ret;Quoted 7 lines
drivers/clk/zynqmp/pll.c static unsigned long zynqmp_pll_recalc_rate(struct clk_hw *hw, · line 149
Quoted 7 lines
drivers/clk/zynqmp/pll.c +3
-1ret =zynqmp_pm_clock_getdivider (clk_id, &fbdiv); -if (ret) +if (ret) {pr_warn_once ("%s() get divider failed for %s, ret = %d\n" , __func__, clk_name, ret); +return -1ul ; + }Same here.
Quoted 4 lines
Quoted 3 lines
drivers/clk/zynqmp/pll.c +0
-0rate = parent_rate * fbdiv;if (zynqmp_pll_get_mode (hw) ==PLL_MODE_FRAC ) {
Quoted 7 lines
Quoted 4 lines
The changes make sense in that the functions error out sensibly when the zynqmp firmware driver is not enabled.
Acked-by Punit Agrawal <[email protected]>I think code should be checked that these error values are handled how they should be handled.
I only looked at it from the point of view of getting rid of the warnings - based on the commit log, Arnd's patch is only taking care of the compiler warnings when the driver is built with CONFIG_COMPILE_TEST=y and likely CONFIG_ZYNQMP_FIRMWARE=n.
In practice, the code should not be hit at runtime due to the dependency on the firmware driver. Even then, a better fix would indeed be taking the returned values at call sites into account.
[...]
Stephen Boyd
maintainerQuoting Punit Agrawal (2021-04-22 23:37:25)
Quoted 12 lines
Quoted 11 lines · Michal Simek
Quoted 10 lines
Quoted 5 lines
rate = parent_rate * fbdiv; if (zynqmp_pll_get_mode(hw) == PLL_MODE_FRAC) {The changes make sense in that the functions error out sensibly when the zynqmp firmware driver is not enabled.
Acked-by Punit Agrawal <[email protected]>
Quoted 7 lines
Quoted 2 lines
I think code should be checked that these error values are handled how they should be handled.
I only looked at it from the point of view of getting rid of the warnings - based on the commit log, Arnd's patch is only taking care of the compiler warnings when the driver is built with
CONFIG_COMPILE_TEST=yand likelyCONFIG_ZYNQMP_FIRMWARE=n.
The subject line basically says this.
Quoted 4 lines
In practice, the code should not be hit at runtime due to the dependency on the firmware driver. Even then, a better fix would indeed be taking the returned values at call sites into account.
Still needs to be fixed. If a better patch is sent I would apply it, but if that isn't going to happen I'll apply this one.
Quoted 24 lines · Stephen Boyd
Quoting Punit Agrawal (2021-04-22 23:37:25)
Quoted 12 lines
Quoted 11 lines · Michal Simek
Quoted 10 lines
Quoted 5 lines
rate = parent_rate * fbdiv; if (zynqmp_pll_get_mode(hw) == PLL_MODE_FRAC) {The changes make sense in that the functions error out sensibly when the zynqmp firmware driver is not enabled.
Acked-by Punit Agrawal <[email protected]>Quoted 7 lines
Quoted 2 lines
I think code should be checked that these error values are handled how they should be handled.
I only looked at it from the point of view of getting rid of the warnings - based on the commit log, Arnd's patch is only taking care of the compiler warnings when the driver is built with
CONFIG_COMPILE_TEST=yand likelyCONFIG_ZYNQMP_FIRMWARE=n.The subject line basically says this.
Quoted 1 line
Quoted 6 lines
Quoted 3 lines
In practice, the code should not be hit at runtime due to the dependency on the firmware driver. Even then, a better fix would indeed be taking the returned values at call sites into account.
Still needs to be fixed. If a better patch is sent I would apply it, but if that isn't going to happen I'll apply this one.
I have sent v2 version based on what I have found how that error values should look like. Please take a look at v2.
https://lore.kernel.org/linux-clk/fdee3a286defb103aa07b5493b805d1987885165.1624281224.git.michal.simek@xilinx.com/
Thanks,
Michal