linux-kernel - Re: [PATCH v2] clocksource/drivers/tango

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56151B5B.90404@sigmadesigns.com>
Date:	Wed, 7 Oct 2015 15:17:15 +0200
From:	Marc Gonzalez <marc_gonzalez@...madesigns.com>
To:	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Thomas Gleixner <tglx@...utronix.de>
CC:	LKML <linux-kernel@...r.kernel.org>,
	Mans Rullgard <mans@...sr.com>, Mason <slash.tmp@...e.fr>
Subject: Re: [PATCH v2] clocksource/drivers/tango_xtal: Add new timer for
 Tango SoCs

On 07/10/2015 14:31, Daniel Lezcano wrote:
> On 10/07/2015 01:35 PM, Marc Gonzalez wrote:
>> Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
>> Use it for clocksource, sched_clock, and delay_timer.
>>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@...madesigns.com>
>> ---
>> AFAICS, clocksource_register_hz does not report failures via its
>> return value (always 0) but writes warnings to stdout?
> 
> Yeah, it returns always 0. I suggest you assume it is returning an error
> code, that will be safer for future changes in the framework (if any).

I suppose I'd also need to check the return value of of_clk_get?
(Looks like you mention it implicitly below.)

>> Open question: can I call register_current_timer_delay,
>> sched_clock_register, clocksource_register_hz in any order?
>> ---
> 
> Yes, I think so. Thomas ?
> 
> [ ... ]
> 
>> +static void __init tango_clocksource_init(struct device_node *np)
>> +{
>> +	struct clk *clk = of_clk_get(np, 0);
>> +	unsigned int xtal_freq = clk_get_rate(clk);
>> +	xtal_in_cnt = of_iomap(np, 0);
>> +	if (xtal_in_cnt == NULL)
>> +		panic("%s: of_iomap failed\n", np->full_name);
> 
>    ^^^^^^^^^^^
> 
> That does not comply with the Linux kernel coding style.

<confused>

scripts/checkpatch.pl only complains about a missing blank line after
the declaration block. (Sorry, I'll fix that.)

> 	xtal_in_cnt = of_iomap(np, 0);
> 	if (!xtal_in_cnt) {
> 		pr_err("Argh!");
> 		return;
> 	}

I know "!xtal_in_cnt" is equivalent to "xtal_in_cnt == NULL" but I'd
rather emphasize the fact that xtal_in_cnt is a pointer, not a bool.
(Documentation/CodingStyle does not mandate this particular idiom.)

I'm also confused that you've replaced panic() with pr_err/return.
AFAIU, if I don't have a clocksource/sched_clock, the system is dead
in the water. Might as well stop there, and wait for the operator to
fix whatever needs fixing. (Several clksrc drivers do this.)

> 	clk = of_clk_get(np, 0);
> 	if (!clk) {

AFAICT, checking for NULL is not good enough here.
of_clk_get returns ERR_PTR(rc) style errors.
Looks like I'd need "if (IS_ERR(clk))"

Thanks for the review.

Regards.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/