Skip to contentSkip to conversation

clkdev: report over-sized strings when creating clkdev entries

PATCH Applied Russell King (Oracle) 5 participants 19 messages Mar 15, 2024 – May 22, 2024

Subsystems (4)
Common clk Regressions SiFive drivers Arm/clkdev support
lore
Share & tools
lore

Russell King (Oracle)

author
#0 lore raw

PATCH 1/1 clkdev: report over-sized strings when creating clkdev entries

Report an error when an attempt to register a clkdev entry results in a truncated string so the problem can be easily spotted.

Reported by: Duanqiang Wen <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>
---
 drivers/clk/clkdev.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)
drivers/clk/clkdev.c +24 -2
index ee37d0be6877..3146f26ab997 100644
@@ -158,6 +158,10 @@ vclkdev_alloc(struct clk_hw *hw, const char *con_id, const char *dev_fmt,
 	va_list ap)
 {
 	struct clk_lookup_alloc *cla;
+	struct va_format fmt;
+	const char *failure;
+	size_t max_size;
+	ssize_t res;
 
 	cla = kzalloc(sizeof(*cla), GFP_KERNEL);
 	if (!cla)

@@ -165,16 +169,34 @@ vclkdev_alloc(struct clk_hw *hw, const char *con_id, const char *dev_fmt,
 
 	cla->cl.clk_hw = hw;
 	if (con_id) {
-		strscpy(cla->con_id, con_id, sizeof(cla->con_id));
+		res = strscpy(cla->con_id, con_id, sizeof(cla->con_id));
+		if (res < 0) {
+			max_size = sizeof(cla->con_id);
+			failure = "connection";
+			goto fail;
+		}
 		cla->cl.con_id = cla->con_id;
 	}
 
 	if (dev_fmt) {
-		vscnprintf(cla->dev_id, sizeof(cla->dev_id), dev_fmt, ap);
+		res = vsnprintf(cla->dev_id, sizeof(cla->dev_id), dev_fmt, ap);
+		if (res >= sizeof(cla->dev_id)) {
+			max_size = sizeof(cla->dev_id);
+			failure = "device";
+			goto fail;
+		}
 		cla->cl.dev_id = cla->dev_id;
 	}
 
 	return &cla->cl;
+
+fail:
+	fmt.fmt = dev_fmt;
+	fmt.va = &ap;
+	pr_err("%pV:%s: %s ID is greater than %zu\n",
+	       &fmt, con_id, failure, max_size);
+	kfree(cla);
+	return NULL;
 }
 
 static struct clk_lookup *
Signature · 1 line
2.30.2
maintainer Reviewed-by on v1 1/1
#1 lore raw

Quoting Russell King (Oracle) (2024-03-15 04:47:55)

Quoted 6 lines

Report an error when an attempt to register a clkdev entry results in a truncated string so the problem can be easily spotted.

Reported by: Duanqiang Wen <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

---

Russell, are you taking this through your tree? I took the last one because it was small and you reviewed it instead of applied it. If you're taking it please add:

Reviewed-by Stephen Boyd <[email protected]>
maintainer
#3 lore raw

Quoting Stephen Boyd (2024-05-01 17:59:16)

Quoted 10 lines

Quoting Russell King (Oracle) (2024-03-15 04:47:55)

Quoted 6 lines

Report an error when an attempt to register a clkdev entry results in a truncated string so the problem can be easily spotted.

Reported by: Duanqiang Wen <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

---

Applied to clk-next

And backed out because I get a compilation failure

drivers/clk/clkdev.c: In function 'vclkdev_alloc':
drivers/clk/clkdev.c:182:17: error: function 'vclkdev_alloc' might be a candidate for 'gnu_printf' format attribute [-Werror=suggest-attribute=format]

  182 |                 res = vsnprintf(cla->dev_id, sizeof(cla->dev_id), dev_fmt, ap);
      |                 ^~~

cc1: all warnings being treated as errors
make[5]: *** [scripts/Makefile.build:244: drivers/clk/clkdev.o] Error 1 make[4]: *** [scripts/Makefile.build:485: drivers/clk] Error 2

maintainer
#4 lore raw
Quoted 14 lines · Stephen Boyd

Quoting Stephen Boyd (2024-05-01 17:59:16)

Quoted 10 lines

Quoting Russell King (Oracle) (2024-03-15 04:47:55)

Quoted 6 lines

Report an error when an attempt to register a clkdev entry results in a truncated string so the problem can be easily spotted.

Reported by: Duanqiang Wen <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

---

Applied to clk-next

And backed out because I get a compilation failure

Quoted 7 lines

drivers/clk/clkdev.c: In function 'vclkdev_alloc':
drivers/clk/clkdev.c:182:17: error: function 'vclkdev_alloc' might be a candidate for 'gnu_printf' format attribute [-Werror=suggest-attribute=format]

  182 |                 res = vsnprintf(cla->dev_id, sizeof(cla->dev_id), dev_fmt, ap);
      |                 ^~~

cc1: all warnings being treated as errors
make[5]: *** [scripts/Makefile.build:244: drivers/clk/clkdev.o] Error 1 make[4]: *** [scripts/Makefile.build:485: drivers/clk] Error 2

It builds fine for me. I don't get this _error_, and it's really no different from what it originally was - instead of using vcsnprintf() we're now using vsnprintf(). That should make no difference what so ever.

Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
maintainer
#5 lore raw
Quoted 23 lines · Russell King (Oracle)
Quoted 14 lines · Stephen Boyd

Quoting Stephen Boyd (2024-05-01 17:59:16)

Quoted 10 lines

Quoting Russell King (Oracle) (2024-03-15 04:47:55)

Quoted 6 lines

Report an error when an attempt to register a clkdev entry results in a truncated string so the problem can be easily spotted.

Reported by: Duanqiang Wen <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

---

Applied to clk-next

And backed out because I get a compilation failure

Quoted 7 lines

drivers/clk/clkdev.c: In function 'vclkdev_alloc':
drivers/clk/clkdev.c:182:17: error: function 'vclkdev_alloc' might be a candidate for 'gnu_printf' format attribute [-Werror=suggest-attribute=format]

  182 |                 res = vsnprintf(cla->dev_id, sizeof(cla->dev_id), dev_fmt, ap);
      |                 ^~~

cc1: all warnings being treated as errors
make[5]: *** [scripts/Makefile.build:244: drivers/clk/clkdev.o] Error 1 make[4]: *** [scripts/Makefile.build:485: drivers/clk] Error 2

Quoted 4 lines

It builds fine for me. I don't get this _error_, and it's really no different from what it originally was - instead of using vcsnprintf() we're now using vsnprintf(). That should make no difference what so ever.

... and I've just checked, and it builds entirely cleanly for me.

I'll merge it.

Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
maintainer
#6 lore raw
Quoted 24 lines · Russell King (Oracle)
Quoted 23 lines · Russell King (Oracle)
Quoted 21 lines · Stephen Boyd

Quoting Stephen Boyd (2024-05-01 17:59:16)

Quoted 10 lines

Quoting Russell King (Oracle) (2024-03-15 04:47:55)

Quoted 6 lines

Report an error when an attempt to register a clkdev entry results in a truncated string so the problem can be easily spotted.

Reported by: Duanqiang Wen <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

---

Applied to clk-next

And backed out because I get a compilation failure

drivers/clk/clkdev.c: In function 'vclkdev_alloc':
drivers/clk/clkdev.c:182:17: error: function 'vclkdev_alloc' might be a candidate for 'gnu_printf' format attribute [-Werror=suggest-attribute=format]

  182 |                 res = vsnprintf(cla->dev_id, sizeof(cla->dev_id), dev_fmt, ap);
      |                 ^~~

cc1: all warnings being treated as errors
make[5]: *** [scripts/Makefile.build:244: drivers/clk/clkdev.o] Error 1 make[4]: *** [scripts/Makefile.build:485: drivers/clk] Error 2

Quoted 8 lines
Quoted 4 lines

It builds fine for me. I don't get this _error_, and it's really no different from what it originally was - instead of using vcsnprintf() we're now using vsnprintf(). That should make no difference what so ever.

... and I've just checked, and it builds entirely cleanly for me.

I'll merge it.

It should be in tonight's linux-next tree.

I also note... the kernel build bot never flagged the above, not even a warning for it, and it would've built the patch as it was Cc'd to linux-kernel. So my conclusion is the error you are seeing is somehow specific to your environment. Maybe you're building with an additional set of warning flags and -Werror?

Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
maintainer
#7 lore raw

Quoting Russell King (Oracle) (2024-05-02 04:08:38)

Quoted 30 lines · Russell King (Oracle)
Quoted 29 lines · Russell King (Oracle)
Quoted 27 lines · Stephen Boyd
Quoted 14 lines

Quoting Stephen Boyd (2024-05-01 17:59:16)

Quoted 10 lines

Quoting Russell King (Oracle) (2024-03-15 04:47:55)

Quoted 6 lines

Report an error when an attempt to register a clkdev entry results in a truncated string so the problem can be easily spotted.

Reported by: Duanqiang Wen <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>

---

Applied to clk-next

And backed out because I get a compilation failure

Quoted 7 lines

drivers/clk/clkdev.c: In function 'vclkdev_alloc':
drivers/clk/clkdev.c:182:17: error: function 'vclkdev_alloc' might be a candidate for 'gnu_printf' format attribute [-Werror=suggest-attribute=format]

  182 |                 res = vsnprintf(cla->dev_id, sizeof(cla->dev_id), dev_fmt, ap);
      |                 ^~~

cc1: all warnings being treated as errors
make[5]: *** [scripts/Makefile.build:244: drivers/clk/clkdev.o] Error 1 make[4]: *** [scripts/Makefile.build:485: drivers/clk] Error 2

It builds fine for me. I don't get this _error_, and it's really no different from what it originally was - instead of using vcsnprintf() we're now using vsnprintf(). That should make no difference what so ever.

Quoted 5 lines
Quoted 3 lines

... and I've just checked, and it builds entirely cleanly for me.

I'll merge it.

It should be in tonight's linux-next tree.

Ok thanks

Quoted 4 lines

I also note... the kernel build bot never flagged the above, not even a warning for it, and it would've built the patch as it was Cc'd to linux-kernel.

Indeed. I see a report here[1] but it's only a warning. And there was some work a few years ago[2] that I forgot about. Seems there was a possible fix[3] as well.

Quoted 4 lines

So my conclusion is the error you are seeing is somehow specific to your environment. Maybe you're building with an additional set of warning flags and -Werror?

Yes I build with W=1 but I didn't think that turned warnings into errors. Maybe that part is new.

[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/20210310085937.GF4931@dell/
[3] https://lore.kernel.org/all/[email protected]/

#8 lore raw
In reply to Russell King (Oracle)

Hi,

Quoted 6 lines · Russell King (Oracle)

Report an error when an attempt to register a clkdev entry results in a truncated string so the problem can be easily spotted.

Reported by: Duanqiang Wen <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>

With this patch in the mainline kernel, I get

10000000.clock-controller:corepll: device ID is greater than 24
sifive-clk-prci 10000000.clock-controller: Failed to register clkdev for corepll: -12 sifive-clk-prci 10000000.clock-controller: could not register clocks: -12
sifive-clk-prci 10000000.clock-controller: probe with driver sifive-clk-prci failed with error -12
...
platform 10060000.gpio: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10010000.serial: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10011000.serial: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10040000.spi: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10050000.spi: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10090000.ethernet: deferred probe pending: platform: supplier 10000000.clock-controller not ready

when trying to boot sifive_u in qemu.

Apparently, "10000000.clock-controller" is too long. Any suggestion on how to solve the problem ? I guess using dev_name(dev) as dev_id parameter for clk_hw_register_clkdev() is not or no longer a good idea. What else should be used instead ?

Thanks,
Guenter

maintainer
#9 lore raw
Quoted 24 lines · Guenter Roeck

Hi,

Quoted 6 lines · Russell King (Oracle)

Report an error when an attempt to register a clkdev entry results in a truncated string so the problem can be easily spotted.

Reported by: Duanqiang Wen <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>

With this patch in the mainline kernel, I get

10000000.clock-controller:corepll: device ID is greater than 24
sifive-clk-prci 10000000.clock-controller: Failed to register clkdev for corepll: -12 sifive-clk-prci 10000000.clock-controller: could not register clocks: -12
sifive-clk-prci 10000000.clock-controller: probe with driver sifive-clk-prci failed with error -12
...
platform 10060000.gpio: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10010000.serial: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10011000.serial: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10040000.spi: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10050000.spi: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10090000.ethernet: deferred probe pending: platform: supplier 10000000.clock-controller not ready

Quoted 6 lines

when trying to boot sifive_u in qemu.

Apparently, "10000000.clock-controller" is too long. Any suggestion on how to solve the problem ? I guess using dev_name(dev) as dev_id parameter for clk_hw_register_clkdev() is not or no longer a good idea. What else should be used instead ?

It was *never* a good idea. clkdev uses a fixed buffer size of 20 characters including the NUL character, and "10000000.clock-controller" would have been silently truncated to "10000000.clock-cont", and thus

                        if (!dev_id || strcmp(p->dev_id, dev_id))

would never have matched.

We need to think about (a) whether your use of clk_hw_register_clkdev() is still appropriate, and (b) whether we need to increase the size of the strings.

Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
#10 lore raw
Quoted 36 lines · Russell King (Oracle)
Quoted 24 lines · Guenter Roeck

Hi,

Quoted 6 lines · Russell King (Oracle)

Report an error when an attempt to register a clkdev entry results in a truncated string so the problem can be easily spotted.

Reported by: Duanqiang Wen <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>

With this patch in the mainline kernel, I get

10000000.clock-controller:corepll: device ID is greater than 24
sifive-clk-prci 10000000.clock-controller: Failed to register clkdev for corepll: -12 sifive-clk-prci 10000000.clock-controller: could not register clocks: -12
sifive-clk-prci 10000000.clock-controller: probe with driver sifive-clk-prci failed with error -12
...
platform 10060000.gpio: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10010000.serial: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10011000.serial: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10040000.spi: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10050000.spi: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10090000.ethernet: deferred probe pending: platform: supplier 10000000.clock-controller not ready

Quoted 6 lines

when trying to boot sifive_u in qemu.

Apparently, "10000000.clock-controller" is too long. Any suggestion on how to solve the problem ? I guess using dev_name(dev) as dev_id parameter for clk_hw_register_clkdev() is not or no longer a good idea. What else should be used instead ?

It was *never* a good idea. clkdev uses a fixed buffer size of 20 characters including the NUL character, and "10000000.clock-controller" would have been silently truncated to "10000000.clock-cont", and thus

Quoted 8 lines
                         if (!dev_id || strcmp(p->dev_id, dev_id))

would never have matched.

We need to think about (a) whether your use of clk_hw_register_clkdev() is still appropriate, and (b) whether we need to increase the size of the strings.

It isn't _my_ use, really. I only run a variety of boot tests with qemu. I expect we'll see reports from others trying to boot the mainline kernel on real sifive_u hardware or other hardware using the same driver or other drivers using dev_name() as dev_id parameter. Coccinelle finds the following callers:

./sound/soc/codecs/rt5682s.c: dev_name() used as dev_id at line 2833 ./drivers/clk/sifive/sifive-prci.c: dev_name() used as dev_id at line 540 ./sound/soc/codecs/tlv320aic32x4-clk.c: dev_name() used as dev_id at line 475 ./drivers/i2c/busses/i2c-bcm2835.c: dev_name() used as dev_id at line 189 ./sound/soc/codecs/rt5682.c: dev_name() used as dev_id at line 2909

Guenter

maintainer
#11 lore raw
Quoted 46 lines · Guenter Roeck
Quoted 36 lines · Russell King (Oracle)
Quoted 24 lines · Guenter Roeck

Hi,

Quoted 6 lines · Russell King (Oracle)

Report an error when an attempt to register a clkdev entry results in a truncated string so the problem can be easily spotted.

Reported by: Duanqiang Wen <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>

With this patch in the mainline kernel, I get

10000000.clock-controller:corepll: device ID is greater than 24
sifive-clk-prci 10000000.clock-controller: Failed to register clkdev for corepll: -12 sifive-clk-prci 10000000.clock-controller: could not register clocks: -12
sifive-clk-prci 10000000.clock-controller: probe with driver sifive-clk-prci failed with error -12
...
platform 10060000.gpio: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10010000.serial: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10011000.serial: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10040000.spi: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10050000.spi: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10090000.ethernet: deferred probe pending: platform: supplier 10000000.clock-controller not ready

Quoted 6 lines

when trying to boot sifive_u in qemu.

Apparently, "10000000.clock-controller" is too long. Any suggestion on how to solve the problem ? I guess using dev_name(dev) as dev_id parameter for clk_hw_register_clkdev() is not or no longer a good idea. What else should be used instead ?

It was *never* a good idea. clkdev uses a fixed buffer size of 20 characters including the NUL character, and "10000000.clock-controller" would have been silently truncated to "10000000.clock-cont", and thus

Quoted 8 lines
                         if (!dev_id || strcmp(p->dev_id, dev_id))

would never have matched.

We need to think about (a) whether your use of clk_hw_register_clkdev() is still appropriate, and (b) whether we need to increase the size of the strings.

Quoted 5 lines

It isn't _my_ use, really. I only run a variety of boot tests with qemu. I expect we'll see reports from others trying to boot the mainline kernel on real sifive_u hardware or other hardware using the same driver or other drivers using dev_name() as dev_id parameter. Coccinelle finds the following callers:

Using dev_name() is not an issue. It's when dev_name() exceeds 19 characters that it becomes an issue (and always has been an issue due to the truncation.) clk_get(dev, ...) uses dev_name(dev) to match against its entry in the table.

As I say, dev_name() itself is not an issue. The length used for the name is.

Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
#12 lore raw
Quoted 53 lines · Russell King (Oracle)
Quoted 46 lines · Guenter Roeck
Quoted 36 lines · Russell King (Oracle)
Quoted 24 lines · Guenter Roeck

Hi,

Quoted 6 lines · Russell King (Oracle)

Report an error when an attempt to register a clkdev entry results in a truncated string so the problem can be easily spotted.

Reported by: Duanqiang Wen <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>

With this patch in the mainline kernel, I get

10000000.clock-controller:corepll: device ID is greater than 24
sifive-clk-prci 10000000.clock-controller: Failed to register clkdev for corepll: -12 sifive-clk-prci 10000000.clock-controller: could not register clocks: -12
sifive-clk-prci 10000000.clock-controller: probe with driver sifive-clk-prci failed with error -12
...
platform 10060000.gpio: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10010000.serial: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10011000.serial: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10040000.spi: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10050000.spi: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10090000.ethernet: deferred probe pending: platform: supplier 10000000.clock-controller not ready

Quoted 6 lines

when trying to boot sifive_u in qemu.

Apparently, "10000000.clock-controller" is too long. Any suggestion on how to solve the problem ? I guess using dev_name(dev) as dev_id parameter for clk_hw_register_clkdev() is not or no longer a good idea. What else should be used instead ?

It was *never* a good idea. clkdev uses a fixed buffer size of 20 characters including the NUL character, and "10000000.clock-controller" would have been silently truncated to "10000000.clock-cont", and thus

Quoted 8 lines
                          if (!dev_id || strcmp(p->dev_id, dev_id))

would never have matched.

We need to think about (a) whether your use of clk_hw_register_clkdev() is still appropriate, and (b) whether we need to increase the size of the strings.

Quoted 5 lines

It isn't _my_ use, really. I only run a variety of boot tests with qemu. I expect we'll see reports from others trying to boot the mainline kernel on real sifive_u hardware or other hardware using the same driver or other drivers using dev_name() as dev_id parameter. Coccinelle finds the following callers:

Quoted 8 lines

Using dev_name() is not an issue. It's when dev_name() exceeds 19 characters that it becomes an issue (and always has been an issue due to the truncation.) clk_get(dev, ...) uses dev_name(dev) to match against its entry in the table.

As I say, dev_name() itself is not an issue. The length used for the name is.

Maybe, but the existence of best_dev_name() suggests that this has been seen before and that, as you mentioned, it is not a good idea. Anyway, the patch below fixes the problem for me. I don't know if it is acceptable / correct, so it might serve as guidance for others when fixing the problem for real.

Thanks,
Guenter

---

drivers/clk/sifive/sifive-prci.c +1 -1
index 25b8e1a80ddc..20cc8f42d9eb 100644
@@ -537,7 +537,7 @@ static int __prci_register_clocks(struct device *dev, struct __prci_data *pd,
                        return r;
                }
 
-               r = clk_hw_register_clkdev(&pic->hw, pic->name, dev_name(dev));
+               r = clk_hw_register_clkdev(&pic->hw, pic->name, "prci");
                if (r) {
                        dev_warn(dev, "Failed to register clkdev for %s: %d\n",
                                 init.name, r);
maintainer
#13 lore raw
Quoted 59 lines · Guenter Roeck
Quoted 58 lines · Russell King (Oracle)
Quoted 46 lines · Guenter Roeck
Quoted 36 lines · Russell King (Oracle)
Quoted 24 lines · Guenter Roeck

Hi,

Quoted 6 lines · Russell King (Oracle)

Report an error when an attempt to register a clkdev entry results in a truncated string so the problem can be easily spotted.

Reported by: Duanqiang Wen <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>

With this patch in the mainline kernel, I get

10000000.clock-controller:corepll: device ID is greater than 24
sifive-clk-prci 10000000.clock-controller: Failed to register clkdev for corepll: -12 sifive-clk-prci 10000000.clock-controller: could not register clocks: -12
sifive-clk-prci 10000000.clock-controller: probe with driver sifive-clk-prci failed with error -12
...
platform 10060000.gpio: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10010000.serial: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10011000.serial: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10040000.spi: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10050000.spi: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10090000.ethernet: deferred probe pending: platform: supplier 10000000.clock-controller not ready

Quoted 6 lines

when trying to boot sifive_u in qemu.

Apparently, "10000000.clock-controller" is too long. Any suggestion on how to solve the problem ? I guess using dev_name(dev) as dev_id parameter for clk_hw_register_clkdev() is not or no longer a good idea. What else should be used instead ?

It was *never* a good idea. clkdev uses a fixed buffer size of 20 characters including the NUL character, and "10000000.clock-controller" would have been silently truncated to "10000000.clock-cont", and thus

Quoted 8 lines
                          if (!dev_id || strcmp(p->dev_id, dev_id))

would never have matched.

We need to think about (a) whether your use of clk_hw_register_clkdev() is still appropriate, and (b) whether we need to increase the size of the strings.

Quoted 5 lines

It isn't _my_ use, really. I only run a variety of boot tests with qemu. I expect we'll see reports from others trying to boot the mainline kernel on real sifive_u hardware or other hardware using the same driver or other drivers using dev_name() as dev_id parameter. Coccinelle finds the following callers:

Using dev_name() is not an issue. It's when dev_name() exceeds 19 characters that it becomes an issue (and always has been an issue due to the truncation.) clk_get(dev, ...) uses dev_name(dev) to match against its entry in the table.

Quoted 8 lines
Quoted 3 lines

As I say, dev_name() itself is not an issue. The length used for the name is.

Maybe, but the existence of best_dev_name() suggests that this has been seen before and that, as you mentioned, it is not a good idea. Anyway, the patch below fixes the problem for me. I don't know if it is acceptable / correct, so it might serve as guidance for others when fixing the problem for real.

I get the impression that there's a communication problem here, so I'm not going to continue replying. Thanks.

Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
#14 lore raw

[CCing the regression list, as it should be in the loop for regressions:
https://docs.kernel.org/admin-guide/reporting-regressions.html]

Quoted 61 lines · Russell King (Oracle)
Quoted 60 lines · Guenter Roeck
Quoted 58 lines · Russell King (Oracle)
Quoted 43 lines · Guenter Roeck
Quoted 34 lines · Russell King (Oracle)
Quoted 22 lines · Guenter Roeck
Quoted 6 lines · Russell King (Oracle)

Report an error when an attempt to register a clkdev entry results in a truncated string so the problem can be easily spotted.

Reported by: Duanqiang Wen <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>

With this patch in the mainline kernel, I get

10000000.clock-controller:corepll: device ID is greater than 24
sifive-clk-prci 10000000.clock-controller: Failed to register clkdev for corepll: -12 sifive-clk-prci 10000000.clock-controller: could not register clocks: -12
sifive-clk-prci 10000000.clock-controller: probe with driver sifive-clk-prci failed with error -12
...
platform 10060000.gpio: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10010000.serial: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10011000.serial: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10040000.spi: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10050000.spi: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10090000.ethernet: deferred probe pending: platform: supplier 10000000.clock-controller not ready

Quoted 6 lines

when trying to boot sifive_u in qemu.

Apparently, "10000000.clock-controller" is too long. Any suggestion on how to solve the problem ? I guess using dev_name(dev) as dev_id parameter for clk_hw_register_clkdev() is not or no longer a good idea. What else should be used instead ?

It was *never* a good idea. clkdev uses a fixed buffer size of 20 characters including the NUL character, and "10000000.clock-controller" would have been silently truncated to "10000000.clock-cont", and thus

Quoted 7 lines
                          if (!dev_id || strcmp(p->dev_id, dev_id))

would never have matched.

We need to think about (a) whether your use of clk_hw_register_clkdev() is still appropriate, and (b) whether we need to increase the size of the strings.

Quoted 5 lines

It isn't _my_ use, really. I only run a variety of boot tests with qemu. I expect we'll see reports from others trying to boot the mainline kernel on real sifive_u hardware or other hardware using the same driver or other drivers using dev_name() as dev_id parameter. Coccinelle finds the following callers:

Using dev_name() is not an issue. It's when dev_name() exceeds 19 characters that it becomes an issue (and always has been an issue due to the truncation.) clk_get(dev, ...) uses dev_name(dev) to match against its entry in the table.

As I say, dev_name() itself is not an issue. The length used for the name is.

Quoted 7 lines
Quoted 4 lines

Maybe, but the existence of best_dev_name() suggests that this has been seen before and that, as you mentioned, it is not a good idea. Anyway, the patch below fixes the problem for me. I don't know if it is acceptable / correct, so it might serve as guidance for others when fixing the problem for real.

I get the impression that there's a communication problem here, so I'm not going to continue replying. Thanks.

Hmmm. Communication problem aside, this in the end seems to be a regression that is caused by a change of yours. Maybe not a major one that is making a fuzz about, but still one that would be good to get fixed. So who will take care of that? Alternatively:

Guenter, I assume we could simply fix this by reverting the culprit if Linus wanted to?

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) --
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot poke

maintainer
#15 lore raw
Quoted 4 lines · Linux regression tracking (Thorsten Leemhuis)

Hmmm. Communication problem aside, this in the end seems to be a regression that is caused by a change of yours. Maybe not a major one that is making a fuzz about, but still one that would be good to get fixed. So who will take care of that?

I have suggested several approaches to fixing it, and each time I'm being ignored by Guenter, who seems to have some other agenda - because he seems to believe that using dev_name() when registering the clk with clkdev is wrong... despite the fact that clkdev uses dev_name().

What I am uncertain about is:
1) whether clkdev is even necessary here, or whether it is pure noise. I think it's pure noise. Why? The dev_name() that is being used# to register the clk seems to be the _source_ device of the clock, whereas the name given should be the _consumer_ of the clock (when clk_get(dev, con_id) is called, dev is the _consumer_ device, and this is the device that dev_name() is used internally with.) Thus, if _that_ device is not the same as the struct device that is being passed to dev_name() when registering the clk, the entry in clkdev is utterly useless.

2) why someone would think that using best_dev_name() to work around this would be a good idea. One might as well pass the string "hahaha" when registering the clk - because if the device name is truncated, clk_get() is not going to find it. So, by registering it with clkdev, we're just eating memory for no reason.

Therefore, this change is finding bugs elsewhere. Should it cause a boot failure? No, and I'm happy to make clkdev just warn about it. However, reverting the change means we're not going to find these issues.

Why was the change originally proposed (by Duanqiang Wen) ? The reason was because of this truncation causing clk_get() to fail unexpectedly.

I am all for a _sensible_ discussion over this - not one that seems to have an agenda about "should dev_name() be used when registering a clk" that seems to be Guenter's approach because _that_ is not the root cause of the issue and I've already explained that _that_ is not the issue here. Yet, Guenter insists on that.

Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
maintainer
#16 lore raw
Quoted 34 lines · Russell King (Oracle)
Quoted 4 lines · Linux regression tracking (Thorsten Leemhuis)

Hmmm. Communication problem aside, this in the end seems to be a regression that is caused by a change of yours. Maybe not a major one that is making a fuzz about, but still one that would be good to get fixed. So who will take care of that?

I have suggested several approaches to fixing it, and each time I'm being ignored by Guenter, who seems to have some other agenda - because he seems to believe that using dev_name() when registering the clk with clkdev is wrong... despite the fact that clkdev uses dev_name().

What I am uncertain about is:
1) whether clkdev is even necessary here, or whether it is pure noise. I think it's pure noise. Why? The dev_name() that is being used# to register the clk seems to be the _source_ device of the clock, whereas the name given should be the _consumer_ of the clock (when clk_get(dev, con_id) is called, dev is the _consumer_ device, and this is the device that dev_name() is used internally with.) Thus, if _that_ device is not the same as the struct device that is being passed to dev_name() when registering the clk, the entry in clkdev is utterly useless.

