Re: [PATCH 1/1] genirq/msi: Dynamic remove/add stroage adapter hits EEH

[Thomas Gleixner]

On Wed, Mar 19 2025 at 07:14, wenxiong@xxxxxxxxxxxxx wrote:
> There is very small window: irqbalance daemon kicks in before ipr driver
> calls pci_restore_state(pdev), irqbalance daemon read back all 0 for that
> msix vector in __pci_read_msi_msg().

You fail to explain how irqbalanced is able to invoke __pci_read_msi_msg().

I assume that this happens in the set_affinity() path, which ends up in
pseries_msi_compose_msg(). pseries_msi_compose_msg() reads back the
message from the device.

> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index 396a067a8a56..fcde35efb64c 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -671,7 +671,8 @@ int msi_domain_set_affinity(struct irq_data *irq_data,
>  	if (ret >= 0 && ret != IRQ_SET_MASK_OK_DONE) {
>  		BUG_ON(irq_chip_compose_msi_msg(irq_data, msg));
>  		msi_check_level(irq_data->domain, msg);
> -		irq_chip_write_msi_msg(irq_data, msg);
> +		if ((msg->address_lo != 0) && (msg->address_hi != 0))
> +			irq_chip_write_msi_msg(irq_data, msg);

That's just papering over the underlying problem and thereby breaks all
architetures except your machine. address_hi _IS_ zero on most systems
and there is no requirement for address_lo to be non-zero when
address_hi is non-zero. Not going to happen.

The real problem has nothing to do with a remove/add operation. The
problem is solely in the probe function.

It does:

   pci_alloc_irq_vectors();     // Exposes interrupts in /proc/irq/
                                // which makes them visible to
                                // irqbalanced

   pci_save_state();

#A reset_device();              // Clears the config space

#B pci_restore_state();         // Restores the config space

So the driver creates inconsistent state between #A and #B, which is
wrong to begin with when it first sets up resources which are visible
to other entities.

Why are the interrupts set up _before_ the reset? That does not make any
sense.

And no, we are not going to do a random undocumented hack in generic
code to work around such a driver insanity.

Fix the problem where it was created, i.e. at the driver level.

Thanks,

        tglx