[PATCH v3 0/3] PCI/MSI: Cleanup init and improve 32-bit MSI checking

[PATCH v3 0/3] PCI/MSI: Cleanup init and improve 32-bit MSI checking

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

MSI/MSI-X init was a little unconventional.  We had pci_msi_setup_pci_dev()
to disable MSI and MSI-X, in probe.c instead of msi.c so we could do it
even without CONFIG_PCI_MSI.  Move that to msi.c and fix the config issue
with an #ifdef.

Then add Vidya's patch on top.  Previous postings at

https://lore.kernel.org/linux-pci/20201117145728.4516-1-vidyas@nvidia.com/
https://lore.kernel.org/linux-pci/20201124105035.24573-1-vidyas@nvidia.com/

Bjorn Helgaas (2):
  PCI/MSI: Move MSI/MSI-X init to msi.c
  PCI/MSI: Move MSI/MSI-X flags updaters to msi.c

Vidya Sagar (1):
  PCI/MSI: Set device flag indicating only 32-bit MSI support

 drivers/pci/Makefile |  3 +-
 drivers/pci/msi.c    | 70 ++++++++++++++++++++++++++++++++++++++++----
 drivers/pci/pci.h    | 23 ++-------------
 drivers/pci/probe.c  | 21 ++-----------
 4 files changed, 70 insertions(+), 47 deletions(-)

-- 
2.25.1

[PATCH v3 1/3] PCI/MSI: Move MSI/MSI-X init to msi.c

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Move pci_msi_setup_pci_dev(), which disables MSI and MSI-X interrupts, from
probe.c to msi.c so it's with all the other MSI code and more consistent
with other capability initialization.  This means we must compile msi.c
always, even without CONFIG_PCI_MSI, so wrap the rest of msi.c in an #ifdef
and adjust the Makefile accordingly.  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/Makefile |  3 +--
 drivers/pci/msi.c    | 36 ++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h    |  2 ++
 drivers/pci/probe.c  | 21 ++-------------------
 4 files changed, 41 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 522d2b974e91..11cc79411e2d 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -5,7 +5,7 @@
 obj-$(CONFIG_PCI)		+= access.o bus.o probe.o host-bridge.o \
 				   remove.o pci.o pci-driver.o search.o \
 				   pci-sysfs.o rom.o setup-res.o irq.o vpd.o \
-				   setup-bus.o vc.o mmap.o setup-irq.o
+				   setup-bus.o vc.o mmap.o setup-irq.o msi.o
 
 obj-$(CONFIG_PCI)		+= pcie/
 
@@ -18,7 +18,6 @@ endif
 obj-$(CONFIG_OF)		+= of.o
 obj-$(CONFIG_PCI_QUIRKS)	+= quirks.o
 obj-$(CONFIG_HOTPLUG_PCI)	+= hotplug/
-obj-$(CONFIG_PCI_MSI)		+= msi.o
 obj-$(CONFIG_PCI_ATS)		+= ats.o
 obj-$(CONFIG_PCI_IOV)		+= iov.o
 obj-$(CONFIG_PCI_BRIDGE_EMUL)	+= pci-bridge-emul.o
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d52d118979a6..555791c0ee1a 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -26,6 +26,8 @@
 
 #include "pci.h"
 
+#ifdef CONFIG_MSI
+
 static int pci_msi_enable = 1;
 int pci_msi_ignore_mask;
 
@@ -1577,3 +1579,37 @@ bool pci_dev_has_special_msi_domain(struct pci_dev *pdev)
 }
 
 #endif /* CONFIG_PCI_MSI_IRQ_DOMAIN */