2) why someone would think that using best_dev_name() to work around this would be a good idea. One might as well pass the string "hahaha" when registering the clk - because if the device name is truncated, clk_get() is not going to find it. So, by registering it with clkdev, we're just eating memory for no reason.

Therefore, this change is finding bugs elsewhere. Should it cause a boot failure? No, and I'm happy to make clkdev just warn about it. However, reverting the change means we're not going to find these issues.

Quoted 8 lines

Why was the change originally proposed (by Duanqiang Wen) ? The reason was because of this truncation causing clk_get() to fail unexpectedly.

I am all for a _sensible_ discussion over this - not one that seems to have an agenda about "should dev_name() be used when registering a clk" that seems to be Guenter's approach because _that_ is not the root cause of the issue and I've already explained that _that_ is not the issue here. Yet, Guenter insists on that.

... and I'll also add that I'm up to my eyeballs with an issue at Oracle, and thus have very very little time to deal with mainline kernel issues right now, so if people appear to be intentionally obtuse and difficult, I will end the discussion as I did here _purely_ because I _do_ _not_ _have_ _the_ _time_ to waste my time with them.

Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
#17 lore raw
In reply to Russell King (Oracle)
Quoted 6 lines · Russell King (Oracle)
Quoted 4 lines · Linux regression tracking (Thorsten Leemhuis)

