Hi Ezequiel,

On 04/11/2014 16:11, Ezequiel Garcia wrote:

Hi Gregory,

On 10/31/2014 01:36 PM, Gregory CLEMENT wrote:

quoted

On 22/10/2014 15:43, Ezequiel Garcia wrote:

quoted

The map, mask and unmask is unnecessarily complicated, with a different
implementation for shared and per CPU interrupts. The current code does
the following:

At probe time, all interrupts are disabled and masked on all CPUs.

Shared interrupts:

 * When the interrupt is mapped(), it gets disabled and unmasked on the
   calling CPU.

 * When the interrupt is unmasked(), masked(), it gets enabled and
   disabled.

Per CPU interrupts:

 * When the interrupt is mapped, it gets masked on the calling CPU and
   enabled.

 * When the interrupt is unmasked(), masked(), it gets unmasked and masked,
   on the calling CPU.

This commit simplifies this code, with a much simpler implementation, common
to shared and per CPU interrupts.

 * When the interrupt is mapped, it's enabled.

 * When the interrupt is unmasked(), masked(), it gets unmasked and masked,
   on the calling CPU.

By doing this you change the behavior of the irqchip. Before this patch, masking
a shared interrupt was masking it on all the CPUs in the same time whereas with this
change it will mask the interrupt only on the calling CPU. It worth checking it is
the expected behavior.

The kernel will always mask/unmask on the appropriate CPU, so I don't see any
issues with this.

quoted

Moreover I wonder how it is supposed to work with the irq affinity. Let say that the
irq was enabled on the CPU0,

If you mean enabled as in irq_enable(irq_desc), then let's keep in mind that
currently that calls .irq_unmask.

quoted

then there is irq_unmask call on CPU1, then the irq would be enabled on both CPUs.
It will modify the irq affinity and moreover it will also lead
to an invalidate state with the MPIC because we can't manage an interrupt on more than
one CPU.

From the point of the view of the armada_370_xp_irq driver there are potential bug, but
maybe the use case I described can't happen. Could you check it?

In this irqchip driver, shared interrupts are handled via handle_level_irq(),
which masks() and unmasks() the interrupt. In other words, it's always done
in the same CPU. I guess this is the most frequent mask/unmask operation.

On the other side, if a driver calls enable_irq() on one CPU and then
enable_irq() on the other CPU, that's exactly what will happen. Although the
smp_affinity setting will prevent an interrupt from being dispatched to more
than one CPU.

I really don't see an issue with this patch, but I think it needs as much
discussion and as much testing as possible. This piece of code is rather
critical, and is working just fine, so I'd like to have an excellent 
reason before changing any of it.

We already talked about it on IRC but I write it agin o let the other people know
our status on this subject.

If a driver call irq_enable() then this functions will call irq_enable() and as we didn't
implement a .enbale() operation, it will call our only unmask() function.

So if the IRQ was unmasked on a CPU and a driver call an irq_enable() from an other CPU then
we will end up with the IRQ enabled on 2 different CPUs. It is a problem for 2 reasons:
- our hardware don't handle a IRQ enable on more than one CPU
- it will modify the affinity at the hardware level because a new CPU will be able to
receive an irq whereas we setup the affinity on only one CPU.

By only using the mask and unmask registers we will need to handle the affinity by
software instead of using the hardware. I agree that the current code is not trivial,
but adding more comment instead of removing this capability should be better.


Thanks,

Gregory







-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com