+#endif /* CONFIG_PCI_MSI */
+
+void pci_msi_init(struct pci_dev *dev)
+{
+	u16 ctrl;
+
+	/*
+	 * Disable the MSI hardware to avoid screaming interrupts
+	 * during boot.  This is the power on reset default so
+	 * usually this should be a noop.
+	 */
+	dev->msi_cap = pci_find_capability(dev, PCI_CAP_ID_MSI);
+	if (!dev->msi_cap)
+		return;
+
+	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &ctrl);
+	if (ctrl & PCI_MSI_FLAGS_ENABLE)
+		pci_write_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS,
+				      ctrl & ~PCI_MSI_FLAGS_ENABLE);
+}
+
+void pci_msix_init(struct pci_dev *dev)
+{
+	u16 ctrl;
+
+	dev->msix_cap = pci_find_capability(dev, PCI_CAP_ID_MSIX);
+	if (!dev->msix_cap)
+		return;
+
+	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &ctrl);
+	if (ctrl & PCI_MSIX_FLAGS_ENABLE)
+		pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS,
+				      ctrl & ~PCI_MSIX_FLAGS_ENABLE);
+}
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index f86cae9aa1f4..3f5f303775c4 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -104,6 +104,8 @@ void pci_config_pm_runtime_get(struct pci_dev *dev);
 void pci_config_pm_runtime_put(struct pci_dev *dev);
 void pci_pm_init(struct pci_dev *dev);
 void pci_ea_init(struct pci_dev *dev);
+void pci_msi_init(struct pci_dev *dev);
+void pci_msix_init(struct pci_dev *dev);
 void pci_allocate_cap_save_buffers(struct pci_dev *dev);
 void pci_free_cap_save_buffers(struct pci_dev *dev);
 bool pci_bridge_d3_possible(struct pci_dev *dev);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 4289030b0fff..50b480c016bf 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1716,22 +1716,6 @@ static u8 pci_hdr_type(struct pci_dev *dev)
 
 #define LEGACY_IO_RESOURCE	(IORESOURCE_IO | IORESOURCE_PCI_FIXED)
 
-static void pci_msi_setup_pci_dev(struct pci_dev *dev)
-{
-	/*
-	 * Disable the MSI hardware to avoid screaming interrupts
-	 * during boot.  This is the power on reset default so
-	 * usually this should be a noop.
-	 */
-	dev->msi_cap = pci_find_capability(dev, PCI_CAP_ID_MSI);
-	if (dev->msi_cap)
-		pci_msi_set_enable(dev, 0);
-
-	dev->msix_cap = pci_find_capability(dev, PCI_CAP_ID_MSIX);
-	if (dev->msix_cap)
-		pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
-}
-
 /**
  * pci_intx_mask_broken - Test PCI_COMMAND_INTX_DISABLE writability
  * @dev: PCI device
@@ -2397,9 +2381,8 @@ void pcie_report_downtraining(struct pci_dev *dev)
 static void pci_init_capabilities(struct pci_dev *dev)
 {
 	pci_ea_init(dev);		/* Enhanced Allocation */
-
-	/* Setup MSI caps & disable MSI/MSI-X interrupts */
-	pci_msi_setup_pci_dev(dev);
+	pci_msi_init(dev);		/* Disable MSI */
+	pci_msix_init(dev);		/* Disable MSI-X */
 
 	/* Buffers for saving PCIe and PCI-X capabilities */
 	pci_allocate_cap_save_buffers(dev);
-- 
2.25.1

Re: [PATCH v3 1/3] PCI/MSI: Move MSI/MSI-X init to msi.c

[Thierry Reding]

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

