linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] PCI/MSI: Cleanup init and improve 32-bit MSI checking
@ 2020-12-03 18:51 Bjorn Helgaas
  2020-12-03 18:51 ` [PATCH v3 1/3] PCI/MSI: Move MSI/MSI-X init to msi.c Bjorn Helgaas
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Bjorn Helgaas @ 2020-12-03 18:51 UTC (permalink / raw)
  To: Vidya Sagar
  Cc: Lorenzo Pieralisi, Thierry Reding, Jonathan Hunter,
	Krishna Kishore, Manikanta Maddireddy, Vidya Sagar, linux-pci,
	linux-kernel, 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 1/3] PCI/MSI: Move MSI/MSI-X init to msi.c
  2020-12-03 18:51 [PATCH v3 0/3] PCI/MSI: Cleanup init and improve 32-bit MSI checking Bjorn Helgaas
@ 2020-12-03 18:51 ` Bjorn Helgaas
  2020-12-04 11:05   ` Thierry Reding
  2020-12-03 18:51 ` [PATCH v3 2/3] PCI/MSI: Move MSI/MSI-X flags updaters " Bjorn Helgaas
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Bjorn Helgaas @ 2020-12-03 18:51 UTC (permalink / raw)
  To: Vidya Sagar
  Cc: Lorenzo Pieralisi, Thierry Reding, Jonathan Hunter,
	Krishna Kishore, Manikanta Maddireddy, Vidya Sagar, linux-pci,
	linux-kernel, 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 2/3] PCI/MSI: Move MSI/MSI-X flags updaters to msi.c
  2020-12-03 18:51 [PATCH v3 0/3] PCI/MSI: Cleanup init and improve 32-bit MSI checking Bjorn Helgaas
  2020-12-03 18:51 ` [PATCH v3 1/3] PCI/MSI: Move MSI/MSI-X init to msi.c Bjorn Helgaas
@ 2020-12-03 18:51 ` Bjorn Helgaas
  2020-12-04 11:06   ` Thierry Reding
  2020-12-03 18:51 ` [PATCH v3 3/3] PCI/MSI: Set device flag indicating only 32-bit MSI support Bjorn Helgaas
  2020-12-04 18:21 ` [PATCH v3 0/3] PCI/MSI: Cleanup init and improve 32-bit MSI checking Bjorn Helgaas
  3 siblings, 1 reply; 9+ messages in thread
From: Bjorn Helgaas @ 2020-12-03 18:51 UTC (permalink / raw)
  To: Vidya Sagar
  Cc: Lorenzo Pieralisi, Thierry Reding, Jonathan Hunter,
	Krishna Kishore, Manikanta Maddireddy, Vidya Sagar, linux-pci,
	linux-kernel, 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 3/3] PCI/MSI: Set device flag indicating only 32-bit MSI support
  2020-12-03 18:51 [PATCH v3 0/3] PCI/MSI: Cleanup init and improve 32-bit MSI checking Bjorn Helgaas
  2020-12-03 18:51 ` [PATCH v3 1/3] PCI/MSI: Move MSI/MSI-X init to msi.c Bjorn Helgaas
  2020-12-03 18:51 ` [PATCH v3 2/3] PCI/MSI: Move MSI/MSI-X flags updaters " Bjorn Helgaas
@ 2020-12-03 18:51 ` Bjorn Helgaas
  2020-12-04 11:07   ` Thierry Reding
  2020-12-04 18:21 ` [PATCH v3 0/3] PCI/MSI: Cleanup init and improve 32-bit MSI checking Bjorn Helgaas
  3 siblings, 1 reply; 9+ messages in thread
