recommended use of request_any_context_irq()

recommended use of request_any_context_irq()

[Leo Li]

Hi Marc and Thomas,

With the introduction of request_any_context_irq() routine, driver can
deal with interrupt controllers using either threaded irq or normal
irq.  But I don't see many drivers that have been changed to use this
function to request interrupt.  For on-board devices, the driver
normally don't know which kind of interrupt controller they are
connected to.  Why don't we make the request_any_context_irq()
mandatory or recommended for all drivers?  Is there any drawback for
changing all the request_irq() to the request_any_context_irq()?

Regards,
Leo

Re: recommended use of request_any_context_irq()

[Marc Zyngier]

Leo,

On 21/09/16 22:14, Leo Li wrote:
> Hi Marc and Thomas,
> 
> With the introduction of request_any_context_irq() routine, driver can
> deal with interrupt controllers using either threaded irq or normal
> irq.  But I don't see many drivers that have been changed to use this
> function to request interrupt.  For on-board devices, the driver
> normally don't know which kind of interrupt controller they are
> connected to.  Why don't we make the request_any_context_irq()
> mandatory or recommended for all drivers?  Is there any drawback for
> changing all the request_irq() to the request_any_context_irq()?

In 99.99% of the cases, a device is integrated in one particular way,
always. For the 0.01% that is left, we have the above API. And if a
particular device moves from the first category to the second, whoever
designed the system will change the driver to use this API, and that
driver only.

There is strictly no reason to perform a blanket change of all the
drivers. What would be the reason to change them other than to cater for
a contrived use case that may never happen?

Thanks,

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

Re: recommended use of request_any_context_irq()

[Thomas Gleixner]

On Thu, 22 Sep 2016, Marc Zyngier wrote:
> On 21/09/16 22:14, Leo Li wrote:
> > Hi Marc and Thomas,
> > 
> > With the introduction of request_any_context_irq() routine, driver can
> > deal with interrupt controllers using either threaded irq or normal
> > irq.  But I don't see many drivers that have been changed to use this
> > function to request interrupt.  For on-board devices, the driver
> > normally don't know which kind of interrupt controller they are
> > connected to.  Why don't we make the request_any_context_irq()
> > mandatory or recommended for all drivers?  Is there any drawback for
> > changing all the request_irq() to the request_any_context_irq()?
> 
> In 99.99% of the cases, a device is integrated in one particular way,
> always. For the 0.01% that is left, we have the above API. And if a
> particular device moves from the first category to the second, whoever
> designed the system will change the driver to use this API, and that
> driver only.
> 
> There is strictly no reason to perform a blanket change of all the
> drivers. What would be the reason to change them other than to cater for
> a contrived use case that may never happen?

Aside of that this API is explicitely returning the context type so a
driver can accomodate in which context it runs.

And the difference is not plain interupt or threaded. The difference is
plain interrupt or nested thread. Nested threading happens when the device
sits behind a demultiplex interrupt device, where the demultiplex handler
runs in a thread. So the device handler can reuse the thread context of the
demultiplex handler.

Thanks,

	tglx

Re: recommended use of request_any_context_irq()

[Leo Li]

On Thu, Sep 22, 2016 at 3:10 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Leo,
>
> On 21/09/16 22:14, Leo Li wrote:
>> Hi Marc and Thomas,
>>
>> With the introduction of request_any_context_irq() routine, driver can
>> deal with interrupt controllers using either threaded irq or normal
>> irq.  But I don't see many drivers that have been changed to use this
>> function to request interrupt.  For on-board devices, the driver
>> normally don't know which kind of interrupt controller they are
>> connected to.  Why don't we make the request_any_context_irq()
>> mandatory or recommended for all drivers?  Is there any drawback for
>> changing all the request_irq() to the request_any_context_irq()?
>
> In 99.99% of the cases, a device is integrated in one particular way,
> always. For the 0.01% that is left, we have the above API. And if a
> particular device moves from the first category to the second, whoever
> designed the system will change the driver to use this API, and that
> driver only.

I'm not sure if these are such rare cases.  Devices which are not
integrated in the SoC and not on a bus with interrupt handling such as
PCI/PCIE could easily fall into this category.  For example, I2C
devices and SPI devices are very likely to be in this category.  I did
a quick search:

git grep -l 'i2c_driver\|spi_driver' drivers/ |xargs grep -l request_irq |wc -l

The result is 109.

>
> There is strictly no reason to perform a blanket change of all the
> drivers. What would be the reason to change them other than to cater for
> a contrived use case that may never happen?

Maybe we could do blanket change to drivers that meet certain
criteria?  At least we should improve the messaging when a driver
cannot request interrupt due to nested threading.  Right now, it might
take quite some time for a developer unfamiliar with the threaded
interrupt to figure out the problem.

Regards,
Leo

Re: recommended use of request_any_context_irq()

[Thomas Gleixner]

On Thu, 22 Sep 2016, Leo Li wrote:
> On Thu, Sep 22, 2016 at 3:10 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > There is strictly no reason to perform a blanket change of all the
> > drivers. What would be the reason to change them other than to cater for
> > a contrived use case that may never happen?
> 
> Maybe we could do blanket change to drivers that meet certain
> criteria?  At least we should improve the messaging when a driver
> cannot request interrupt due to nested threading.

