[PATCH 0/3] irq-sim updates for 4.15

[PATCH 0/3] irq-sim updates for 4.15

[Bartosz Golaszewski]

I initially submitted patch 2/3 as a standalone, but it turned out
during some development for iio that some additional functionalities
would be useful as well.

The first patch pulls in slab.h directly instead of relying on the
include from irq.h.

The second exposes an interface for accessing the base of the
allocated interrupt range.

The third implements a function that allows to map the allocated
interrupt number to the array offset within the irq sim object. This
will be later (4.16) used in iio, where for 4.15 we needed to keep the
irq base value and to calculate the offset manually in the driver.

Bartosz Golaszewski (3):
  irq/irq_sim: explicitly pull in slab.h
  irq/irq_sim: implement irq_sim_baseirq()
  irq/irq_sim: implement irq_sim_irq2offset()

 include/linux/irq_sim.h |  2 ++
 kernel/irq/irq_sim.c    | 27 +++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

-- 
2.13.2

[PATCH 1/3] irq/irq_sim: explicitly pull in slab.h

[Bartosz Golaszewski]

We're calling kfree() in the irq_sim code but slab.h is pulled in
indirectly via irq.h. Include it explicitly.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 kernel/irq/irq_sim.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index 24caabf1a0f7..bd7dc1db6a80 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -7,6 +7,7 @@
  * option) any later version.
  */
 
+#include <linux/slab.h>
 #include <linux/irq_sim.h>
 #include <linux/irq.h>
 
-- 
2.13.2

[PATCH 2/3] irq/irq_sim: implement irq_sim_baseirq()

[Bartosz Golaszewski]

Some users need to access the base of the allocated interrupt range.
Although this can be calculated manually, it's more elegant to expose
an interface for that.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 include/linux/irq_sim.h |  1 +
 kernel/irq/irq_sim.c    | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h
index 0380d899b955..246f593face8 100644
--- a/include/linux/irq_sim.h
+++ b/include/linux/irq_sim.h
@@ -40,5 +40,6 @@ int devm_irq_sim_init(struct device *dev, struct irq_sim *sim,
 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);
 
 #endif /* _LINUX_IRQ_SIM_H */
diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index bd7dc1db6a80..1be10d0e295f 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -163,3 +163,14 @@ int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset)
 	return sim->irqs[offset].irqnum;
 }
 EXPORT_SYMBOL_GPL(irq_sim_irqnum);
+
+/**
+ * irq_sim_baseirq - Get the base interrupt number.
+ *
+ * @sim:        The interrupt simulator object.
+ */
+int irq_sim_baseirq(struct irq_sim *sim)
+{
+	return irq_sim_irqnum(sim, 0);
+}
+EXPORT_SYMBOL_GPL(irq_sim_baseirq);
-- 
2.13.2

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

[Bartosz Golaszewski]

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@bgdev.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);
-- 
2.13.2

Re: [PATCH 0/3] irq-sim updates for 4.15

[Bartosz Golaszewski]

2017-10-05 14:44 GMT+02:00 Bartosz Golaszewski <brgl@bgdev.pl>:
> I initially submitted patch 2/3 as a standalone, but it turned out
> during some development for iio that some additional functionalities
> would be useful as well.
>
> The first patch pulls in slab.h directly instead of relying on the
> include from irq.h.
>
> The second exposes an interface for accessing the base of the
> allocated interrupt range.
>
> The third implements a function that allows to map the allocated
> interrupt number to the array offset within the irq sim object. This
> will be later (4.16) used in iio, where for 4.15 we needed to keep the
> irq base value and to calculate the offset manually in the driver.
>
> Bartosz Golaszewski (3):
>   irq/irq_sim: explicitly pull in slab.h
>   irq/irq_sim: implement irq_sim_baseirq()
>   irq/irq_sim: implement irq_sim_irq2offset()
>
>  include/linux/irq_sim.h |  2 ++
>  kernel/irq/irq_sim.c    | 27 +++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
>
> --
> 2.13.2
>

Hi Thomas, Marc,

can these be merged or are there any issues with the series?

Thanks,
Bartosz

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

[Marc Zyngier]

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@bgdev.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.

Thanks,

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

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

[Bartosz Golaszewski]

2017-10-18 10:58 GMT+02:00 Marc Zyngier <marc.zyngier@arm.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@bgdev.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.

I hope it's clear enough.

Best regards,
Bartosz Golaszewski

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

[Marc Zyngier]

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

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

[Bartosz Golaszewski]

2017-10-18 12:13 GMT+02:00 Marc Zyngier <marc.zyngier@arm.com>:
>

[snip!]

> 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.
>

This is what we have now in next for iio. The thing is we want to get
rid of irq_base from the iio driver and store it (and relevant logic)
in irq_sim to have less code in the caller. Otherwise we duplicate
this info.

Thanks,
Bartosz

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

[Marc Zyngier]

On Wed, Oct 18 2017 at  2:49:07 pm BST, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 2017-10-18 12:13 GMT+02:00 Marc Zyngier <marc.zyngier@arm.com>:
>>
>
> [snip!]
>
>> 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.
>>
>
> This is what we have now in next for iio. The thing is we want to get
> rid of irq_base from the iio driver and store it (and relevant logic)
> in irq_sim to have less code in the caller. Otherwise we duplicate
> this info.

The IIO subsystem caching this information is a side effect of the way
it decides to deal with it. It is no different from the PCI layer
requesting 16 MSIs and having to work out which ones it got. It doesn't
go back to the irqchip to find out what's there.

How big is the saving on the IIO side?

Thanks,

	M.
-- 
Jazz is not dead, it just smell funny.