Messages in this thread Patch in this message | | | Date | Fri, 17 May 2024 20:24:19 -0700 | Subject | Re: [PATCH] clkdev: report over-sized strings when creating clkdev entries | From | Guenter Roeck <> |
| |
On 5/17/24 16:37, Russell King (Oracle) wrote: > On Fri, May 17, 2024 at 04:34:06PM -0700, Guenter Roeck wrote: >> On 5/17/24 15:22, Russell King (Oracle) wrote: >>> On Fri, May 17, 2024 at 03:09:12PM -0700, Guenter Roeck wrote: >>>> Hi, >>>> >>>> On Fri, Mar 15, 2024 at 11:47:55AM +0000, Russell King (Oracle) wrote: >>>>> 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 <duanqiangwen@net-swift.com> >>>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> >>>>> Reviewed-by: Stephen Boyd <sboyd@kernel.org> >>>> >>>> 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 ? >>> >>> 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. >>> >> >> 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. >
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
--- diff --git a/drivers/clk/sifive/sifive-prci.c b/drivers/clk/sifive/sifive-prci.c index 25b8e1a80ddc..20cc8f42d9eb 100644 --- a/drivers/clk/sifive/sifive-prci.c +++ b/drivers/clk/sifive/sifive-prci.c @@ -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);
| |