Hmmm. Communication problem aside, this in the end seems to be a regression that is caused by a change of yours. Maybe not a major one that is making a fuzz about, but still one that would be good to get fixed. So who will take care of that?

Quoted 6 lines

I have suggested several approaches to fixing it, and each time I'm being ignored by Guenter, who seems to have some other agenda - because he seems to believe that using dev_name() when registering the clk with clkdev is wrong... despite the fact that clkdev uses dev_name().

I don't have an agenda. I did not intentionally ignore anyone. Misunderstand, maybe, but not ignore. I don't care how this is fixed. All I intended to do was to report it. My apologies to anyone who I managed to offend.

Quoted 22 lines

What I am uncertain about is:
1) whether clkdev is even necessary here, or whether it is pure noise. I think it's pure noise. Why? The dev_name() that is being used# to register the clk seems to be the _source_ device of the clock, whereas the name given should be the _consumer_ of the clock (when clk_get(dev, con_id) is called, dev is the _consumer_ device, and this is the device that dev_name() is used internally with.) Thus, if _that_ device is not the same as the struct device that is being passed to dev_name() when registering the clk, the entry in clkdev is utterly useless.

2) why someone would think that using best_dev_name() to work around this would be a good idea. One might as well pass the string "hahaha" when registering the clk - because if the device name is truncated, clk_get() is not going to find it. So, by registering it with clkdev, we're just eating memory for no reason.