On Thu, Dec 03, 2020 at 12:51:08PM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Move pci_msi_setup_pci_dev(), which disables MSI and MSI-X interrupts, from
> probe.c to msi.c so it's with all the other MSI code and more consistent
> with other capability initialization.  This means we must compile msi.c
> always, even without CONFIG_PCI_MSI, so wrap the rest of msi.c in an #ifdef
> and adjust the Makefile accordingly.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/Makefile |  3 +--
>  drivers/pci/msi.c    | 36 ++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.h    |  2 ++
>  drivers/pci/probe.c  | 21 ++-------------------
>  4 files changed, 41 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 522d2b974e91..11cc79411e2d 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -5,7 +5,7 @@
>  obj-$(CONFIG_PCI)		+= access.o bus.o probe.o host-bridge.o \
>  				   remove.o pci.o pci-driver.o search.o \
>  				   pci-sysfs.o rom.o setup-res.o irq.o vpd.o \
> -				   setup-bus.o vc.o mmap.o setup-irq.o
> +				   setup-bus.o vc.o mmap.o setup-irq.o msi.o
>  
>  obj-$(CONFIG_PCI)		+= pcie/
>  
> @@ -18,7 +18,6 @@ endif
>  obj-$(CONFIG_OF)		+= of.o
>  obj-$(CONFIG_PCI_QUIRKS)	+= quirks.o
>  obj-$(CONFIG_HOTPLUG_PCI)	+= hotplug/
> -obj-$(CONFIG_PCI_MSI)		+= msi.o
>  obj-$(CONFIG_PCI_ATS)		+= ats.o
>  obj-$(CONFIG_PCI_IOV)		+= iov.o
>  obj-$(CONFIG_PCI_BRIDGE_EMUL)	+= pci-bridge-emul.o
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index d52d118979a6..555791c0ee1a 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -26,6 +26,8 @@
>  
>  #include "pci.h"
>  
> +#ifdef CONFIG_MSI
> +
>  static int pci_msi_enable = 1;
>  int pci_msi_ignore_mask;
>  
> @@ -1577,3 +1579,37 @@ bool pci_dev_has_special_msi_domain(struct pci_dev *pdev)
>  }
>  
>  #endif /* CONFIG_PCI_MSI_IRQ_DOMAIN */
> +#endif /* CONFIG_PCI_MSI */
> +
> +void pci_msi_init(struct pci_dev *dev)
> +{
> +	u16 ctrl;
> +
> +	/*
> +	 * Disable the MSI hardware to avoid screaming interrupts
> +	 * during boot.  This is the power on reset default so
> +	 * usually this should be a noop.
> +	 */
> +	dev->msi_cap = pci_find_capability(dev, PCI_CAP_ID_MSI);
> +	if (!dev->msi_cap)
> +		return;
> +
> +	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &ctrl);
> +	if (ctrl & PCI_MSI_FLAGS_ENABLE)
> +		pci_write_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS,
> +				      ctrl & ~PCI_MSI_FLAGS_ENABLE);
> +}

The old code used the pci_msi_set_enable() helper here...

> +
> +void pci_msix_init(struct pci_dev *dev)
> +{
> +	u16 ctrl;
> +
> +	dev->msix_cap = pci_find_capability(dev, PCI_CAP_ID_MSIX);
> +	if (!dev->msix_cap)
> +		return;
> +
> +	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &ctrl);
> +	if (ctrl & PCI_MSIX_FLAGS_ENABLE)
> +		pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS,
> +				      ctrl & ~PCI_MSIX_FLAGS_ENABLE);
> +}

... and pci_msix_clear_and_set_ctrl() here. I like your version here
better because it avoids the unnecessary write in case the flag isn't
set. But it got me thinking if perhaps the helpers aren't very useful
and perhaps should be dropped in favour of open-coded variants.
Especially since there seem to be only 4 and 6 occurrences of them after
this patch.

Anyway, this patch looks correct to me and is a nice improvement, so:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 1/3] PCI/MSI: Move MSI/MSI-X init to msi.c

[Bjorn Helgaas]