From: Bjorn Helgaas @ 2020-12-03 18:51 UTC (permalink / raw)
  To: Vidya Sagar
  Cc: Lorenzo Pieralisi, Thierry Reding, Jonathan Hunter,
	Krishna Kishore, Manikanta Maddireddy, Vidya Sagar, linux-pci,
	linux-kernel, 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/3] PCI/MSI: Move MSI/MSI-X init to msi.c
  2020-12-03 18:51 ` [PATCH v3 1/3] PCI/MSI: Move MSI/MSI-X init to msi.c Bjorn Helgaas
@ 2020-12-04 11:05   ` Thierry Reding
  2020-12-04 18:11     ` Bjorn Helgaas
  0 siblings, 1 reply; 9+ messages in thread
From: Thierry Reding @ 2020-12-04 11:05 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Vidya Sagar, Lorenzo Pieralisi, Jonathan Hunter, Krishna Kishore,
	Manikanta Maddireddy, Vidya Sagar, linux-pci, linux-kernel,
	Bjorn Helgaas

[-- 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 --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 2/3] PCI/MSI: Move MSI/MSI-X flags updaters to msi.c
  2020-12-03 18:51 ` [PATCH v3 2/3] PCI/MSI: Move MSI/MSI-X flags updaters " Bjorn Helgaas
@ 2020-12-04 11:06   ` Thierry Reding
  0 siblings, 0 replies; 9+ messages in thread
From: Thierry Reding @ 2020-12-04 11:06 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Vidya Sagar, Lorenzo Pieralisi, Jonathan Hunter, Krishna Kishore,
	Manikanta Maddireddy, Vidya Sagar, linux-pci, linux-kernel,
	Bjorn Helgaas

[-- 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 --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 3/3] PCI/MSI: Set device flag indicating only 32-bit MSI support
  2020-12-03 18:51 ` [PATCH v3 3/3] PCI/MSI: Set device flag indicating only 32-bit MSI support Bjorn Helgaas
@ 2020-12-04 11:07   ` Thierry Reding
  0 siblings, 0 replies; 9+ messages in thread
From: Thierry Reding @ 2020-12-04 11:07 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Vidya Sagar, Lorenzo Pieralisi, Jonathan Hunter, Krishna Kishore,
	Manikanta Maddireddy, Vidya Sagar, linux-pci, linux-kernel,
	Bjorn Helgaas

[-- 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 --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/3] PCI/MSI: Move MSI/MSI-X init to msi.c
  2020-12-04 11:05   ` Thierry Reding
@ 2020-12-04 18:11     ` Bjorn Helgaas
  0 siblings, 0 replies; 9+ messages in thread
From: Bjorn Helgaas @ 2020-12-04 18:11 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Vidya Sagar, Lorenzo Pieralisi, Jonathan Hunter, Krishna Kishore,
	Manikanta Maddireddy, Vidya Sagar, linux-pci, linux-kernel,
	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>



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 0/3] PCI/MSI: Cleanup init and improve 32-bit MSI checking
  2020-12-03 18:51 [PATCH v3 0/3] PCI/MSI: Cleanup init and improve 32-bit MSI checking Bjorn Helgaas
                   ` (2 preceding siblings ...)
  2020-12-03 18:51 ` [PATCH v3 3/3] PCI/MSI: Set device flag indicating only 32-bit MSI support Bjorn Helgaas
@ 2020-12-04 18:21 ` Bjorn Helgaas
  3 siblings, 0 replies; 9+ messages in thread
From: Bjorn Helgaas @ 2020-12-04 18:21 UTC (permalink / raw)
  To: Vidya Sagar
  Cc: Lorenzo Pieralisi, Thierry Reding, Jonathan Hunter,
	Krishna Kishore, Manikanta Maddireddy, Vidya Sagar, linux-pci,
	linux-kernel, 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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-12-04 18:22 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-03 18:51 [PATCH v3 0/3] PCI/MSI: Cleanup init and improve 32-bit MSI checking Bjorn Helgaas
2020-12-03 18:51 ` [PATCH v3 1/3] PCI/MSI: Move MSI/MSI-X init to msi.c Bjorn Helgaas
2020-12-04 11:05   ` Thierry Reding
2020-12-04 18:11     ` Bjorn Helgaas
2020-12-03 18:51 ` [PATCH v3 2/3] PCI/MSI: Move MSI/MSI-X flags updaters " Bjorn Helgaas
2020-12-04 11:06   ` Thierry Reding
2020-12-03 18:51 ` [PATCH v3 3/3] PCI/MSI: Set device flag indicating only 32-bit MSI support Bjorn Helgaas
2020-12-04 11:07   ` Thierry Reding
2020-12-04 18:21 ` [PATCH v3 0/3] PCI/MSI: Cleanup init and improve 32-bit MSI checking Bjorn Helgaas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).