Therefore, this change is finding bugs elsewhere. Should it cause a boot failure? No, and I'm happy to make clkdev just warn about it. However, reverting the change means we're not going to find these issues.

Quoted 9 lines

Why was the change originally proposed (by Duanqiang Wen) ? The reason was because of this truncation causing clk_get() to fail unexpectedly.

I am all for a _sensible_ discussion over this - not one that seems to have an agenda about "should dev_name() be used when registering a clk" that seems to be Guenter's approach because _that_ is not the root cause of the issue and I've already explained that _that_ is not the issue here. Yet, Guenter insists on that.

I don't know if using dev_name() is wrong or not. I don't know much if anything about the clock subsystem. Someone else said that using dev_name() would be a "bad idea", or at least that was my understanding. That is all. Again, my apologies for any misunderstanding and for getting involved beyond the initial report.

Either case, I really don't want to be involved further, and _please_ don't make any assumptions or decisions based on anything I may have said out of ignorance.

Thanks,
Guenter

#18 lore raw
In reply to Guenter Roeck
Quoted 24 lines · Guenter Roeck

Hi,

Quoted 6 lines · Russell King (Oracle)

Report an error when an attempt to register a clkdev entry results in a truncated string so the problem can be easily spotted.

Reported by: Duanqiang Wen <[email protected]>

Signed-off-by Russell King (Oracle) <[email protected]>
Reviewed-by Stephen Boyd <[email protected]>

With this patch in the mainline kernel, I get

10000000.clock-controller:corepll: device ID is greater than 24
sifive-clk-prci 10000000.clock-controller: Failed to register clkdev for corepll: -12 sifive-clk-prci 10000000.clock-controller: could not register clocks: -12
sifive-clk-prci 10000000.clock-controller: probe with driver sifive-clk-prci failed with error -12
...
platform 10060000.gpio: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10010000.serial: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10011000.serial: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10040000.spi: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10050000.spi: deferred probe pending: platform: supplier 10000000.clock-controller not ready platform 10090000.ethernet: deferred probe pending: platform: supplier 10000000.clock-controller not ready

Quoted 7 lines

when trying to boot sifive_u in qemu.

Apparently, "10000000.clock-controller" is too long. Any suggestion on how to solve the problem ? I guess using dev_name(dev) as dev_id parameter for clk_hw_register_clkdev() is not or no longer a good idea. What else should be used instead ?

Copying regzbot.

#regzbot ^introduced 8d532528ff6a
#regzbot title sifive_u boot failure
#regzbot ignore-activity