diff mbox series

clk: microchip: mpfs-ccc: Use devm_kasprintf() for allocating formatted strings

Message ID f904fd28b2087d1463ea65f059924e3b1acc193c.1672764239.git.geert+renesas@glider.be
State New
Headers
Series clk: microchip: mpfs-ccc: Use devm_kasprintf() for allocating formatted strings |

Commit Message

Geert Uytterhoeven Jan. 3, 2023, 4:45 p.m. UTC 
  In various places, string buffers of a fixed size are allocated, and
filled using snprintf() with the same fixed size, which is error-prone.

Replace this by calling devm_kasprintf() instead, which always uses the
appropriate size.

While at it, remove an unneeded intermediate variable, which allows us
to drop a cast as a bonus.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/clk/microchip/clk-mpfs-ccc.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Comments

Conor Dooley Jan. 3, 2023, 5:32 p.m. UTC | #1 
On Tue, Jan 03, 2023 at 05:45:30PM +0100, Geert Uytterhoeven wrote:
> In various places, string buffers of a fixed size are allocated, and
> filled using snprintf() with the same fixed size, which is error-prone.
> 
> Replace this by calling devm_kasprintf() instead, which always uses the
> appropriate size.
> 
> While at it, remove an unneeded intermediate variable, which allows us
> to drop a cast as a bonus.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Tested-by: Conor Dooley <conor.dooley@microchip.com>

I half wonder if this should actually have a fixes tag too. Since it
used what came after the @ in $full_name, it'd be possible to create
(an incorrect) DTS that would lead to a clash between pll names &
therefore probe would fail.
The tag would be:
Fixes: d39fb172760e ("clk: microchip: add PolarFire SoC fabric clock support")

Either way, thanks for pointing out a function that does exactly what I
initially wanted to do but could not find! Glad to be rid of those
"oddly specific" allocations.

