Re: [PATCH v1 12/14] xen/riscv: implement setup_irq()

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2573 bytes --]


On 4/15/25 5:55 PM, Jan Beulich wrote
>> +#ifdef CONFIG_IRQ_HAS_MULTIPLE_ACTION
>> +    new->next = desc->action;
>> +    smp_mb();
>> +#endif
>> +
>> +    desc->action = new;
>> +    smp_mb();
> Aren't smp_wmb() sufficient on both places? If not, I think comments
> want adding.

smp_wmb() will be sufficient but I think the barrier could be dropped at all
as __setup_irq() is called only in setup_irq() and __setup_irq() call is wrapped
by spinlock_{un}lock_irqsave() where spinlock_unlock_*() will put barrier.

>
>> +    return 0;
>> +}
>> +
>> +void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
>> +{
>> +    if ( desc != NULL )
> Can desc really be NULL here?

It can't as irq_desc[] isn't dynamically allocated.

>   Isn't desc->lock required to be held?

It is and it is called in setup_irq() which calls spin_lock_irqsave().
Anyway, I think it could be dropped at all and use 'desc->handler->set_affinity(desc, cpu_mask);'
explicitly in setup_irq().

>> +    spin_lock_irqsave(&desc->lock, flags);
>> +
>> +    disabled = (desc->action == NULL);
>> +
>> +    if ( test_bit(_IRQ_GUEST, &desc->status) )
>> +    {
>> +        spin_unlock_irqrestore(&desc->lock, flags);
>> +        /*
>> +         * TODO: would be nice to have functionality to print which domain owns
>> +         *       an IRQ.
>> +         */
>> +        printk(XENLOG_ERR "ERROR: IRQ %u is already in use by a domain\n", irq);
>> +        return -EBUSY;
>> +    }
>> +
>> +    rc = __setup_irq(desc, irqflags, new);
>> +    if ( rc )
>> +        goto err;
>> +
>> +    /* First time the IRQ is setup */
>> +    if ( disabled )
>> +    {
>> +        /* disable irq by default */
>> +        set_bit(_IRQ_DISABLED, &desc->status);
> Shouldn't this be set when we make it here?

It should be. I'll drop the setting of _IRQ_DISABLED.

>
>> +        /* route interrupt to xen */
>> +        intc_route_irq_to_xen(desc, IRQ_NO_PRIORITY);
>> +
>> +        /*
>> +         * we don't care for now which CPU will receive the
>> +         * interrupt
>> +         *
>> +         * TODO: Handle case where IRQ is setup on different CPU than
>> +         * the targeted CPU and the priority.
>> +         */
>> +        irq_set_affinity(desc, cpumask_of(smp_processor_id()));
>> +        desc->handler->startup(desc);
>> +        /* enable irq */
>> +        clear_bit(_IRQ_DISABLED, &desc->status);
> Now it turns out this is really done twice: Once in aplic_irq_enable(),
> and once here.

Agree, this is a job of *_startup()->*_aplic_irq_enable(). I'll drop that too.

~ Oleksii


[-- Attachment #2: Type: text/html, Size: 4051 bytes --]