* [Xenomai] one-shot fasteoi irqs @ 2013-03-26 23:05 Gilles Chanteperdrix 2013-03-27 6:50 ` Jan Kiszka 0 siblings, 1 reply; 14+ messages in thread From: Gilles Chanteperdrix @ 2013-03-26 23:05 UTC (permalink / raw) To: Jan Kiszka Hi Jan, I seem to recall you sent patches to fix threaded irqs some time ago. I am having a problem here without SMP, whereas the system works with SMP. A fasteoi irq handler triggers repeatedly while its threaded counterpart never gets triggered. I believe this is because handle_fasteoi_irq unconditionally releases the irq line. The following patch seems to fix the issue though I would prefer a cleaner solution. diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index 11e75d1..2ff8d3a 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -463,8 +463,11 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc) #ifdef CONFIG_IPIPE /* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */ - if (desc->irq_data.chip->irq_release) - desc->irq_data.chip->irq_release(&desc->irq_data); + if (desc->irq_data.chip->irq_release) { + if ((!(desc->istate & IRQS_ONESHOT) || + !desc->threads_oneshot)) + desc->irq_data.chip->irq_release(&desc->irq_data); + } out_eoi: #else /* !CONFIG_IPIPE */ if (desc->istate & IRQS_ONESHOT) diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index e49a288..485c2c4 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -715,9 +715,15 @@ again: desc->threads_oneshot &= ~action->thread_mask; - if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) && - irqd_irq_masked(&desc->irq_data)) - unmask_irq(desc); + if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data)) { +#ifdef CONFIG_IPIPE + if (desc->irq_data.chip->irq_release) + desc->irq_data.chip->irq_release(&desc->irq_data); + else +#endif + if (irqd_irq_masked(&desc->irq_data)) + unmask_irq(desc); + } out_unlock: raw_spin_unlock_irq(&desc->lock); -- Gilles. ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Xenomai] one-shot fasteoi irqs 2013-03-26 23:05 [Xenomai] one-shot fasteoi irqs Gilles Chanteperdrix @ 2013-03-27 6:50 ` Jan Kiszka 2013-03-27 9:23 ` Gilles Chanteperdrix 0 siblings, 1 reply; 14+ messages in thread From: Jan Kiszka @ 2013-03-27 6:50 UTC (permalink / raw) To: Gilles Chanteperdrix; +Cc: Xenomai On 2013-03-27 00:05, Gilles Chanteperdrix wrote: > > Hi Jan, > > I seem to recall you sent patches to fix threaded irqs some time ago. I In fact, that was Wolfgang with commits fdda86bca9 and 54d161b85b for 2.6.38 back then. > am having a problem here without SMP, whereas the system works with > SMP. A fasteoi irq handler triggers repeatedly while its threaded > counterpart never gets triggered. I believe this is because > handle_fasteoi_irq unconditionally releases the irq line. You mean it doesn't respect the conditions in cond_unmask_irq? > The following > patch seems to fix the issue though I would prefer a cleaner solution. > > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c > index 11e75d1..2ff8d3a 100644 > --- a/kernel/irq/chip.c > +++ b/kernel/irq/chip.c > @@ -463,8 +463,11 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc) > > #ifdef CONFIG_IPIPE > /* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */ > - if (desc->irq_data.chip->irq_release) > - desc->irq_data.chip->irq_release(&desc->irq_data); > + if (desc->irq_data.chip->irq_release) { > + if ((!(desc->istate & IRQS_ONESHOT) || > + !desc->threads_oneshot)) > + desc->irq_data.chip->irq_release(&desc->irq_data); > + } > out_eoi: > #else /* !CONFIG_IPIPE */ > if (desc->istate & IRQS_ONESHOT) > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index e49a288..485c2c4 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -715,9 +715,15 @@ again: > > desc->threads_oneshot &= ~action->thread_mask; > > - if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) && > - irqd_irq_masked(&desc->irq_data)) > - unmask_irq(desc); > + if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data)) { > +#ifdef CONFIG_IPIPE > + if (desc->irq_data.chip->irq_release) > + desc->irq_data.chip->irq_release(&desc->irq_data); > + else > +#endif > + if (irqd_irq_masked(&desc->irq_data)) > + unmask_irq(desc); > + } > > out_unlock: > raw_spin_unlock_irq(&desc->lock); > > So this basically extends I-pipe's held phase of the IRQ to the threaded handler if we are working in oneshot mode, right? I will have to dig into this again, but without remembering all details, there are some things that do not look OK: - Shouldn't we just replace unmask_irq with irq_release for the I-pipe case? IOW: There is too much code under #ifdef CONFIG_IPIPE. - It seems we do not respect masking done by Linux, rather unconditionally release (AKA unmask) the line. The latter was right in 2.6.38 but apparently got broken during to port from 3.2 to 3.4. Jan -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 263 bytes Desc: OpenPGP digital signature URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130327/d590dc7c/attachment.pgp> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xenomai] one-shot fasteoi irqs 2013-03-27 6:50 ` Jan Kiszka @ 2013-03-27 9:23 ` Gilles Chanteperdrix 2013-03-27 9:30 ` Jan Kiszka 0 siblings, 1 reply; 14+ messages in thread From: Gilles Chanteperdrix @ 2013-03-27 9:23 UTC (permalink / raw) To: Jan Kiszka; +Cc: Xenomai On 03/27/2013 07:50 AM, Jan Kiszka wrote: > On 2013-03-27 00:05, Gilles Chanteperdrix wrote: >> >> Hi Jan, >> >> I seem to recall you sent patches to fix threaded irqs some time ago. I > > In fact, that was Wolfgang with commits fdda86bca9 and 54d161b85b for > 2.6.38 back then. > >> am having a problem here without SMP, whereas the system works with >> SMP. A fasteoi irq handler triggers repeatedly while its threaded >> counterpart never gets triggered. I believe this is because >> handle_fasteoi_irq unconditionally releases the irq line. > > You mean it doesn't respect the conditions in cond_unmask_irq? > >> The following >> patch seems to fix the issue though I would prefer a cleaner solution. >> >> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c >> index 11e75d1..2ff8d3a 100644 >> --- a/kernel/irq/chip.c >> +++ b/kernel/irq/chip.c >> @@ -463,8 +463,11 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc) >> >> #ifdef CONFIG_IPIPE >> /* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */ >> - if (desc->irq_data.chip->irq_release) >> - desc->irq_data.chip->irq_release(&desc->irq_data); >> + if (desc->irq_data.chip->irq_release) { >> + if ((!(desc->istate & IRQS_ONESHOT) || >> + !desc->threads_oneshot)) >> + desc->irq_data.chip->irq_release(&desc->irq_data); >> + } >> out_eoi: >> #else /* !CONFIG_IPIPE */ >> if (desc->istate & IRQS_ONESHOT) >> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c >> index e49a288..485c2c4 100644 >> --- a/kernel/irq/manage.c >> +++ b/kernel/irq/manage.c >> @@ -715,9 +715,15 @@ again: >> >> desc->threads_oneshot &= ~action->thread_mask; >> >> - if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) && >> - irqd_irq_masked(&desc->irq_data)) >> - unmask_irq(desc); >> + if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data)) { >> +#ifdef CONFIG_IPIPE >> + if (desc->irq_data.chip->irq_release) >> + desc->irq_data.chip->irq_release(&desc->irq_data); >> + else >> +#endif >> + if (irqd_irq_masked(&desc->irq_data)) >> + unmask_irq(desc); >> + } >> >> out_unlock: >> raw_spin_unlock_irq(&desc->lock); >> >> > > So this basically extends I-pipe's held phase of the IRQ to the threaded > handler if we are working in oneshot mode, right? > > I will have to dig into this again, but without remembering all details, > there are some things that do not look OK: > - Shouldn't we just replace unmask_irq with irq_release for the I-pipe > case? IOW: There is too much code under #ifdef CONFIG_IPIPE. No, unmask_irq still has to use ->irq_unmask, while the masking done by the I-pipe has to use ->irq_release. > - It seems we do not respect masking done by Linux, rather > unconditionally release (AKA unmask) the line. You mean masking which could be done by the interrupt handler? > > The latter was right in 2.6.38 but apparently got broken during to port > from 3.2 to 3.4. fasteoi irqs were reworked with the introduction of ->irq_hold and ->irq_release, because the 2.6.38 was found not to work either... -- Gilles. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xenomai] one-shot fasteoi irqs 2013-03-27 9:23 ` Gilles Chanteperdrix @ 2013-03-27 9:30 ` Jan Kiszka 2013-03-27 12:55 ` Gilles Chanteperdrix 0 siblings, 1 reply; 14+ messages in thread From: Jan Kiszka @ 2013-03-27 9:30 UTC (permalink / raw) To: Gilles Chanteperdrix; +Cc: Xenomai On 2013-03-27 10:23, Gilles Chanteperdrix wrote: > On 03/27/2013 07:50 AM, Jan Kiszka wrote: > >> On 2013-03-27 00:05, Gilles Chanteperdrix wrote: >>> >>> Hi Jan, >>> >>> I seem to recall you sent patches to fix threaded irqs some time ago. I >> >> In fact, that was Wolfgang with commits fdda86bca9 and 54d161b85b for >> 2.6.38 back then. >> >>> am having a problem here without SMP, whereas the system works with >>> SMP. A fasteoi irq handler triggers repeatedly while its threaded >>> counterpart never gets triggered. I believe this is because >>> handle_fasteoi_irq unconditionally releases the irq line. >> >> You mean it doesn't respect the conditions in cond_unmask_irq? >> >>> The following >>> patch seems to fix the issue though I would prefer a cleaner solution. >>> >>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c >>> index 11e75d1..2ff8d3a 100644 >>> --- a/kernel/irq/chip.c >>> +++ b/kernel/irq/chip.c >>> @@ -463,8 +463,11 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc) >>> >>> #ifdef CONFIG_IPIPE >>> /* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */ >>> - if (desc->irq_data.chip->irq_release) >>> - desc->irq_data.chip->irq_release(&desc->irq_data); >>> + if (desc->irq_data.chip->irq_release) { >>> + if ((!(desc->istate & IRQS_ONESHOT) || >>> + !desc->threads_oneshot)) >>> + desc->irq_data.chip->irq_release(&desc->irq_data); >>> + } >>> out_eoi: >>> #else /* !CONFIG_IPIPE */ >>> if (desc->istate & IRQS_ONESHOT) >>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c >>> index e49a288..485c2c4 100644 >>> --- a/kernel/irq/manage.c >>> +++ b/kernel/irq/manage.c >>> @@ -715,9 +715,15 @@ again: >>> >>> desc->threads_oneshot &= ~action->thread_mask; >>> >>> - if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) && >>> - irqd_irq_masked(&desc->irq_data)) >>> - unmask_irq(desc); >>> + if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data)) { >>> +#ifdef CONFIG_IPIPE >>> + if (desc->irq_data.chip->irq_release) >>> + desc->irq_data.chip->irq_release(&desc->irq_data); >>> + else >>> +#endif >>> + if (irqd_irq_masked(&desc->irq_data)) >>> + unmask_irq(desc); >>> + } >>> >>> out_unlock: >>> raw_spin_unlock_irq(&desc->lock); >>> >>> >> >> So this basically extends I-pipe's held phase of the IRQ to the threaded >> handler if we are working in oneshot mode, right? >> >> I will have to dig into this again, but without remembering all details, >> there are some things that do not look OK: >> - Shouldn't we just replace unmask_irq with irq_release for the I-pipe >> case? IOW: There is too much code under #ifdef CONFIG_IPIPE. > > > No, unmask_irq still has to use ->irq_unmask, while the masking done by > the I-pipe has to use ->irq_release. No, I mean that unmask should be replaced by release, but we must not apply different conditions on the unmask, at least the logical one. If Linux wants the line to remain masked, we have to respect this. That is currently (also) broken after the last refactoring. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xenomai] one-shot fasteoi irqs 2013-03-27 9:30 ` Jan Kiszka @ 2013-03-27 12:55 ` Gilles Chanteperdrix 2013-03-27 13:30 ` Jan Kiszka 0 siblings, 1 reply; 14+ messages in thread From: Gilles Chanteperdrix @ 2013-03-27 12:55 UTC (permalink / raw) To: Jan Kiszka; +Cc: Xenomai On 03/27/2013 10:30 AM, Jan Kiszka wrote: > On 2013-03-27 10:23, Gilles Chanteperdrix wrote: >> On 03/27/2013 07:50 AM, Jan Kiszka wrote: >> >>> On 2013-03-27 00:05, Gilles Chanteperdrix wrote: >>>> >>>> Hi Jan, >>>> >>>> I seem to recall you sent patches to fix threaded irqs some time ago. I >>> >>> In fact, that was Wolfgang with commits fdda86bca9 and 54d161b85b for >>> 2.6.38 back then. >>> >>>> am having a problem here without SMP, whereas the system works with >>>> SMP. A fasteoi irq handler triggers repeatedly while its threaded >>>> counterpart never gets triggered. I believe this is because >>>> handle_fasteoi_irq unconditionally releases the irq line. >>> >>> You mean it doesn't respect the conditions in cond_unmask_irq? >>> >>>> The following >>>> patch seems to fix the issue though I would prefer a cleaner solution. >>>> >>>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c >>>> index 11e75d1..2ff8d3a 100644 >>>> --- a/kernel/irq/chip.c >>>> +++ b/kernel/irq/chip.c >>>> @@ -463,8 +463,11 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc) >>>> >>>> #ifdef CONFIG_IPIPE >>>> /* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */ >>>> - if (desc->irq_data.chip->irq_release) >>>> - desc->irq_data.chip->irq_release(&desc->irq_data); >>>> + if (desc->irq_data.chip->irq_release) { >>>> + if ((!(desc->istate & IRQS_ONESHOT) || >>>> + !desc->threads_oneshot)) >>>> + desc->irq_data.chip->irq_release(&desc->irq_data); >>>> + } >>>> out_eoi: >>>> #else /* !CONFIG_IPIPE */ >>>> if (desc->istate & IRQS_ONESHOT) >>>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c >>>> index e49a288..485c2c4 100644 >>>> --- a/kernel/irq/manage.c >>>> +++ b/kernel/irq/manage.c >>>> @@ -715,9 +715,15 @@ again: >>>> >>>> desc->threads_oneshot &= ~action->thread_mask; >>>> >>>> - if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) && >>>> - irqd_irq_masked(&desc->irq_data)) >>>> - unmask_irq(desc); >>>> + if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data)) { >>>> +#ifdef CONFIG_IPIPE >>>> + if (desc->irq_data.chip->irq_release) >>>> + desc->irq_data.chip->irq_release(&desc->irq_data); >>>> + else >>>> +#endif >>>> + if (irqd_irq_masked(&desc->irq_data)) >>>> + unmask_irq(desc); >>>> + } >>>> >>>> out_unlock: >>>> raw_spin_unlock_irq(&desc->lock); >>>> >>>> >>> >>> So this basically extends I-pipe's held phase of the IRQ to the threaded >>> handler if we are working in oneshot mode, right? >>> >>> I will have to dig into this again, but without remembering all details, >>> there are some things that do not look OK: >>> - Shouldn't we just replace unmask_irq with irq_release for the I-pipe >>> case? IOW: There is too much code under #ifdef CONFIG_IPIPE. >> >> >> No, unmask_irq still has to use ->irq_unmask, while the masking done by >> the I-pipe has to use ->irq_release. > > No, I mean that unmask should be replaced by release, but we must not > apply different conditions on the unmask, at least the logical one. If > Linux wants the line to remain masked, we have to respect this. That is > currently (also) broken after the last refactoring. Ok, how about this one: iff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index 11e75d1..47d1d27 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -256,13 +256,19 @@ void mask_irq(struct irq_desc *desc) void unmask_irq(struct irq_desc *desc) { unsigned long flags; - + + flags = hard_cond_local_irq_save(); +#ifdef CONFIG_IPIPE + if (desc->irq_data.chip->irq_release) { + desc->irq_data.chip->irq_release(&desc->irq_data); + irq_state_clr_masked(desc); + } else +#endif /* CONFIG_IPIPE */ if (desc->irq_data.chip->irq_unmask) { - flags = hard_cond_local_irq_save(); desc->irq_data.chip->irq_unmask(&desc->irq_data); irq_state_clr_masked(desc); - hard_cond_local_irq_restore(flags); } + hard_cond_local_irq_restore(flags); } /* @@ -463,8 +469,7 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc) #ifdef CONFIG_IPIPE /* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */ - if (desc->irq_data.chip->irq_release) - desc->irq_data.chip->irq_release(&desc->irq_data); + cond_unmask_irq(desc); out_eoi: #else /* !CONFIG_IPIPE */ if (desc->istate & IRQS_ONESHOT) @@ -682,12 +687,15 @@ void __ipipe_end_level_irq(unsigned irq, struct irq_desc *desc) void __ipipe_ack_fasteoi_irq(unsigned irq, struct irq_desc *desc) { desc->irq_data.chip->irq_hold(&desc->irq_data); + irq_state_set_masked(desc); } void __ipipe_end_fasteoi_irq(unsigned irq, struct irq_desc *desc) { - if (desc->irq_data.chip->irq_release) + if (desc->irq_data.chip->irq_release) { + irq_state_clr_masked(desc); desc->irq_data.chip->irq_release(&desc->irq_data); + } } void __ipipe_ack_edge_irq(unsigned irq, struct irq_desc *desc) -- Gilles. ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Xenomai] one-shot fasteoi irqs 2013-03-27 12:55 ` Gilles Chanteperdrix @ 2013-03-27 13:30 ` Jan Kiszka 2013-03-27 20:50 ` Gilles Chanteperdrix 2013-04-02 21:44 ` Gilles Chanteperdrix 0 siblings, 2 replies; 14+ messages in thread From: Jan Kiszka @ 2013-03-27 13:30 UTC (permalink / raw) To: Gilles Chanteperdrix; +Cc: Xenomai On 2013-03-27 13:55, Gilles Chanteperdrix wrote: > Ok, how about this one: > > iff --git a/kernel/irq/chip.c b/kernel/irq/chip.c > index 11e75d1..47d1d27 100644 > --- a/kernel/irq/chip.c > +++ b/kernel/irq/chip.c > @@ -256,13 +256,19 @@ void mask_irq(struct irq_desc *desc) > void unmask_irq(struct irq_desc *desc) > { > unsigned long flags; > - > + > + flags = hard_cond_local_irq_save(); > +#ifdef CONFIG_IPIPE > + if (desc->irq_data.chip->irq_release) { > + desc->irq_data.chip->irq_release(&desc->irq_data); > + irq_state_clr_masked(desc); Are you sure that every call to unmask_irq implies (under I-pipe) that the IRQ was held before so that release_irq is appropriate here? Or did you rather want to patch cond_unmask_irq this way? But that has unfortunately also more users than our fast_eoi path. > + } else > +#endif /* CONFIG_IPIPE */ > if (desc->irq_data.chip->irq_unmask) { > - flags = hard_cond_local_irq_save(); > desc->irq_data.chip->irq_unmask(&desc->irq_data); > irq_state_clr_masked(desc); > - hard_cond_local_irq_restore(flags); > } > + hard_cond_local_irq_restore(flags); > } > > /* > @@ -463,8 +469,7 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc) > > #ifdef CONFIG_IPIPE > /* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */ > - if (desc->irq_data.chip->irq_release) > - desc->irq_data.chip->irq_release(&desc->irq_data); > + cond_unmask_irq(desc); I'm wondering now why we need this differently for the I-pipe case. Let's revisit what happens with a fasteoi normally: - it's masked only if it's a oneshot IRQ before calling the flow handler - it's unmasked after the flow handling if the thread was not woken up With I-pipe we already enter handle_fasteoi_irq with the IRQ masked. The conditions and spots to unmask are: - from handle_fasteoi_irq if the thread wasn't woken up or we have non-threaded or non-oneshot handling - otherwise on end_irq from the handler thread Do you think this is correct? If so, I do not think it matches this patch yet. Jan > out_eoi: > #else /* !CONFIG_IPIPE */ > if (desc->istate & IRQS_ONESHOT) > @@ -682,12 +687,15 @@ void __ipipe_end_level_irq(unsigned irq, struct irq_desc *desc) > void __ipipe_ack_fasteoi_irq(unsigned irq, struct irq_desc *desc) > { > desc->irq_data.chip->irq_hold(&desc->irq_data); > + irq_state_set_masked(desc); > } > > void __ipipe_end_fasteoi_irq(unsigned irq, struct irq_desc *desc) > { > - if (desc->irq_data.chip->irq_release) > + if (desc->irq_data.chip->irq_release) { > + irq_state_clr_masked(desc); > desc->irq_data.chip->irq_release(&desc->irq_data); > + } > } > > void __ipipe_ack_edge_irq(unsigned irq, struct irq_desc *desc) > > > -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xenomai] one-shot fasteoi irqs 2013-03-27 13:30 ` Jan Kiszka @ 2013-03-27 20:50 ` Gilles Chanteperdrix 2013-04-02 21:44 ` Gilles Chanteperdrix 1 sibling, 0 replies; 14+ messages in thread From: Gilles Chanteperdrix @ 2013-03-27 20:50 UTC (permalink / raw) To: Jan Kiszka; +Cc: Xenomai On 03/27/2013 02:30 PM, Jan Kiszka wrote: > On 2013-03-27 13:55, Gilles Chanteperdrix wrote: >> Ok, how about this one: >> >> iff --git a/kernel/irq/chip.c b/kernel/irq/chip.c >> index 11e75d1..47d1d27 100644 >> --- a/kernel/irq/chip.c >> +++ b/kernel/irq/chip.c >> @@ -256,13 +256,19 @@ void mask_irq(struct irq_desc *desc) >> void unmask_irq(struct irq_desc *desc) >> { >> unsigned long flags; >> - >> + >> + flags = hard_cond_local_irq_save(); >> +#ifdef CONFIG_IPIPE >> + if (desc->irq_data.chip->irq_release) { >> + desc->irq_data.chip->irq_release(&desc->irq_data); >> + irq_state_clr_masked(desc); > > Are you sure that every call to unmask_irq implies (under I-pipe) that > the IRQ was held before so that release_irq is appropriate here? Yes, unmask_irq is always called after testing for irqd_irq_masked, so, since ipipe_ack_fasteoi_irq is modified for setting the corresponding bit, and ipipe_end_fasteoi_irq clearing it, if the bit is set, the irq can be unmasked. > Or did > you rather want to patch cond_unmask_irq this way? But that has > unfortunately also more users than our fast_eoi path. No, the reason unmask_irq has to be changed that way is because irq_finalize_oneshot, which does not know whether it is dealing with a fasteoi, level, edge, irq, calls unmask_irq unconditionnaly, so, if we want irq_release to be called from that point for fasteoi irqs, we have to change unmask_irq. Alternatively, we could call ->ipipe_end with a #ifdef CONFIG_IPIPE in irq_finalize_oneshot.. > >> + } else >> +#endif /* CONFIG_IPIPE */ >> if (desc->irq_data.chip->irq_unmask) { >> - flags = hard_cond_local_irq_save(); >> desc->irq_data.chip->irq_unmask(&desc->irq_data); >> irq_state_clr_masked(desc); >> - hard_cond_local_irq_restore(flags); >> } >> + hard_cond_local_irq_restore(flags); >> } >> >> /* >> @@ -463,8 +469,7 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc) >> >> #ifdef CONFIG_IPIPE >> /* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */ >> - if (desc->irq_data.chip->irq_release) >> - desc->irq_data.chip->irq_release(&desc->irq_data); >> + cond_unmask_irq(desc); > > I'm wondering now why we need this differently for the I-pipe case. In the non I-pipe case, we have: if (desc->istate & IRQS_ONESHOT) cond_unmask_irq(desc); Which is wrong with I-pipe because we want the irq unmasked even for irqs which are not one-host. Then: desc->irq_data.chip->irq_eoi(&desc->irq_data); Which is wrong because the EOI was already sent as part of the irq_hold callback. > > Let's revisit what happens with a fasteoi normally: > > - it's masked only if it's a oneshot IRQ before calling the flow handler > - it's unmasked after the flow handling if the thread was not woken up > > With I-pipe we already enter handle_fasteoi_irq with the IRQ masked. The > conditions and spots to unmask are: > - from handle_fasteoi_irq if the thread wasn't woken up or we have > non-threaded or non-oneshot handling That is what the unconditional call to cond_unmask_irq does. > - otherwise on end_irq from the handler thread That is what the call to unmask_irq at the end of irq_finalize_oneshot does. > > Do you think this is correct? If so, I do not think it matches this > patch yet. Seems to me that it does (and indeed the patched kernel works for me, I tested before sending the patch). -- Gilles. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xenomai] one-shot fasteoi irqs 2013-03-27 13:30 ` Jan Kiszka 2013-03-27 20:50 ` Gilles Chanteperdrix @ 2013-04-02 21:44 ` Gilles Chanteperdrix 2013-04-06 9:43 ` Jan Kiszka 1 sibling, 1 reply; 14+ messages in thread From: Gilles Chanteperdrix @ 2013-04-02 21:44 UTC (permalink / raw) To: Jan Kiszka; +Cc: Xenomai On 03/27/2013 02:30 PM, Jan Kiszka wrote: > I'm wondering now why we need this differently for the I-pipe case. > > Let's revisit what happens with a fasteoi normally: > > - it's masked only if it's a oneshot IRQ before calling the flow handler > - it's unmasked after the flow handling if the thread was not woken up > > With I-pipe we already enter handle_fasteoi_irq with the IRQ masked. The > conditions and spots to unmask are: > - from handle_fasteoi_irq if the thread wasn't woken up or we have > non-threaded or non-oneshot handling > - otherwise on end_irq from the handler thread > > Do you think this is correct? If so, I do not think it matches this > patch yet. Hi, here is a much simpler patch: diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index 11e75d1..a1c9918 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -421,6 +421,13 @@ static inline void preflow_handler(struct irq_desc *desc) static inline void preflow_handler(struct irq_desc *desc) { } #endif +static void cond_release_fasteoi_irq(struct irq_desc *desc) +{ + if (desc->irq_data.chip->irq_release && + !irqd_irq_disabled(&desc->irq_data) && !desc->threads_oneshot) + desc->irq_data.chip->irq_release(&desc->irq_data); +} + /** * handle_fasteoi_irq - irq handler for transparent controllers * @irq: the interrupt number @@ -463,8 +470,7 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc) #ifdef CONFIG_IPIPE /* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */ - if (desc->irq_data.chip->irq_release) - desc->irq_data.chip->irq_release(&desc->irq_data); + cond_release_fasteoi_irq(desc); out_eoi: #else /* !CONFIG_IPIPE */ if (desc->istate & IRQS_ONESHOT) diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index e49a288..e7ae8bd 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -715,9 +715,14 @@ again: desc->threads_oneshot &= ~action->thread_mask; +#ifndef CONFIG_IPIPE if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) && irqd_irq_masked(&desc->irq_data)) unmask_irq(desc); +#else /* CONFIG_IPIPE */ + if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data)) + desc->ipipe_end(desc->irq_data.irq, desc); +#endif /* CONFIG_IPIPE */ out_unlock: raw_spin_unlock_irq(&desc->lock); -- Gilles. ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Xenomai] one-shot fasteoi irqs 2013-04-02 21:44 ` Gilles Chanteperdrix @ 2013-04-06 9:43 ` Jan Kiszka 2013-04-08 21:09 ` Gilles Chanteperdrix 0 siblings, 1 reply; 14+ messages in thread From: Jan Kiszka @ 2013-04-06 9:43 UTC (permalink / raw) To: Gilles Chanteperdrix; +Cc: Xenomai On 2013-04-02 23:44, Gilles Chanteperdrix wrote: > On 03/27/2013 02:30 PM, Jan Kiszka wrote: > >> I'm wondering now why we need this differently for the I-pipe case. >> >> Let's revisit what happens with a fasteoi normally: >> >> - it's masked only if it's a oneshot IRQ before calling the flow handler >> - it's unmasked after the flow handling if the thread was not woken up >> >> With I-pipe we already enter handle_fasteoi_irq with the IRQ masked. The >> conditions and spots to unmask are: >> - from handle_fasteoi_irq if the thread wasn't woken up or we have >> non-threaded or non-oneshot handling >> - otherwise on end_irq from the handler thread >> >> Do you think this is correct? If so, I do not think it matches this >> patch yet. > > > Hi, > > here is a much simpler patch: > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c > index 11e75d1..a1c9918 100644 > --- a/kernel/irq/chip.c > +++ b/kernel/irq/chip.c > @@ -421,6 +421,13 @@ static inline void preflow_handler(struct irq_desc *desc) > static inline void preflow_handler(struct irq_desc *desc) { } > #endif > > +static void cond_release_fasteoi_irq(struct irq_desc *desc) > +{ > + if (desc->irq_data.chip->irq_release && > + !irqd_irq_disabled(&desc->irq_data) && !desc->threads_oneshot) > + desc->irq_data.chip->irq_release(&desc->irq_data); > +} > + > /** > * handle_fasteoi_irq - irq handler for transparent controllers > * @irq: the interrupt number > @@ -463,8 +470,7 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc) > > #ifdef CONFIG_IPIPE > /* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */ Makes me wonder what this comment wants to tell us. That there is an unhandled corner case or that this is intentionally ignored? I support the latter as I-pipe already does EOI when accepting the IRQ, no? Maybe you can clarify that line at this chance. > - if (desc->irq_data.chip->irq_release) > - desc->irq_data.chip->irq_release(&desc->irq_data); > + cond_release_fasteoi_irq(desc); > out_eoi: > #else /* !CONFIG_IPIPE */ > if (desc->istate & IRQS_ONESHOT) > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index e49a288..e7ae8bd 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -715,9 +715,14 @@ again: > > desc->threads_oneshot &= ~action->thread_mask; > > +#ifndef CONFIG_IPIPE > if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) && > irqd_irq_masked(&desc->irq_data)) > unmask_irq(desc); > +#else /* CONFIG_IPIPE */ > + if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data)) > + desc->ipipe_end(desc->irq_data.irq, desc); > +#endif /* CONFIG_IPIPE */ > > out_unlock: > raw_spin_unlock_irq(&desc->lock); > > Looks good. I should dig out our test case again and check it on x86. I think it was PCI device assignment with KVM where the IRQ of the assigned device is handled by an IRQ thread of KVM. Jan -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 263 bytes Desc: OpenPGP digital signature URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130406/ef69ff66/attachment.pgp> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xenomai] one-shot fasteoi irqs 2013-04-06 9:43 ` Jan Kiszka @ 2013-04-08 21:09 ` Gilles Chanteperdrix 2013-04-10 9:32 ` Philippe Gerum 0 siblings, 1 reply; 14+ messages in thread From: Gilles Chanteperdrix @ 2013-04-08 21:09 UTC (permalink / raw) To: Jan Kiszka; +Cc: Xenomai On 04/06/2013 11:43 AM, Jan Kiszka wrote: > On 2013-04-02 23:44, Gilles Chanteperdrix wrote: >> On 03/27/2013 02:30 PM, Jan Kiszka wrote: >> >>> I'm wondering now why we need this differently for the I-pipe case. >>> >>> Let's revisit what happens with a fasteoi normally: >>> >>> - it's masked only if it's a oneshot IRQ before calling the flow handler >>> - it's unmasked after the flow handling if the thread was not woken up >>> >>> With I-pipe we already enter handle_fasteoi_irq with the IRQ masked. The >>> conditions and spots to unmask are: >>> - from handle_fasteoi_irq if the thread wasn't woken up or we have >>> non-threaded or non-oneshot handling >>> - otherwise on end_irq from the handler thread >>> >>> Do you think this is correct? If so, I do not think it matches this >>> patch yet. >> >> >> Hi, >> >> here is a much simpler patch: >> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c >> index 11e75d1..a1c9918 100644 >> --- a/kernel/irq/chip.c >> +++ b/kernel/irq/chip.c >> @@ -421,6 +421,13 @@ static inline void preflow_handler(struct irq_desc *desc) >> static inline void preflow_handler(struct irq_desc *desc) { } >> #endif >> >> +static void cond_release_fasteoi_irq(struct irq_desc *desc) >> +{ >> + if (desc->irq_data.chip->irq_release && >> + !irqd_irq_disabled(&desc->irq_data) && !desc->threads_oneshot) >> + desc->irq_data.chip->irq_release(&desc->irq_data); >> +} >> + >> /** >> * handle_fasteoi_irq - irq handler for transparent controllers >> * @irq: the interrupt number >> @@ -463,8 +470,7 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc) >> >> #ifdef CONFIG_IPIPE >> /* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */ > > Makes me wonder what this comment wants to tell us. That there is an > unhandled corner case or that this is intentionally ignored? I support > the latter as I-pipe already does EOI when accepting the IRQ, no? Maybe > you can clarify that line at this chance. I have no idea where this comment come from. I do not think this flag can be handled with the I-pipe kernel, as the EOI is sent before trying to handle the IRQ. The only users are in arch/sparc and arch/powerpc, maybe Philippe knows more? -- Gilles. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xenomai] one-shot fasteoi irqs 2013-04-08 21:09 ` Gilles Chanteperdrix @ 2013-04-10 9:32 ` Philippe Gerum 2013-04-10 10:36 ` Jan Kiszka 0 siblings, 1 reply; 14+ messages in thread From: Philippe Gerum @ 2013-04-10 9:32 UTC (permalink / raw) To: Gilles Chanteperdrix; +Cc: Jan Kiszka, Xenomai On 04/08/2013 11:09 PM, Gilles Chanteperdrix wrote: > On 04/06/2013 11:43 AM, Jan Kiszka wrote: > >> On 2013-04-02 23:44, Gilles Chanteperdrix wrote: >>> On 03/27/2013 02:30 PM, Jan Kiszka wrote: >>> >>>> I'm wondering now why we need this differently for the I-pipe case. >>>> >>>> Let's revisit what happens with a fasteoi normally: >>>> >>>> - it's masked only if it's a oneshot IRQ before calling the flow handler >>>> - it's unmasked after the flow handling if the thread was not woken up >>>> >>>> With I-pipe we already enter handle_fasteoi_irq with the IRQ masked. The >>>> conditions and spots to unmask are: >>>> - from handle_fasteoi_irq if the thread wasn't woken up or we have >>>> non-threaded or non-oneshot handling >>>> - otherwise on end_irq from the handler thread >>>> >>>> Do you think this is correct? If so, I do not think it matches this >>>> patch yet. >>> >>> >>> Hi, >>> >>> here is a much simpler patch: >>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c >>> index 11e75d1..a1c9918 100644 >>> --- a/kernel/irq/chip.c >>> +++ b/kernel/irq/chip.c >>> @@ -421,6 +421,13 @@ static inline void preflow_handler(struct irq_desc *desc) >>> static inline void preflow_handler(struct irq_desc *desc) { } >>> #endif >>> >>> +static void cond_release_fasteoi_irq(struct irq_desc *desc) >>> +{ >>> + if (desc->irq_data.chip->irq_release && >>> + !irqd_irq_disabled(&desc->irq_data) && !desc->threads_oneshot) >>> + desc->irq_data.chip->irq_release(&desc->irq_data); >>> +} >>> + >>> /** >>> * handle_fasteoi_irq - irq handler for transparent controllers >>> * @irq: the interrupt number >>> @@ -463,8 +470,7 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc) >>> >>> #ifdef CONFIG_IPIPE >>> /* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */ >> >> Makes me wonder what this comment wants to tell us. That there is an >> unhandled corner case or that this is intentionally ignored? I support >> the latter as I-pipe already does EOI when accepting the IRQ, no? Maybe >> you can clarify that line at this chance. > > > I have no idea where this comment come from. I do not think this flag > can be handled with the I-pipe kernel, as the EOI is sent before trying > to handle the IRQ. The only users are in arch/sparc and arch/powerpc, > maybe Philippe knows more? > Jan is right, we ignore EOI_IF_HANDLED and never send EOI from the regular flow handler, as a consequence of doing eoi+mask when holding the IRQ in the ipipe's flow handler. -- Philippe. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xenomai] one-shot fasteoi irqs 2013-04-10 9:32 ` Philippe Gerum @ 2013-04-10 10:36 ` Jan Kiszka 2013-04-10 19:36 ` Gilles Chanteperdrix 0 siblings, 1 reply; 14+ messages in thread From: Jan Kiszka @ 2013-04-10 10:36 UTC (permalink / raw) To: Philippe Gerum; +Cc: Xenomai On 2013-04-10 11:32, Philippe Gerum wrote: > On 04/08/2013 11:09 PM, Gilles Chanteperdrix wrote: >> On 04/06/2013 11:43 AM, Jan Kiszka wrote: >> >>> On 2013-04-02 23:44, Gilles Chanteperdrix wrote: >>>> On 03/27/2013 02:30 PM, Jan Kiszka wrote: >>>> >>>>> I'm wondering now why we need this differently for the I-pipe case. >>>>> >>>>> Let's revisit what happens with a fasteoi normally: >>>>> >>>>> - it's masked only if it's a oneshot IRQ before calling the flow >>>>> handler >>>>> - it's unmasked after the flow handling if the thread was not woken up >>>>> >>>>> With I-pipe we already enter handle_fasteoi_irq with the IRQ >>>>> masked. The >>>>> conditions and spots to unmask are: >>>>> - from handle_fasteoi_irq if the thread wasn't woken up or we have >>>>> non-threaded or non-oneshot handling >>>>> - otherwise on end_irq from the handler thread >>>>> >>>>> Do you think this is correct? If so, I do not think it matches this >>>>> patch yet. >>>> >>>> >>>> Hi, >>>> >>>> here is a much simpler patch: >>>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c >>>> index 11e75d1..a1c9918 100644 >>>> --- a/kernel/irq/chip.c >>>> +++ b/kernel/irq/chip.c >>>> @@ -421,6 +421,13 @@ static inline void preflow_handler(struct >>>> irq_desc *desc) >>>> static inline void preflow_handler(struct irq_desc *desc) { } >>>> #endif >>>> >>>> +static void cond_release_fasteoi_irq(struct irq_desc *desc) >>>> +{ >>>> + if (desc->irq_data.chip->irq_release && >>>> + !irqd_irq_disabled(&desc->irq_data) && !desc->threads_oneshot) >>>> + desc->irq_data.chip->irq_release(&desc->irq_data); >>>> +} >>>> + >>>> /** >>>> * handle_fasteoi_irq - irq handler for transparent controllers >>>> * @irq: the interrupt number >>>> @@ -463,8 +470,7 @@ handle_fasteoi_irq(unsigned int irq, struct >>>> irq_desc *desc) >>>> >>>> #ifdef CONFIG_IPIPE >>>> /* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */ >>> >>> Makes me wonder what this comment wants to tell us. That there is an >>> unhandled corner case or that this is intentionally ignored? I support >>> the latter as I-pipe already does EOI when accepting the IRQ, no? Maybe >>> you can clarify that line at this chance. >> >> >> I have no idea where this comment come from. I do not think this flag >> can be handled with the I-pipe kernel, as the EOI is sent before trying >> to handle the IRQ. The only users are in arch/sparc and arch/powerpc, >> maybe Philippe knows more? >> > > Jan is right, we ignore EOI_IF_HANDLED and never send EOI from the > regular flow handler, as a consequence of doing eoi+mask when holding > the IRQ in the ipipe's flow handler. OK, then I'll write a clarification patch for that comment - to get rid of that triple X... Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xenomai] one-shot fasteoi irqs 2013-04-10 10:36 ` Jan Kiszka @ 2013-04-10 19:36 ` Gilles Chanteperdrix 2013-04-11 7:35 ` Philippe Gerum 0 siblings, 1 reply; 14+ messages in thread From: Gilles Chanteperdrix @ 2013-04-10 19:36 UTC (permalink / raw) To: Jan Kiszka; +Cc: Xenomai On 04/10/2013 12:36 PM, Jan Kiszka wrote: > On 2013-04-10 11:32, Philippe Gerum wrote: >> On 04/08/2013 11:09 PM, Gilles Chanteperdrix wrote: >>> On 04/06/2013 11:43 AM, Jan Kiszka wrote: >>> >>>> On 2013-04-02 23:44, Gilles Chanteperdrix wrote: >>>>> On 03/27/2013 02:30 PM, Jan Kiszka wrote: >>>>> >>>>>> I'm wondering now why we need this differently for the I-pipe case. >>>>>> >>>>>> Let's revisit what happens with a fasteoi normally: >>>>>> >>>>>> - it's masked only if it's a oneshot IRQ before calling the flow >>>>>> handler >>>>>> - it's unmasked after the flow handling if the thread was not woken up >>>>>> >>>>>> With I-pipe we already enter handle_fasteoi_irq with the IRQ >>>>>> masked. The >>>>>> conditions and spots to unmask are: >>>>>> - from handle_fasteoi_irq if the thread wasn't woken up or we have >>>>>> non-threaded or non-oneshot handling >>>>>> - otherwise on end_irq from the handler thread >>>>>> >>>>>> Do you think this is correct? If so, I do not think it matches this >>>>>> patch yet. >>>>> >>>>> >>>>> Hi, >>>>> >>>>> here is a much simpler patch: >>>>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c >>>>> index 11e75d1..a1c9918 100644 >>>>> --- a/kernel/irq/chip.c >>>>> +++ b/kernel/irq/chip.c >>>>> @@ -421,6 +421,13 @@ static inline void preflow_handler(struct >>>>> irq_desc *desc) >>>>> static inline void preflow_handler(struct irq_desc *desc) { } >>>>> #endif >>>>> >>>>> +static void cond_release_fasteoi_irq(struct irq_desc *desc) >>>>> +{ >>>>> + if (desc->irq_data.chip->irq_release && >>>>> + !irqd_irq_disabled(&desc->irq_data) && !desc->threads_oneshot) >>>>> + desc->irq_data.chip->irq_release(&desc->irq_data); >>>>> +} >>>>> + >>>>> /** >>>>> * handle_fasteoi_irq - irq handler for transparent controllers >>>>> * @irq: the interrupt number >>>>> @@ -463,8 +470,7 @@ handle_fasteoi_irq(unsigned int irq, struct >>>>> irq_desc *desc) >>>>> >>>>> #ifdef CONFIG_IPIPE >>>>> /* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */ >>>> >>>> Makes me wonder what this comment wants to tell us. That there is an >>>> unhandled corner case or that this is intentionally ignored? I support >>>> the latter as I-pipe already does EOI when accepting the IRQ, no? Maybe >>>> you can clarify that line at this chance. >>> >>> >>> I have no idea where this comment come from. I do not think this flag >>> can be handled with the I-pipe kernel, as the EOI is sent before trying >>> to handle the IRQ. The only users are in arch/sparc and arch/powerpc, >>> maybe Philippe knows more? >>> >> >> Jan is right, we ignore EOI_IF_HANDLED and never send EOI from the >> regular flow handler, as a consequence of doing eoi+mask when holding >> the IRQ in the ipipe's flow handler. > > OK, then I'll write a clarification patch for that comment - to get rid > of that triple X... I understand the flag with a negative sense: do not eoi the interrupt if the handler reports that it has not handled it. Obviously, if that is what the flag means, we can not honour it when the I-pipe is on. > > Jan > -- Gilles. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xenomai] one-shot fasteoi irqs 2013-04-10 19:36 ` Gilles Chanteperdrix @ 2013-04-11 7:35 ` Philippe Gerum 0 siblings, 0 replies; 14+ messages in thread From: Philippe Gerum @ 2013-04-11 7:35 UTC (permalink / raw) To: Gilles Chanteperdrix; +Cc: Jan Kiszka, Xenomai On 04/10/2013 09:36 PM, Gilles Chanteperdrix wrote: > On 04/10/2013 12:36 PM, Jan Kiszka wrote: > >> On 2013-04-10 11:32, Philippe Gerum wrote: >>> On 04/08/2013 11:09 PM, Gilles Chanteperdrix wrote: >>>> On 04/06/2013 11:43 AM, Jan Kiszka wrote: >>>> >>>>> On 2013-04-02 23:44, Gilles Chanteperdrix wrote: >>>>>> On 03/27/2013 02:30 PM, Jan Kiszka wrote: >>>>>> >>>>>>> I'm wondering now why we need this differently for the I-pipe case. >>>>>>> >>>>>>> Let's revisit what happens with a fasteoi normally: >>>>>>> >>>>>>> - it's masked only if it's a oneshot IRQ before calling the flow >>>>>>> handler >>>>>>> - it's unmasked after the flow handling if the thread was not woken up >>>>>>> >>>>>>> With I-pipe we already enter handle_fasteoi_irq with the IRQ >>>>>>> masked. The >>>>>>> conditions and spots to unmask are: >>>>>>> - from handle_fasteoi_irq if the thread wasn't woken up or we have >>>>>>> non-threaded or non-oneshot handling >>>>>>> - otherwise on end_irq from the handler thread >>>>>>> >>>>>>> Do you think this is correct? If so, I do not think it matches this >>>>>>> patch yet. >>>>>> >>>>>> >>>>>> Hi, >>>>>> >>>>>> here is a much simpler patch: >>>>>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c >>>>>> index 11e75d1..a1c9918 100644 >>>>>> --- a/kernel/irq/chip.c >>>>>> +++ b/kernel/irq/chip.c >>>>>> @@ -421,6 +421,13 @@ static inline void preflow_handler(struct >>>>>> irq_desc *desc) >>>>>> static inline void preflow_handler(struct irq_desc *desc) { } >>>>>> #endif >>>>>> >>>>>> +static void cond_release_fasteoi_irq(struct irq_desc *desc) >>>>>> +{ >>>>>> + if (desc->irq_data.chip->irq_release && >>>>>> + !irqd_irq_disabled(&desc->irq_data) && !desc->threads_oneshot) >>>>>> + desc->irq_data.chip->irq_release(&desc->irq_data); >>>>>> +} >>>>>> + >>>>>> /** >>>>>> * handle_fasteoi_irq - irq handler for transparent controllers >>>>>> * @irq: the interrupt number >>>>>> @@ -463,8 +470,7 @@ handle_fasteoi_irq(unsigned int irq, struct >>>>>> irq_desc *desc) >>>>>> >>>>>> #ifdef CONFIG_IPIPE >>>>>> /* XXX: IRQCHIP_EOI_IF_HANDLED is ignored. */ >>>>> >>>>> Makes me wonder what this comment wants to tell us. That there is an >>>>> unhandled corner case or that this is intentionally ignored? I support >>>>> the latter as I-pipe already does EOI when accepting the IRQ, no? Maybe >>>>> you can clarify that line at this chance. >>>> >>>> >>>> I have no idea where this comment come from. I do not think this flag >>>> can be handled with the I-pipe kernel, as the EOI is sent before trying >>>> to handle the IRQ. The only users are in arch/sparc and arch/powerpc, >>>> maybe Philippe knows more? >>>> >>> >>> Jan is right, we ignore EOI_IF_HANDLED and never send EOI from the >>> regular flow handler, as a consequence of doing eoi+mask when holding >>> the IRQ in the ipipe's flow handler. >> >> OK, then I'll write a clarification patch for that comment - to get rid >> of that triple X... > > > I understand the flag with a negative sense: do not eoi the interrupt if > the handler reports that it has not handled it. Obviously, if that is > what the flag means, we can not honour it when the I-pipe is on. > Which is the same as saying that we shall ignore EOI_IF_HANDLED because it does not apply when interrupts are pipelined, yes. Hence the comment. -- Philippe. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2013-04-11 7:35 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-03-26 23:05 [Xenomai] one-shot fasteoi irqs Gilles Chanteperdrix 2013-03-27 6:50 ` Jan Kiszka 2013-03-27 9:23 ` Gilles Chanteperdrix 2013-03-27 9:30 ` Jan Kiszka 2013-03-27 12:55 ` Gilles Chanteperdrix 2013-03-27 13:30 ` Jan Kiszka 2013-03-27 20:50 ` Gilles Chanteperdrix 2013-04-02 21:44 ` Gilles Chanteperdrix 2013-04-06 9:43 ` Jan Kiszka 2013-04-08 21:09 ` Gilles Chanteperdrix 2013-04-10 9:32 ` Philippe Gerum 2013-04-10 10:36 ` Jan Kiszka 2013-04-10 19:36 ` Gilles Chanteperdrix 2013-04-11 7:35 ` Philippe Gerum
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.