On Fri, 09 Jun 2017 06:47:48 PDT (-0700), will.deacon@xxxxxxx wrote:
> On Wed, Jun 07, 2017 at 11:52:10AM +0100, Marc Zyngier wrote:
>> On 07/06/17 00:00, Palmer Dabbelt wrote:
>> > +static void plic_disable(struct plic_data *data, int i, int hwirq)
>> > +{
>> > + struct plic_enable_context *enable = plic_enable_context(data, i);
>> > +
>> > + atomic_and(~(1 << (hwirq % 32)), &enable->mask[hwirq / 32]);
>>
>> This is still a device access, right? What does it mean to use the
>> atomic primitives on that? What are you racing against? I thought the
>> various context were private to an execution context...
>>
>> Adding Will and PeterZ to the CC list because they will probably have
>> their own views on this...
>
> atomic_* accesses to MMIO is almost certainly a bad idea. Is this atomic
> because you want to allow the function to run concurrently, or is it atomic
> because you want some guarantees from the endpoint's view?
Concurrency: while most operations on the PLIC are per-hart, in order to
disable an interrupt you have to go clear a bit for every hart. The AMO ensure
the read-modify-write cycle didn't drop some other hart disabling a different
interrupt (as the bits are all in the same word).
I've done a big cleanup on this driver to avoid memory-mapped structures, use a
lock instead of AMOs, and use the FastEOI flow. I'm getting close to getting
through all the code reviews for my v2 RISC-V patch set (which included this,
our arch support, and all our other driver patches). I'm going to split out
the drivers for the next patch set, as it appears that's the better way to do
it.
Hopefully I can get the cleaned up patches out tonight.
Thanks!
