Messages in this thread |  | | Date | Wed, 31 May 2017 17:18:38 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [patch 1/2] genirq: Handle NOAUTOEN interrupt setup proper |
| |
On Wed, 31 May 2017, Marc Zyngier wrote: > On 31/05/17 10:58, Thomas Gleixner wrote: > > #define __irqd_to_state(d) ACCESS_PRIVATE((d)->common, state_use_accessors) > > @@ -329,6 +330,11 @@ static inline void irqd_clr_activated(st > > __irqd_to_state(d) &= ~IRQD_ACTIVATED; > > } > > > > +static inline bool irqd_is_started_up(struct irq_data *d) > > nit: since we have set/clr_started, consider making this is_started > (without the up)? Or add the _up to clr/set?
Sure.
> > +{ > > + return __irqd_to_state(d) & IRQD_IRQ_STARTED_UP; > > +} > > + > > #undef __irqd_to_state > > > > static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) > > --- a/kernel/irq/chip.c > > +++ b/kernel/irq/chip.c > > @@ -185,37 +185,71 @@ static void irq_state_set_masked(struct > > irqd_set(&desc->irq_data, IRQD_IRQ_MASKED); > > } > > > > +static void irq_state_clr_started(struct irq_desc *desc) > > +{ > > + irqd_clear(&desc->irq_data, IRQD_IRQ_STARTED_UP); > > +} > > + > > +static void irq_state_set_started(struct irq_desc *desc) > > +{ > > + irqd_set(&desc->irq_data, IRQD_IRQ_STARTED_UP); > > +} > > + > > int irq_startup(struct irq_desc *desc, bool resend) > > { > > int ret = 0; > > > > - irq_state_clr_disabled(desc); > > + if (irqd_is_started_up(&desc->irq_data)) > > + return 0; > > + > > How about: > if (irqd_is_started_up(&desc->irq_data)) { > irq_enable(desc); > return 0;
Why would we do that? I rather have a WARN_ON() there as this should never happen.
> > + /* > > + * This must be called even if the interrupt was never started up, > > + * because the activation can happen before the interrupt is > > + * available for request/startup. It has it's own state tracking so > > + * it's safe to call it unconditonally. > > unconditionally?
Oops.
> > extern int irq_startup(struct irq_desc *desc, bool resend); > > extern void irq_shutdown(struct irq_desc *desc); > > +extern void irq_enable_or_startup(struct irq_desc *desc); > > extern void irq_enable(struct irq_desc *desc); > > extern void irq_disable(struct irq_desc *desc); > > extern void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu); > > --- a/kernel/irq/manage.c > > +++ b/kernel/irq/manage.c > > @@ -533,7 +533,7 @@ void __enable_irq(struct irq_desc *desc) > > goto err_out; > > /* Prevent probing on this irq: */ > > irq_settings_set_noprobe(desc); > > - irq_enable(desc); > > + irq_enable_or_startup(desc); > > and make this irq_startup(desc, false)?
Ah, now I see where you were heading with that irq_startup() modification. Makes sense, but let me think about it.
Thanks,
tglx
|  |