Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1320491 > unrolled thread

Re: [PATCH] genirq: fix trigger flags check for shared irqs

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] genirq: fix trigger flags check for shared irqs Brian Starkey <brian.starkey@arm.com> - 2016-01-28 11:10 +0100
    Re: [PATCH] genirq: fix trigger flags check for shared irqs Thomas Gleixner <tglx@linutronix.de> - 2016-01-28 11:40 +0100
      Re: [PATCH] genirq: fix trigger flags check for shared irqs Brian Starkey <brian.starkey@arm.com> - 2016-01-28 12:20 +0100
        Re: [PATCH] genirq: fix trigger flags check for shared irqs Thomas Gleixner <tglx@linutronix.de> - 2016-01-28 13:00 +0100
          Re: [PATCH] genirq: fix trigger flags check for shared irqs Brian Starkey <brian.starkey@arm.com> - 2016-01-28 13:30 +0100
            Re: [PATCH] genirq: fix trigger flags check for shared irqs Thomas Gleixner <tglx@linutronix.de> - 2016-01-28 14:40 +0100
              Re: [PATCH] genirq: fix trigger flags check for shared irqs Brian Starkey <brian.starkey@arm.com> - 2016-02-08 12:10 +0100
          Re: [PATCH] genirq: fix trigger flags check for shared irqs Rob Herring <robh+dt@kernel.org> - 2016-01-28 17:00 +0100
            Re: [PATCH] genirq: fix trigger flags check for shared irqs Thomas Gleixner <tglx@linutronix.de> - 2016-01-28 21:10 +0100
              Re: [PATCH] genirq: fix trigger flags check for shared irqs Tomasz Figa <tomasz.figa@gmail.com> - 2016-01-29 05:20 +0100

#1320491 — Re: [PATCH] genirq: fix trigger flags check for shared irqs

Hi Thomas,

Thanks for taking a look at this.

On Tue, Jan 26, 2016 at 09:45:32PM +0100, Thomas Gleixner wrote:
>On Tue, 26 Jan 2016, Brian Starkey wrote:
>
>> For shared interrupts, if one requester passes in any IRQF_TRIGGER_*
>> flags whilst another doesn't, __setup_irq() can erroneously fail.
>>
>> The no-flags case should be treated as "already configured", so change
>> __setup_irq() to only check that the flags match if any have been
>> provided.
>
>What happens if that "already configured", i.e. the default setting, is
>conflicting with the newly requested interrupt?
>
>I rather prefer the failure than the resulting silent wreckage.
>

Yes, I agree that would be best avoided. It seems to me that this case
is actually handled a bit lower down:

	} else if (new->flags & IRQF_TRIGGER_MASK) {
		unsigned int nmsk = new->flags & IRQF_TRIGGER_MASK;
		unsigned int omsk = irq_settings_get_trigger_mask(desc);

		if (nmsk != omsk)
			/* hope the handler works with current  trigger mode */
			pr_warning("irq %d uses trigger mode %u; requested %u\n",
				   irq, nmsk, omsk);
	}

Perhaps that should be louder/fatal?

Best regards,

-Brian

>Thanks,
>
>	tglx
>

[toc] | [next] | [standalone]

#1320537

Brian,

On Thu, 28 Jan 2016, Brian Starkey wrote:
> On Tue, Jan 26, 2016 at 09:45:32PM +0100, Thomas Gleixner wrote:
> > On Tue, 26 Jan 2016, Brian Starkey wrote:
> > 
> > > For shared interrupts, if one requester passes in any IRQF_TRIGGER_*
> > > flags whilst another doesn't, __setup_irq() can erroneously fail.
> > > 
> > > The no-flags case should be treated as "already configured", so change
> > > __setup_irq() to only check that the flags match if any have been
> > > provided.
> > 
> > What happens if that "already configured", i.e. the default setting, is
> > conflicting with the newly requested interrupt?
> > 
> > I rather prefer the failure than the resulting silent wreckage.
> > 
> 
> Yes, I agree that would be best avoided. It seems to me that this case
> is actually handled a bit lower down:
> 
> 	} else if (new->flags & IRQF_TRIGGER_MASK) {
> 		unsigned int nmsk = new->flags & IRQF_TRIGGER_MASK;
> 		unsigned int omsk = irq_settings_get_trigger_mask(desc);
> 
> 		if (nmsk != omsk)
> 			/* hope the handler works with current  trigger mode
> */
> 			pr_warning("irq %d uses trigger mode %u; requested
> %u\n",
> 				   irq, nmsk, omsk);
> 	}
> 
> Perhaps that should be louder/fatal?

