linux-kernel - Re: [PATCH 3/3] irq/irq_sim: implement irq_sim

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e5d643d2-94d7-acdf-ef87-0b490f6fb8c5@arm.com>
Date:   Wed, 18 Oct 2017 11:13:35 +0100
From:   Marc Zyngier <marc.zyngier@....com>
To:     Bartosz Golaszewski <brgl@...ev.pl>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] irq/irq_sim: implement irq_sim_irq2offset()

On 18/10/17 10:51, Bartosz Golaszewski wrote:
> 2017-10-18 10:58 GMT+02:00 Marc Zyngier <marc.zyngier@....com>:
>> 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@...ev.pl>
>>> ---
>>>  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...