Re: MSI address mask quirk issue

Re: MSI address mask quirk issue

[Benjamin Herrenschmidt]

On Tue, 2014-09-30 at 09:04 -0600, Bjorn Helgaas wrote:
> On Mon, Sep 29, 2014 at 9:40 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > Hi folks !

(Adding linux-pci and Yijing, which I stupidly forgot)

> > Some ATM devices have a limitation in the number of address bits they
> > can generate for MSI and MSI-X (similar to their DMA limitations).
> >
> > This makes it impossible to "reach" the MSI regions for 64-bit MSI on
> > our POWER server chips since those must have some of the top address
> > bits set.
> >
> > For DMA today, we have the DMA mask that handles that limitation fine
> > at the arch level.
> >
> > However we have nothing for MSIs.
> >
> > So far, we've workaround it with a kludge. We maintain a powerpc
> > specific flag in some auxilliary data structure that we keep along
> > with PCI devices and we have a quirk in arch/powerpc/kernel/pci_64.c
> > that will set that flag for some known devices.
> >
> > At this point we have only added two known ATI IDs corresponding to
> > radeons we ship with our machines, but as you can imagine, that not
> > a very good long term solution.
> >
> > Ideally we want that quirk to be set by the driver. The radeon driver
> > "knows" what the addressing capacity of the device it.
> >
> > So the easiest way to handle that for me would be to move that flag
> > "force_32bit_msi" from our arch data structure to pci_dev, and have
> > the radeon driver set it before it enables MSIs.
> >
> > However that would have the side effect of also limiting other archs
> > to 32-bit MSIs while the 40-bit of address (or so ....) that the radeon
> > supports might be sufficient for 64-bit MSIs to work on these.
> >
> > The slightly fancier approach would be to have something like an
> > msi64_set_address_mask() where the driver can configure the supported
> > mask for 64-bit MSIs.
> >
> > This would allow the architecture code to make a smarter decision based
> > on what is actually possible.
> >
> > Any preference, comment or suggestion ?
> 
> Please CC: linux-pci and Yijing Wang <wangyijing@huawei.com> unless
> there's something secret here.  I didn't add them myself in case you
> think this is sensitive.
> 
> If I understand correctly, per spec, devices can support MSI with
> either 32-bit or 64-bit addresses, and they advertise their capability
> via the "64 bit address capable" bit in the Message Control register.
> And per spec, devices that support MSI-X must support 64-bit
> addresses.
> 
> So apparently these devices are not spec compliant.  I suggest that
> you move the "force_32bit" flag into struct pci_dev, set it via a
> quirk, and change drivers/pci/msi.c to use it to override the
> "msi_attrib.is_64" settings in msi_setup_entry() and/or
> msix_setup_entries().
> 
> At this point, I don't see value in adding the complexity to support
> intermediate address sizes between 32 and 64 bits.

Ok, I tend to agree.

Cheers,
Ben.

[PATCH 1/4] pci/msi: Move "force_32bit_msi" flag from powerpc to generic pci_dev

[Benjamin Herrenschmidt]

Some devices have broken 64-bit MSI support which only support some
address bits (40 to 48 typically). This doesn't work on some platforms
such as POWER servers, so we need a quirk.

Currently we keep a flag in a powerpc specific data structure which we
have per PCI device. However this is impractical as we really want the
driver to set that flag appropriately (and the driver shouldn't touch
that arch specific data structure).

It's also not unlikely that this limitation will affect other architectures
in the long run so may as well be prepared for it.

So this moves the flag to struct pci_dev instead and adjusts the
corresponding arch/powerpc code to look for it there. At this point,
there is no attempt at making other architectures honor it just yet
though from what I can tell, x86 seems to always use 32-bit addresses
for MSIs.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: <stable@vger.kernel.org>
---

Note: CC'ing stable as I really want this to hit distros, without that
(and the three subsequent patches), we crash during boot with a number
of radeon cards on power machines.

Note2: Alex, we can wait for the response of the HW guys for which revisions
actually need the quirk in hda_intel but I don't see a big risk or issue in
just doing it for all AMD/ATI for now and fix that up later

 arch/powerpc/include/asm/pci-bridge.h     | 2 --
 arch/powerpc/kernel/pci_64.c              | 5 +----
 arch/powerpc/platforms/powernv/pci-ioda.c | 3 +--
 arch/powerpc/platforms/powernv/pci.c      | 3 +--
 arch/powerpc/platforms/pseries/msi.c      | 2 +-
 include/linux/pci.h                       | 1 +
 6 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