Perhaps. So what's the actual problem case you are trying to solve?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1320566

On Thu, Jan 28, 2016 at 11:29:21AM +0100, Thomas Gleixner wrote:
>Brian,
>
>On Thu, 28 Jan 2016, Brian Starkey wrote:
>> On Tue, Jan 26, 2016 at 09:45:32PM +0100, Thomas Gleixner wrote:
>> > On Tue, 26 Jan 2016, Brian Starkey wrote:
>> >
>> > > For shared interrupts, if one requester passes in any IRQF_TRIGGER_*
>> > > flags whilst another doesn't, __setup_irq() can erroneously fail.
>> > >
>> > > The no-flags case should be treated as "already configured", so change
>> > > __setup_irq() to only check that the flags match if any have been
>> > > provided.
>> >
>> > What happens if that "already configured", i.e. the default setting, is
>> > conflicting with the newly requested interrupt?
>> >
>> > I rather prefer the failure than the resulting silent wreckage.
>> >
>>
>> Yes, I agree that would be best avoided. It seems to me that this case
>> is actually handled a bit lower down:
>>
>> 	} else if (new->flags & IRQF_TRIGGER_MASK) {
>> 		unsigned int nmsk = new->flags & IRQF_TRIGGER_MASK;
>> 		unsigned int omsk = irq_settings_get_trigger_mask(desc);
>>
>> 		if (nmsk != omsk)
>> 			/* hope the handler works with current  trigger mode
>> */
>> 			pr_warning("irq %d uses trigger mode %u; requested
>> %u\n",
>> 				   irq, nmsk, omsk);
>> 	}
>>
>> Perhaps that should be louder/fatal?
>
>Perhaps. So what's the actual problem case you are trying to solve?

I've got a few devices on the same interrupt line. One driver does
something along these lines:

	res = platform_get_resource(dev, IORESOURCE_IRQ, 0);
	flags = (res->flags & IRQF_TRIGGER_MASK) | IRQF_SHARED;
	request_irq(res->start, handler, flags, "name", dev);

This seems pretty reasonable. The problem is since 4a43d686fe33:
    of/irq: Pass trigger type in IRQ resource flags[1]
the trigger type information from device-tree is in res->flags.

So when the other drivers don't pass in any flags, they fail the check
in __setup_irq().

Changing the former driver to remove the flags doesn't seem right, and
adding flags to the latter would imply adding flags to _every_ driver,
which is an awful lot to change - and I'm not sure it would be possible
and/or effective in all cases.

Cheers,

-Brian

[1]http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4a43d686fe336cc0e955c4400ba4d3fcff788786

>
>Thanks,
>
>	tglx
>

[toc] | [prev] | [next] | [standalone]

#1320605

On Thu, 28 Jan 2016, Brian Starkey wrote:
> I've got a few devices on the same interrupt line. One driver does

Just for the record: When will hardware folks finally understand that shared
interrupt lines are a nightmare?

> something along these lines:
> 
> 	res = platform_get_resource(dev, IORESOURCE_IRQ, 0);
> 	flags = (res->flags & IRQF_TRIGGER_MASK) | IRQF_SHARED;
> 	request_irq(res->start, handler, flags, "name", dev);
> 
> This seems pretty reasonable. The problem is since 4a43d686fe33:
>    of/irq: Pass trigger type in IRQ resource flags[1]
> the trigger type information from device-tree is in res->flags.
> 
> So when the other drivers don't pass in any flags, they fail the check
> in __setup_irq().
> 
> Changing the former driver to remove the flags doesn't seem right, and
> adding flags to the latter would imply adding flags to _every_ driver,
> which is an awful lot to change - and I'm not sure it would be possible
> and/or effective in all cases.

So that commit does:

   r->flags = IORESOURCE_IRQ | irqd_get_trigger_type(irq_get_irq_data(irq));

which reads the current setting of the interrupt line.

Now we pass exactly that to request_irq(). So first irq_of_parse_and_map()
configures the interrupt type when mapping it and then hands in the same type
information when requesting the irq.

I have no idea what the purpose of this is and the changelog of that commit is
completely useless, sigh!

I've cc'ed the author and the device tree folks. Perhaps are they able to
explain what this commit tries to 'fix'.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1320619

