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

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

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))

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

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

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