Re: [PATCH 08/16 v4] pramfs: headers

[Marco Stornelli <marco.stornelli@gmail.com>]

2010/11/24 Paul Mundt <lethal@linux-sh.org>:
> On Wed, Nov 24, 2010 at 09:23:02AM +0100, Marco Stornelli wrote:
>> 2010/11/24 Paul Mundt <lethal@linux-sh.org>:
>> >
>> >> +#ifdef CONFIG_PRAMFS_WRITE_PROTECT
>> >> +static inline void pram_memunlock_range(void *p, unsigned long len)
>> >> +{
>> >> +#ifndef CONFIG_X86
>> >> + ? ? local_irq_disable();
>> >> +#endif
>> >> + ? ? preempt_disable();
>> >> + ? ? pram_writeable(p, len, 1);
>> >> +}
>> >> +
>> > This needs some explaining, or killing. While the latter is preferable,
>> > we can also work with the former.
>> >
>>
>> Maybe I didn't understand, you mean preemt_disable() without disabling
>> the interrupt?
>
> I mean what exactly is this supposed to be doing? It looks pretty
> questionable for SMP for starters, it doesn't explain why x86 is special,
> etc. It looks like you wanted a spinlock with IRQs disabled but probably
> opted not to use it because you were throwing this around interfaces that
> could sleep, which looks like a really scary thing for latency. It's also
> making architecture assumptions without any explanation of why. This
> needs to be explained, and in some amount of detail, as it's not entirely
> obvious.
>

Ok, it's not clear, I'll try to explain. My idea here is avoid "open
windows" with the memory not locked (lock here means not protected).
The problem here is that if you call set_memory_rw/set_memory_ro with
interrupt disabled (for x86) you'll fall in a bug_on() condition. I
can add that the use of these functions has been discussed in the v1
patch series with Andi Kleen.

Marco