Hi Marc,
On Friday 11 December 2015 04:53 PM, Marc Zyngier wrote:
> On Fri, 11 Dec 2015 05:26:02 +0000
>> I think we can make percpu irq API a bit easier to use.
>>
>> (1) First thing which request_percpu_irq() does is check for
>> irq_settings_is_per_cpu_devid(). Thus irq_set_percpu_devid() can be built
>> into the
>> API itself eliding the need to set it apriori.
>
> I don't think we can. At least in the case I'm concerned about (GIC's
> PPIs), this is a hardware requirement. You cannot turn a global
> interrupt into a per-CPU one, nor the other way around.
Understood.
> We also have
> drivers (at least our PMUs) that do test the state of that interrupt
> (per-CPU or not) to find out how they should be requested.
But they call request_percpu_irq() only after determining that irq is percpu.
Otherwise they will call vanilla request_irq()
e.g. drivers/perf/arm/arc_pmu.c
Which means that request_percpu_irq() can safely assume that caller absolutely
wants percpu semantics and hence do equivalent of irq_set_percpu_devid()
internally - NO. I'm sure I'm missing something.
> I agree that the API is probably not the ideal one, but there is HW
> constraints that we cannot just ignore.
The API is pretty nice :-) there are these quirks which I want to avoid.
My naive'ity in this area of code fails me to see how the hardware constraint is
coming into play.
>> (2) It seems that disabling autoen by default for percpu irq makes sense as
>> evident from drivers/net/ethernet/marvell/mvneta.c where users want to
>> control
>> this. However the comment there is misleading
>>
>> /* Even though the documentation says that request_percpu_irq
>> * doesn't enable the interrupts automatically, it actually
>> * does so on the local CPU.
>> *
>> * Make sure it's disabled.
>> */
>>
>> Either sme core code is clearing NOAUTOEN or calling enable_precpu_irq()
>> making
>> request_percpu_irq() enable it.
>
> If that's the case, this is a bug. Nobody should enable that interrupt
> until the driver has chosen to do so.
Perhaps Maxim can shed more light as this seems to be his comment.
>> IMHO it makes more sense to make autoen explicit in the API.
>> Perhaps introduce a API flavour, which takes the autoen as arg.
>> It could take flags to make it more extensible / future safe but that will
>> be an
>> overkill I think.
>
> But auto-enabling cannot be done from a single CPU. It can only be done
> from the core that is going to be delivered that interrupt. This
> requires access to registers that are simply not available to other CPUs.
I'm not talking about eliminating enable_percpu_irq() call from all cores and
still getting the auto-enable semantics. What I mean is doing the equivalent of
irq_set_status_flags(irq, IRQ_NOAUTOEN);
from within request_percpu_irq_xxx() based on an additional arg (vs. doing it
aprioiri outside).
OTOH, thinking a bit more abt this, I think the current semantics of
auto-disable
w/o any arg is just fine. Most percpu irqs in general purpose drivers would want
the auto-disable anyways. Only for core irws such as timer / IPI etc do we want
auto-enable.
Thx,
-Vineet
_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