On Thu, Jan 28, 2016 at 12:49:37PM +0100, Thomas Gleixner wrote:
>On Thu, 28 Jan 2016, Brian Starkey wrote:
>> I've got a few devices on the same interrupt line. One driver does
>
>Just for the record: When will hardware folks finally understand that shared
>interrupt lines are a nightmare?
>

In general, agreed. In this case though I think they get some grace - my
devices are all in an FPGA which only has one interrupt line to the SoC.

>> something along these lines:
>>
>> 	res = platform_get_resource(dev, IORESOURCE_IRQ, 0);
>> 	flags = (res->flags & IRQF_TRIGGER_MASK) | IRQF_SHARED;
>> 	request_irq(res->start, handler, flags, "name", dev);
>>
>> This seems pretty reasonable. The problem is since 4a43d686fe33:
>>    of/irq: Pass trigger type in IRQ resource flags[1]
>> the trigger type information from device-tree is in res->flags.
>>
>> So when the other drivers don't pass in any flags, they fail the check
>> in __setup_irq().
>>
>> Changing the former driver to remove the flags doesn't seem right, and
>> adding flags to the latter would imply adding flags to _every_ driver,
>> which is an awful lot to change - and I'm not sure it would be possible
>> and/or effective in all cases.
>
>So that commit does:
>
>   r->flags = IORESOURCE_IRQ | irqd_get_trigger_type(irq_get_irq_data(irq));
>
>which reads the current setting of the interrupt line.
>
>Now we pass exactly that to request_irq(). So first irq_of_parse_and_map()
>configures the interrupt type when mapping it and then hands in the same type
>information when requesting the irq.

Right, there's some redundancy here.

>
>I have no idea what the purpose of this is and the changelog of that commit is
>completely useless, sigh!
>
>I've cc'ed the author and the device tree folks. Perhaps are they able to
>explain what this commit tries to 'fix'.

This 'fix' is what makes me hit the problem - but even without it I
think the problem still exists.

It seems like in principle two drivers ought to be able to do

	request_irq(irq, handler, IRQF_SHARED | IRQF_TRIGGER_HIGH, ...);

and

	request_irq(irq, handler, IRQF_SHARED, ...);

without the latter call failing. Or do you disagree?

-Brian

>
>Thanks,
>
>	tglx
>

[toc] | [prev] | [next] | [standalone]

#1320679

On Thu, 28 Jan 2016, Brian Starkey wrote:

> On Thu, Jan 28, 2016 at 12:49:37PM +0100, Thomas Gleixner wrote:
> > On Thu, 28 Jan 2016, Brian Starkey wrote:
> > > I've got a few devices on the same interrupt line. One driver does
> > 
> > Just for the record: When will hardware folks finally understand that shared
> > interrupt lines are a nightmare?
> > 
> 
> In general, agreed. In this case though I think they get some grace - my
> devices are all in an FPGA which only has one interrupt line to the SoC.

No grace at all. Adding an real irqchip which lets you mask/unmask each device
irq seperately and having a demux handler on the interrupt line to the SoC is
the proper solution for this for lots of reasons.

> > So that commit does:
> > 
> >   r->flags = IORESOURCE_IRQ | irqd_get_trigger_type(irq_get_irq_data(irq));
> > 
> > which reads the current setting of the interrupt line.
> > 
> > Now we pass exactly that to request_irq(). So first irq_of_parse_and_map()
> > configures the interrupt type when mapping it and then hands in the same
> > type
> > information when requesting the irq.
> 
> Right, there's some redundancy here.

Some?

> > I have no idea what the purpose of this is and the changelog of that commit
> > is
> > completely useless, sigh!
> > 
> > I've cc'ed the author and the device tree folks. Perhaps are they able to
> > explain what this commit tries to 'fix'.
> 
> This 'fix' is what makes me hit the problem - but even without it I
> think the problem still exists.
> 
> It seems like in principle two drivers ought to be able to do
> 
> 	request_irq(irq, handler, IRQF_SHARED | IRQF_TRIGGER_HIGH, ...);
> 
> and
> 
> 	request_irq(irq, handler, IRQF_SHARED, ...);
> 
> without the latter call failing. Or do you disagree?

In principle I agree. The issue is that it really depends on the particular
hardware situation.

If there is an explicit requirement for one driver - expressed by a trigger
flag - and the other driver relies on the default configuration, then this
might cause malfunction.

The hassle is, that IRQF_TRIGGER_NONE has unclear semantics. It can mean "I
don't care" or "I rely on the hw configuration". The latter is what worries
me.

