drivers/pwm/pwm-mediatek.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
The following commit has been merged into the objtool/urgent branch of tip:
Commit-ID: 2becbc66a151500636046503f541255af6acf4fa
Gitweb: https://git.kernel.org/tip/2becbc66a151500636046503f541255af6acf4fa
Author: Josh Poimboeuf <jpoimboe@kernel.org>
AuthorDate: Mon, 24 Mar 2025 14:56:11 -07:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 25 Mar 2025 09:20:32 +01:00
objtool, pwm: mediatek: Prevent theoretical divide-by-zero in pwm_mediatek_config()
With CONFIG_COMPILE_TEST && !CONFIG_CLK, pwm_mediatek_config() has a
divide-by-zero in the following line:
do_div(resolution, clk_get_rate(pc->clk_pwms[pwm->hwpwm]));
due to the fact that the !CONFIG_CLK version of clk_get_rate() returns
zero.
This is presumably just a theoretical problem: COMPILE_TEST overrides
the dependency on RALINK which would select COMMON_CLK. Regardless it's
a good idea to check for the error explicitly to avoid divide-by-zero.
Fixes the following warning:
drivers/pwm/pwm-mediatek.o: warning: objtool: .text: unexpected end of section
Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: "Uwe Kleine-König" <ukleinek@kernel.org> (maintainer:PWM SUBSYSTEM)
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/r/fb56444939325cc173e752ba199abd7aeae3bf12.1742852847.git.jpoimboe@kernel.org
---
drivers/pwm/pwm-mediatek.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 01dfa0f..7eaab58 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -121,21 +121,25 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
u32 clkdiv = 0, cnt_period, cnt_duty, reg_width = PWMDWIDTH,
reg_thres = PWMTHRES;
+ unsigned long clk_rate;
u64 resolution;
int ret;
ret = pwm_mediatek_clk_enable(chip, pwm);
-
if (ret < 0)
return ret;
+ clk_rate = clk_get_rate(pc->clk_pwms[pwm->hwpwm]);
+ if (!clk_rate)
+ return -EINVAL;
+
/* Make sure we use the bus clock and not the 26MHz clock */
if (pc->soc->has_ck_26m_sel)
writel(0, pc->regs + PWM_CK_26M_SEL);
/* Using resolution in picosecond gets accuracy higher */
resolution = (u64)NSEC_PER_SEC * 1000;
- do_div(resolution, clk_get_rate(pc->clk_pwms[pwm->hwpwm]));
+ do_div(resolution, clk_rate);
cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000, resolution);
while (cnt_period > 8191) {[I forgot a comma in the original post and so the list of Cc: was
broken. Here comes a resend, sorry to everyone who get the mail twice
now. If you reply, please do so on this copy to not continue using the
broken address.]
Hello,
On Tue, Mar 25, 2025 at 08:34:51AM +0000, tip-bot2 for Josh Poimboeuf wrote:
> The following commit has been merged into the objtool/urgent branch of tip:
>
> Commit-ID: 2becbc66a151500636046503f541255af6acf4fa
> Gitweb: https://git.kernel.org/tip/2becbc66a151500636046503f541255af6acf4fa
> Author: Josh Poimboeuf <jpoimboe@kernel.org>
> AuthorDate: Mon, 24 Mar 2025 14:56:11 -07:00
> Committer: Ingo Molnar <mingo@kernel.org>
> CommitterDate: Tue, 25 Mar 2025 09:20:32 +01:00
I wonder a bit about procedures here. While I like that warnings that
pop up in drivers/pwm (and elsewhere) are cared for, I think that the
sensible way to change warning related settings is to make it hard to
enable them first (harder than "depends on !COMPILE_TEST" "To avoid
breaking bots too badly") and then work on the identified problems
before warning broadly. The way chosen here instead seems to be enabling
the warning immediately and then post fixes to the warnings and merge
them without respective maintainer feedback in less than 12 hours.
> objtool, pwm: mediatek: Prevent theoretical divide-by-zero in pwm_mediatek_config()
>
> With CONFIG_COMPILE_TEST && !CONFIG_CLK, pwm_mediatek_config() has a
> divide-by-zero in the following line:
>
> do_div(resolution, clk_get_rate(pc->clk_pwms[pwm->hwpwm]));
>
> due to the fact that the !CONFIG_CLK version of clk_get_rate() returns
> zero.
>
> This is presumably just a theoretical problem: COMPILE_TEST overrides
> the dependency on RALINK which would select COMMON_CLK. Regardless it's
RALINK and ARCH_MEDIATEK. The latter enables COMMON_CLK as it depends on
ARM or ARM64 which both select COMMON_CLK.
> a good idea to check for the error explicitly to avoid divide-by-zero.
>
> Fixes the following warning:
>
> drivers/pwm/pwm-mediatek.o: warning: objtool: .text: unexpected end of section
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
Fixes: caf065f8fd58 ("pwm: Add MediaTek PWM support")
would be nice.
> Cc: "Uwe Kleine-König" <ukleinek@kernel.org> (maintainer:PWM SUBSYSTEM)
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Link: https://lore.kernel.org/r/fb56444939325cc173e752ba199abd7aeae3bf12.1742852847.git.jpoimboe@kernel.org
I fail to reproduce the warning here for an x86_64 build on
1e26c5e28ca5. I have:
$ grep -E 'CONFIG_(CLK|PWM_MEDIATEK|OBJTOOL_WERROR)\>' .config
CONFIG_PWM_MEDIATEK=m
CONFIG_OBJTOOL_WERROR=y
and the build works fine for me and there is no warning about
drivers/pwm/pwm-mediatek.o. What am I missing?
Best regards
Uwe
On Wed, Mar 26, 2025 at 11:35:28AM +0100, Uwe Kleine-König wrote: > I wonder a bit about procedures here. While I like that warnings that > pop up in drivers/pwm (and elsewhere) are cared for, I think that the > sensible way to change warning related settings is to make it hard to > enable them first (harder than "depends on !COMPILE_TEST" "To avoid > breaking bots too badly") and then work on the identified problems > before warning broadly. The way chosen here instead seems to be enabling > the warning immediately and then post fixes to the warnings and merge > them without respective maintainer feedback in less than 12 hours. Actually, this type of warning has existed for years. Nothing in the recent objtool patches enabled it. I only discovered this particular one a few days ago. I suspect it only exists with newer compilers. > I fail to reproduce the warning here for an x86_64 build on > 1e26c5e28ca5. I have: > > $ grep -E 'CONFIG_(CLK|PWM_MEDIATEK|OBJTOOL_WERROR)\>' .config > CONFIG_PWM_MEDIATEK=m > CONFIG_OBJTOOL_WERROR=y > > and the build works fine for me and there is no warning about > drivers/pwm/pwm-mediatek.o. What am I missing? Sorry, I should have given more details about that. It was likely something with KCOV and/or UBSAN, though I can't seem to recreate it at the moment either :-/ -- Josh
Hello Josh, On Wed, Mar 26, 2025 at 10:44:04PM -0700, Josh Poimboeuf wrote: > On Wed, Mar 26, 2025 at 11:35:28AM +0100, Uwe Kleine-König wrote: > > I wonder a bit about procedures here. While I like that warnings that > > pop up in drivers/pwm (and elsewhere) are cared for, I think that the > > sensible way to change warning related settings is to make it hard to > > enable them first (harder than "depends on !COMPILE_TEST" "To avoid > > breaking bots too badly") and then work on the identified problems > > before warning broadly. The way chosen here instead seems to be enabling > > the warning immediately and then post fixes to the warnings and merge > > them without respective maintainer feedback in less than 12 hours. > > Actually, this type of warning has existed for years. Nothing in the > recent objtool patches enabled it. Yes, I understood that the recent patch only made the warning fatal. > I only discovered this particular one a few days ago. I suspect it only > exists with newer compilers. > > > I fail to reproduce the warning here for an x86_64 build on > > 1e26c5e28ca5. I have: > > > > $ grep -E 'CONFIG_(CLK|PWM_MEDIATEK|OBJTOOL_WERROR)\>' .config > > CONFIG_PWM_MEDIATEK=m > > CONFIG_OBJTOOL_WERROR=y > > > > and the build works fine for me and there is no warning about > > drivers/pwm/pwm-mediatek.o. What am I missing? > > Sorry, I should have given more details about that. It was likely > something with KCOV and/or UBSAN, though I can't seem to recreate it at > the moment either :-/ The combination "existed for years" + "discovered a few days ago" + needs particular combination of .config and toolchain makes me believe the right way to fix is a medium priority patch via the maintainer of the affected driver. I don't see the urgency to justify the current procedure. I don't know what the current merge plan is, but if you want to merge it through the pwm tree, I'm willing to take it. I wouldn't create a pull request just for that before 6.15, but send it along if something more urgent pops up. Best regards Uwe
On Wed, Mar 26, 2025 at 11:35:28AM +0100, Uwe Kleine-König wrote: > and the build works fine for me and there is no warning about > drivers/pwm/pwm-mediatek.o. What am I missing? Could be compiler related; IIRC it is mostly clang that does this. When it finds /0 it simply stops code-gen. I *really* dislike this behaviour, but since C declares this UB, they're basically free to do whatever. IMO it should just emit the code; kernel has exceptions to deal with this and userspace gets signals.
© 2016 - 2025 Red Hat, Inc.