On Fri, Dec 04, 2020 at 12:05:15PM +0100, Thierry Reding wrote:
> On Thu, Dec 03, 2020 at 12:51:08PM -0600, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > Move pci_msi_setup_pci_dev(), which disables MSI and MSI-X interrupts, from
> > probe.c to msi.c so it's with all the other MSI code and more consistent
> > with other capability initialization.  This means we must compile msi.c
> > always, even without CONFIG_PCI_MSI, so wrap the rest of msi.c in an #ifdef
> > and adjust the Makefile accordingly.  No functional change intended.
> > 
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/Makefile |  3 +--
> >  drivers/pci/msi.c    | 36 ++++++++++++++++++++++++++++++++++++
> >  drivers/pci/pci.h    |  2 ++
> >  drivers/pci/probe.c  | 21 ++-------------------
> >  4 files changed, 41 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> > index 522d2b974e91..11cc79411e2d 100644
> > --- a/drivers/pci/Makefile
> > +++ b/drivers/pci/Makefile
> > @@ -5,7 +5,7 @@
> >  obj-$(CONFIG_PCI)		+= access.o bus.o probe.o host-bridge.o \
> >  				   remove.o pci.o pci-driver.o search.o \
> >  				   pci-sysfs.o rom.o setup-res.o irq.o vpd.o \
> > -				   setup-bus.o vc.o mmap.o setup-irq.o
> > +				   setup-bus.o vc.o mmap.o setup-irq.o msi.o
> >  
> >  obj-$(CONFIG_PCI)		+= pcie/
> >  
> > @@ -18,7 +18,6 @@ endif
> >  obj-$(CONFIG_OF)		+= of.o
> >  obj-$(CONFIG_PCI_QUIRKS)	+= quirks.o
> >  obj-$(CONFIG_HOTPLUG_PCI)	+= hotplug/
> > -obj-$(CONFIG_PCI_MSI)		+= msi.o
> >  obj-$(CONFIG_PCI_ATS)		+= ats.o
> >  obj-$(CONFIG_PCI_IOV)		+= iov.o
> >  obj-$(CONFIG_PCI_BRIDGE_EMUL)	+= pci-bridge-emul.o
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index d52d118979a6..555791c0ee1a 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -26,6 +26,8 @@
> >  
> >  #include "pci.h"
> >  
> > +#ifdef CONFIG_MSI
> > +
> >  static int pci_msi_enable = 1;
> >  int pci_msi_ignore_mask;
> >  
> > @@ -1577,3 +1579,37 @@ bool pci_dev_has_special_msi_domain(struct pci_dev *pdev)
> >  }
> >  
> >  #endif /* CONFIG_PCI_MSI_IRQ_DOMAIN */
> > +#endif /* CONFIG_PCI_MSI */
> > +
> > +void pci_msi_init(struct pci_dev *dev)
> > +{
> > +	u16 ctrl;
> > +
> > +	/*
> > +	 * Disable the MSI hardware to avoid screaming interrupts
> > +	 * during boot.  This is the power on reset default so
> > +	 * usually this should be a noop.
> > +	 */
> > +	dev->msi_cap = pci_find_capability(dev, PCI_CAP_ID_MSI);
> > +	if (!dev->msi_cap)
> > +		return;
> > +
> > +	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &ctrl);
> > +	if (ctrl & PCI_MSI_FLAGS_ENABLE)
> > +		pci_write_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS,
> > +				      ctrl & ~PCI_MSI_FLAGS_ENABLE);
> > +}
> 
> The old code used the pci_msi_set_enable() helper here...
> 
> > +
> > +void pci_msix_init(struct pci_dev *dev)
> > +{
> > +	u16 ctrl;
> > +
> > +	dev->msix_cap = pci_find_capability(dev, PCI_CAP_ID_MSIX);
> > +	if (!dev->msix_cap)
> > +		return;
> > +
> > +	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &ctrl);
> > +	if (ctrl & PCI_MSIX_FLAGS_ENABLE)
> > +		pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS,
> > +				      ctrl & ~PCI_MSIX_FLAGS_ENABLE);
> > +}
> 
> ... and pci_msix_clear_and_set_ctrl() here. I like your version here
> better because it avoids the unnecessary write in case the flag isn't
> set. But it got me thinking if perhaps the helpers aren't very useful
> and perhaps should be dropped in favour of open-coded variants.
> Especially since there seem to be only 4 and 6 occurrences of them after
> this patch.

I agree, they might be overkill.  I didn't want to spend that much
time on it, so I just left them for now.  Thanks for your review!

> Anyway, this patch looks correct to me and is a nice improvement, so:
> 
> Reviewed-by: Thierry Reding <treding@nvidia.com>