Nested threading is a result of requesting an any context interrupt not
something which is there already.

> Right now, it might take quite some time for a developer unfamiliar with
> the threaded interrupt to figure out the problem.

Did you have issues with a driver which was not able to request an
interrupt? If yes, please explain in detail what the failure was and why
you think that this should be changed. If not, please explain which problem
you are trying to solve.

Thanks,

	tglx

Re: recommended use of request_any_context_irq()

[Leo Li]

On Thu, Sep 22, 2016 at 3:47 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 22 Sep 2016, Leo Li wrote:
>> On Thu, Sep 22, 2016 at 3:10 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> > There is strictly no reason to perform a blanket change of all the
>> > drivers. What would be the reason to change them other than to cater for
>> > a contrived use case that may never happen?
>>
>> Maybe we could do blanket change to drivers that meet certain
>> criteria?  At least we should improve the messaging when a driver
>> cannot request interrupt due to nested threading.
>
> Nested threading is a result of requesting an any context interrupt not
> something which is there already.
>
>> Right now, it might take quite some time for a developer unfamiliar with
>> the threaded interrupt to figure out the problem.
>
> Did you have issues with a driver which was not able to request an
> interrupt? If yes, please explain in detail what the failure was and why
> you think that this should be changed. If not, please explain which problem
> you are trying to solve.

My problem was with sc16is7xx, an SPI-to-UART
device(drivers/tty/serial/sc16is7xx.c), the interrupt of it is
connected together with interrupts from other on-board devices to a
GPIO expander (drivers/gpio/gpio-pca953x.c).  The interrupt handler of
pca953x interrupt controller is threaded, but the sc16is7xx driver is
currently requesting plain interrupt.  So the sc16is7xx just fails
when requesting irq.  The problem can be fixed by changing the
sc16is7xx driver to use the request_any_context_irq().  But it is not
easy to see this is because of requesting plain interrupt on a
threaded interrupt controller without some debugging effort into the
core code.  And my concerns is that there are other drivers can hit
the same problem if connected to the threaded interrupt controller.
What can we do prevent similar problem in the future?

Regards,
Leo

Re: recommended use of request_any_context_irq()

[Thomas Gleixner]

Leo,

On Thu, 22 Sep 2016, Leo Li wrote:
> core code.  And my concerns is that there are other drivers can hit
> the same problem if connected to the threaded interrupt controller.
> What can we do prevent similar problem in the future?

The simplest way is to be more informative in the failure case. See patch
below.

Converting drivers is surely something which can be done, but I wouldn't
try to do a wholesale conversion blindly.

Thanks,

	tglx

8<----------------

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 73a2b786b5e9..605915f689b4 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1146,6 +1146,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	nested = irq_settings_is_nested_thread(desc);
 	if (nested) {
 		if (!new->thread_fn) {
+			pr_warn("IRQ%u is nested, but thread_fn is NULL\n", irq);
 			ret = -EINVAL;
 			goto out_mput;
 		}
@@ -1158,8 +1159,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	} else {
 		if (irq_settings_can_thread(desc)) {
 			ret = irq_setup_forced_threading(new);
-			if (ret)
+			if (ret) {
+				pr_warn("IRQ%u failed to setup thread\n", irq);
 				goto out_mput;
+			}
 		}
 	}
 

Re: recommended use of request_any_context_irq()

[Leo Li]

On Thu, Sep 22, 2016 at 5:03 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> Leo,
>
> On Thu, 22 Sep 2016, Leo Li wrote:
>> core code.  And my concerns is that there are other drivers can hit
>> the same problem if connected to the threaded interrupt controller.
>> What can we do prevent similar problem in the future?
>
> The simplest way is to be more informative in the failure case. See patch
> below.
>
> Converting drivers is surely something which can be done, but I wouldn't
> try to do a wholesale conversion blindly.

Thomas,

It is not ideal but definitely helpful to provide more information
when there is an issue.

>
> Thanks,
>
>         tglx
>
> 8<----------------
>
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 73a2b786b5e9..605915f689b4 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1146,6 +1146,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>         nested = irq_settings_is_nested_thread(desc);
>         if (nested) {
>                 if (!new->thread_fn) {
> +                       pr_warn("IRQ%u is nested, but thread_fn is NULL\n", irq);

Can we provide more information in this message?  It is still not
clear enough to people who are not familiar with threaded interrupt.

How about "IRQ%u is requesting standard interrupt while connected to
threaded interrupt controller\n Consider using
request_any_context_irq()."?

>                         ret = -EINVAL;
>                         goto out_mput;
>                 }
> @@ -1158,8 +1159,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>         } else {
>                 if (irq_settings_can_thread(desc)) {
>                         ret = irq_setup_forced_threading(new);
> -                       if (ret)
> +                       if (ret) {
> +                               pr_warn("IRQ%u failed to setup thread\n", irq);
>                                 goto out_mput;
> +                       }
>                 }
>         }
>
>
>