| Message ID | 20231222163710.215362-1-romain.naour@smile.fr |
|---|---|
| State | New |
| Headers | show |
| Series | bus: ti-sysc: Fix error handling for sysc_check_active_timer() again | expand |
Hi, On Fri, 22 Dec 2023 17:37:10 +0100 Romain Naour <romain.naour@smile.fr> wrote: > From: Romain Naour <romain.naour@skf.com> > > sysc_check_active_timer() has been introduced by 6cfcd5563b4f > ("clocksource/drivers/timer-ti-dm: Fix suspend and resume for am3 and am4") > and initially returned -EBUSY to ignore timers tagged with no-reset > and no-idle. > > But the return code has been updated from -EBUSY to -ENXIO by > 65fb73676112 ("bus: ti-sysc: suppress err msg for timers used as clockevent/source") > and introduced a regression fixed by 06a089ef6449 > ("bus: ti-sysc: Fix error handling for sysc_check_active_timer()") > since sysc_probe() was still checking for -EBUSY. > > Finally the sysc_check_active_timer() return code was reverted > back to -EBUSY by a12315d6d270 > ("bus: ti-sysc: Make omap3 gpt12 quirk handling SoC specific") except > for SOC_3430. > > Now sysc_check_active_timer() may return ENXIO for SOC_3430 and > EBUSY for all other SoC. > > But sysc_probe() still check for -ENXIO leading to the following > errors in dmesg on AM57xx: > > ti-sysc: probe of 4ae18000.target-module failed with error -16 (timer1_target) > ti-sysc: probe of 4882c000.target-module failed with error -16 (timer15_target) > ti-sysc: probe of 4882e000.target-module failed with error -16 (timer6_target) > > Fix this by checking for both error code... > Well, fix what? As long as -EBUSY comes form sysc_check_active_timer(), there is no problem besides the noise. So clearly state what you want to fix, so is it only the noise. Of course I would also like the noise to be gone. I also stumbled across this. Bringing this to discussion is of course good. Changing it to -ENXIO has side effects as more lines are executed and the device is touched although it might be already in use by dmtimer_systimer. As far as I understand things: there are broken timers, timers used by clocksource and timers generally useable. And we return -ENXIO for the broken ones... The main issue here is that this needs more documentation/comments. I might of course be wrong... Regards, Andreas
Hello Andreas, Le 23/12/2023 à 23:41, Andreas Kemnade a écrit : > Hi, > > On Fri, 22 Dec 2023 17:37:10 +0100 > Romain Naour <romain.naour@smile.fr> wrote: > >> From: Romain Naour <romain.naour@skf.com> >> >> sysc_check_active_timer() has been introduced by 6cfcd5563b4f >> ("clocksource/drivers/timer-ti-dm: Fix suspend and resume for am3 and am4") >> and initially returned -EBUSY to ignore timers tagged with no-reset >> and no-idle. >> >> But the return code has been updated from -EBUSY to -ENXIO by >> 65fb73676112 ("bus: ti-sysc: suppress err msg for timers used as clockevent/source") >> and introduced a regression fixed by 06a089ef6449 >> ("bus: ti-sysc: Fix error handling for sysc_check_active_timer()") >> since sysc_probe() was still checking for -EBUSY. >> >> Finally the sysc_check_active_timer() return code was reverted >> back to -EBUSY by a12315d6d270 >> ("bus: ti-sysc: Make omap3 gpt12 quirk handling SoC specific") except >> for SOC_3430. >> >> Now sysc_check_active_timer() may return ENXIO for SOC_3430 and >> EBUSY for all other SoC. >> >> But sysc_probe() still check for -ENXIO leading to the following >> errors in dmesg on AM57xx: >> >> ti-sysc: probe of 4ae18000.target-module failed with error -16 (timer1_target) >> ti-sysc: probe of 4882c000.target-module failed with error -16 (timer15_target) >> ti-sysc: probe of 4882e000.target-module failed with error -16 (timer6_target) >> >> Fix this by checking for both error code... >> > Well, fix what? As long as -EBUSY comes form sysc_check_active_timer(), there > is no problem besides the noise. So clearly state what you want to fix, > so is it only the noise. > Of course I would also like the noise to be gone. I also stumbled across this. > Bringing this to discussion is of course good. I'm working on a custom board and such "noise" makes difficult to know if the issue come from the board hardware, the SoC, the devicetree, the TI driver or the kernel itself. > > Changing it to -ENXIO has side effects as more lines are executed and the > device is touched although it might be already in use by dmtimer_systimer. The previous fix [1] doesn't say anything about the -EBUSY case and seems to forgot updating the error handling for sysc_check_active_timer() (similar to [2]) > > As far as I understand things: there are broken timers, timers used by clocksource > and timers generally useable. And we return -ENXIO for the broken ones... The > main issue here is that this needs more documentation/comments. > I might of course be wrong... I get it now, the last commit [1] actually revert the commit [3] (removing the error message) by adding back -EBUSY as intended initially [4] for all TI SoC except for SOC_3430. sysc_check_active_timer() returning -EBUSY affect 3 timers (timer1 (clockevent), timer15, timer16) used on AM57xx [5]. ti-sysc: probe of 4ae18000.target-module failed with error -16 (timer1_target) ti-sysc: probe of 4882c000.target-module failed with error -16 (timer15_target) ti-sysc: probe of 4882e000.target-module failed with error -16 (timer6_target) I'm agree, this issue deserve more documentation/comments and it would be better to avoid such noise. [1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=a12315d6d27093392b6c634e1d35a59f1d1f7a59 [2] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=e0b603c89a931790dc54b9d6b14b3ec45a82a888 [3] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=65fb73676112f6fd107c5e542b2cbcfb206fe881 [4] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=6cfcd5563b4fadbf49ba8fa481978e5e86d30322 [5] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm/boot/dts/dra7.dtsi?h=v6.1.73#n1331 Best regards, Romain > > Regards, > Andreas
diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c index d57bc066dce6..0c6d5e3d5dc7 100644 --- a/drivers/bus/ti-sysc.c +++ b/drivers/bus/ti-sysc.c @@ -3314,7 +3314,7 @@ static int sysc_probe(struct platform_device *pdev) return error; error = sysc_check_active_timer(ddata); - if (error == -ENXIO) + if ((error == -ENXIO) || (error == -EBUSY)) ddata->reserved = true; else if (error) return error;