Make clk_hw::init NULL after clk registration
Subsystems (11)
Stephen Boyd
maintainercover Make clk_hw::init NULL after clk registration
We don't want clk provider drivers using the init member of struct clk_hw after the clk is registered. It isn't guaranteed to be a valid pointer and all the necessary information inside the structure is copied over into struct clk_core anyway. This patch series fixes up the handful of users that do this and then overwrites the pointer to NULL within the clk registration path.
Stephen Boyd (9):
clk: actions: Don't reference clk_init_data after registration
clk: lochnagar: Don't reference clk_init_data after registration
clk: meson: axg-audio: Don't reference clk_init_data after
registration
clk: qcom: Don't reference clk_init_data after registration
clk: sirf: Don't reference clk_init_data after registration
clk: socfpga: Don't reference clk_init_data after registration
clk: sprd: Don't reference clk_init_data after registration
phy: ti: am654-serdes: Don't reference clk_init_data after
registration
clk: Overwrite clk_hw::init with NULL during clk_register()
drivers/clk/actions/owl-common.c | 3 ++- drivers/clk/clk-lochnagar.c | 2 +- drivers/clk/clk.c | 24 ++++++++++++++++-------- drivers/clk/meson/axg-audio.c | 7 +++++-- drivers/clk/qcom/clk-rpmh.c | 4 ++-- drivers/clk/sirf/clk-common.c | 12 ++++++++---- drivers/clk/socfpga/clk-gate.c | 21 +++++++++++---------- drivers/clk/socfpga/clk-periph-a10.c | 7 ++++--- drivers/clk/sprd/common.c | 5 +++-- drivers/phy/ti/phy-am654-serdes.c | 4 ++-- include/linux/clk-provider.h | 3 ++- 11 files changed, 56 insertions(+), 36 deletions(-)
base-commit: 5f9e832c137075045d15cd6899ab0505cfb2ca4b
Signature · 1 line
Sent by a computer through tubes
Stephen Boyd
maintainerPATCH 1/9 clk: actions: Don't reference clk_init_data after registration
A future patch is going to change semantics of clk_register() so that clk_hw::init is guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.
---
Please ack so I can take this through clk tree
drivers/clk/actions/owl-common.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/clk/actions/owl-common.c +2 -1
@@ -68,6 +68,7 @@ int owl_clk_probe(struct device *dev, struct clk_hw_onecell_data *hw_clks)struct clk_hw *hw;for (i =0 ; i < hw_clks->num; i++) { +const char *name = hw->init->name; hw = hw_clks->hws[i];
@@ -77,7 +78,7 @@ int owl_clk_probe(struct device *dev, struct clk_hw_onecell_data *hw_clks) ret =devm_clk_hw_register (dev, hw);if (ret) {dev_err (dev,"Couldn't register clock %d - %s\n" , - i, hw->init->name); + i, name);return ret; } }
Signature · 1 line
Sent by a computer through tubes
Re: [PATCH 1/9] clk: actions: Don't reference clk_init_data after registration
Quoted 7 lines · Stephen Boyd
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.Cc Manivannan Sadhasivam <[email protected]>Signed-off-by Stephen Boyd <[email protected]>
Thanks,
Mani
Quoted 31 lines
---
Please ack so I can take this through clk tree
drivers/clk/actions/owl-common.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/clk/actions/owl-common.c +2
-1@@ -68,6 +68,7 @@ int owl_clk_probe(struct device *dev, struct clk_hw_onecell_data *hw_clks)struct clk_hw *hw;for (i =0 ; i < hw_clks->num; i++) { +const char *name = hw->init->name; hw = hw_clks->hws[i];@@ -77,7 +78,7 @@ int owl_clk_probe(struct device *dev, struct clk_hw_onecell_data *hw_clks) ret =devm_clk_hw_register (dev, hw);if (ret) {dev_err (dev,"Couldn't register clock %d - %s\n" , - i, hw->init->name); + i, name);return ret; } }Signature · 1 line
Sent by a computer through tubes
Re: [PATCH 1/9] clk: actions: Don't reference clk_init_data after registration
In reply to Stephen BoydQuoting Stephen Boyd (2019-07-31 12:35:09)
Quoted 8 lines
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.Cc Manivannan Sadhasivam <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Applied to clk-next
Re: [PATCH 1/9] clk: actions: Don't reference clk_init_data after registration
In reply to Stephen BoydQuoted 19 lines · Stephen Boyd
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.Cc Manivannan Sadhasivam <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Please ack so I can take this through clk tree
drivers/clk/actions/owl-common.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/clk/actions/owl-common.c +0
-0
drivers/clk/actions/owl-common.c int owl_clk_probe(struct device *dev, struct clk_hw_onecell_data *hw_clks) · line 72
Quoted 5 lines
drivers/clk/actions/owl-common.c +1
-0@@ -68,6 +68,7 @@ int owl_clk_probe(struct device *dev, struct clk_hw_onecell_data *hw_clks)struct clk_hw *hw;for (i =0 ; i < hw_clks->num; i++) { +const char *name = hw->init->name;
This should come after below statement and hence the warning is generated in linux-next. Sorry for missing!
Thanks,
Mani
Quoted 14 lines
drivers/clk/actions/owl-common.c +1
-1hw = hw_clks->hws[i];@@ -77,7 +78,7 @@ int owl_clk_probe(struct device *dev, struct clk_hw_onecell_data *hw_clks) ret =devm_clk_hw_register (dev, hw);if (ret) {dev_err (dev,"Couldn't register clock %d - %s\n" , - i, hw->init->name); + i, name);return ret; } }Signature · 1 line
Sent by a computer through tubes
Stephen Boyd
maintainerRe: [PATCH 1/9] clk: actions: Don't reference clk_init_data after registration
Quoting Manivannan Sadhasivam (2019-08-16 04:22:10)
Quoted 22 lines · Stephen Boyd
Quoted 21 lines
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.Cc Manivannan Sadhasivam <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Please ack so I can take this through clk tree
drivers/clk/actions/owl-common.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/clk/actions/owl-common.c +0
-0@@ -68,6 +68,7 @@ int owl_clk_probe(struct device *dev, struct clk_hw_onecell_data *hw_clks)struct clk_hw *hw;
Quoted 7 lines
drivers/clk/actions/owl-common.c int owl_clk_probe(struct device *dev, struct clk_hw_onecell_data *hw_clks) · line 72
Quoted 3 lines
drivers/clk/actions/owl-common.c +1
-0for (i =0 ; i < hw_clks->num; i++) { +const char *name = hw->init->name;This should come after below statement and hence the warning is generated in linux-next. Sorry for missing!
Oh right. Will fix it.
Stephen Boyd
maintainerPATCH 2/9 clk: lochnagar: Don't reference clk_init_data after registration
In reply to Stephen BoydA future patch is going to change semantics of clk_register() so that clk_hw::init is guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.
---
Please ack so I can take this through clk tree
drivers/clk/clk-lochnagar.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/clk/clk-lochnagar.c +1 -1
@@ -198,7 +198,7 @@ static u8 lochnagar_clk_get_parent(struct clk_hw *hw)if (ret <0 ) {dev_dbg (priv->dev,"Failed to read parent of %s: %d\n" , lclk->name, ret); -return hw->init->num_parents; +return clk_hw_get_num_parents (hw); } val &= lclk->src_mask;
Signature · 1 line
Sent by a computer through tubes
Re: [PATCH 2/9] clk: lochnagar: Don't reference clk_init_data after registration
Quoted 10 lines · Stephen Boyd
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.Cc Charles Keepax <[email protected]>Cc Richard Fitzgerald <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Thanks,
Charles
Re: [PATCH 2/9] clk: lochnagar: Don't reference clk_init_data after registration
In reply to Stephen BoydQuoting Stephen Boyd (2019-07-31 12:35:10)
Quoted 9 lines
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.Cc Charles Keepax <[email protected]>Cc Richard Fitzgerald <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Applied to clk-next
Stephen Boyd
maintainerPATCH 3/9 clk: meson: axg-audio: Don't reference clk_init_data after registration
In reply to Stephen BoydA future patch is going to change semantics of clk_register() so that clk_hw::init is guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.
---
Please ack so I can take this through clk tree
drivers/clk/meson/axg-audio.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/clk/meson/axg-audio.c +5 -2
@@ -992,15 +992,18 @@ static int axg_audio_clkc_probe(struct platform_device *pdev)/* Take care to skip the registered input clocks */ for (i =AUD_CLKID_DDR_ARB ; i < data->hw_onecell_data->num; i++) { +const char *name; + hw = data->hw_onecell_data->hws[i];/* array might be sparse */ if (!hw)continue ; + name = hw->init->name; + ret =devm_clk_hw_register (dev, hw);if (ret) { -dev_err (dev,"failed to register clock %s\n" , - hw->init->name); +dev_err (dev,"failed to register clock %s\n" , name);return ret; } }
Signature · 1 line
Sent by a computer through tubes
Re: [PATCH 3/9] clk: meson: axg-audio: Don't reference clk_init_data after registration
Hi Stephen,
Quoted 31 lines · Stephen Boyd
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.Cc Neil Armstrong <[email protected]>Cc Jerome Brunet <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Please ack so I can take this through clk tree
drivers/clk/meson/axg-audio.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)drivers/clk/meson/axg-audio.c +2
-0@@ -992,15 +992,18 @@ static int axg_audio_clkc_probe(struct platform_device *pdev)/* Take care to skip the registered input clocks */ for (i =AUD_CLKID_DDR_ARB ; i < data->hw_onecell_data->num; i++) { +const char *name; + hw = data->hw_onecell_data->hws[i];/* array might be sparse */ if (!hw)continue ;
drivers/clk/meson/axg-audio.c static int axg_audio_clkc_probe(struct platform_device *pdev) · line 1009
Quoted 10 lines
drivers/clk/meson/axg-audio.c +3
-2+ name = hw->init->name; + ret =devm_clk_hw_register (dev, hw);if (ret) { -dev_err (dev,"failed to register clock %s\n" , - hw->init->name); +dev_err (dev,"failed to register clock %s\n" , name);return ret; } }
Jerome Brunet
maintainerRe: [PATCH 3/9] clk: meson: axg-audio: Don't reference clk_init_data after registration
In reply to Stephen BoydQuoted 4 lines · Stephen Boyd
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.
Hi Stephen,
What to do you indend to do with this one ? Will you apply directly or should we take it ?
We have several changes for the controller which may conflict with this one. It is nothing major but the sooner I know how this changes goes in, the sooner I can rebase the rest.
Also, We were (re)using the init_data only on register failures. I understand that you want to guarantee .init is NULL when the clock is registered, but it this particular case, the registeration failed so the clock is not registered.
IMO, it would be better if devm_clk_hw_register() left the init_data untouched if the registration fails.
Quoted 38 lines
Cc Neil Armstrong <[email protected]>Cc Jerome Brunet <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Please ack so I can take this through clk tree
drivers/clk/meson/axg-audio.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)drivers/clk/meson/axg-audio.c +5
-2@@ -992,15 +992,18 @@ static int axg_audio_clkc_probe(struct platform_device *pdev)/* Take care to skip the registered input clocks */ for (i =AUD_CLKID_DDR_ARB ; i < data->hw_onecell_data->num; i++) { +const char *name; + hw = data->hw_onecell_data->hws[i];/* array might be sparse */ if (!hw)continue ; + name = hw->init->name; + ret =devm_clk_hw_register (dev, hw);if (ret) { -dev_err (dev,"failed to register clock %s\n" , - hw->init->name); +dev_err (dev,"failed to register clock %s\n" , name);return ret; } }Signature · 1 line
Sent by a computer through tubes
Stephen Boyd
maintainerRe: [PATCH 3/9] clk: meson: axg-audio: Don't reference clk_init_data after registration
Quoting Jerome Brunet (2019-08-06 01:49:47)
Quoted 11 lines · Stephen Boyd
Quoted 5 lines
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.Hi Stephen,
What to do you indend to do with this one ? Will you apply directly or should we take it ?
I said below:
Please ack so I can take this through clk tree
Quoted 4 lines
We have several changes for the controller which may conflict with this one. It is nothing major but the sooner I know how this changes goes in, the sooner I can rebase the rest.
Will it conflict? I can deal with conflicts.
Quoted 8 lines
Also, We were (re)using the
init_dataonly on register failures. I understand that you want to guarantee .init is NULL when the clock is registered, but it this particular case, the registeration failed so the clock is not registered.IMO, it would be better if
devm_clk_hw_register()left theinit_datauntouched if the registration fails.
Do you have other usage of the init_data besides printing out the name? I think we could have devm_clk_hw_register() print out the name of the clk that failed to register instead, and get rid of more code in drivers that way. Unless of course there are other uses of the init struct?
Quoted 12 lines
Quoted 12 lines
Cc Neil Armstrong <[email protected]>Cc Jerome Brunet <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Please ack so I can take this through clk tree
drivers/clk/meson/axg-audio.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
Jerome Brunet
maintainerRe: [PATCH 3/9] clk: meson: axg-audio: Don't reference clk_init_data after registration
Quoted 8 lines · Stephen Boyd
Quoting Jerome Brunet (2019-08-06 01:49:47)
Quoted 7 lines · Stephen Boyd
Quoted 5 lines
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.
Quoted 9 lines
Quoted 4 lines
Hi Stephen,
What to do you indend to do with this one ? Will you apply directly or should we take it ?
I said below:
Please ack so I can take this through clk tree
Missed it, sorry.
Quoted 6 lines
Quoted 4 lines
We have several changes for the controller which may conflict with this one. It is nothing major but the sooner I know how this changes goes in, the sooner I can rebase the rest.
Will it conflict? I can deal with conflicts.
I'll check to be sure and notify you in the PR if necessary
Quoted 7 lines
Quoted 7 lines
Also, We were (re)using the
init_dataonly on register failures. I understand that you want to guarantee .init is NULL when the clock is registered, but it this particular case, the registeration failed so the clock is not registered.
Quoted 5 lines
Quoted 2 lines
IMO, it would be better if
devm_clk_hw_register()left theinit_datauntouched if the registration fails.Do you have other usage of the
init_databesides printing out the name?
No other use
Quoted 3 lines
I think we could have
devm_clk_hw_register()print out the name of the clk that failed to register instead, and get rid of more code in drivers that way.
Sure, why not.
Quoted 1 line
Unless of course there are other uses of the init struct?
I was just commenting on the fact that initialization function tends to rollback what they can in case of failures, usually.
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Cc Neil Armstrong <[email protected]>Cc Jerome Brunet <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Please ack so I can take this through clk tree
drivers/clk/meson/axg-audio.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
Re: [PATCH 3/9] clk: meson: axg-audio: Don't reference clk_init_data after registration
In reply to Stephen BoydQuoting Stephen Boyd (2019-07-31 12:35:11)
Quoted 9 lines
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.Cc Neil Armstrong <[email protected]>Cc Jerome Brunet <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Applied to clk-next
Stephen Boyd
maintainerPATCH 4/9 clk: qcom: Don't reference clk_init_data after registration
In reply to Stephen BoydA future patch is going to change semantics of clk_register() so that clk_hw::init is guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.
---
Please ack so I can take this through clk tree
drivers/clk/qcom/clk-rpmh.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/clk/qcom/clk-rpmh.c +2 -2
@@ -396,6 +396,7 @@ static int clk_rpmh_probe(struct platform_device *pdev) hw_clks = desc->clks;for (i =0 ; i < desc->num_clks; i++) { +const char *name = hw_clks[i]->init->name;u32 res_addr;size_t aux_data_len;const struct bcm_db *data;
@@ -426,8 +427,7 @@ static int clk_rpmh_probe(struct platform_device *pdev) ret =devm_clk_hw_register (&pdev->dev, hw_clks[i]);if (ret) { -dev_err (&pdev->dev,"failed to register %s\n" , - hw_clks[i]->init->name); +dev_err (&pdev->dev,"failed to register %s\n" , name);return ret; } }
Signature · 1 line
Sent by a computer through tubes
Re: [PATCH 4/9] clk: qcom: Don't reference clk_init_data after registration
Quoted 8 lines · Stephen Boyd
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.Cc Taniya Das <[email protected]>Cc Andy Gross <[email protected]>Signed-off-by Stephen Boyd <[email protected]>
Quoted 30 lines
---
Please ack so I can take this through clk tree
drivers/clk/qcom/clk-rpmh.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/clk/qcom/clk-rpmh.c +2
-2@@ -396,6 +396,7 @@ static int clk_rpmh_probe(struct platform_device *pdev) hw_clks = desc->clks;for (i =0 ; i < desc->num_clks; i++) { +const char *name = hw_clks[i]->init->name;u32 res_addr;size_t aux_data_len;const struct bcm_db *data;@@ -426,8 +427,7 @@ static int clk_rpmh_probe(struct platform_device *pdev) ret =devm_clk_hw_register (&pdev->dev, hw_clks[i]);if (ret) { -dev_err (&pdev->dev,"failed to register %s\n" , - hw_clks[i]->init->name); +dev_err (&pdev->dev,"failed to register %s\n" , name);return ret; } }
Signature · 4 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation. --
Re: [PATCH 4/9] clk: qcom: Don't reference clk_init_data after registration
In reply to Stephen BoydQuoting Stephen Boyd (2019-07-31 12:35:12)
Quoted 9 lines
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.Cc Taniya Das <[email protected]>Cc Andy Gross <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Applied to clk-next
Stephen Boyd
maintainerPATCH 5/9 clk: sirf: Don't reference clk_init_data after registration
In reply to Stephen BoydA future patch is going to change semantics of clk_register() so that clk_hw::init is guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.
---
Please ack so I can take this through clk tree
drivers/clk/sirf/clk-common.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
drivers/clk/sirf/clk-common.c +8 -4
@@ -297,9 +297,10 @@ static u8 dmn_clk_get_parent(struct clk_hw *hw) {struct clk_dmn *clk =to_dmnclk (hw);u32 cfg =clkc_readl (clk->regofs); +const char *name =clk_hw_get_name (hw);/* parent of io domain can only be pll3 */ -if (strcmp (hw->init->name,"io" ) ==0 ) +if (strcmp (name,"io" ) ==0 )return 4 ;WARN_ON ((cfg & (BIT (3 ) -1 )) >4 );
@@ -311,9 +312,10 @@ static int dmn_clk_set_parent(struct clk_hw *hw, u8 parent) {struct clk_dmn *clk =to_dmnclk (hw);u32 cfg =clkc_readl (clk->regofs); +const char *name =clk_hw_get_name (hw);/* parent of io domain can only be pll3 */ -if (strcmp (hw->init->name,"io" ) ==0 ) +if (strcmp (name,"io" ) ==0 )return -EINVAL ; cfg &= ~(BIT (3 ) -1 );
@@ -353,7 +355,8 @@ static long dmn_clk_round_rate(struct clk_hw *hw, unsigned long rate, {unsigned long fin;unsigned ratio, wait, hold; -unsigned bits = (strcmp (hw->init->name,"mem" ) ==0 ) ?3 :4 ; +const char *name =clk_hw_get_name (hw); +unsigned bits = (strcmp (name,"mem" ) ==0 ) ?3 :4 ; fin = *parent_rate; ratio = fin / rate;
@@ -375,7 +378,8 @@ static int dmn_clk_set_rate(struct clk_hw *hw, unsigned long rate,struct clk_dmn *clk =to_dmnclk (hw);unsigned long fin;unsigned ratio, wait, hold, reg; -unsigned bits = (strcmp (hw->init->name,"mem" ) ==0 ) ?3 :4 ; +const char *name =clk_hw_get_name (hw); +unsigned bits = (strcmp (name,"mem" ) ==0 ) ?3 :4 ; fin = parent_rate; ratio = fin / rate;
Signature · 1 line
Sent by a computer through tubes
Re: [PATCH 5/9] clk: sirf: Don't reference clk_init_data after registration
Quoting Stephen Boyd (2019-07-31 12:35:13)
Quoted 9 lines
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.Cc Guo Zeng <[email protected]>Cc Barry Song <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Applied to clk-next
Stephen Boyd
maintainerPATCH 6/9 clk: socfpga: Don't reference clk_init_data after registration
In reply to Stephen BoydA future patch is going to change semantics of clk_register() so that clk_hw::init is guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.
---
Please ack so I can take this through clk tree
drivers/clk/socfpga/clk-gate.c | 21 +++++++++++---------- drivers/clk/socfpga/clk-periph-a10.c | 7 ++++--- 2 files changed, 15 insertions(+), 13 deletions(-)
drivers/clk/socfpga/clk-gate.c +11 -10
@@ -31,20 +31,20 @@ static u8 socfpga_clk_get_parent(struct clk_hw *hwclk)u32 l4_src;u32 perpll_src; -if (streq (hwclk->init->name,SOCFPGA_L4_MP_CLK )) { +if (streq (name,SOCFPGA_L4_MP_CLK )) { l4_src =readl (clk_mgr_base_addr +CLKMGR_L4SRC );return l4_src &=0x1 ; } -if (streq (hwclk->init->name,SOCFPGA_L4_SP_CLK )) { +if (streq (name,SOCFPGA_L4_SP_CLK )) { l4_src =readl (clk_mgr_base_addr +CLKMGR_L4SRC );return !!(l4_src &2 ); } perpll_src =readl (clk_mgr_base_addr +CLKMGR_PERPLL_SRC ); -if (streq (hwclk->init->name,SOCFPGA_MMC_CLK )) +if (streq (name,SOCFPGA_MMC_CLK ))return perpll_src &=0x3 ; -if (streq (hwclk->init->name,SOCFPGA_NAND_CLK ) || -streq (hwclk->init->name,SOCFPGA_NAND_X_CLK )) +if (streq (name,SOCFPGA_NAND_CLK ) || +streq (name,SOCFPGA_NAND_X_CLK ))return (perpll_src >>2 ) &3 ;/* QSPI clock */
@@ -55,24 +55,25 @@ static u8 socfpga_clk_get_parent(struct clk_hw *hwclk)static int socfpga_clk_set_parent (struct clk_hw *hwclk,u8 parent) {u32 src_reg; +const char *name =clk_hw_get_name (hwclk); -if (streq (hwclk->init->name,SOCFPGA_L4_MP_CLK )) { +if (streq (name,SOCFPGA_L4_MP_CLK )) { src_reg =readl (clk_mgr_base_addr +CLKMGR_L4SRC ); src_reg &= ~0x1 ; src_reg |= parent;writel (src_reg, clk_mgr_base_addr +CLKMGR_L4SRC ); - }else if (streq (hwclk->init->name,SOCFPGA_L4_SP_CLK )) { + }else if (streq (name,SOCFPGA_L4_SP_CLK )) { src_reg =readl (clk_mgr_base_addr +CLKMGR_L4SRC ); src_reg &= ~0x2 ; src_reg |= (parent <<1 );writel (src_reg, clk_mgr_base_addr +CLKMGR_L4SRC ); }else { src_reg =readl (clk_mgr_base_addr +CLKMGR_PERPLL_SRC ); -if (streq (hwclk->init->name,SOCFPGA_MMC_CLK )) { +if (streq (name,SOCFPGA_MMC_CLK )) { src_reg &= ~0x3 ; src_reg |= parent; - }else if (streq (hwclk->init->name,SOCFPGA_NAND_CLK ) || -streq (hwclk->init->name,SOCFPGA_NAND_X_CLK )) { + }else if (streq (name,SOCFPGA_NAND_CLK ) || +streq (name,SOCFPGA_NAND_X_CLK )) { src_reg &= ~0xC ; src_reg |= (parent <<2 ); }else {/* QSPI clock */
drivers/clk/socfpga/clk-periph-a10.c +4 -3
@@ -40,11 +40,12 @@ static u8 clk_periclk_get_parent(struct clk_hw *hwclk) {struct socfpga_periph_clk *socfpgaclk =to_socfpga_periph_clk (hwclk);u32 clk_src; +const char *name =clk_hw_get_name (hwclk); clk_src =readl (socfpgaclk->hw.reg); -if (streq (hwclk->init->name,SOCFPGA_MPU_FREE_CLK ) || -streq (hwclk->init->name,SOCFPGA_NOC_FREE_CLK ) || -streq (hwclk->init->name,SOCFPGA_SDMMC_FREE_CLK )) +if (streq (name,SOCFPGA_MPU_FREE_CLK ) || +streq (name,SOCFPGA_NOC_FREE_CLK ) || +streq (name,SOCFPGA_SDMMC_FREE_CLK ))return (clk_src >>CLK_MGR_FREE_SHIFT ) &CLK_MGR_FREE_MASK ;else
Signature · 1 line
Sent by a computer through tubes
Re: [PATCH 6/9] clk: socfpga: Don't reference clk_init_data after registration
Hi Stephen,
Quoted 20 lines · Stephen Boyd
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.Cc Dinh Nguyen <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Please ack so I can take this through clk tree
drivers/clk/socfpga/clk-gate.c | 21 +++++++++++---------- drivers/clk/socfpga/clk-periph-a10.c | 7 ++++--- 2 files changed, 15 insertions(+), 13 deletions(-)drivers/clk/socfpga/clk-gate.c +0
-0
drivers/clk/socfpga/clk-gate.c static u8 socfpga_clk_get_parent(struct clk_hw *hwclk) · line 32
Quoted 2 lines
drivers/clk/socfpga/clk-gate.c +0
-0@@ -31,20 +31,20 @@ static u8 socfpga_clk_get_parent(struct clk_hw *hwclk)u32 l4_src;u32 perpll_src;
You need this line here:
const char *name = clk_hw_get_name(hwclk);
Otherwise, it fails to build. With the above change:
Thanks,
Dinh
Stephen Boyd
maintainerRe: [PATCH 6/9] clk: socfpga: Don't reference clk_init_data after registration
Quoting Dinh Nguyen (2019-08-01 08:12:58)
Quoted 28 lines
Hi Stephen,
Quoted 20 lines · Stephen Boyd
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.Cc Dinh Nguyen <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Please ack so I can take this through clk tree
drivers/clk/socfpga/clk-gate.c | 21 +++++++++++---------- drivers/clk/socfpga/clk-periph-a10.c | 7 ++++--- 2 files changed, 15 insertions(+), 13 deletions(-)drivers/clk/socfpga/clk-gate.c +0
-0drivers/clk/socfpga/clk-gate.c static u8 socfpga_clk_get_parent(struct clk_hw *hwclk) · line 32
Quoted 2 lines
drivers/clk/socfpga/clk-gate.c +0
-0@@ -31,20 +31,20 @@ static u8 socfpga_clk_get_parent(struct clk_hw *hwclk)u32 l4_src;u32 perpll_src;You need this line here:
Quoted 5 lines
const char *name = clk_hw_get_name(hwclk);Otherwise, it fails to build. With the above change:
Acked-by Dinh Nguyen <[email protected]>
Awesome thanks!
Re: [PATCH 6/9] clk: socfpga: Don't reference clk_init_data after registration
In reply to Stephen BoydQuoting Stephen Boyd (2019-07-31 12:35:14)
Quoted 8 lines
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.Cc Dinh Nguyen <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Applied to clk-next
Stephen Boyd
maintainerPATCH 7/9 clk: sprd: Don't reference clk_init_data after registration
In reply to Stephen BoydA future patch is going to change semantics of clk_register() so that clk_hw::init is guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.
---
Please ack so I can take this through clk tree
drivers/clk/sprd/common.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/clk/sprd/common.c +3 -2
@@ -76,16 +76,17 @@ int sprd_clk_probe(struct device *dev, struct clk_hw_onecell_data *clkhw)struct clk_hw *hw;for (i =0 ; i < clkhw->num; i++) { +const char *name; hw = clkhw->hws[i]; -if (!hw)continue ; + name = hw->init->name; ret =devm_clk_hw_register (dev, hw);if (ret) {dev_err (dev,"Couldn't register clock %d - %s\n" , - i, hw->init->name); + i, name);return ret; } }
Signature · 1 line
Sent by a computer through tubes
Re: [PATCH 7/9] clk: sprd: Don't reference clk_init_data after registration
Hi,
Quoted 6 lines · Stephen Boyd
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.
Quoted 6 lines
Cc Chunyan Zhang <[email protected]>Cc Baolin Wang <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Please ack so I can take this through clk tree
Thanks.
Signature · 2 lines
Baolin Wang Best Regards
Re: [PATCH 7/9] clk: sprd: Don't reference clk_init_data after registration
In reply to Stephen BoydQuoted 6 lines · Stephen Boyd
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.
Quoted 6 lines
Cc Chunyan Zhang <[email protected]>Cc Baolin Wang <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Please ack so I can take this through clk tree
Thanks.
Quoted 31 lines
drivers/clk/sprd/common.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/clk/sprd/common.c +3
-3@@ -76,16 +76,17 @@ int sprd_clk_probe(struct device *dev, struct clk_hw_onecell_data *clkhw)struct clk_hw *hw;for (i =0 ; i < clkhw->num; i++) { +const char *name; hw = clkhw->hws[i]; -if (!hw)continue ; + name = hw->init->name; ret =devm_clk_hw_register (dev, hw);if (ret) {dev_err (dev,"Couldn't register clock %d - %s\n" , - i, hw->init->name); + i, name);return ret; } } --Sent by a computer through tubes
Re: [PATCH 7/9] clk: sprd: Don't reference clk_init_data after registration
In reply to Stephen BoydQuoting Stephen Boyd (2019-07-31 12:35:15)
Quoted 9 lines
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.Cc Chunyan Zhang <[email protected]>Cc Baolin Wang <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Applied to clk-next
Stephen Boyd
maintainerPATCH 8/9 phy: ti: am654-serdes: Don't reference clk_init_data after registration
In reply to Stephen BoydA future patch is going to change semantics of clk_register() so that clk_hw::init is guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.
---
Please ack so I can take this through clk tree
drivers/phy/ti/phy-am654-serdes.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/phy/ti/phy-am654-serdes.c +2 -2
@@ -335,6 +335,7 @@ static int serdes_am654_clk_mux_set_parent(struct clk_hw *hw, u8 index) {struct serdes_am654_clk_mux *mux =to_serdes_am654_clk_mux (hw);struct regmap *regmap = mux->regmap; +const char *name =clk_hw_get_name (hw);unsigned int reg = mux->reg;int clk_id = mux->clk_id;int parents[SERDES_NUM_CLOCKS ];
@@ -374,8 +375,7 @@ static int serdes_am654_clk_mux_set_parent(struct clk_hw *hw, u8 index)* This can never happen, unless we missed * a valid combination in serdes_am654_mux_table. */ -WARN (1 ,"Failed to find the parent of %s clock\n" , - hw->init->name); +WARN (1 ,"Failed to find the parent of %s clock\n" , name);return -EINVAL ; }
Signature · 1 line
Sent by a computer through tubes
Re: [PATCH 8/9] phy: ti: am654-serdes: Don't reference clk_init_data after registration
Quoted 11 lines · Stephen Boyd
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.Cc Roger Quadros <[email protected]>Cc Kishon Vijay Abraham I <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Please ack so I can take this through clk tree
Quoted 27 lines
drivers/phy/ti/phy-am654-serdes.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/phy/ti/phy-am654-serdes.c +2
-2@@ -335,6 +335,7 @@ static int serdes_am654_clk_mux_set_parent(struct clk_hw *hw, u8 index) {struct serdes_am654_clk_mux *mux =to_serdes_am654_clk_mux (hw);struct regmap *regmap = mux->regmap; +const char *name =clk_hw_get_name (hw);unsigned int reg = mux->reg;int clk_id = mux->clk_id;int parents[SERDES_NUM_CLOCKS ];@@ -374,8 +375,7 @@ static int serdes_am654_clk_mux_set_parent(struct clk_hw *hw, u8 index)* This can never happen, unless we missed * a valid combination in serdes_am654_mux_table. */ -WARN (1 ,"Failed to find the parent of %s clock\n" , - hw->init->name); +WARN (1 ,"Failed to find the parent of %s clock\n" , name);return -EINVAL ; }
Re: [PATCH 8/9] phy: ti: am654-serdes: Don't reference clk_init_data after registration
In reply to Stephen BoydQuoting Stephen Boyd (2019-07-31 12:35:16)
Quoted 9 lines
A future patch is going to change semantics of
clk_register()so thatclk_hw::initis guaranteed to be NULL after a clk is registered. Avoid referencing this member here so that we don't run into NULL pointer exceptions.Cc Roger Quadros <[email protected]>Cc Kishon Vijay Abraham I <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Applied to clk-next
Stephen Boyd
maintainerPATCH 9/9 clk: Overwrite clk_hw::init with NULL during clk_register()
In reply to Stephen BoydWe don't want clk provider drivers to use the init structure after clk registration time, but we leave a dangling reference to it by means of clk_hw::init. Let's overwrite the member with NULL during clk_register() so that this can't be used anymore after registration time.
---
Please ack so I can take this through clk tree
drivers/clk/clk.c | 24 ++++++++++++++++-------- include/linux/clk-provider.h | 3 ++- 2 files changed, 18 insertions(+), 9 deletions(-)
drivers/clk/clk.c +16 -8
@@ -3484,9 +3484,9 @@ static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)return 0 ; } -static int clk_core_populate_parent_map (struct clk_core *core) +static int clk_core_populate_parent_map (struct clk_core *core, +const struct clk_init_data *init) { -const struct clk_init_data *init = core->hw->init;u8 num_parents = init->num_parents;const char *const *parent_names = init->parent_names;const struct clk_hw **parent_hws = init->parent_hws;
@@ -3566,6 +3566,14 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw) {int ret;struct clk_core *core; +const struct clk_init_data *init = hw->init; + +/* +* The init data is not supposed to be used outside of registration path. +* Set it to NULL so that provider drivers can't use it either and so that +* we catch use of hw->init early on in the core. +*/ + hw->init =NULL ; core =kzalloc (sizeof (*core),GFP_KERNEL );if (!core) {
@@ -3573,17 +3581,17 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw) goto fail_out; } - core->name =kstrdup_const (hw->init->name,GFP_KERNEL ); + core->name =kstrdup_const (init->name,GFP_KERNEL );if (!core->name) { ret = -ENOMEM ; goto fail_name; } -if (WARN_ON (!hw->init->ops)) { +if (WARN_ON (!init->ops)) { ret = -EINVAL ; goto fail_ops; } - core->ops = hw->init->ops; + core->ops = init->ops;if (dev &&pm_runtime_enabled (dev)) core->rpm_enabled = true;
@@ -3592,13 +3600,13 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)if (dev && dev->driver) core->owner = dev->driver->owner; core->hw = hw; - core->flags = hw->init->flags; - core->num_parents = hw->init->num_parents; + core->flags = init->flags; + core->num_parents = init->num_parents; core->min_rate =0 ; core->max_rate =ULONG_MAX ; hw->core = core; - ret =clk_core_populate_parent_map (core); + ret =clk_core_populate_parent_map (core, init);if (ret) goto fail_parents;
include/linux/clk-provider.h +2 -1
@@ -299,7 +299,8 @@ struct clk_init_data {* into the clk API * * @init: pointer to struct clk_init_data that contains the init data shared -* with the common clock framework. +* with the common clock framework. This pointer will be set to NULL once +* a clk_register() variant is called on this clk_hw pointer. */ struct clk_hw {struct clk_core *core;
Signature · 1 line
Sent by a computer through tubes
Re: [PATCH 9/9] clk: Overwrite clk_hw::init with NULL during clk_register()
Quoted 8 lines · Stephen Boyd
We don't want clk provider drivers to use the init structure after clk registration time, but we leave a dangling reference to it by means of
clk_hw::init. Let's overwrite the member with NULL duringclk_register()so that this can't be used anymore after registration time.Cc Bjorn Andersson <[email protected]>Cc Doug Anderson <[email protected]>Signed-off-by Stephen Boyd <[email protected]>
Quoted 21 lines
drivers/clk/clk.c | 24 ++++++++++++++++-------- include/linux/clk-provider.h | 3 ++- 2 files changed, 18 insertions(+), 9 deletions(-)drivers/clk/clk.c +2
-2@@ -3484,9 +3484,9 @@ static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)return 0 ; } -static int clk_core_populate_parent_map (struct clk_core *core) +static int clk_core_populate_parent_map (struct clk_core *core, +const struct clk_init_data *init) { -const struct clk_init_data *init = core->hw->init;u8 num_parents = init->num_parents;const char *const *parent_names = init->parent_names;const struct clk_hw **parent_hws = init->parent_hws;
drivers/clk/clk.c __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw) · line 3575
Quoted 10 lines
drivers/clk/clk.c +7
-0@@ -3566,6 +3566,14 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw) {int ret;struct clk_core *core; +const struct clk_init_data *init = hw->init; + +/* +* The init data is not supposed to be used outside of registration path. +* Set it to NULL so that provider drivers can't use it either and so that +* we catch use of hw->init early on in the core. +*/
nit: This comment could be re-edited to not exceed 80 columns limit.
Quoted 1 line
drivers/clk/clk.c +1
-0+ hw->init =NULL ;
Re: [PATCH 9/9] clk: Overwrite clk_hw::init with NULL during clk_register()
In reply to Stephen BoydQuoting Stephen Boyd (2019-07-31 12:35:17)
Quoted 9 lines
We don't want clk provider drivers to use the init structure after clk registration time, but we leave a dangling reference to it by means of
clk_hw::init. Let's overwrite the member with NULL duringclk_register()so that this can't be used anymore after registration time.Cc Bjorn Andersson <[email protected]>Cc Doug Anderson <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Applied to clk-next
Re: [PATCH 9/9] clk: Overwrite clk_hw::init with NULL during clk_register()
In reply to Stephen BoydHi, Stephen,
Quoted 34 lines · Stephen Boyd
We don't want clk provider drivers to use the init structure after clk registration time, but we leave a dangling reference to it by means of
clk_hw::init. Let's overwrite the member with NULL duringclk_register()so that this can't be used anymore after registration time.Cc Bjorn Andersson <[email protected]>Cc Doug Anderson <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
Please ack so I can take this through clk tree
drivers/clk/clk.c | 24 ++++++++++++++++-------- include/linux/clk-provider.h | 3 ++- 2 files changed, 18 insertions(+), 9 deletions(-)drivers/clk/clk.c +2
-2@@ -3484,9 +3484,9 @@ static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)return 0 ; } -static int clk_core_populate_parent_map (struct clk_core *core) +static int clk_core_populate_parent_map (struct clk_core *core, +const struct clk_init_data *init) { -const struct clk_init_data *init = core->hw->init;u8 num_parents = init->num_parents;const char *const *parent_names = init->parent_names;const struct clk_hw **parent_hws = init->parent_hws;@@ -3566,6 +3566,14 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw) {
drivers/clk/clk.c __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw) · line 3576
Quoted 10 lines
drivers/clk/clk.c +8
-0int ret;struct clk_core *core; +const struct clk_init_data *init = hw->init; + +/* +* The init data is not supposed to be used outside of registration path. +* Set it to NULL so that provider drivers can't use it either and so that +* we catch use of hw->init early on in the core. +*/ + hw->init =NULL ;
I found that this line impact the drivers/clk/clk-versaclock3.c driver when doing unbind/bind. The driver provides statically initialized hw.init object for registration, the hw.init pointers are set to NULL after clock registrations then the next registration (when re-binding) fails.
In the driver probe the clock are registered like:
for (i = 0; i < ARRAY_SIZE(clk_pfd_mux); i++) {
clk_pfd_mux[i].regmap = regmap;
ret = devm_clk_hw_register(dev, &clk_pfd_mux[i].hw);
if (ret)
return dev_err_probe(dev, ret, "%s failed\n",
clk_pfd_mux[i].hw.init->name);
}and entries in clk_pfd_mux[] are like:
static struct vc3_hw_data clk_pfd_mux[] = {
[VC3_PFD2_MUX] = {
.data = &(struct vc3_clk_data) {
.offs = VC3_PLL_OP_CTRL,
.bitmsk = BIT(VC3_PLL_OP_CTRL_PLL2_REFIN_SEL)
},
.hw.init = &(struct clk_init_data) {
.name = "pfd2_mux",
.ops = &vc3_pfd_mux_ops,
.parent_data = pfd_mux_parent_data,
.num_parents = 2,
.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT
}
},// ...
};
I tried with the following diff, too:
drivers/clk/clk-versaclock3.c +1 -0
@@ -1022,10 +1022,11 @@ static int vc3_probe(struct i2c_client *client)/* Register pfd muxes */ for (i =0 ; i <ARRAY_SIZE (clk_pfd_mux); i++) { clk_pfd_mux[i].regmap = regmap; + pr_err("%s(): hw init=%08x\n", __func__,
clk_pfd_mux[i].hw.init);
ret = devm_clk_hw_register(dev, &clk_pfd_mux[i].hw); if (ret)
return dev_err_probe(dev, ret, "%s failed\n",
- clk_pfd_mux[i].hw.init->name);
+ "test");
//clk_pfd_mux[i].hw.init->name);
}
/* Register pfd's */@@ -1034,7 +1035,7 @@ static int vc3_probe(struct i2c_client *client)
ret = devm_clk_hw_register(dev, &clk_pfd[i].hw); if (ret)
return dev_err_probe(dev, ret, "%s failed\n",
- clk_pfd[i].hw.init->name);
+ "test");
//clk_pfd[i].hw.init->name);
}
data = i2c_get_match_data(client);@@ -1050,7 +1051,7 @@ static int vc3_probe(struct i2c_client *client)
ret = devm_clk_hw_register(dev, &clk_pll[i].hw); if (ret)
return dev_err_probe(dev, ret, "%s failed\n",
- clk_pll[i].hw.init->name);
+ "test");
//clk_pll[i].hw.init->name);
}
/* Register divider muxes */@@ -1059,7 +1060,7 @@ static int vc3_probe(struct i2c_client *client)
ret = devm_clk_hw_register(dev, &clk_div_mux[i].hw); if (ret)
return dev_err_probe(dev, ret, "%s failed\n",
- clk_div_mux[i].hw.init->name);
+
"test");//clk_div_mux[i].hw.init->name);
}
/* Register dividers */@@ -1068,7 +1069,7 @@ static int vc3_probe(struct i2c_client *client)
ret = devm_clk_hw_register(dev, &clk_div[i].hw); if (ret)
return dev_err_probe(dev, ret, "%s failed\n",
- clk_div[i].hw.init->name);
+ "test");
//clk_div[i].hw.init->name);
}
/* Register clk muxes */@@ -1082,7 +1083,7 @@ static int vc3_probe(struct i2c_client *client)
ret = devm_clk_hw_register(dev, &clk_mux[i].hw); if (ret)
return dev_err_probe(dev, ret, "%s failed\n",
- clk_mux[i].hw.init->name);
+
"test");//clk_mux[i].hw.init->name);
}
/* Register clk outputs */Can you please let me know if this is a wrong pattern? I noticed there are other drivers following similar approach, e.g.:
- drivers/clk/clk-axm5516.c
- drivers/clk/qcom/camcc-sm6350.c
- drivers/clk/meson/c3-pll.c
Full console log with my trials can be found here:
https://p.fr33tux.org/9d976f
Thank you,
Claudiu
Stephen Boyd
maintainerRe: [PATCH 9/9] clk: Overwrite clk_hw::init with NULL during clk_register()
Quoting Claudiu Beznea (2025-02-15 05:55:51)
Quoted 25 lines
Hi, Stephen,
Quoted 22 lines · Stephen Boyd
We don't want clk provider drivers to use the init structure after clk registration time, but we leave a dangling reference to it by means of
clk_hw::init. Let's overwrite the member with NULL duringclk_register()so that this can't be used anymore after registration time.Cc Bjorn Andersson <[email protected]>Cc Doug Anderson <[email protected]>Signed-off-by Stephen Boyd <[email protected]>---
drivers/clk/clk.c +5
-0@@ -3566,6 +3566,14 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw) {int ret;struct clk_core *core; +const struct clk_init_data *init = hw->init; + +/* +* The init data is not supposed to be used outside of registration path. +* Set it to NULL so that provider drivers can't use it either and so that
Quoted 8 lines
drivers/clk/clk.c __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw) · line 3576
Quoted 3 lines
drivers/clk/clk.c +3
-0+* we catch use of hw->init early on in the core. +*/ + hw->init =NULL ;I found that this line impact the
drivers/clk/clk-versaclock3.cdriver when doing unbind/bind. The driver provides statically initialized hw.init object for registration, the hw.init pointers are set to NULL after clock registrations then the next registration (when re-binding) fails.
That's nice </sarcasm>. Yeah I suspect a lot of drivers would fail if they're unbound and rebound right now, for various reasons including what you mention. I think Doug or Bjorn was concerned that this pointer was left dangling when it could potentially lead to some driver accessing the stack. That's what started the whole series.
Quoted 25 lines
drivers/clk/clk-versaclock3.c +1
-1@@ -1068,7 +1069,7 @@ static int vc3_probe(struct i2c_client *client) ret =devm_clk_hw_register (dev, &clk_div[i].hw);if (ret)return dev_err_probe (dev, ret,"%s failed\n" , - clk_div[i].hw.init->name); +"test" );//clk_div[i].hw.init->name); }/* Register clk muxes */drivers/clk/clk-versaclock3.c +1
-1@@ -1082,7 +1083,7 @@ static int vc3_probe(struct i2c_client *client) ret =devm_clk_hw_register (dev, &clk_mux[i].hw);if (ret)return dev_err_probe (dev, ret,"%s failed\n" , - clk_mux[i].hw.init->name); +"test" );//clk_mux[i].hw.init->name); }/* Register clk outputs */
Quoted 5 lines
Can you please let me know if this is a wrong pattern? I noticed there are other drivers following similar approach, e.g.:
-drivers/clk/clk-axm5516.c
-drivers/clk/qcom/camcc-sm6350.c
-drivers/clk/meson/c3-pll.c
Maybe we should only overwrite it to NULL when it succeeds. Or inversely, restore the pointer when it fails so that drivers can print the init->name. Of course, that doesn't fix the problem that a driver may be stashing the init pointer in there anonymously, like the qcom driver does.
Actually, I think what we want to do is pull that error message into devm_clk_hw_register() and get rid of it everywhere else. Drivers will still have to restore the pointer before registering if the driver is unbound and bound. It isn't good to keep a pointer to the stack so I'd like to keep the logic that knocks out the pointer.
---8<---
From: Stephen Boyd <[email protected]>
Subject: [PATCH] clk: Print an error when clk registration fails
We have a lot of driver code that prints an error message when registering a clk fails. Do that in the core function instead to consolidate code. This also helps drivers avoid the anti-pattern of accessing the struct clk_hw::init pointer after registration.
--- drivers/clk/clk.c | 7 +++++++ 1 file changed, 7 insertions(+)
drivers/clk/clk.c +7 -0
@@ -4397,6 +4397,13 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw) fail_name:kref_put (&core->ref, __clk_release); fail_out: +if (dev) { +dev_err_probe (dev, ret,"failed to register clk '%s' (%pS)\n" , + init->name, hw); + }else { +pr_err ("%pOF: error %pe: failed to register clk '%s' (%pS)\n" , + np, &ret, init->name, hw); + }return ERR_PTR (ret); }
Signature · 2 lines
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/ https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git