[PATCH v3 2/3] PCI/MSI: Move MSI/MSI-X flags updaters to msi.c

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

pci_msi_set_enable() and pci_msix_clear_and_set_ctrl() are only used from
msi.c, so move them from drivers/pci/pci.h to msi.c.  No functional change
intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/msi.c | 21 +++++++++++++++++++++
 drivers/pci/pci.h | 21 ---------------------
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 555791c0ee1a..3e302ca8a96f 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -412,6 +412,17 @@ static void pci_intx_for_msi(struct pci_dev *dev, int enable)
 		pci_intx(dev, enable);
 }
 
+static void pci_msi_set_enable(struct pci_dev *dev, int enable)
+{
+	u16 control;
+
+	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &control);
+	control &= ~PCI_MSI_FLAGS_ENABLE;
+	if (enable)
+		control |= PCI_MSI_FLAGS_ENABLE;
+	pci_write_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, control);
+}
+
 static void __pci_restore_msi_state(struct pci_dev *dev)
 {
 	u16 control;
@@ -434,6 +445,16 @@ static void __pci_restore_msi_state(struct pci_dev *dev)
 	pci_write_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, control);
 }
 
+static void pci_msix_clear_and_set_ctrl(struct pci_dev *dev, u16 clear, u16 set)
+{
+	u16 ctrl;
+
+	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &ctrl);
+	ctrl &= ~clear;
+	ctrl |= set;
+	pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, ctrl);
+}
+
 static void __pci_restore_msix_state(struct pci_dev *dev)
 {
 	struct msi_desc *entry;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 3f5f303775c4..5692f11bc146 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -187,27 +187,6 @@ void pci_no_msi(void);
 static inline void pci_no_msi(void) { }
 #endif
 
-static inline void pci_msi_set_enable(struct pci_dev *dev, int enable)
-{
-	u16 control;
-
-	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &control);
-	control &= ~PCI_MSI_FLAGS_ENABLE;
-	if (enable)
-		control |= PCI_MSI_FLAGS_ENABLE;
-	pci_write_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, control);
-}
-
-static inline void pci_msix_clear_and_set_ctrl(struct pci_dev *dev, u16 clear, u16 set)
-{
-	u16 ctrl;
-
-	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &ctrl);
-	ctrl &= ~clear;
-	ctrl |= set;
-	pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, ctrl);
-}
-
 void pci_realloc_get_opt(char *);
 
 static inline int pci_no_d1d2(struct pci_dev *dev)
-- 
2.25.1

Re: [PATCH v3 2/3] PCI/MSI: Move MSI/MSI-X flags updaters to msi.c

[Thierry Reding]

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

On Thu, Dec 03, 2020 at 12:51:09PM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> pci_msi_set_enable() and pci_msix_clear_and_set_ctrl() are only used from
> msi.c, so move them from drivers/pci/pci.h to msi.c.  No functional change
> intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/msi.c | 21 +++++++++++++++++++++
>  drivers/pci/pci.h | 21 ---------------------
>  2 files changed, 21 insertions(+), 21 deletions(-)

Ah... I suppose this cleans this up a little more. I have no objection
to these helpers, though I still think they are a bit unnecessary:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 3/3] PCI/MSI: Set device flag indicating only 32-bit MSI support

[Bjorn Helgaas]

From: Vidya Sagar <vidyas@nvidia.com>

The MSI-X Capability requires devices to support 64-bit Message Addresses,
but the MSI Capability can support either 32- or 64-bit addresses.

Previously, we set dev->no_64bit_msi for a few broken devices that
advertise 64-bit MSI support but don't correctly support it.

In addition, check the MSI "64-bit Address Capable" bit for all devices and
set dev->no_64bit_msi for devices that don't advertise 64-bit support.
This allows msi_verify_entries() to catch arch code defects that assign
64-bit addresses when they're not supported.

