Anup Patel <apatel@xxxxxxxxxxxxxxxx> writes:
>> > +void imsic_vector_mask(struct imsic_vector *vec)
>> > +{
>> > + struct imsic_local_priv *lpriv;
>> > +
>> > + lpriv = per_cpu_ptr(imsic->lpriv, vec->cpu);
>> > + if (WARN_ON(&lpriv->vectors[vec->local_id] != vec))
>> > + return;
>> > +
>> > + /*
>> > + * This function is called through Linux irq subsystem with
>> > + * irqs disabled so no need to save/restore irq flags.
>> > + */
>> > +
>> > + raw_spin_lock(&lpriv->lock);
>> > +
>> > + vec->enable = false;
>> > + bitmap_set(lpriv->dirty_bitmap, vec->local_id, 1);
>> > + __imsic_remote_sync(lpriv, vec->cpu);
>> > +
>> > + raw_spin_unlock(&lpriv->lock);
>> > +}
>>
>> Really nice that you're using a timer for the vector affinity change,
>> and got rid of the special/weird IMSIC/sync IPI. Can you really use a
>> timer for mask/unmask? That makes the mask/unmask operation
>> asynchronous!
>>
>> That was what I was trying to get though with this comment:
>> https://lore.kernel.org/linux-riscv/87sf24mo1g.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
>>
>> Also, using the smp_* IPI functions, you can pass arguments, so you
>> don't need the dirty_bitmap tracking the changes.
>
> The mask/unmask operations are called with irqs disabled so if
> CPU X does synchronous IPI to another CPU Y from mask/unmask
> operation then while CPU X is waiting for IPI to complete it cannot
> receive IPI from other CPUs which can lead to crashes and stalls.
>
> In general, we should not do any block/busy-wait work in
> mask/unmask operation of an irqchip driver.
Hmm, OK. Still, a bit odd that when the .irq_mask callback return, the
masking is not actually completed.
1. CPU 0 tries to mask an interrupt tried to CPU 1.
2. The timer is queued on CPU 1.
3. The call irq_mask returns on CPU 0
4. ...the irq is masked at some future point, determined by the callback
at CPU 1
Is that the expected outcome?
There are .irq_mask implementation that does seem to go at length
(blocking) to perform the mask, e.g.: gic_mask_irq() which calls
gic_{re,}dist_wait_for_rwp that have sleep/retry loops. The GIC3 ITS
code has similar things going on.
I'm not saying you're wrong, I'm just trying to wrap my head around the
masking semantics.
> The AIA IMSIC spec allows setting ID pending bit using MSI write
> irrespective whether ID is enabled or not but the interrupt will be
> taken only after ID is enabled. In other words, there will be no
> loss of interrupt with delayed mask/unmask using async IPI or
> lazy timer.
No loss, but we might *get* an interrupt when we explicitly asked not to
get any. Maybe that's ok?
Björn
