Re: [patch V2 12/19] s390/pci: Do not mask MSI[-X] entries on teardown

[Niklas Schnelle]

On Thu, 2021-07-29 at 23:51 +0200, Thomas Gleixner wrote:
> The PCI core already ensures that the MSI[-X] state is correct when MSI[-X]
> is disabled. For MSI the reset state is all entries unmasked and for MSI-X
> all vectors are masked.
> 
> S390 masks all MSI entries and masks the already masked MSI-X entries
> again. Remove it and let the device in the correct state.
> 
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: linux-s390@xxxxxxxxxxxxxxx
> Cc: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>
> Cc: Gerald Schaefer <gerald.schaefer@xxxxxxxxxxxxx>
> Cc: Heiko Carstens <hca@xxxxxxxxxxxxx>
> Cc: Christian Borntraeger <borntraeger@xxxxxxxxxx>
> ---
>  arch/s390/pci/pci_irq.c |    4 ----
>  drivers/pci/msi.c       |    4 ++--
>  include/linux/msi.h     |    2 --
>  3 files changed, 2 insertions(+), 8 deletions(-)
> 
> --- a/arch/s390/pci/pci_irq.c
> +++ b/arch/s390/pci/pci_irq.c
> @@ -365,10 +365,6 @@ void arch_teardown_msi_irqs(struct pci_d
>  	for_each_pci_msi_entry(msi, pdev) {
>  		if (!msi->irq)
>  			continue;
> -		if (msi->msi_attrib.is_msix)
> -			__pci_msix_desc_mask_irq(msi, 1);
> -		else
> -			__pci_msi_desc_mask_irq(msi, 1, 1);
>  		irq_set_msi_desc(msi->irq, NULL);
>  		irq_free_desc(msi->irq);
>  		msi->msg.address_lo = 0;
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -143,7 +143,7 @@ static inline __attribute_const__ u32 ms
>   * reliably as devices without an INTx disable bit will then generate a
>   * level IRQ which will never be cleared.
>   */
> -void __pci_msi_desc_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
> +static void __pci_msi_desc_mask_irq(struct msi_desc *desc, u32 mask, u32 flag)
>  {
>  	raw_spinlock_t *lock = &desc->dev->msi_lock;
>  	unsigned long flags;
> @@ -180,7 +180,7 @@ static void __iomem *pci_msix_desc_addr(
>   * file.  This saves a few milliseconds when initialising devices with lots
>   * of MSI-X interrupts.
>   */
> -u32 __pci_msix_desc_mask_irq(struct msi_desc *desc, u32 flag)
> +static u32 __pci_msix_desc_mask_irq(struct msi_desc *desc, u32 flag)
>  {
>  	u32 mask_bits = desc->masked;
>  	void __iomem *desc_addr;
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -232,8 +232,6 @@ void free_msi_entry(struct msi_desc *ent
>  void __pci_read_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
>  void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg);
>  
> -u32 __pci_msix_desc_mask_irq(struct msi_desc *desc, u32 flag);
> -void __pci_msi_desc_mask_irq(struct msi_desc *desc, u32 mask, u32 flag);
>  void pci_msi_mask_irq(struct irq_data *data);
>  void pci_msi_unmask_irq(struct irq_data *data);
>  
> 

I gave this patch a try, adapted for v5.14-rc4 where
__pci_msi_desc_msg() returns a u32, and all looks good. I tested with
our pretty quirky ISM devices too, which are the only MSI ones on
s390x.

It also makes sense to me to let the common code handle this so feel
free to add my:

Acked-by: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>

and/or

Tested-by: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>

Thanks.