[bhelgaas: set no_64bit_msi in pci_msi_init(), commit log]
Link: https://lore.kernel.org/r/20201124105035.24573-1-vidyas@nvidia.com
Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/msi.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 3e302ca8a96f..29baa81e7be9 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -623,11 +623,11 @@ static int msi_verify_entries(struct pci_dev *dev)
 	struct msi_desc *entry;
 
 	for_each_pci_msi_entry(entry, dev) {
-		if (!dev->no_64bit_msi || !entry->msg.address_hi)
-			continue;
-		pci_err(dev, "Device has broken 64-bit MSI but arch"
-			" tried to assign one above 4G\n");
-		return -EIO;
+		if (entry->msg.address_hi && dev->no_64bit_msi) {
+			pci_err(dev, "arch assigned 64-bit MSI address %#x%08x but device only supports 32 bits\n",
+				entry->msg.address_hi, entry->msg.address_lo);
+			return -EIO;
+		}
 	}
 	return 0;
 }
@@ -1619,6 +1619,9 @@ void pci_msi_init(struct pci_dev *dev)
 	if (ctrl & PCI_MSI_FLAGS_ENABLE)
 		pci_write_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS,
 				      ctrl & ~PCI_MSI_FLAGS_ENABLE);
+
+	if (!(ctrl & PCI_MSI_FLAGS_64BIT))
+		dev->no_64bit_msi = 1;
 }
 
 void pci_msix_init(struct pci_dev *dev)
-- 
2.25.1

Re: [PATCH v3 3/3] PCI/MSI: Set device flag indicating only 32-bit MSI support

[Thierry Reding]

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

On Thu, Dec 03, 2020 at 12:51:10PM -0600, Bjorn Helgaas wrote:
> From: Vidya Sagar <vidyas@nvidia.com>
> 
> The MSI-X Capability requires devices to support 64-bit Message Addresses,
> but the MSI Capability can support either 32- or 64-bit addresses.
> 
> Previously, we set dev->no_64bit_msi for a few broken devices that
> advertise 64-bit MSI support but don't correctly support it.
> 
> In addition, check the MSI "64-bit Address Capable" bit for all devices and
> set dev->no_64bit_msi for devices that don't advertise 64-bit support.
> This allows msi_verify_entries() to catch arch code defects that assign
> 64-bit addresses when they're not supported.
> 
> [bhelgaas: set no_64bit_msi in pci_msi_init(), commit log]
> Link: https://lore.kernel.org/r/20201124105035.24573-1-vidyas@nvidia.com
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/msi.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 0/3] PCI/MSI: Cleanup init and improve 32-bit MSI checking

[Bjorn Helgaas]

On Thu, Dec 03, 2020 at 12:51:07PM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> MSI/MSI-X init was a little unconventional.  We had pci_msi_setup_pci_dev()
> to disable MSI and MSI-X, in probe.c instead of msi.c so we could do it
> even without CONFIG_PCI_MSI.  Move that to msi.c and fix the config issue
> with an #ifdef.
> 
> Then add Vidya's patch on top.  Previous postings at
> 
> https://lore.kernel.org/linux-pci/20201117145728.4516-1-vidyas@nvidia.com/
> https://lore.kernel.org/linux-pci/20201124105035.24573-1-vidyas@nvidia.com/
> 
> Bjorn Helgaas (2):
>   PCI/MSI: Move MSI/MSI-X init to msi.c
>   PCI/MSI: Move MSI/MSI-X flags updaters to msi.c
> 
> Vidya Sagar (1):
>   PCI/MSI: Set device flag indicating only 32-bit MSI support
> 
>  drivers/pci/Makefile |  3 +-
>  drivers/pci/msi.c    | 70 ++++++++++++++++++++++++++++++++++++++++----
>  drivers/pci/pci.h    | 23 ++-------------
>  drivers/pci/probe.c  | 21 ++-----------
>  4 files changed, 70 insertions(+), 47 deletions(-)

I fixed my typo ("#ifdef CONFIG_MSI" when it should have been
"#ifdef CONFIG_PCI_MSI"), added the reference from Vidya, added
Thierry's Reviewed-by, and put these on pci/msi for v5.11.