| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <828da89cff6dd2c49df9af6131aa3b43675abc87.camel@siemens.com>
Date: Mon, 16 Dec 2024 08:57:05 +0000
From: "Sverdlin, Alexander" <alexander.sverdlin@...mens.com>
To: "mazziesaccount@...il.com" <mazziesaccount@...il.com>, "brgl@...ev.pl"
<brgl@...ev.pl>
CC: "bartosz.golaszewski@...aro.org" <bartosz.golaszewski@...aro.org>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>
Subject: Re: [PATCH 2/2] gpio: omap: save two lines by using
devm_clk_get_prepared()
Hi Matti!
On Fri, 2024-12-13 at 15:55 +0200, Matti Vaittinen wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> > > >
> > > > We can drop the else branch if we get the clock already prepared using
> > > > the relevant helper.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> > > > ---
> > >
> > > Booting a beaglebone black with the linux-next from Today fails
> > > (next-20241213). Enabling earlycon + debug yields below splat to be
> > > printed to the console:
> > >
> > > [ 2.628019] ------------[ cut here ]------------
> > > [ 2.632793] WARNING: CPU: 0 PID: 34 at drivers/clk/clk.c:1254
> > > clk_core_enable+0xb4/0x1b0
> > > [ 2.641156] Enabling unprepared l4-wkup-clkctrl:0008:18
> > > [ 2.646530] Modules linked in:
> > > [ 2.649688] CPU: 0 UID: 0 PID: 34 Comm: kworker/u4:3 Not tainted
> > > 6.13.0-rc2-next-20241213-00002-gf2d4b29c8330 #15
> > > [ 2.660256] Hardware name: Generic AM33XX (Flattened Device Tree)
> > > [ 2.666531] Workqueue: events_unbound deferred_probe_work_func
> > > [ 2.672553] Call trace:
> > > [ 2.672570] unwind_backtrace from show_stack+0x10/0x14
> > > [ 2.680578] show_stack from dump_stack_lvl+0x50/0x64
> > > [ 7 2.685788] dump_stack_lvl from __warn+0xc0/0x130
> > > [ 2.690734] __warn from warn_slowpath_fmt+0x80/0x1a0
> > > [ 2.695944] warn_slowpath_fmt from clk_core_enable+0xb4/0x1b0
> > > [ 2.701963] clk_core_enable from clk_core_enable_lock+0x18/0x2c
> > > [ 2.708159] clk_core_enable_lock from
> > > sysc_enable_opt_clocks.part.9+0x28/0x84
> > > [ 2.715611] sysc_enable_opt_clocks.part.9 from
> > > sysc_enable_module+0x254/0x2dc
> > > [ 2.723052] sysc_enable_module from sysc_runtime_resume+0x17c/0x1c0
> > > [ 2.729599] sysc_runtime_resume from __rpm_callback+0x4c/0x130
> > > [ 2.735709] __rpm_callback from rpm_callback+0x50/0x54
> > > [ 2.741096] rpm_callback from rpm_resume+0x614/0x660
> > > [ 2.746304] rpm_resume from __pm_runtime_resume+0x4c/0x64
> > > [ 2.751960] __pm_runtime_resume from __device_attach+0xd0/0x188
> > > [ 2.758155] __device_attach from bus_probe_device+0x88/0x8c
> > > or_thread from kthread+0x188/0x24c
> > > [ 2.789476] kthread from ret_from_fork+0x14/0x20
> > > [ 2.794327] Exception stack(0xe0091fb0 to 0xe0091ff8)
> > > [ 2.799528] 1fa0: 00000000
> > > 00000000 00000000 00000000
> > > [ 2.807947] 1fc0: 00000000 00000000 00000000 00000000 00000000
> > > 00000000 00000000 00000000
> > > [ 2.816365] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> > > [ 2.823173] ---[ end trace 0000000000000000 ]---
> > > [ 2.828070] ti-sysc 44e07000.target-module: Optional clocks failed
> > > for enable: -108
> > > [ 2.835998] ------------[ cut here ]------------
> > >
> > > reverting
> > > b7bbaff8c1bc ("gpio: omap: save two lines by using devm_clk_get_prepared()")
> > >
> > > fixes the boot for me.
> > >
> > >
> > > > drivers/gpio/gpio-omap.c | 4 +---
> > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> > > > index 54c4bfdccf568..57d299d5d0b16 100644
> > > > --- a/drivers/gpio/gpio-omap.c
> > > > +++ b/drivers/gpio/gpio-omap.c
> > > > @@ -1449,13 +1449,11 @@ static int omap_gpio_probe(struct platform_device *pdev)
> > > > }
> > > >
> > > > if (bank->dbck_flag) {
> > > > - bank->dbck = devm_clk_get(dev, "dbclk");
> > > > + bank->dbck = devm_clk_get_prepared(dev, "dbclk");
> > > > if (IS_ERR(bank->dbck)) {
> > > > dev_err(dev,
> > > > "Could not get gpio dbck. Disable debounce\n");
> > > > bank->dbck_flag = false;
> > > > - } else {
> > > > - clk_prepare(bank->dbck);
> > > > }
> > > > }
> > > >
> > >
> > > I can only spot a minor functional change. The code prior this commit
> > > does not check the result of clk_prepare(), and does neither set
> > > bank->dbck_flag = false; nor call clk_put();
> > >
> > > Other than that, timing is likely to be changed. Not sure what is the
> > > thing here.
> >
> > The new code looks more correct, with the return code check from clk_prepare().
>
> I agree. Unfortunately something breaks though.
>
> > Could it be that two problems eliminated themselves in your case before? ;-)
>
> Ha. Would it be the first time? ;) Well, unfortunately I really don't
> know the omap/am335x stuff too well. Maybe Tony (CC) or Grygori know
> better - I think they have helped me with BBB related stuff before :)
>
> > Would it be possible for you to provide the logs with "initcall_debug" with
> > and without the patch in question?
>
> Logs attached :) minicom_reverted.cap has is the next-20241213 having
> this commit reverted. minicom_not_reverted.cap is just the next-20241213.
>
> Unfortunately it seems there are some errors (flipped bits?) in the
> serial log. Not sure what causes them - I have new FTDI chip, perhaps it
> causes these problems. (Comparison of the logs shows single, seemingly
> random, letters being changed.)
No problem! Thanks for the logs! I think I know what happened: I suppose
it's "prepared" counter underflow on probe deferral of GPIO driver
(there are "probe of 44e07000.gpio returned 517" visible).
If you'd still have a chance to test 6.13.0-rc2-next-20241213,
I believe this was missing in the
"gpio: omap: save two lines by using devm_clk_get_prepared()":
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 76d5d87e9681..0c30013d2b48 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1473,8 +1473,6 @@ static int omap_gpio_probe(struct platform_device *pdev)
if (ret) {
pm_runtime_put_sync(dev);
pm_runtime_disable(dev);
- if (bank->dbck_flag)
- clk_unprepare(bank->dbck);
return ret;
}
@@ -1495,8 +1493,6 @@ static void omap_gpio_remove(struct platform_device *pdev)
cpu_pm_unregister_notifier(&bank->nb);
gpiochip_remove(&bank->chip);
pm_runtime_disable(&pdev->dev);
- if (bank->dbck_flag)
- clk_unprepare(bank->dbck);
}
static int __maybe_unused omap_gpio_runtime_suspend(struct device *dev)
--
Alexander Sverdlin
Siemens AG
www.siemens.com
Powered by blists - more mailing lists