linux-kernel - Re: [PATCH] cs5535-clockevt: don't ignore MFGPT on SMP-capable kernels

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: <4F18133B.9020801@LiPPERTEmbedded.de>
Date:	Thu, 19 Jan 2012 13:57:31 +0100
From:	Jens Rottmann <JRottmann@...PERTEmbedded.de>
To:	Andres Salomon <dilinger@...ued.net>
CC:	Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org,
	linux-geode@...ts.infradead.org
Subject: Re: [PATCH] cs5535-clockevt: don't ignore MFGPT on SMP-capable kernels

Hi Andres,

and a happy new year to you. Sorry for the delay.

Andres Salomon wrote:
> Jens Rottmann <JRottmann@...PERTEmbedded.de> wrote:
>> cs5535-clockevt: don't ignore MFGPT on SMP-capable kernels
>>
>> On SMP-capable kernels (e.g. generic distro kernel) the
>> cs5535-clockevt driver loads but is not actually used.
>>
>> Setting cpumask to cpu_all_mask works for UP-only kernels, but if
>> compiled for SMP - though still running on the same UP hardware -
>> kernel/time/tick-common.c:tick_check_new_device() reads this as
>> "non-cpu-local" and silently ignores the device.
>>
>> If we leave cpumask unset clockevents_register_device() will
>> initialize it and the cs5535-clockevt driver will be used no matter
>> how the kernel was compiled. Should anyone ever manage to stick a
>> CS553x in an SMP system (is this even possible?) then a warning will
>> be printed.  This is fine as the cs5535-clockevt driver was never
>> written/tested for SMP.
>>
>> If bisecting led you here this patch may have exposed a pre-existing
>> MFGPT problem.  Configure for UP-only and re-check.
>>
>> Signed-off-by: Jens Rottmann <JRottmann@...PERTEmbedded.de>
>> ---
>>
>> --- linux-3.2-rc6/drivers/clocksource/cs5535-clockevt.c
>> +++ use_mfgpt_on_smp_kernels/drivers/clocksource/cs5535-clockevt.c
>> @@ -100,7 +100,6 @@ static struct clock_event_device cs5535_
>>  	.set_mode = mfgpt_set_mode,
>>  	.set_next_event = mfgpt_next_event,
>>  	.rating = 250,
>> -	.cpumask = cpu_all_mask,
>>  	.shift = 32
>>  };
>>
>> _
>
> Hm, have you tried setting cpumask to cpumask_of(0), like a bunch of
> the other clock_event_device drivers do?

Yes, I've seen that, and it was my initial approach. cpumask_of(0)
isn't regarded constant, so moved it from the struct init to
cs5535_mfgpt_init() ... but then I saw

clockevents.c:clockevents_register_device():
	if (!dev->cpumask) {
		WARN_ON(num_possible_cpus() > 1);
		dev->cpumask = cpumask_of(smp_processor_id());
	}

Looks to me like meant exactly for this purpose. This will set cpumask to
cpumask_of(0), i.e. does exactly what you're suggesting.

Both do work fine, but I understand cpumask=cpumask_of(0) to mean "this timer
is hardware-tied to a specific CPU", whereas cpumask unset means more like
"wow, we never expected anybody putting this hardware in an SMP system".

And you've said it yourself:

> I've only ever tested it on UP kernels. [...] I hope all of those cs5535
> drivers that hadn't had their locking primitives tested aren't racy!

So I see the WARN_ON as a bonus.

Cheers
Jens
--
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/