first driver: 

      creates the mapping and sets the trigger type according to the DT
      setting.

      driver calls request_irq() with IRQF_TRIGGER_NONE. It relies on the DT
      setting to be correct.

second driver:

      Finds an existing mapping. Now we have two cases:

      1) flat irqdomains:

      	 The DT setting is applied to the trigger type unconditionally.

	 So if that setting is contrary to first drivers DT setting then we
	 are already in trouble.

	 If the driver uses IRQF_TRIGGER_NONE, bad luck as nothing will notice
	 the issue.

      2) hierarchical irqdomains

      	 That code path ignores the type setting of the second driver and
      	 leaves the irq line in the existing state.

	 If the driver uses IRQF_TRIGGER_NONE, bad luck as nothing will notice
	 the issue.
      	 
So we have two problems here.

1) We should detect the mismatch already in the mapping function.

   But, that's hard for legacy reasons. Interrupts can be mapped at early boot
   with hardware default settings and we currently have no way to distinguish
   that. It shouldn't be hard to fix that.

2) How to deal with the mismatch in request_irq()

   Relaxing the check is not really a good decision. So what we could do is:

   if ((new_action->flags & IRQF_TRIGGER_MASK) == IRQF_TRIGGER_NONE)
      	new_action->flags |= irqd_get_trigger_type(irqdata);

   Now that has an issue as well. If the driver requests with
   IRQF_TRIGGER_NONE and does an explicit type setting afterwards, the
   action->flags still do not reflect it.

The whole trigger handling versus shared interrupts needs some deep thoughts
and I really want to understand what that of commit 4a43d686fe336 before
making any decisions.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1328926

Hi Thomas,

Any further thoughts on this? (some comments below)

On Thu, Jan 28, 2016 at 02:37:24PM +0100, Thomas Gleixner wrote:
>In principle I agree. The issue is that it really depends on the 
>particular
>hardware situation.
>
>If there is an explicit requirement for one driver - expressed by a trigger
>flag - and the other driver relies on the default configuration, then this
>might cause malfunction.
>
>The hassle is, that IRQF_TRIGGER_NONE has unclear semantics. It can mean "I
>don't care" or "I rely on the hw configuration". The latter is what worries
>me.
>
>first driver:
>
>      creates the mapping and sets the trigger type according to the DT
>      setting.
>
>      driver calls request_irq() with IRQF_TRIGGER_NONE. It relies on the DT
>      setting to be correct.
>
>second driver:
>
>      Finds an existing mapping. Now we have two cases:
>
>      1) flat irqdomains:
>
>      	 The DT setting is applied to the trigger type unconditionally.
>
>	 So if that setting is contrary to first drivers DT setting then we
>	 are already in trouble.
>
>	 If the driver uses IRQF_TRIGGER_NONE, bad luck as nothing will notice
>	 the issue.
>
>      2) hierarchical irqdomains
>
>      	 That code path ignores the type setting of the second driver and
>      	 leaves the irq line in the existing state.
>
>	 If the driver uses IRQF_TRIGGER_NONE, bad luck as nothing will notice
>	 the issue.
>      	
>So we have two problems here.
>
>1) We should detect the mismatch already in the mapping function.
>
>   But, that's hard for legacy reasons. Interrupts can be mapped at early boot
>   with hardware default settings and we currently have no way to distinguish
>   that. It shouldn't be hard to fix that.
>

Would you agree that this is a separate issue that should be fixed
separately? Even with this fixed, my problem would still exist.

>2) How to deal with the mismatch in request_irq()
>
>   Relaxing the check is not really a good decision. So what we could do is:
>
>   if ((new_action->flags & IRQF_TRIGGER_MASK) == IRQF_TRIGGER_NONE)
>      	new_action->flags |= irqd_get_trigger_type(irqdata);
>
>   Now that has an issue as well. If the driver requests with
>   IRQF_TRIGGER_NONE and does an explicit type setting afterwards, the
>   action->flags still do not reflect it.
>

Yes, an explicit type-setting afterwards would make action->flags get
out-of-sync, but isn't that already the case, regardless of the relaxed
check?

My patch fixes a bogus error for a real use-case, and as far as I can
see doesn't make any of the existing problems worse - so I feel like
that's a net win.

>The whole trigger handling versus shared interrupts needs some deep thoughts
>and I really want to understand what that of commit 4a43d686fe336 before
>making any decisions.
>

If you'd rather see a patch something like your 2) above, I can do that,
let me know what you think.

Many thanks,

Brian

>Thanks,
>
>	tglx
>

