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()
Cc: Andy Gross <[email protected]>
Cc: Baolin Wang <[email protected]>
Cc: Barry Song <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: Charles Keepax <[email protected]>
Cc: Chunyan Zhang <[email protected]>
Cc: Dinh Nguyen <[email protected]>
Cc: Doug Anderson <[email protected]>
Cc: Guo Zeng <[email protected]>
Cc: Jerome Brunet <[email protected]>
Cc: Kishon Vijay Abraham I <[email protected]>
Cc: Manivannan Sadhasivam <[email protected]>
Cc: Neil Armstrong <[email protected]>
Cc: Richard Fitzgerald <[email protected]>
Cc: Roger Quadros <[email protected]>
Cc: Taniya Das <[email protected]>
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
--
Sent by a computer through tubes
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.
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(-)
diff --git a/drivers/clk/actions/owl-common.c b/drivers/clk/actions/owl-common.c
index 32dd29e0a37e..71b683c4e643 100644
--- a/drivers/clk/actions/owl-common.c
+++ b/drivers/clk/actions/owl-common.c
@@ -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;
}
}
--
Sent by a computer through tubes
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.
Cc: Charles Keepax <[email protected]>
Cc: Richard Fitzgerald <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
Please ack so I can take this through clk tree
drivers/clk/clk-lochnagar.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/clk-lochnagar.c b/drivers/clk/clk-lochnagar.c
index fa8c91758b1d..565bcd0cdde9 100644
--- a/drivers/clk/clk-lochnagar.c
+++ b/drivers/clk/clk-lochnagar.c
@@ -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;
--
Sent by a computer through tubes
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.
Cc: Taniya Das <[email protected]>
Cc: Andy Gross <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
Please ack so I can take this through clk tree
drivers/clk/qcom/clk-rpmh.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
index c3fd632af119..7a8a84dcb70d 100644
--- a/drivers/clk/qcom/clk-rpmh.c
+++ b/drivers/clk/qcom/clk-rpmh.c
@@ -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;
}
}
--
Sent by a computer through tubes
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.
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(-)
diff --git a/drivers/clk/meson/axg-audio.c b/drivers/clk/meson/axg-audio.c
index 8028ff6f6610..db0b73d53551 100644
--- a/drivers/clk/meson/axg-audio.c
+++ b/drivers/clk/meson/axg-audio.c
@@ -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;
}
}
--
Sent by a computer through tubes
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.
Cc: Guo Zeng <[email protected]>
Cc: Barry Song <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
Please ack so I can take this through clk tree
drivers/clk/sirf/clk-common.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/sirf/clk-common.c b/drivers/clk/sirf/clk-common.c
index ad7951b6b285..dcf4e25a0216 100644
--- a/drivers/clk/sirf/clk-common.c
+++ b/drivers/clk/sirf/clk-common.c
@@ -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;
--
Sent by a computer through tubes
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.
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(-)
diff --git a/drivers/clk/socfpga/clk-gate.c b/drivers/clk/socfpga/clk-gate.c
index 3966cd43b552..b3c8143909dc 100644
--- a/drivers/clk/socfpga/clk-gate.c
+++ b/drivers/clk/socfpga/clk-gate.c
@@ -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 */
diff --git a/drivers/clk/socfpga/clk-periph-a10.c b/drivers/clk/socfpga/clk-periph-a10.c
index a8ff7229611d..3e0c55727b89 100644
--- a/drivers/clk/socfpga/clk-periph-a10.c
+++ b/drivers/clk/socfpga/clk-periph-a10.c
@@ -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
--
Sent by a computer through tubes
Hi Stephen,
Le 31/07/2019 ? 21:35, Stephen Boyd a ?crit :
> 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.
>
> 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(-)
>
> diff --git a/drivers/clk/meson/axg-audio.c b/drivers/clk/meson/axg-audio.c
> index 8028ff6f6610..db0b73d53551 100644
> --- a/drivers/clk/meson/axg-audio.c
> +++ b/drivers/clk/meson/axg-audio.c
> @@ -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;
> }
> }
>
Acked-by: Neil Armstrong <[email protected]>
On Wed, Jul 31, 2019 at 12:35:10PM -0700, Stephen Boyd wrote:
> 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.
>
> Cc: Charles Keepax <[email protected]>
> Cc: Richard Fitzgerald <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
>
Acked-by: Charles Keepax <[email protected]>
Thanks,
Charles
Hi Stephen,
On 7/31/19 2:35 PM, Stephen Boyd wrote:
> 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.
>
> 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(-)
>
> diff --git a/drivers/clk/socfpga/clk-gate.c b/drivers/clk/socfpga/clk-gate.c
> index 3966cd43b552..b3c8143909dc 100644
> --- a/drivers/clk/socfpga/clk-gate.c
> +++ b/drivers/clk/socfpga/clk-gate.c
> @@ -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:
Acked-by: Dinh Nguyen <[email protected]>
Thanks,
Dinh
Quoting Dinh Nguyen (2019-08-01 08:12:58)
> Hi Stephen,
>
> On 7/31/19 2:35 PM, Stephen Boyd wrote:
> > 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.
> >
> > 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(-)
> >
> > diff --git a/drivers/clk/socfpga/clk-gate.c b/drivers/clk/socfpga/clk-gate.c
> > index 3966cd43b552..b3c8143909dc 100644
> > --- a/drivers/clk/socfpga/clk-gate.c
> > +++ b/drivers/clk/socfpga/clk-gate.c
> > @@ -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:
>
> Acked-by: Dinh Nguyen <[email protected]>
Awesome thanks!
On Wed, Jul 31, 2019 at 12:35:09PM -0700, Stephen Boyd wrote:
> 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.
>
> Cc: Manivannan Sadhasivam <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
Acked-by: Manivannan Sadhasivam <[email protected]>
Thanks,
Mani
> ---
>
> Please ack so I can take this through clk tree
>
> drivers/clk/actions/owl-common.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/actions/owl-common.c b/drivers/clk/actions/owl-common.c
> index 32dd29e0a37e..71b683c4e643 100644
> --- a/drivers/clk/actions/owl-common.c
> +++ b/drivers/clk/actions/owl-common.c
> @@ -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;
> }
> }
> --
> Sent by a computer through tubes
>
On 8/1/2019 1:05 AM, Stephen Boyd wrote:
> 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.
>
> Cc: Taniya Das <[email protected]>
> Cc: Andy Gross <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
Acked-by: Taniya Das <[email protected]>
> ---
>
> Please ack so I can take this through clk tree
>
> drivers/clk/qcom/clk-rpmh.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> index c3fd632af119..7a8a84dcb70d 100644
> --- a/drivers/clk/qcom/clk-rpmh.c
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -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;
> }
> }
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
--
On Wed 31 Jul 2019 at 12:35, Stephen Boyd <[email protected]> wrote:
> 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.
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.
>
> 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(-)
>
> diff --git a/drivers/clk/meson/axg-audio.c b/drivers/clk/meson/axg-audio.c
> index 8028ff6f6610..db0b73d53551 100644
> --- a/drivers/clk/meson/axg-audio.c
> +++ b/drivers/clk/meson/axg-audio.c
> @@ -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;
> }
> }
> --
> Sent by a computer through tubes
Quoting Jerome Brunet (2019-08-06 01:49:47)
> On Wed 31 Jul 2019 at 12:35, Stephen Boyd <[email protected]> wrote:
>
> > 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.
>
> 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
>
> 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.
>
> 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.
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?
>
> >
> > 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(-)
> >
On Tue 06 Aug 2019 at 14:48, Stephen Boyd <[email protected]> wrote:
> Quoting Jerome Brunet (2019-08-06 01:49:47)
>> On Wed 31 Jul 2019 at 12:35, Stephen Boyd <[email protected]> wrote:
>>
>> > 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.
>>
>> 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.
>>
>> 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
>
>>
>> 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.
>
> Do you have other usage of the init_data besides printing out the
> name?
No other use
> 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.
> 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.
>
>>
>> >
>> > 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(-)
>> >
Quoting Stephen Boyd (2019-07-31 12:35:09)
> 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.
>
> Cc: Manivannan Sadhasivam <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
Applied to clk-next
Quoting Stephen Boyd (2019-07-31 12:35:10)
> 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.
>
> Cc: Charles Keepax <[email protected]>
> Cc: Richard Fitzgerald <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
Applied to clk-next
Quoting Stephen Boyd (2019-07-31 12:35:11)
> 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.
>
> Cc: Neil Armstrong <[email protected]>
> Cc: Jerome Brunet <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
Applied to clk-next
Quoting Stephen Boyd (2019-07-31 12:35:12)
> 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.
>
> Cc: Taniya Das <[email protected]>
> Cc: Andy Gross <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
Applied to clk-next
Quoting Stephen Boyd (2019-07-31 12:35:13)
> 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.
>
> Cc: Guo Zeng <[email protected]>
> Cc: Barry Song <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
Applied to clk-next
Quoting Stephen Boyd (2019-07-31 12:35:14)
> 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.
>
> Cc: Dinh Nguyen <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
Applied to clk-next
On Wed, Jul 31, 2019 at 12:35:09PM -0700, Stephen Boyd wrote:
> 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.
>
> 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(-)
>
> diff --git a/drivers/clk/actions/owl-common.c b/drivers/clk/actions/owl-common.c
> index 32dd29e0a37e..71b683c4e643 100644
> --- a/drivers/clk/actions/owl-common.c
> +++ b/drivers/clk/actions/owl-common.c
> @@ -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
> 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;
> }
> }
> --
> Sent by a computer through tubes
>
Quoting Manivannan Sadhasivam (2019-08-16 04:22:10)
> On Wed, Jul 31, 2019 at 12:35:09PM -0700, Stephen Boyd wrote:
> > 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.
> >
> > 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(-)
> >
> > diff --git a/drivers/clk/actions/owl-common.c b/drivers/clk/actions/owl-common.c
> > index 32dd29e0a37e..71b683c4e643 100644
> > --- a/drivers/clk/actions/owl-common.c
> > +++ b/drivers/clk/actions/owl-common.c
> > @@ -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!
>
Oh right. Will fix it.