Re: [PATCH 3/3] irq/irq_sim: implement irq_sim_irq2offset()

From: Marc Zyngier
Date: Wed Oct 18 2017 - 06:13:42 EST


On 18/10/17 10:51, Bartosz Golaszewski wrote:
> 2017-10-18 10:58 GMT+02:00 Marc Zyngier <marc.zyngier@xxxxxxx>:
>> On 05/10/17 13:44, Bartosz Golaszewski wrote:
>>> Add a routine allowing to retrieve the offset corresponding with an
>>> allocated interrupt number from an irq_sim object.
>>>
>>> Signed-off-by: Bartosz Golaszewski <brgl@xxxxxxxx>
>>> ---
>>> include/linux/irq_sim.h | 1 +
>>> kernel/irq/irq_sim.c | 15 +++++++++++++++
>>> 2 files changed, 16 insertions(+)
>>>
>>> diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h
>>> index 246f593face8..e9768ca81e3e 100644
>>> --- a/include/linux/irq_sim.h
>>> +++ b/include/linux/irq_sim.h
>>> @@ -41,5 +41,6 @@ void irq_sim_fini(struct irq_sim *sim);
>>> void irq_sim_fire(struct irq_sim *sim, unsigned int offset);
>>> int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset);
>>> int irq_sim_baseirq(struct irq_sim *sim);
>>> +unsigned int irq_sim_irq2offset(struct irq_sim *sim, int irq);
>>>
>>> #endif /* _LINUX_IRQ_SIM_H */
>>> diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
>>> index 1be10d0e295f..9a9a1bc8853c 100644
>>> --- a/kernel/irq/irq_sim.c
>>> +++ b/kernel/irq/irq_sim.c
>>> @@ -174,3 +174,18 @@ int irq_sim_baseirq(struct irq_sim *sim)
>>> return irq_sim_irqnum(sim, 0);
>>> }
>>> EXPORT_SYMBOL_GPL(irq_sim_baseirq);
>>> +
>>> +/**
>>> + * irq_sim_irq2offset - Get the offset of an allocated interrupt.
>>> + *
>>> + * @sim: The interrupt simulator object.
>>> + * irq: Allocated interrupt number.
>>> + *
>>> + * Perform a reverse lookup for the interrupt number and return its offset in
>>> + * the array allocated by sim_irq.
>>> + */
>>> +unsigned int irq_sim_irq2offset(struct irq_sim *sim, int irq)
>>> +{
>>> + return irq - sim->irq_base;
>>> +}
>>> +EXPORT_SYMBOL_GPL(irq_sim_irq2offset);
>>>
>>
>> How is that useful in a random driver? The irq base and the offset are
>> things that should be internal to an irqchip. Maybe you could explain
>> the context you want to use this in.
>>
>
> The interrupts are allocated within the interrupt simulator code, but
> it's up to users to actually request these irqs. That's why we already
> have sim_irq_irqnum().
>
> The IIO dummy driver (one of the users of irq_sim) uses these irq
> numbers retrieved from the simulator to also access private data
> stored in the iio_dummy_evgen module. Currently we store the base
> interrupt number in a separate variable in the dummy_evgen context
> struct and calculate the offset in the interrupt array manually. We
> could simplify that code by moving this logic into the interrupt
> simulator.

It looks to me that this is all because the irq_sim creation is a bit awkward.
You end-up with all kind of exotic interfaces because you don't know the base
of the irq range at creation time.

How about something like this:

diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index 24caabf1a0f7..484c3544c0d1 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -49,7 +49,8 @@ static void irq_sim_handle_irq(struct irq_work *work)
* @sim: The interrupt simulator object to initialize.
* @num_irqs: Number of interrupts to allocate
*
- * Returns 0 on success and a negative error number on failure.
+ * Returns the interrupt base on success and a negative error number
+ * on failure.
*/
int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs)
{
@@ -78,7 +79,7 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs)
init_irq_work(&sim->work_ctx.work, irq_sim_handle_irq);
sim->irq_count = num_irqs;

- return 0;
+ return sim->irq_base;
}
EXPORT_SYMBOL_GPL(irq_sim_init);

You can then deal with the offset directly in your driver, as you're
guaranteed a 1:1 mapping.

Thoughts?

M.
--
Jazz is not dead. It just smells funny...