2021-07-29 22:40:02

by Thomas Gleixner

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

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 <[email protected]>
Cc: [email protected]
Cc: Niklas Schnelle <[email protected]>
Cc: Gerald Schaefer <[email protected]>
Cc: Heiko Carstens <[email protected]>
Cc: Christian Borntraeger <[email protected]>
---
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);




2021-08-03 12:52:21

by Niklas Schnelle

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

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 <[email protected]>
> Cc: [email protected]
> Cc: Niklas Schnelle <[email protected]>
> Cc: Gerald Schaefer <[email protected]>
> Cc: Heiko Carstens <[email protected]>
> Cc: Christian Borntraeger <[email protected]>
> ---
> 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 <[email protected]>

and/or

Tested-by: Niklas Schnelle <[email protected]>

Thanks.



Subject: [tip: irq/core] s390/pci: Do not mask MSI[-X] entries on teardown

The following commit has been merged into the irq/core branch of tip:

Commit-ID: 3998527d2e3ee2bfdf710a45b7b90968ff87babc
Gitweb: https://git.kernel.org/tip/3998527d2e3ee2bfdf710a45b7b90968ff87babc
Author: Thomas Gleixner <[email protected]>
AuthorDate: Thu, 29 Jul 2021 23:51:51 +02:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Tue, 10 Aug 2021 11:03:29 +02:00

s390/pci: Do not mask MSI[-X] entries on teardown

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 <[email protected]>
Tested-by: Niklas Schnelle <[email protected]>
Tested-by: Marc Zyngier <[email protected]>
Reviewed-by: Marc Zyngier <[email protected]>
Acked-by: Niklas Schnelle <[email protected]>
Link: https://lore.kernel.org/r/[email protected]

---
arch/s390/pci/pci_irq.c | 4 ----
drivers/pci/msi.c | 4 ++--
include/linux/msi.h | 2 --
3 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
index 9c7de90..3823e15 100644
--- 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_dev *pdev)
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;
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index e5e7533..95e6ce4 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -143,7 +143,7 @@ static inline __attribute_const__ u32 msi_mask(unsigned x)
* 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(struct msi_desc *desc)
* 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;
diff --git a/include/linux/msi.h b/include/linux/msi.h
index e8bdcb8..3d0e747 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -232,8 +232,6 @@ void free_msi_entry(struct msi_desc *entry);
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);