Thanks,
Conor.
> ---
>  drivers/clk/microchip/clk-mpfs-ccc.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/microchip/clk-mpfs-ccc.c b/drivers/clk/microchip/clk-mpfs-ccc.c
> index 32aae880a14f3b1c..0ddc73e07be42973 100644
> --- a/drivers/clk/microchip/clk-mpfs-ccc.c
> +++ b/drivers/clk/microchip/clk-mpfs-ccc.c
> @@ -164,12 +164,11 @@ static int mpfs_ccc_register_outputs(struct device *dev, struct mpfs_ccc_out_hw_
>  
>  	for (unsigned int i = 0; i < num_clks; i++) {
>  		struct mpfs_ccc_out_hw_clock *out_hw = &out_hws[i];
> -		char *name = devm_kzalloc(dev, 23, GFP_KERNEL);
> +		char *name = devm_kasprintf(dev, GFP_KERNEL, "%s_out%u", parent->name, i);
>  
>  		if (!name)
>  			return -ENOMEM;
>  
> -		snprintf(name, 23, "%s_out%u", parent->name, i);
>  		out_hw->divider.hw.init = CLK_HW_INIT_HW(name, &parent->hw, &clk_divider_ops, 0);
>  		out_hw->divider.reg = data->pll_base[i / MPFS_CCC_OUTPUTS_PER_PLL] +
>  			out_hw->reg_offset;
> @@ -201,14 +200,13 @@ static int mpfs_ccc_register_plls(struct device *dev, struct mpfs_ccc_pll_hw_clo
>  
>  	for (unsigned int i = 0; i < num_clks; i++) {
>  		struct mpfs_ccc_pll_hw_clock *pll_hw = &pll_hws[i];
> -		char *name = devm_kzalloc(dev, 18, GFP_KERNEL);
>  
> -		if (!name)
> +		pll_hw->name = devm_kasprintf(dev, GFP_KERNEL, "ccc%s_pll%u",
> +					      strchrnul(dev->of_node->full_name, '@'), i);
> +		if (!pll_hw->name)
>  			return -ENOMEM;
>  
>  		pll_hw->base = data->pll_base[i];
> -		snprintf(name, 18, "ccc%s_pll%u", strchrnul(dev->of_node->full_name, '@'), i);
> -		pll_hw->name = (const char *)name;
>  		pll_hw->hw.init = CLK_HW_INIT_PARENTS_DATA_FIXED_SIZE(pll_hw->name,
>  								      pll_hw->parents,
>  								      &mpfs_ccc_pll_ops, 0);
> -- 
> 2.25.1
> 
>
Geert Uytterhoeven Jan. 4, 2023, 1:26 p.m. UTC | #2 
Hi Conor,

On Tue, Jan 3, 2023 at 6:32 PM Conor Dooley <conor@kernel.org> wrote:
> On Tue, Jan 03, 2023 at 05:45:30PM +0100, Geert Uytterhoeven wrote:
> > In various places, string buffers of a fixed size are allocated, and
> > filled using snprintf() with the same fixed size, which is error-prone.
> >
> > Replace this by calling devm_kasprintf() instead, which always uses the
> > appropriate size.
> >
> > While at it, remove an unneeded intermediate variable, which allows us
> > to drop a cast as a bonus.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> Tested-by: Conor Dooley <conor.dooley@microchip.com>

Thanks!

> I half wonder if this should actually have a fixes tag too. Since it
> used what came after the @ in $full_name, it'd be possible to create
> (an incorrect) DTS that would lead to a clash between pll names &
> therefore probe would fail.
> The tag would be:
> Fixes: d39fb172760e ("clk: microchip: add PolarFire SoC fabric clock support")

But I don't change any of that in my patch?
/me confused.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Conor Dooley Jan. 4, 2023, 1:40 p.m. UTC | #3 
On Wed, Jan 04, 2023 at 02:26:53PM +0100, Geert Uytterhoeven wrote:
> Hi Conor,
> 
> On Tue, Jan 3, 2023 at 6:32 PM Conor Dooley <conor@kernel.org> wrote:

> > I half wonder if this should actually have a fixes tag too. Since it
> > used what came after the @ in $full_name, it'd be possible to create
> > (an incorrect) DTS that would lead to a clash between pll names &
> > therefore probe would fail.
> > The tag would be:
> > Fixes: d39fb172760e ("clk: microchip: add PolarFire SoC fabric clock support")
> 
> But I don't change any of that in my patch?
> /me confused.

The numbers in there were chosen to fit exactly what is in mpfs.dtsi
(IOW the correct node address), so doing
@@ -201,14 +200,13 @@ static int mpfs_ccc_register_plls(struct device *dev, struct mpfs_ccc_pll_hw_clo
 
 	for (unsigned int i = 0; i < num_clks; i++) {
 		struct mpfs_ccc_pll_hw_clock *pll_hw = &pll_hws[i];
-		char *name = devm_kzalloc(dev, 18, GFP_KERNEL);
 
-		if (!name)
+		pll_hw->name = devm_kasprintf(dev, GFP_KERNEL, "ccc%s_pll%u",
+					      strchrnul(dev->of_node->full_name, '@'), i);
+		if (!pll_hw->name)
 			return -ENOMEM;
 
 		pll_hw->base = data->pll_base[i];
-		snprintf(name, 18, "ccc%s_pll%u", strchrnul(dev->of_node->full_name, '@'), i);
-		pll_hw->name = (const char *)name;
 		pll_hw->hw.init = CLK_HW_INIT_PARENTS_DATA_FIXED_SIZE(pll_hw->name,
 								      pll_hw->parents,
 								      &mpfs_ccc_pll_ops, 0);

means that we no longer have to worry that someone would provide a
device tree with a node address that would make "ccc<node_address>_pll<N>"
exceed 18 characters. If that happened, the <N> would be cut off & both
pll 0 & 1 would be named identically. If that happens, pll1 would fail
to register.

Or am I misunderstanding something? Probably am..

Thanks,
Conor.
Geert Uytterhoeven Jan. 4, 2023, 1:44 p.m. UTC | #4 
Hi Conor,

On Wed, Jan 4, 2023 at 2:40 PM Conor Dooley <conor@kernel.org> wrote:
> On Wed, Jan 04, 2023 at 02:26:53PM +0100, Geert Uytterhoeven wrote:
> > On Tue, Jan 3, 2023 at 6:32 PM Conor Dooley <conor@kernel.org> wrote:
> > > I half wonder if this should actually have a fixes tag too. Since it
> > > used what came after the @ in $full_name, it'd be possible to create
> > > (an incorrect) DTS that would lead to a clash between pll names &
> > > therefore probe would fail.
> > > The tag would be:
> > > Fixes: d39fb172760e ("clk: microchip: add PolarFire SoC fabric clock support")
> >
> > But I don't change any of that in my patch?
> > /me confused.
>
> The numbers in there were chosen to fit exactly what is in mpfs.dtsi
> (IOW the correct node address), so doing
> @@ -201,14 +200,13 @@ static int mpfs_ccc_register_plls(struct device *dev, struct mpfs_ccc_pll_hw_clo
>
>         for (unsigned int i = 0; i < num_clks; i++) {
>                 struct mpfs_ccc_pll_hw_clock *pll_hw = &pll_hws[i];
> -               char *name = devm_kzalloc(dev, 18, GFP_KERNEL);
>
> -               if (!name)
> +               pll_hw->name = devm_kasprintf(dev, GFP_KERNEL, "ccc%s_pll%u",
> +                                             strchrnul(dev->of_node->full_name, '@'), i);
> +               if (!pll_hw->name)
>                         return -ENOMEM;
>
>                 pll_hw->base = data->pll_base[i];
> -               snprintf(name, 18, "ccc%s_pll%u", strchrnul(dev->of_node->full_name, '@'), i);
> -               pll_hw->name = (const char *)name;
>                 pll_hw->hw.init = CLK_HW_INIT_PARENTS_DATA_FIXED_SIZE(pll_hw->name,
>                                                                       pll_hw->parents,
>                                                                       &mpfs_ccc_pll_ops, 0);
>
> means that we no longer have to worry that someone would provide a
> device tree with a node address that would make "ccc<node_address>_pll<N>"
> exceed 18 characters. If that happened, the <N> would be cut off & both
> pll 0 & 1 would be named identically. If that happens, pll1 would fail
> to register.

Oh right.  Yeah, before we would get collisions, now we would get
a different broken system.
DTS garbage in, garbage behavior out ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Conor Dooley Jan. 11, 2023, 7:26 p.m. UTC | #5 
Hey Claudiu,

Could you pick this one up when you get a chance please?

Thanks,
Conor.

On Tue, Jan 03, 2023 at 05:45:30PM +0100, Geert Uytterhoeven wrote:
> In various places, string buffers of a fixed size are allocated, and
> filled using snprintf() with the same fixed size, which is error-prone.
> 
> Replace this by calling devm_kasprintf() instead, which always uses the
> appropriate size.
> 
> While at it, remove an unneeded intermediate variable, which allows us
> to drop a cast as a bonus.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/clk/microchip/clk-mpfs-ccc.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/microchip/clk-mpfs-ccc.c b/drivers/clk/microchip/clk-mpfs-ccc.c
> index 32aae880a14f3b1c..0ddc73e07be42973 100644
> --- a/drivers/clk/microchip/clk-mpfs-ccc.c
> +++ b/drivers/clk/microchip/clk-mpfs-ccc.c
> @@ -164,12 +164,11 @@ static int mpfs_ccc_register_outputs(struct device *dev, struct mpfs_ccc_out_hw_
>  
>  	for (unsigned int i = 0; i < num_clks; i++) {
>  		struct mpfs_ccc_out_hw_clock *out_hw = &out_hws[i];
> -		char *name = devm_kzalloc(dev, 23, GFP_KERNEL);
> +		char *name = devm_kasprintf(dev, GFP_KERNEL, "%s_out%u", parent->name, i);
>  
>  		if (!name)
>  			return -ENOMEM;
>  
> -		snprintf(name, 23, "%s_out%u", parent->name, i);
>  		out_hw->divider.hw.init = CLK_HW_INIT_HW(name, &parent->hw, &clk_divider_ops, 0);
>  		out_hw->divider.reg = data->pll_base[i / MPFS_CCC_OUTPUTS_PER_PLL] +
>  			out_hw->reg_offset;
> @@ -201,14 +200,13 @@ static int mpfs_ccc_register_plls(struct device *dev, struct mpfs_ccc_pll_hw_clo
>  
>  	for (unsigned int i = 0; i < num_clks; i++) {
>  		struct mpfs_ccc_pll_hw_clock *pll_hw = &pll_hws[i];
> -		char *name = devm_kzalloc(dev, 18, GFP_KERNEL);
>  
> -		if (!name)
> +		pll_hw->name = devm_kasprintf(dev, GFP_KERNEL, "ccc%s_pll%u",
> +					      strchrnul(dev->of_node->full_name, '@'), i);
> +		if (!pll_hw->name)
>  			return -ENOMEM;
>  
>  		pll_hw->base = data->pll_base[i];
> -		snprintf(name, 18, "ccc%s_pll%u", strchrnul(dev->of_node->full_name, '@'), i);
> -		pll_hw->name = (const char *)name;
>  		pll_hw->hw.init = CLK_HW_INIT_PARENTS_DATA_FIXED_SIZE(pll_hw->name,
>  								      pll_hw->parents,
>  								      &mpfs_ccc_pll_ops, 0);
> -- 
> 2.25.1
>
Claudiu Beznea Jan. 12, 2023, 8:52 a.m. UTC | #6 
Hi, Conor,

On 11.01.2023 21:26, Conor Dooley wrote:
> Hey Claudiu,
> 
> Could you pick this one up when you get a chance please?

It's already in my tree, not pushed yet, though. It'll do it soon. I'll let
you know.

Claudiu

> 
> Thanks,
Claudiu Beznea Jan. 12, 2023, 10:04 a.m. UTC | #7 
On 03.01.2023 18:45, Geert Uytterhoeven wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> In various places, string buffers of a fixed size are allocated, and
> filled using snprintf() with the same fixed size, which is error-prone.
> 
> Replace this by calling devm_kasprintf() instead, which always uses the
> appropriate size.
> 
> While at it, remove an unneeded intermediate variable, which allows us
> to drop a cast as a bonus.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Applied to clk-microchip-fixes, thanks!

> ---
>  drivers/clk/microchip/clk-mpfs-ccc.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/microchip/clk-mpfs-ccc.c b/drivers/clk/microchip/clk-mpfs-ccc.c
> index 32aae880a14f3b1c..0ddc73e07be42973 100644
> --- a/drivers/clk/microchip/clk-mpfs-ccc.c
> +++ b/drivers/clk/microchip/clk-mpfs-ccc.c
> @@ -164,12 +164,11 @@ static int mpfs_ccc_register_outputs(struct device *dev, struct mpfs_ccc_out_hw_
> 
>         for (unsigned int i = 0; i < num_clks; i++) {
>                 struct mpfs_ccc_out_hw_clock *out_hw = &out_hws[i];
> -               char *name = devm_kzalloc(dev, 23, GFP_KERNEL);
> +               char *name = devm_kasprintf(dev, GFP_KERNEL, "%s_out%u", parent->name, i);
> 
>                 if (!name)
>                         return -ENOMEM;
> 
> -               snprintf(name, 23, "%s_out%u", parent->name, i);
>                 out_hw->divider.hw.init = CLK_HW_INIT_HW(name, &parent->hw, &clk_divider_ops, 0);
>                 out_hw->divider.reg = data->pll_base[i / MPFS_CCC_OUTPUTS_PER_PLL] +
>                         out_hw->reg_offset;
> @@ -201,14 +200,13 @@ static int mpfs_ccc_register_plls(struct device *dev, struct mpfs_ccc_pll_hw_clo
> 
>         for (unsigned int i = 0; i < num_clks; i++) {
>                 struct mpfs_ccc_pll_hw_clock *pll_hw = &pll_hws[i];
> -               char *name = devm_kzalloc(dev, 18, GFP_KERNEL);
> 
> -               if (!name)
> +               pll_hw->name = devm_kasprintf(dev, GFP_KERNEL, "ccc%s_pll%u",
> +                                             strchrnul(dev->of_node->full_name, '@'), i);
> +               if (!pll_hw->name)
>                         return -ENOMEM;
> 
>                 pll_hw->base = data->pll_base[i];
> -               snprintf(name, 18, "ccc%s_pll%u", strchrnul(dev->of_node->full_name, '@'), i);
> -               pll_hw->name = (const char *)name;
>                 pll_hw->hw.init = CLK_HW_INIT_PARENTS_DATA_FIXED_SIZE(pll_hw->name,
>                                                                       pll_hw->parents,
>                                                                       &mpfs_ccc_pll_ops, 0);
> --
> 2.25.1
>
diff mbox series

Patch

diff --git a/drivers/clk/microchip/clk-mpfs-ccc.c b/drivers/clk/microchip/clk-mpfs-ccc.c
index 32aae880a14f3b1c..0ddc73e07be42973 100644
--- a/drivers/clk/microchip/clk-mpfs-ccc.c
+++ b/drivers/clk/microchip/clk-mpfs-ccc.c
@@ -164,12 +164,11 @@  static int mpfs_ccc_register_outputs(struct device *dev, struct mpfs_ccc_out_hw_
 
 	for (unsigned int i = 0; i < num_clks; i++) {
 		struct mpfs_ccc_out_hw_clock *out_hw = &out_hws[i];
-		char *name = devm_kzalloc(dev, 23, GFP_KERNEL);
+		char *name = devm_kasprintf(dev, GFP_KERNEL, "%s_out%u", parent->name, i);
 
 		if (!name)
 			return -ENOMEM;
 
-		snprintf(name, 23, "%s_out%u", parent->name, i);
 		out_hw->divider.hw.init = CLK_HW_INIT_HW(name, &parent->hw, &clk_divider_ops, 0);
 		out_hw->divider.reg = data->pll_base[i / MPFS_CCC_OUTPUTS_PER_PLL] +
 			out_hw->reg_offset;
@@ -201,14 +200,13 @@  static int mpfs_ccc_register_plls(struct device *dev, struct mpfs_ccc_pll_hw_clo
 
 	for (unsigned int i = 0; i < num_clks; i++) {
 		struct mpfs_ccc_pll_hw_clock *pll_hw = &pll_hws[i];
-		char *name = devm_kzalloc(dev, 18, GFP_KERNEL);
 
-		if (!name)
+		pll_hw->name = devm_kasprintf(dev, GFP_KERNEL, "ccc%s_pll%u",
+					      strchrnul(dev->of_node->full_name, '@'), i);
+		if (!pll_hw->name)
 			return -ENOMEM;
 
 		pll_hw->base = data->pll_base[i];
-		snprintf(name, 18, "ccc%s_pll%u", strchrnul(dev->of_node->full_name, '@'), i);
-		pll_hw->name = (const char *)name;
 		pll_hw->hw.init = CLK_HW_INIT_PARENTS_DATA_FIXED_SIZE(pll_hw->name,
 								      pll_hw->parents,
 								      &mpfs_ccc_pll_ops, 0);