On Sun, 18 Mar 2018, Fredrik Noring wrote:
> Hi Maciej and Thomas,
>
> Thomas: Please have a look at the first questions below, regarding
> irq_data->mask and irq_chip->irq_calc_mask. Are they supposed to be usable?
>
> > +static volatile unsigned long intc_mask = 0; /* FIXME: Why volatile? */
> > +
> > +static inline void intc_enable_irq(struct irq_data *data)
> > +{
> > + if (!(intc_mask & (1 << data->irq))) {
> > + intc_mask |= (1 << data->irq);
> > + outl(1 << data->irq, INTC_MASK);
> > + }
> > +}
>
> The intc_mask variable can be removed, since INTC_MASK is readable, although
> perhaps there are performance reasons to not read the register directly?
>
> I also noticed that struct irq_data contains a mask field, which allows
> simplifications to
>
> static inline void intc_enable_irq(struct irq_data *data)
> {
> if (!(inl(INTC_MASK) & data->mask))
> outl(data->mask, INTC_MASK);
That's a pointless exercise. The core code already knows whether an
interrupt is masked or not and makes the calls conditionally.
> }
>
> provided the following patch is applied to kernel/irq/irqdesc.c:
>
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -109,6 +109,7 @@ static void desc_set_defaults(unsigned int irq, struct irq_desc *desc, int node,
> desc->irq_common_data.msi_desc = NULL;
>
> desc->irq_data.common = &desc->irq_common_data;
> + desc->irq_data.mask = 1 << irq; /* FIXME: What about irq_calc_mask? */
>
> Perhaps the mask field ought to be assigned "1 << irq_data->hwirq" instead,
> unless irq_calc_mask is provided. The mask documentation is not entirely
> clear on the use and any restrictions, and it does not seem to be used all
> that much.
Neither works. @irq is the virtual Linux interrupt number and there is no
guarantee that it maps 1:1 to a hardware irq number. Also this falls apart
when @irq >= 32 because the mask field is 32bit....
> The mask field and irq_calc_mask were introduced by Thomas Gleixner in
> commits 966dc736b819 "genirq: Generic chip: Cache per irq bit mask" and
> d0051816e619 "genirq: irqchip: Add a mask calculation function" in 2013.
Yes, The generic irq chip uses this. I havent seen the full irqchip patch
so I can't tell whether this driver could/should use the generic chip.
Thanks,
tglx
