Re: [PATCH] genirq: Introduce irq_read_line()

[Bjorn Andersson]

On Fri, Oct 24, 2014 at 10:59 AM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> Hi Bjorn,
>
> On 24/10/14 18:31, Bjorn Andersson wrote:
>> On Tue, Oct 21, 2014 at 2:34 AM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
[..]
>> I would prefer to see that you had explicit functions for the various things
>> that you would like to set and get. It would in my eyes give cleaner client
>> code, at the cost of a few extra functions.
>
> I don't really like the idea of having a gazillion of accessors for each
> individual property. I'd rather have an extensible API.
>

Okay, either way will work. Feel free to proceed with your approach.

>>> Basic idea is that you can read (and possibly write back) various
>>> low-level attributes (pending, masked, active) that an interrupt
>>> controller may implement. Given your use case, we should loose the
>>> checks on the interrupt being forwarded, as this makes little sense
>>> outside of virtualization.
>>>
>>
>> Relaxing the forwarding requirement seems to solve my use cases. May I ask why
>> your accessors would only be made available for forwarded IRQs - at a framework
>> level?
>
> This was never intended for wider use, and restricting it to forwarded
> interrupts was a concious design decision, as I find the idea of a
> device driver messing with the internal state of the interrupt
> controller positively repulsive... But hey, the more the merrier.
>

Makes sense and now that it comes without the forwarding part it looks
reasonable.

[..]
>>> I'm planning to respin the series this week, as I have a number of
>>> changes (there is hardly any need for the various states to be reported
>>> atomically, for example), and a number of bugs have been found.
>>>
>>
>> Sounds good, for clarity I would like them to be explicit separate functions.
>> But as long as it's not limited to forwarded IRQs we should be able to use it.
>
> I just pushed out a branch:
> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
> irq/irqchip_state
>
> Please let me know if that's useful for you.
>

I think that my irq_read_line() would be equivalent of
irq_get_irqchip_state(IRQCHIP_STATE_PENDING), i.e. the state of the
interrupt ignoring masking. And the rest would be EINVAL.
So I think this would work out just fine for us!


Is ACTIVE the line level with the mask considered and setting ACTIVE,
would that be the same as acking the interrupt? What's the expected
behaviour of setting PENDING?

I would appreciate if you commented the state enum, to make it obvious
what pending and active means.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/