Linux-MIPS Archive mirror
 help / color / mirror / Atom feed

From: Daniel Lezcano <daniel.lezcano@oss.qualcomm.com>
To: Philipp Matthias Hahn <phahn-oss@avm.de>,
	Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
	Daniel Lezcano <daniel.lezcano@kernel.org>,
	Thomas Gleixner <tglx@kernel.org>,
	linux-mips@vger.kernel.org
Subject: Re: drivers/clocksource/mips-gic-timer.c:283 looks suspicious: ret != clk
Date: Wed, 4 Mar 2026 19:58:54 +0100	[thread overview]
Message-ID: <b84dc28a-34ca-4243-a0ae-3b61db04fcd5@oss.qualcomm.com> (raw)
In-Reply-To: <aahwZ3iJVbo55int@mail-auth.avm.de>


Hi Philipp,

thanks for reporting a potential issue

Actually, the code seems correct.

AFAICT, there are two configurations with the clock. One is a dynamic 
clock connected with the clock framework which can change the frequency 
and the other is a static clock described in the device tree.

If the clk is managed by the clock framework the code subscribes to the 
clock notifier to get the information of the clock frequency change. So 
it is the condition (!IS_ERR(clk))

On 3/4/26 18:48, Philipp Matthias Hahn wrote:
> Hello,
> 
> while looking where IS_ERR(…) plus a manual NULL check can be converted
> to IS_ERR_OR_NULL(…) I stumbled by accident over
> gic_clocksource_of_init() line 283, where `IS_ERR(clk)` is used combined
> with `!ret`:
> 
> static int __init gic_clocksource_of_init(struct device_node *node)
> {
> 	struct clk *clk;
> 	int ret;
> 	…
> 	clk = of_clk_get(node, 0);
> 	if (!IS_ERR(clk)) {
> 		ret = clk_prepare_enable(clk);

Clock is managed by the clock framework -> (IS_ERR(clk) == false)

> 		if (ret < 0) {
> 			pr_err("Failed to enable clock\n");
> 			clk_put(clk);
> 			return ret;
> 		}
> 		gic_frequency = clk_get_rate(clk);
> 	} else {
> 		…

Clock is statically described in the DT -> (IS_ERR(clk) == true)

> 	}
> 	ret = gic_clockevent_init();
> 	if (!ret && !IS_ERR(clk)) {
> 	     ^^^            ^^^

Adding the missing line:

	if (clk_notifier_register(clk, &gic_clk_nb) < 0)

So the condition is 'the clockevent successfully registered' and 'the 
clock is managed by the clock framework, IOW frequency can change'
	--> register a notifier callback to track the freq changes

> 
> If this right or should with be changed to
> 	if (!ret && !IS_ERR(ret)) {
> or even better
> 	if (!IS_ERR_OR_NULL(ret)) {
> 
> Philipp Hahn

     prev parent reply	other threads:[~2026-03-04 18:58 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-03-04 17:48 drivers/clocksource/mips-gic-timer.c:283 looks suspicious: ret != clk Philipp Matthias Hahn
2026-03-04 18:58 ` Daniel Lezcano [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b84dc28a-34ca-4243-a0ae-3b61db04fcd5@oss.qualcomm.com \
    --to=daniel.lezcano@oss.qualcomm.com \
    --cc=daniel.lezcano@kernel.org \
    --cc=linux-mips@vger.kernel.org \
    --cc=phahn-oss@avm.de \
    --cc=tglx@kernel.org \
    --cc=tsbogend@alpha.franken.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).