drivers/clocksource/mips-gic-timer.c:283 looks suspicious: ret != clk

drivers/clocksource/mips-gic-timer.c:283 looks suspicious: ret != clk

[Philipp Matthias Hahn]

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);
		if (ret < 0) {
			pr_err("Failed to enable clock\n");
			clk_put(clk);
			return ret;
		}
		gic_frequency = clk_get_rate(clk);
	} else {
		…
	}
	ret = gic_clockevent_init();
	if (!ret && !IS_ERR(clk)) {
	     ^^^            ^^^

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

Re: drivers/clocksource/mips-gic-timer.c:283 looks suspicious: ret != clk

[Daniel Lezcano]

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