[toc] | [prev] | [next] | [standalone]

#1320823

On Thu, Jan 28, 2016 at 5:49 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 28 Jan 2016, Brian Starkey wrote:
>> I've got a few devices on the same interrupt line. One driver does
>
> Just for the record: When will hardware folks finally understand that shared
> interrupt lines are a nightmare?
>
>> something along these lines:
>>
>>       res = platform_get_resource(dev, IORESOURCE_IRQ, 0);
>>       flags = (res->flags & IRQF_TRIGGER_MASK) | IRQF_SHARED;
>>       request_irq(res->start, handler, flags, "name", dev);
>>
>> This seems pretty reasonable. The problem is since 4a43d686fe33:
>>    of/irq: Pass trigger type in IRQ resource flags[1]
>> the trigger type information from device-tree is in res->flags.
>>
>> So when the other drivers don't pass in any flags, they fail the check
>> in __setup_irq().
>>
>> Changing the former driver to remove the flags doesn't seem right, and
>> adding flags to the latter would imply adding flags to _every_ driver,
>> which is an awful lot to change - and I'm not sure it would be possible
>> and/or effective in all cases.
>
> So that commit does:
>
>    r->flags = IORESOURCE_IRQ | irqd_get_trigger_type(irq_get_irq_data(irq));
>
> which reads the current setting of the interrupt line.
>
> Now we pass exactly that to request_irq(). So first irq_of_parse_and_map()
> configures the interrupt type when mapping it and then hands in the same type
> information when requesting the irq.
>
> I have no idea what the purpose of this is and the changelog of that commit is
> completely useless, sigh!
>
> I've cc'ed the author and the device tree folks. Perhaps are they able to
> explain what this commit tries to 'fix'.

It's certainly not clear what driver was being fixed. I think the
intention was to provide the trigger flags from the DT to drivers in a
standard, non-DT specific way. The implementation of it certainly
seems convoluted. The usecase I can think of is drivers may need the
trigger information for cases where the device can support different
trigger modes/polarity. The DT says which trigger mode to use and the
device and irqchip need to be programmed to match. IIRC, SMSC ethernet
chips frequently used on ARM boards do this.

Rob

[toc] | [prev] | [next] | [standalone]

#1321018

On Thu, 28 Jan 2016, Rob Herring wrote:
> On Thu, Jan 28, 2016 at 5:49 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > I've cc'ed the author and the device tree folks. Perhaps are they able to
> > explain what this commit tries to 'fix'.
> 
> It's certainly not clear what driver was being fixed. I think the
> intention was to provide the trigger flags from the DT to drivers in a
> standard, non-DT specific way. The implementation of it certainly
> seems convoluted. The usecase I can think of is drivers may need the
> trigger information for cases where the device can support different
> trigger modes/polarity. The DT says which trigger mode to use and the
> device and irqchip need to be programmed to match. IIRC, SMSC ethernet
> chips frequently used on ARM boards do this.

I can see that the driver wants this information, but why would one feed that
back into request_irq() when the interrupt was already configured by DT.

Still confused.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1321476

Hi Thomas,

2016-01-29 5:06 GMT+09:00 Thomas Gleixner <tglx@linutronix.de>:
> On Thu, 28 Jan 2016, Rob Herring wrote:
>> On Thu, Jan 28, 2016 at 5:49 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > I've cc'ed the author and the device tree folks. Perhaps are they able to
>> > explain what this commit tries to 'fix'.
>>
>> It's certainly not clear what driver was being fixed. I think the
>> intention was to provide the trigger flags from the DT to drivers in a
>> standard, non-DT specific way. The implementation of it certainly
>> seems convoluted. The usecase I can think of is drivers may need the
>> trigger information for cases where the device can support different
>> trigger modes/polarity. The DT says which trigger mode to use and the
>> device and irqchip need to be programmed to match. IIRC, SMSC ethernet
>> chips frequently used on ARM boards do this.
>
> I can see that the driver wants this information, but why would one feed that
> back into request_irq() when the interrupt was already configured by DT.
>

I think Rob already explained the original intention of the patch in
question I sent quite long time ago.

To clarify, I have to add that passing interrupt trigger in resource
flags is not something specific for DT, but it is the general way used
for platform devices and dating times even before DT adoption on ARM
(=== wide spread DT adoption). The key point here is that on pre-DT
systems, the interrupt would not be already configured by DT code and
that's why you have to pass the trigger flags to request_irq().

Best regards,
Tomasz

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web