index 4ca90a3..725247b 100644
--- a/arch/powerpc/include/asm/pci-bridge.h
+++ b/arch/powerpc/include/asm/pci-bridge.h
@@ -159,8 +159,6 @@ struct pci_dn {
 
 	int	pci_ext_config_space;	/* for pci devices */
 
-	bool	force_32bit_msi;
-
 	struct	pci_dev *pcidev;	/* back-pointer to the pci device */
 #ifdef CONFIG_EEH
 	struct eeh_dev *edev;		/* eeh device */
diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
index 155013d..a6ce5fe 100644
--- a/arch/powerpc/kernel/pci_64.c
+++ b/arch/powerpc/kernel/pci_64.c
@@ -269,10 +269,7 @@ EXPORT_SYMBOL(pcibus_to_node);
 
 static void quirk_radeon_32bit_msi(struct pci_dev *dev)
 {
-	struct pci_dn *pdn = pci_get_pdn(dev);
-
-	if (pdn)
-		pdn->force_32bit_msi = true;
+	dev->force_32bit_msi = true;
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x68f2, quirk_radeon_32bit_msi);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0xaa68, quirk_radeon_32bit_msi);
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index df241b1..9d98475 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -1311,7 +1311,6 @@ static int pnv_pci_ioda_msi_setup(struct pnv_phb *phb, struct pci_dev *dev,
 				  unsigned int is_64, struct msi_msg *msg)
 {
 	struct pnv_ioda_pe *pe = pnv_ioda_get_pe(dev);
-	struct pci_dn *pdn = pci_get_pdn(dev);
 	struct irq_data *idata;
 	struct irq_chip *ichip;
 	unsigned int xive_num = hwirq - phb->msi_base;
@@ -1327,7 +1326,7 @@ static int pnv_pci_ioda_msi_setup(struct pnv_phb *phb, struct pci_dev *dev,
 		return -ENXIO;
 
 	/* Force 32-bit MSI on some broken devices */
-	if (pdn && pdn->force_32bit_msi)
+	if (dev->force_32bit_msi)
 		is_64 = 0;
 
 	/* Assign XIVE to PE */
diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index b854b57..4e43a6f 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -50,9 +50,8 @@ static int pnv_msi_check_device(struct pci_dev* pdev, int nvec, int type)
 {
 	struct pci_controller *hose = pci_bus_to_host(pdev->bus);
 	struct pnv_phb *phb = hose->private_data;
-	struct pci_dn *pdn = pci_get_pdn(pdev);
 
-	if (pdn && pdn->force_32bit_msi && !phb->msi32_support)
+	if (pdev->force_32bit_msi && !phb->msi32_support)
 		return -ENODEV;
 
 	return (phb && phb->msi_bmp.bitmap) ? 0 : -ENODEV;
diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
index 18ff462..b3f2c1a 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c
@@ -429,7 +429,7 @@ static int rtas_setup_msi_irqs(struct pci_dev *pdev, int nvec_in, int type)
 	 */
 again:
 	if (type == PCI_CAP_ID_MSI) {
-		if (pdn->force_32bit_msi) {
+		if (pdev->force_32bit_msi) {
 			rc = rtas_change_msi(pdn, RTAS_CHANGE_32MSI_FN, nvec);
 			if (rc < 0) {
 				/*
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 96453f9..740cadd 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -331,6 +331,7 @@ struct pci_dev {
 	unsigned int	is_added:1;
 	unsigned int	is_busmaster:1; /* device is busmaster */
 	unsigned int	no_msi:1;	/* device may not use msi */
+	unsigned int    force_32bit_msi:1;	/* Device has broken 64-bit MSIs */
 	unsigned int	block_cfg_access:1;	/* config space access is blocked */
 	unsigned int	broken_parity_status:1;	/* Device generates false positive parity */
 	unsigned int	irq_reroute_variant:2;	/* device needs IRQ rerouting variant */

Re: [PATCH 1/4] pci/msi: Move "force_32bit_msi" flag from powerpc to generic pci_dev

[Bjorn Helgaas]

On Wed, Oct 01, 2014 at 12:09:23PM +1000, Benjamin Herrenschmidt wrote:
> 
> Some devices have broken 64-bit MSI support which only support some
> address bits (40 to 48 typically). This doesn't work on some platforms
> such as POWER servers, so we need a quirk.
> 
> Currently we keep a flag in a powerpc specific data structure which we
> have per PCI device. However this is impractical as we really want the
> driver to set that flag appropriately (and the driver shouldn't touch
> that arch specific data structure).
> 
> It's also not unlikely that this limitation will affect other architectures
> in the long run so may as well be prepared for it.
> 
> So this moves the flag to struct pci_dev instead and adjusts the
> corresponding arch/powerpc code to look for it there. At this point,
> there is no attempt at making other architectures honor it just yet
> though from what I can tell, x86 seems to always use 32-bit addresses
> for MSIs.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: <stable@vger.kernel.org>
> ---
> 
> Note: CC'ing stable as I really want this to hit distros, without that
> (and the three subsequent patches), we crash during boot with a number
> of radeon cards on power machines.
> 
> Note2: Alex, we can wait for the response of the HW guys for which revisions
> actually need the quirk in hda_intel but I don't see a big risk or issue in
> just doing it for all AMD/ATI for now and fix that up later
> 
>  arch/powerpc/include/asm/pci-bridge.h     | 2 --
>  arch/powerpc/kernel/pci_64.c              | 5 +----
>  arch/powerpc/platforms/powernv/pci-ioda.c | 3 +--
>  arch/powerpc/platforms/powernv/pci.c      | 3 +--
>  arch/powerpc/platforms/pseries/msi.c      | 2 +-
>  include/linux/pci.h                       | 1 +
>  6 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
> index 4ca90a3..725247b 100644
> --- a/arch/powerpc/include/asm/pci-bridge.h
> +++ b/arch/powerpc/include/asm/pci-bridge.h
> @@ -159,8 +159,6 @@ struct pci_dn {
>  
>  	int	pci_ext_config_space;	/* for pci devices */
>  
> -	bool	force_32bit_msi;
> -
>  	struct	pci_dev *pcidev;	/* back-pointer to the pci device */
>  #ifdef CONFIG_EEH
>  	struct eeh_dev *edev;		/* eeh device */
> diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
> index 155013d..a6ce5fe 100644
> --- a/arch/powerpc/kernel/pci_64.c
> +++ b/arch/powerpc/kernel/pci_64.c
> @@ -269,10 +269,7 @@ EXPORT_SYMBOL(pcibus_to_node);
>  
>  static void quirk_radeon_32bit_msi(struct pci_dev *dev)
>  {
> -	struct pci_dn *pdn = pci_get_pdn(dev);
> -
> -	if (pdn)
> -		pdn->force_32bit_msi = true;
> +	dev->force_32bit_msi = true;
>  }
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x68f2, quirk_radeon_32bit_msi);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0xaa68, quirk_radeon_32bit_msi);
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index df241b1..9d98475 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -1311,7 +1311,6 @@ static int pnv_pci_ioda_msi_setup(struct pnv_phb *phb, struct pci_dev *dev,
>  				  unsigned int is_64, struct msi_msg *msg)
>  {
>  	struct pnv_ioda_pe *pe = pnv_ioda_get_pe(dev);
> -	struct pci_dn *pdn = pci_get_pdn(dev);
>  	struct irq_data *idata;
>  	struct irq_chip *ichip;
>  	unsigned int xive_num = hwirq - phb->msi_base;
> @@ -1327,7 +1326,7 @@ static int pnv_pci_ioda_msi_setup(struct pnv_phb *phb, struct pci_dev *dev,
>  		return -ENXIO;
>  
>  	/* Force 32-bit MSI on some broken devices */
> -	if (pdn && pdn->force_32bit_msi)
> +	if (dev->force_32bit_msi)
>  		is_64 = 0;
>  
>  	/* Assign XIVE to PE */
> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
> index b854b57..4e43a6f 100644
> --- a/arch/powerpc/platforms/powernv/pci.c
> +++ b/arch/powerpc/platforms/powernv/pci.c
> @@ -50,9 +50,8 @@ static int pnv_msi_check_device(struct pci_dev* pdev, int nvec, int type)
>  {
>  	struct pci_controller *hose = pci_bus_to_host(pdev->bus);
>  	struct pnv_phb *phb = hose->private_data;
> -	struct pci_dn *pdn = pci_get_pdn(pdev);
>  
> -	if (pdn && pdn->force_32bit_msi && !phb->msi32_support)
> +	if (pdev->force_32bit_msi && !phb->msi32_support)
>  		return -ENODEV;
>  
>  	return (phb && phb->msi_bmp.bitmap) ? 0 : -ENODEV;
> diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
> index 18ff462..b3f2c1a 100644
> --- a/arch/powerpc/platforms/pseries/msi.c
> +++ b/arch/powerpc/platforms/pseries/msi.c
> @@ -429,7 +429,7 @@ static int rtas_setup_msi_irqs(struct pci_dev *pdev, int nvec_in, int type)
>  	 */
>  again:
>  	if (type == PCI_CAP_ID_MSI) {
> -		if (pdn->force_32bit_msi) {
> +		if (pdev->force_32bit_msi) {
>  			rc = rtas_change_msi(pdn, RTAS_CHANGE_32MSI_FN, nvec);
>  			if (rc < 0) {
>  				/*
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 96453f9..740cadd 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -331,6 +331,7 @@ struct pci_dev {
>  	unsigned int	is_added:1;
>  	unsigned int	is_busmaster:1; /* device is busmaster */
>  	unsigned int	no_msi:1;	/* device may not use msi */
> +	unsigned int    force_32bit_msi:1;	/* Device has broken 64-bit MSIs */

I like the idea of handling this more generically, e.g., with a bit like
this in struct pci_dev (I'd probably name it something like "no_64bit_msi"
along the lines of your driver #defines).

What I don't like is that we haven't done anything to help other
architectures, because the only code that *looks* at this bit is in
arch/powerpc.  The next arch that tries to use 64-bit MSI addresses for
these devices will trip over the same problem.

Can we check in pci_enable_msi_range() and pci_enable_msix_range() whether
the MSI addresses allocated by the arch are too big, and fail the call if
they are?

Bjorn

>  	unsigned int	block_cfg_access:1;	/* config space access is blocked */
>  	unsigned int	broken_parity_status:1;	/* Device generates false positive parity */
>  	unsigned int	irq_reroute_variant:2;	/* device needs IRQ rerouting variant */
> 
> 
> 

Re: [PATCH 1/4] pci/msi: Move "force_32bit_msi" flag from powerpc to generic pci_dev

[Benjamin Herrenschmidt]

On Wed, 2014-10-01 at 14:33 -0600, Bjorn Helgaas wrote:
> I like the idea of handling this more generically, e.g., with a bit like
> this in struct pci_dev (I'd probably name it something like "no_64bit_msi"
> along the lines of your driver #defines).
> 
> What I don't like is that we haven't done anything to help other
> architectures, because the only code that *looks* at this bit is in
> arch/powerpc.  The next arch that tries to use 64-bit MSI addresses for
> these devices will trip over the same problem.

I started looking. From my (limited) understanding of x86, it doesn't
even look at the "size" of MSIs which makes me think it's always 32-bit,
and I plan to look at the others next (though not for stable).

> Can we check in pci_enable_msi_range() and pci_enable_msix_range() whether
> the MSI addresses allocated by the arch are too big, and fail the call if
> they are?

Yes, good idea, I'll add something.

Cheers,
Ben.

[PATCH v2 1/4] pci/msi: Move "no_64bit_msi" flag from powerpc to generic pci_dev

[Benjamin Herrenschmidt]

Some devices have broken 64-bit MSI support which only support some
address bits (40 to 48 typically). This doesn't work on some platforms
such as POWER servers, so we need a quirk.

Currently we keep a flag in a powerpc specific data structure which we
have per PCI device. However this is impractical as we really want the
driver to set that flag appropriately (and the driver shouldn't touch
that arch specific data structure).

It's also not unlikely that this limitation will affect other architectures
in the long run.

So this moves the flag to struct pci_dev instead and adjusts the
corresponding arch/powerpc code to look for it there. At this point,
there is no attempt at making other architectures honor it just yet,
though it appears that x86 doesn't care and always generates 32-bit
MSI addresses.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: <stable@vger.kernel.org>
---

v2: Rename flag to "no_64bit_msi" as suggested by Bjorn

 arch/powerpc/include/asm/pci-bridge.h     | 2 --
 arch/powerpc/kernel/pci_64.c              | 5 +----
 arch/powerpc/platforms/powernv/pci-ioda.c | 3 +--
 arch/powerpc/platforms/powernv/pci.c      | 3 +--
 arch/powerpc/platforms/pseries/msi.c      | 2 +-
 include/linux/pci.h                       | 1 +
 6 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
index 4ca90a3..725247b 100644
--- a/arch/powerpc/include/asm/pci-bridge.h
+++ b/arch/powerpc/include/asm/pci-bridge.h
@@ -159,8 +159,6 @@ struct pci_dn {
 
 	int	pci_ext_config_space;	/* for pci devices */
 
-	bool	force_32bit_msi;
-
 	struct	pci_dev *pcidev;	/* back-pointer to the pci device */
 #ifdef CONFIG_EEH
 	struct eeh_dev *edev;		/* eeh device */
diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
index 155013d..d41a831 100644
--- a/arch/powerpc/kernel/pci_64.c
+++ b/arch/powerpc/kernel/pci_64.c
@@ -269,10 +269,7 @@ EXPORT_SYMBOL(pcibus_to_node);
 
 static void quirk_radeon_32bit_msi(struct pci_dev *dev)
 {
-	struct pci_dn *pdn = pci_get_pdn(dev);
-
-	if (pdn)
-		pdn->force_32bit_msi = true;
+	dev->no_64bit_msi = true;
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x68f2, quirk_radeon_32bit_msi);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0xaa68, quirk_radeon_32bit_msi);
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index df241b1..a188bb8 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -1311,7 +1311,6 @@ static int pnv_pci_ioda_msi_setup(struct pnv_phb *phb, struct pci_dev *dev,
 				  unsigned int is_64, struct msi_msg *msg)
 {
 	struct pnv_ioda_pe *pe = pnv_ioda_get_pe(dev);
-	struct pci_dn *pdn = pci_get_pdn(dev);
 	struct irq_data *idata;
 	struct irq_chip *ichip;
 	unsigned int xive_num = hwirq - phb->msi_base;
@@ -1327,7 +1326,7 @@ static int pnv_pci_ioda_msi_setup(struct pnv_phb *phb, struct pci_dev *dev,
 		return -ENXIO;
 
 	/* Force 32-bit MSI on some broken devices */
-	if (pdn && pdn->force_32bit_msi)
+	if (dev->no_64bit_msi)
 		is_64 = 0;
 
 	/* Assign XIVE to PE */
diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index b854b57..89c6608 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -50,9 +50,8 @@ static int pnv_msi_check_device(struct pci_dev* pdev, int nvec, int type)
 {
 	struct pci_controller *hose = pci_bus_to_host(pdev->bus);
 	struct pnv_phb *phb = hose->private_data;
-	struct pci_dn *pdn = pci_get_pdn(pdev);
 
-	if (pdn && pdn->force_32bit_msi && !phb->msi32_support)
+	if (pdev->no_64bit_msi && !phb->msi32_support)
 		return -ENODEV;
 
 	return (phb && phb->msi_bmp.bitmap) ? 0 : -ENODEV;
diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
index 18ff462..6fd96d8 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c
@@ -429,7 +429,7 @@ static int rtas_setup_msi_irqs(struct pci_dev *pdev, int nvec_in, int type)
 	 */
 again:
 	if (type == PCI_CAP_ID_MSI) {
-		if (pdn->force_32bit_msi) {
+		if (pdev->no_64bit_msi) {
 			rc = rtas_change_msi(pdn, RTAS_CHANGE_32MSI_FN, nvec);
 			if (rc < 0) {
 				/*
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 96453f9..fc938003 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -331,6 +331,7 @@ struct pci_dev {
 	unsigned int	is_added:1;
 	unsigned int	is_busmaster:1; /* device is busmaster */
 	unsigned int	no_msi:1;	/* device may not use msi */
+	unsigned int    no_64bit_msi:1;	/* Device has broken 64-bit MSIs */
 	unsigned int	block_cfg_access:1;	/* config space access is blocked */
 	unsigned int	broken_parity_status:1;	/* Device generates false positive parity */
 	unsigned int	irq_reroute_variant:2;	/* device needs IRQ rerouting variant */

[PATCH v2 1/4] pci/msi: Move "no_64bit_msi" flag from powerpc to generic pci_dev

[Benjamin Herrenschmidt]

Some devices have broken 64-bit MSI support which only support some
address bits (40 to 48 typically). This doesn't work on some platforms
such as POWER servers, so we need a quirk.

Currently we keep a flag in a powerpc specific data structure which we
have per PCI device. However this is impractical as we really want the
driver to set that flag appropriately (and the driver shouldn't touch
that arch specific data structure).

It's also not unlikely that this limitation will affect other architectures
in the long run.

So this moves the flag to struct pci_dev instead and adjusts the
corresponding arch/powerpc code to look for it there. At this point,
there is no attempt at making other architectures honor it just yet,
though it appears that x86 doesn't care and always generates 32-bit
MSI addresses.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: <stable@vger.kernel.org>
---

v2: Rename flag to "no_64bit_msi" as suggested by Bjorn

 arch/powerpc/include/asm/pci-bridge.h     | 2 --
 arch/powerpc/kernel/pci_64.c              | 5 +----
 arch/powerpc/platforms/powernv/pci-ioda.c | 3 +--
 arch/powerpc/platforms/powernv/pci.c      | 3 +--
 arch/powerpc/platforms/pseries/msi.c      | 2 +-
 include/linux/pci.h                       | 1 +
 6 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
index 4ca90a3..725247b 100644
--- a/arch/powerpc/include/asm/pci-bridge.h
+++ b/arch/powerpc/include/asm/pci-bridge.h
@@ -159,8 +159,6 @@ struct pci_dn {
 
 	int	pci_ext_config_space;	/* for pci devices */
 
-	bool	force_32bit_msi;
-
 	struct	pci_dev *pcidev;	/* back-pointer to the pci device */
 #ifdef CONFIG_EEH
 	struct eeh_dev *edev;		/* eeh device */
diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
index 155013d..d41a831 100644
--- a/arch/powerpc/kernel/pci_64.c
+++ b/arch/powerpc/kernel/pci_64.c
@@ -269,10 +269,7 @@ EXPORT_SYMBOL(pcibus_to_node);
 
 static void quirk_radeon_32bit_msi(struct pci_dev *dev)
 {
-	struct pci_dn *pdn = pci_get_pdn(dev);
-
-	if (pdn)
-		pdn->force_32bit_msi = true;
+	dev->no_64bit_msi = true;
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x68f2, quirk_radeon_32bit_msi);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0xaa68, quirk_radeon_32bit_msi);
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index df241b1..a188bb8 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -1311,7 +1311,6 @@ static int pnv_pci_ioda_msi_setup(struct pnv_phb *phb, struct pci_dev *dev,
 				  unsigned int is_64, struct msi_msg *msg)
 {
 	struct pnv_ioda_pe *pe = pnv_ioda_get_pe(dev);
-	struct pci_dn *pdn = pci_get_pdn(dev);
 	struct irq_data *idata;
 	struct irq_chip *ichip;
 	unsigned int xive_num = hwirq - phb->msi_base;
@@ -1327,7 +1326,7 @@ static int pnv_pci_ioda_msi_setup(struct pnv_phb *phb, struct pci_dev *dev,
 		return -ENXIO;
 
 	/* Force 32-bit MSI on some broken devices */
-	if (pdn && pdn->force_32bit_msi)
+	if (dev->no_64bit_msi)
 		is_64 = 0;
 
 	/* Assign XIVE to PE */
diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index b854b57..89c6608 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -50,9 +50,8 @@ static int pnv_msi_check_device(struct pci_dev* pdev, int nvec, int type)
 {
 	struct pci_controller *hose = pci_bus_to_host(pdev->bus);
 	struct pnv_phb *phb = hose->private_data;
-	struct pci_dn *pdn = pci_get_pdn(pdev);
 
-	if (pdn && pdn->force_32bit_msi && !phb->msi32_support)
+	if (pdev->no_64bit_msi && !phb->msi32_support)
 		return -ENODEV;
 
 	return (phb && phb->msi_bmp.bitmap) ? 0 : -ENODEV;
diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
index 18ff462..6fd96d8 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c
@@ -429,7 +429,7 @@ static int rtas_setup_msi_irqs(struct pci_dev *pdev, int nvec_in, int type)
 	 */
 again:
 	if (type == PCI_CAP_ID_MSI) {
-		if (pdn->force_32bit_msi) {
+		if (pdev->no_64bit_msi) {
 			rc = rtas_change_msi(pdn, RTAS_CHANGE_32MSI_FN, nvec);
 			if (rc < 0) {
 				/*
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 96453f9..fc938003 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -331,6 +331,7 @@ struct pci_dev {
 	unsigned int	is_added:1;
 	unsigned int	is_busmaster:1; /* device is busmaster */
 	unsigned int	no_msi:1;	/* device may not use msi */
+	unsigned int    no_64bit_msi:1;	/* Device has broken 64-bit MSIs */
 	unsigned int	block_cfg_access:1;	/* config space access is blocked */
 	unsigned int	broken_parity_status:1;	/* Device generates false positive parity */
 	unsigned int	irq_reroute_variant:2;	/* device needs IRQ rerouting variant */

Re: [PATCH v2 1/4] pci/msi: Move "no_64bit_msi" flag from powerpc to generic pci_dev

[Stephen Rothwell]

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

Hi Ben,

On Thu, 02 Oct 2014 10:33:32 +1000 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> --- a/arch/powerpc/kernel/pci_64.c
> +++ b/arch/powerpc/kernel/pci_64.c
> @@ -269,10 +269,7 @@ EXPORT_SYMBOL(pcibus_to_node);
>  
>  static void quirk_radeon_32bit_msi(struct pci_dev *dev)
>  {
> -	struct pci_dn *pdn = pci_get_pdn(dev);
> -
> -	if (pdn)
> -		pdn->force_32bit_msi = true;
> +	dev->no_64bit_msi = true;
>  }
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x68f2, quirk_radeon_32bit_msi);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0xaa68, quirk_radeon_32bit_msi);
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -331,6 +331,7 @@ struct pci_dev {
>  	unsigned int	is_added:1;
>  	unsigned int	is_busmaster:1; /* device is busmaster */
>  	unsigned int	no_msi:1;	/* device may not use msi */
> +	unsigned int    no_64bit_msi:1;	/* Device has broken 64-bit MSIs */
>  	unsigned int	block_cfg_access:1;	/* config space access is blocked */
>  	unsigned int	broken_parity_status:1;	/* Device generates false positive parity */
>  	unsigned int	irq_reroute_variant:2;	/* device needs IRQ rerouting variant */

You really should not be assigning "true" to a single bit field ...
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

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

Re: [PATCH v2 1/4] pci/msi: Move "no_64bit_msi" flag from powerpc to generic pci_dev

[Bjorn Helgaas]

On Thu, Oct 02, 2014 at 10:34:12AM +1000, Benjamin Herrenschmidt wrote:
> 
> Some devices have broken 64-bit MSI support which only support some
> address bits (40 to 48 typically). This doesn't work on some platforms
> such as POWER servers, so we need a quirk.
> 
> Currently we keep a flag in a powerpc specific data structure which we
> have per PCI device. However this is impractical as we really want the
> driver to set that flag appropriately (and the driver shouldn't touch
> that arch specific data structure).
> 
> It's also not unlikely that this limitation will affect other architectures
> in the long run.
> 
> So this moves the flag to struct pci_dev instead and adjusts the
> corresponding arch/powerpc code to look for it there. At this point,
> there is no attempt at making other architectures honor it just yet,
> though it appears that x86 doesn't care and always generates 32-bit
> MSI addresses.

I thought you were going to add something in drivers/pci to make this more
generic?

Is it feasible to structure this series as follows?

  - add generic stuff (struct pci_dev bit, msi.c checks)
  - add quirks to drivers to set the pci_dev bit
  - change powerpc to check pci_dev bit instead of pdn bit
  - remove powerpc-specific quirks since nobody looks as the pdn bit

I think it would be easier to follow if the powerpc-specific parts weren't
intertwined with the rest.

> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: <stable@vger.kernel.org>
> ---
> 
> v2: Rename flag to "no_64bit_msi" as suggested by Bjorn
> 
>  arch/powerpc/include/asm/pci-bridge.h     | 2 --
>  arch/powerpc/kernel/pci_64.c              | 5 +----
>  arch/powerpc/platforms/powernv/pci-ioda.c | 3 +--
>  arch/powerpc/platforms/powernv/pci.c      | 3 +--
>  arch/powerpc/platforms/pseries/msi.c      | 2 +-
>  include/linux/pci.h                       | 1 +
>  6 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
> index 4ca90a3..725247b 100644
> --- a/arch/powerpc/include/asm/pci-bridge.h
> +++ b/arch/powerpc/include/asm/pci-bridge.h
> @@ -159,8 +159,6 @@ struct pci_dn {
>  
>  	int	pci_ext_config_space;	/* for pci devices */
>  
> -	bool	force_32bit_msi;
> -
>  	struct	pci_dev *pcidev;	/* back-pointer to the pci device */
>  #ifdef CONFIG_EEH
>  	struct eeh_dev *edev;		/* eeh device */
> diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
> index 155013d..d41a831 100644
> --- a/arch/powerpc/kernel/pci_64.c
> +++ b/arch/powerpc/kernel/pci_64.c
> @@ -269,10 +269,7 @@ EXPORT_SYMBOL(pcibus_to_node);
>  
>  static void quirk_radeon_32bit_msi(struct pci_dev *dev)
>  {
> -	struct pci_dn *pdn = pci_get_pdn(dev);
> -
> -	if (pdn)
> -		pdn->force_32bit_msi = true;
> +	dev->no_64bit_msi = true;
>  }
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x68f2, quirk_radeon_32bit_msi);
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0xaa68, quirk_radeon_32bit_msi);
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index df241b1..a188bb8 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -1311,7 +1311,6 @@ static int pnv_pci_ioda_msi_setup(struct pnv_phb *phb, struct pci_dev *dev,
>  				  unsigned int is_64, struct msi_msg *msg)
>  {
>  	struct pnv_ioda_pe *pe = pnv_ioda_get_pe(dev);
> -	struct pci_dn *pdn = pci_get_pdn(dev);
>  	struct irq_data *idata;
>  	struct irq_chip *ichip;
>  	unsigned int xive_num = hwirq - phb->msi_base;
> @@ -1327,7 +1326,7 @@ static int pnv_pci_ioda_msi_setup(struct pnv_phb *phb, struct pci_dev *dev,
>  		return -ENXIO;
>  
>  	/* Force 32-bit MSI on some broken devices */
> -	if (pdn && pdn->force_32bit_msi)
> +	if (dev->no_64bit_msi)
>  		is_64 = 0;
>  
>  	/* Assign XIVE to PE */
> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
> index b854b57..89c6608 100644
> --- a/arch/powerpc/platforms/powernv/pci.c
> +++ b/arch/powerpc/platforms/powernv/pci.c
> @@ -50,9 +50,8 @@ static int pnv_msi_check_device(struct pci_dev* pdev, int nvec, int type)
>  {
>  	struct pci_controller *hose = pci_bus_to_host(pdev->bus);
>  	struct pnv_phb *phb = hose->private_data;
> -	struct pci_dn *pdn = pci_get_pdn(pdev);
>  
> -	if (pdn && pdn->force_32bit_msi && !phb->msi32_support)
> +	if (pdev->no_64bit_msi && !phb->msi32_support)
>  		return -ENODEV;
>  
>  	return (phb && phb->msi_bmp.bitmap) ? 0 : -ENODEV;
> diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
> index 18ff462..6fd96d8 100644
> --- a/arch/powerpc/platforms/pseries/msi.c
> +++ b/arch/powerpc/platforms/pseries/msi.c
> @@ -429,7 +429,7 @@ static int rtas_setup_msi_irqs(struct pci_dev *pdev, int nvec_in, int type)
>  	 */
>  again:
>  	if (type == PCI_CAP_ID_MSI) {
> -		if (pdn->force_32bit_msi) {
> +		if (pdev->no_64bit_msi) {
>  			rc = rtas_change_msi(pdn, RTAS_CHANGE_32MSI_FN, nvec);
>  			if (rc < 0) {
>  				/*
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 96453f9..fc938003 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -331,6 +331,7 @@ struct pci_dev {
>  	unsigned int	is_added:1;
>  	unsigned int	is_busmaster:1; /* device is busmaster */
>  	unsigned int	no_msi:1;	/* device may not use msi */
> +	unsigned int    no_64bit_msi:1;	/* Device has broken 64-bit MSIs */
>  	unsigned int	block_cfg_access:1;	/* config space access is blocked */
>  	unsigned int	broken_parity_status:1;	/* Device generates false positive parity */
>  	unsigned int	irq_reroute_variant:2;	/* device needs IRQ rerouting variant */
> 
> 
> 

Re: [PATCH v2 1/4] pci/msi: Move "no_64bit_msi" flag from powerpc to generic pci_dev

[Benjamin Herrenschmidt]

On Thu, 2014-10-02 at 09:31 -0600, Bjorn Helgaas wrote:

> > So this moves the flag to struct pci_dev instead and adjusts the
> > corresponding arch/powerpc code to look for it there. At this point,
> > there is no attempt at making other architectures honor it just yet,
> > though it appears that x86 doesn't care and always generates 32-bit
> > MSI addresses.
> 
> I thought you were going to add something in drivers/pci to make this more
> generic?

I don't see how. The only thing I can add is the check which I still
plan to do. But the decision on the address is in the arch.

> Is it feasible to structure this series as follows?
> 
>   - add generic stuff (struct pci_dev bit, msi.c checks)
>   - add quirks to drivers to set the pci_dev bit
>   - change powerpc to check pci_dev bit instead of pdn bit
>   - remove powerpc-specific quirks since nobody looks as the pdn bit
> 
> I think it would be easier to follow if the powerpc-specific parts weren't
> intertwined with the rest.

Well, in that case the first patch just adds one bit to pci_dev and
maybe one check for address to msi.c. Unfortunately there isn't more
I can do generically. But ok, I'll do it that way.

> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > CC: <stable@vger.kernel.org>
> > ---
> > 
> > v2: Rename flag to "no_64bit_msi" as suggested by Bjorn
> > 
> >  arch/powerpc/include/asm/pci-bridge.h     | 2 --
> >  arch/powerpc/kernel/pci_64.c              | 5 +----
> >  arch/powerpc/platforms/powernv/pci-ioda.c | 3 +--
> >  arch/powerpc/platforms/powernv/pci.c      | 3 +--
> >  arch/powerpc/platforms/pseries/msi.c      | 2 +-
> >  include/linux/pci.h                       | 1 +
> >  6 files changed, 5 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
> > index 4ca90a3..725247b 100644
> > --- a/arch/powerpc/include/asm/pci-bridge.h
> > +++ b/arch/powerpc/include/asm/pci-bridge.h
> > @@ -159,8 +159,6 @@ struct pci_dn {
> >  
> >  	int	pci_ext_config_space;	/* for pci devices */
> >  
> > -	bool	force_32bit_msi;
> > -
> >  	struct	pci_dev *pcidev;	/* back-pointer to the pci device */
> >  #ifdef CONFIG_EEH
> >  	struct eeh_dev *edev;		/* eeh device */
> > diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
> > index 155013d..d41a831 100644
> > --- a/arch/powerpc/kernel/pci_64.c
> > +++ b/arch/powerpc/kernel/pci_64.c
> > @@ -269,10 +269,7 @@ EXPORT_SYMBOL(pcibus_to_node);
> >  
> >  static void quirk_radeon_32bit_msi(struct pci_dev *dev)
> >  {
> > -	struct pci_dn *pdn = pci_get_pdn(dev);
> > -
> > -	if (pdn)
> > -		pdn->force_32bit_msi = true;
> > +	dev->no_64bit_msi = true;
> >  }
> >  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x68f2, quirk_radeon_32bit_msi);
> >  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0xaa68, quirk_radeon_32bit_msi);
> > diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> > index df241b1..a188bb8 100644
> > --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> > +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> > @@ -1311,7 +1311,6 @@ static int pnv_pci_ioda_msi_setup(struct pnv_phb *phb, struct pci_dev *dev,
> >  				  unsigned int is_64, struct msi_msg *msg)
> >  {
> >  	struct pnv_ioda_pe *pe = pnv_ioda_get_pe(dev);
> > -	struct pci_dn *pdn = pci_get_pdn(dev);
> >  	struct irq_data *idata;
> >  	struct irq_chip *ichip;
> >  	unsigned int xive_num = hwirq - phb->msi_base;
> > @@ -1327,7 +1326,7 @@ static int pnv_pci_ioda_msi_setup(struct pnv_phb *phb, struct pci_dev *dev,
> >  		return -ENXIO;
> >  
> >  	/* Force 32-bit MSI on some broken devices */
> > -	if (pdn && pdn->force_32bit_msi)
> > +	if (dev->no_64bit_msi)
> >  		is_64 = 0;
> >  
> >  	/* Assign XIVE to PE */
> > diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
> > index b854b57..89c6608 100644
> > --- a/arch/powerpc/platforms/powernv/pci.c
> > +++ b/arch/powerpc/platforms/powernv/pci.c
> > @@ -50,9 +50,8 @@ static int pnv_msi_check_device(struct pci_dev* pdev, int nvec, int type)
> >  {
> >  	struct pci_controller *hose = pci_bus_to_host(pdev->bus);
> >  	struct pnv_phb *phb = hose->private_data;
> > -	struct pci_dn *pdn = pci_get_pdn(pdev);
> >  
> > -	if (pdn && pdn->force_32bit_msi && !phb->msi32_support)
> > +	if (pdev->no_64bit_msi && !phb->msi32_support)
> >  		return -ENODEV;
> >  
> >  	return (phb && phb->msi_bmp.bitmap) ? 0 : -ENODEV;
> > diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
> > index 18ff462..6fd96d8 100644
> > --- a/arch/powerpc/platforms/pseries/msi.c
> > +++ b/arch/powerpc/platforms/pseries/msi.c
> > @@ -429,7 +429,7 @@ static int rtas_setup_msi_irqs(struct pci_dev *pdev, int nvec_in, int type)
> >  	 */
> >  again:
> >  	if (type == PCI_CAP_ID_MSI) {
> > -		if (pdn->force_32bit_msi) {
> > +		if (pdev->no_64bit_msi) {
> >  			rc = rtas_change_msi(pdn, RTAS_CHANGE_32MSI_FN, nvec);
> >  			if (rc < 0) {
> >  				/*
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 96453f9..fc938003 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -331,6 +331,7 @@ struct pci_dev {
> >  	unsigned int	is_added:1;
> >  	unsigned int	is_busmaster:1; /* device is busmaster */
> >  	unsigned int	no_msi:1;	/* device may not use msi */
> > +	unsigned int    no_64bit_msi:1;	/* Device has broken 64-bit MSIs */
> >  	unsigned int	block_cfg_access:1;	/* config space access is blocked */
> >  	unsigned int	broken_parity_status:1;	/* Device generates false positive parity */
> >  	unsigned int	irq_reroute_variant:2;	/* device needs IRQ rerouting variant */
> > 
> > 
> > 

Re: [PATCH v2 1/4] pci/msi: Move "no_64bit_msi" flag from powerpc to generic pci_dev

[Bjorn Helgaas]

On Thu, Oct 2, 2014 at 3:01 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Thu, 2014-10-02 at 09:31 -0600, Bjorn Helgaas wrote:
>
>> > So this moves the flag to struct pci_dev instead and adjusts the
>> > corresponding arch/powerpc code to look for it there. At this point,
>> > there is no attempt at making other architectures honor it just yet,
>> > though it appears that x86 doesn't care and always generates 32-bit
>> > MSI addresses.
>>
>> I thought you were going to add something in drivers/pci to make this more
>> generic?
>
> I don't see how. The only thing I can add is the check which I still
> plan to do. But the decision on the address is in the arch.

Yep, the check is what I meant.  That should at least allow the driver
to fail gracefully instead of crashing the machine.

>> Is it feasible to structure this series as follows?
>>
>>   - add generic stuff (struct pci_dev bit, msi.c checks)
>>   - add quirks to drivers to set the pci_dev bit
>>   - change powerpc to check pci_dev bit instead of pdn bit
>>   - remove powerpc-specific quirks since nobody looks as the pdn bit
>>
>> I think it would be easier to follow if the powerpc-specific parts weren't
>> intertwined with the rest.
>
> Well, in that case the first patch just adds one bit to pci_dev and
> maybe one check for address to msi.c. Unfortunately there isn't more
> I can do generically. But ok, I'll do it that way.
>
>> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> > CC: <stable@vger.kernel.org>
>> > ---
>> >
>> > v2: Rename flag to "no_64bit_msi" as suggested by Bjorn
>> >
>> >  arch/powerpc/include/asm/pci-bridge.h     | 2 --
>> >  arch/powerpc/kernel/pci_64.c              | 5 +----
>> >  arch/powerpc/platforms/powernv/pci-ioda.c | 3 +--
>> >  arch/powerpc/platforms/powernv/pci.c      | 3 +--
>> >  arch/powerpc/platforms/pseries/msi.c      | 2 +-
>> >  include/linux/pci.h                       | 1 +
>> >  6 files changed, 5 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
>> > index 4ca90a3..725247b 100644
>> > --- a/arch/powerpc/include/asm/pci-bridge.h
>> > +++ b/arch/powerpc/include/asm/pci-bridge.h
>> > @@ -159,8 +159,6 @@ struct pci_dn {
>> >
>> >     int     pci_ext_config_space;   /* for pci devices */
>> >
>> > -   bool    force_32bit_msi;
>> > -
>> >     struct  pci_dev *pcidev;        /* back-pointer to the pci device */
>> >  #ifdef CONFIG_EEH
>> >     struct eeh_dev *edev;           /* eeh device */
>> > diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c
>> > index 155013d..d41a831 100644
>> > --- a/arch/powerpc/kernel/pci_64.c
>> > +++ b/arch/powerpc/kernel/pci_64.c
>> > @@ -269,10 +269,7 @@ EXPORT_SYMBOL(pcibus_to_node);
>> >
>> >  static void quirk_radeon_32bit_msi(struct pci_dev *dev)
>> >  {
>> > -   struct pci_dn *pdn = pci_get_pdn(dev);
>> > -
>> > -   if (pdn)
>> > -           pdn->force_32bit_msi = true;
>> > +   dev->no_64bit_msi = true;
>> >  }
>> >  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x68f2, quirk_radeon_32bit_msi);
>> >  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0xaa68, quirk_radeon_32bit_msi);
>> > diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>> > index df241b1..a188bb8 100644
>> > --- a/arch/powerpc/platforms/powernv/pci-ioda.c
>> > +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>> > @@ -1311,7 +1311,6 @@ static int pnv_pci_ioda_msi_setup(struct pnv_phb *phb, struct pci_dev *dev,
>> >                               unsigned int is_64, struct msi_msg *msg)
>> >  {
>> >     struct pnv_ioda_pe *pe = pnv_ioda_get_pe(dev);
>> > -   struct pci_dn *pdn = pci_get_pdn(dev);
>> >     struct irq_data *idata;
>> >     struct irq_chip *ichip;
>> >     unsigned int xive_num = hwirq - phb->msi_base;
>> > @@ -1327,7 +1326,7 @@ static int pnv_pci_ioda_msi_setup(struct pnv_phb *phb, struct pci_dev *dev,
>> >             return -ENXIO;
>> >
>> >     /* Force 32-bit MSI on some broken devices */
>> > -   if (pdn && pdn->force_32bit_msi)
>> > +   if (dev->no_64bit_msi)
>> >             is_64 = 0;
>> >
>> >     /* Assign XIVE to PE */
>> > diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
>> > index b854b57..89c6608 100644
>> > --- a/arch/powerpc/platforms/powernv/pci.c
>> > +++ b/arch/powerpc/platforms/powernv/pci.c
>> > @@ -50,9 +50,8 @@ static int pnv_msi_check_device(struct pci_dev* pdev, int nvec, int type)
>> >  {
>> >     struct pci_controller *hose = pci_bus_to_host(pdev->bus);
>> >     struct pnv_phb *phb = hose->private_data;
>> > -   struct pci_dn *pdn = pci_get_pdn(pdev);
>> >
>> > -   if (pdn && pdn->force_32bit_msi && !phb->msi32_support)
>> > +   if (pdev->no_64bit_msi && !phb->msi32_support)
>> >             return -ENODEV;
>> >
>> >     return (phb && phb->msi_bmp.bitmap) ? 0 : -ENODEV;
>> > diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
>> > index 18ff462..6fd96d8 100644
>> > --- a/arch/powerpc/platforms/pseries/msi.c
>> > +++ b/arch/powerpc/platforms/pseries/msi.c
>> > @@ -429,7 +429,7 @@ static int rtas_setup_msi_irqs(struct pci_dev *pdev, int nvec_in, int type)
>> >      */
>> >  again:
>> >     if (type == PCI_CAP_ID_MSI) {
>> > -           if (pdn->force_32bit_msi) {
>> > +           if (pdev->no_64bit_msi) {
>> >                     rc = rtas_change_msi(pdn, RTAS_CHANGE_32MSI_FN, nvec);
>> >                     if (rc < 0) {
>> >                             /*
>> > diff --git a/include/linux/pci.h b/include/linux/pci.h
>> > index 96453f9..fc938003 100644
>> > --- a/include/linux/pci.h
>> > +++ b/include/linux/pci.h
>> > @@ -331,6 +331,7 @@ struct pci_dev {
>> >     unsigned int    is_added:1;
>> >     unsigned int    is_busmaster:1; /* device is busmaster */
>> >     unsigned int    no_msi:1;       /* device may not use msi */
>> > +   unsigned int    no_64bit_msi:1; /* Device has broken 64-bit MSIs */
>> >     unsigned int    block_cfg_access:1;     /* config space access is blocked */
>> >     unsigned int    broken_parity_status:1; /* Device generates false positive parity */
>> >     unsigned int    irq_reroute_variant:2;  /* device needs IRQ rerouting variant */
>> >
>> >
>> >
>
>