On Fri, May 17, 2024 at 03:09:14PM -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 <[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 ?
This issue causes a complete boot failure on real hardware (SiFive
Unmatched). The boot only gets as far as "Starting kernel ..." with no
other indication of what's going on.
Guenter's suggested patch solves the issue.
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);
On 27.05.24 12:45, Ron Economos wrote:
> On Fri, May 17, 2024 at 03:09:14PM -0700, Guenter Roeck wrote:
>>
>> 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 <[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
>> [...]
>> 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 ?
>
> This issue causes a complete boot failure on real hardware (SiFive
> Unmatched).
Hmmm. That and because nobody afaics has time/motivation to fix this
anytime soon (or am I mistaken there?) makes me wonder if we should
revert this change for now (and remerge it later once the problem this
change exposed was fixed). Or is another solution in sight somewhere?
Ciao, Thorsten
> The boot only gets as far as "Starting kernel ..." with no
> other indication of what's going on.
>
> Guenter's suggested patch solves the issue.
>
> 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);
>
>
>
#regzbot dup:
https://lore.kernel.org/lkml/[email protected]/
On Mon, May 27, 2024 at 01:16:06PM +0200, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 27.05.24 12:45, Ron Economos wrote:
> > On Fri, May 17, 2024 at 03:09:14PM -0700, Guenter Roeck wrote:
> >>
> >> 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 <[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
> >> [...]
> >> 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 ?
> >
> > This issue causes a complete boot failure on real hardware (SiFive
> > Unmatched).
>
> Hmmm. That and because nobody afaics has time/motivation to fix this
> anytime soon (or am I mistaken there?) makes me wonder if we should
> revert this change for now (and remerge it later once the problem this
> change exposed was fixed). Or is another solution in sight somewhere?
I'm sorry, but clearly I should tell my employer that I can't do work
for them because there's been a mainline kernel regression, and of
course I should be working on this bank holiday Monday...
No, please wait a bit longer.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Mon, May 27, 2024 at 03:45:15AM -0700, Ron Economos wrote:
> On Fri, May 17, 2024 at 03:09:14PM -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 <[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 ?
>
> This issue causes a complete boot failure on real hardware (SiFive
> Unmatched). The boot only gets as far as "Starting kernel ..." with no other
> indication of what's going on.
>
> Guenter's suggested patch solves the issue.
>
> 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");
How about just changing this to:
r = clk_hw_register(dev, &pic->hw);
?
Since, if the device name is over-sized and thus truncated in the clk
lookup array that clkdev maintains, *nothing* will be able to match
the entry. Hence, I suspect all those clkdev registrations are
completely redundant for this driver (and do nothing other than
waste memory!)
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Mon, May 27, 2024 at 02:18:23PM +0100, Russell King (Oracle) wrote:
> On Mon, May 27, 2024 at 03:45:15AM -0700, Ron Economos wrote:
> > On Fri, May 17, 2024 at 03:09:14PM -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 <[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 ?
> >
> > This issue causes a complete boot failure on real hardware (SiFive
> > Unmatched). The boot only gets as far as "Starting kernel ..." with no other
> > indication of what's going on.
> >
> > Guenter's suggested patch solves the issue.
> >
> > 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");
>
> How about just changing this to:
>
> r = clk_hw_register(dev, &pic->hw);
>
> ?
>
> Since, if the device name is over-sized and thus truncated in the clk
> lookup array that clkdev maintains, *nothing* will be able to match
> the entry. Hence, I suspect all those clkdev registrations are
> completely redundant for this driver (and do nothing other than
> waste memory!)
Note that I mentioned *exactly* this point in my first reply to the
report of the regression in:
https://lore.kernel.org/r/[email protected]
"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."
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Don't fail clkdev_alloc() if the strings are over-sized. In this case,
the entry will not match during lookup, so its useless. However, since
code fails if we return NULL leading to boot failure, return a dummy
entry with the connection and device IDs set to "bad".
Leave the warning so these problems can be found, and the useless
wasteful clkdev registrations removed.
Fixes: 8d532528ff6a ("clkdev: report over-sized strings when creating clkdev entries")
Closes: https://lore.kernel.org/linux-clk/[email protected].
Signed-off-by: Russell King (Oracle) <[email protected]>
---
Please try this patch, which should allow the platform to boot, bit will
intentionally issue lots of warnings. There is a separate patch posted
recently that removes the useless registration with clkdev.
drivers/clk/clkdev.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 6a77d7e201a9..2f83fb97c6fb 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -204,8 +204,15 @@ vclkdev_alloc(struct clk_hw *hw, const char *con_id, const char *dev_fmt,
pr_err("%pV:%s: %s ID is greater than %zu\n",
&vaf, con_id, failure, max_size);
va_end(ap_copy);
- kfree(cla);
- return NULL;
+
+ /*
+ * Don't fail in this case, but as the entry won't ever match just
+ * fill it with something that also won't match.
+ */
+ strscpy(cla->con_id, "bad", sizeof(cla->con_id));
+ strscpy(cla->dev_id, "bad", sizeof(cla->dev_id));
+
+ return &cla->cl;
}
static struct clk_lookup *
--
2.30.2
On 5/28/24 1:15 AM, Russell King (Oracle) wrote:
> Don't fail clkdev_alloc() if the strings are over-sized. In this case,
> the entry will not match during lookup, so its useless. However, since
> code fails if we return NULL leading to boot failure, return a dummy
> entry with the connection and device IDs set to "bad".
>
> Leave the warning so these problems can be found, and the useless
> wasteful clkdev registrations removed.
>
> Fixes: 8d532528ff6a ("clkdev: report over-sized strings when creating clkdev entries")
> Closes: https://lore.kernel.org/linux-clk/[email protected].
> Signed-off-by: Russell King (Oracle) <[email protected]>
> ---
>
> Please try this patch, which should allow the platform to boot, bit will
> intentionally issue lots of warnings. There is a separate patch posted
> recently that removes the useless registration with clkdev.
>
> drivers/clk/clkdev.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 6a77d7e201a9..2f83fb97c6fb 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -204,8 +204,15 @@ vclkdev_alloc(struct clk_hw *hw, const char *con_id, const char *dev_fmt,
> pr_err("%pV:%s: %s ID is greater than %zu\n",
> &vaf, con_id, failure, max_size);
> va_end(ap_copy);
> - kfree(cla);
> - return NULL;
> +
> + /*
> + * Don't fail in this case, but as the entry won't ever match just
> + * fill it with something that also won't match.
> + */
> + strscpy(cla->con_id, "bad", sizeof(cla->con_id));
> + strscpy(cla->dev_id, "bad", sizeof(cla->dev_id));
> +
> + return &cla->cl;
> }
>
> static struct clk_lookup *
Works good. Here's what it looks like on HiFive Unmatched.
[0.389138] riscv-plic c000000.interrupt-controller: mapped 69 interrupts
with 4 handlers for 9 contexts.
[0.390710] shpchp: Standard Hot Plug PCI Controller Driver version: 0.4
[0.392743] 10000000.clock-controller:corepll: device ID is greater than 24
[0.392792] 10000000.clock-controller:ddrpll: device ID is greater than 24
[0.392820] 10000000.clock-controller:gemgxlpll: device ID is greater
than 24
[0.392847] 10000000.clock-controller:dvfscorepll: device ID is greater
than 24
[0.392876] 10000000.clock-controller:hfpclkpll: device ID is greater
than 24
[0.392903] 10000000.clock-controller:cltxpll: device ID is greater than 24
[0.392929] 10000000.clock-controller:tlclk: device ID is greater than 24
[0.392955] 10000000.clock-controller:pclk: device ID is greater than 24
[0.392981] 10000000.clock-controller:pcie_aux: device ID is greater than 24
[0.394620] Serial: 8250/16550 driver, 32 ports, IRQ sharing enabled
[0.413222] SuperH (H)SCI(F) driver initialized
Tested-by: Ron Economos <[email protected]>