PCI probe leaves master abort disabled in PCI_BRIDGE_CONTROL

PCI probe leaves master abort disabled in PCI_BRIDGE_CONTROL

[David S. Miller]

In drivers/pci/probe.c:pci_scan_bridge(), if this is not the first
pass (pass != 0) we don't restore the PCI_BRIDGE_CONTROL_REGISTER and
thus leave PCI_BRIDGE_CTL_MASTER_ABORT off:

int __devinit pci_scan_bridge(struct pci_bus *bus, struct pci_dev * dev, int max, int pass)
{
 ...
	/* Disable MasterAbortMode during probing to avoid reporting
	   of bus errors (in some architectures) */ 
	pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &bctl);
	pci_write_config_word(dev, PCI_BRIDGE_CONTROL,
			      bctl & ~PCI_BRIDGE_CTL_MASTER_ABORT);
 ...
	if ((buses & 0xffff00) && !pcibios_assign_all_busses() && !is_cardbus) {
		unsigned int cmax, busnr;
		/*
		 * Bus already configured by firmware, process it in the first
		 * pass and just note the configuration.
		 */
		if (pass)
			return max;
 ...
	}

	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl);
 ...

This doesn't seem intentional.

Re: PCI probe leaves master abort disabled in PCI_BRIDGE_CONTROL

[Greg KH]

On Mon, Feb 13, 2006 at 05:13:21PM -0800, David S. Miller wrote:
> 
> In drivers/pci/probe.c:pci_scan_bridge(), if this is not the first
> pass (pass != 0) we don't restore the PCI_BRIDGE_CONTROL_REGISTER and
> thus leave PCI_BRIDGE_CTL_MASTER_ABORT off:
> 
> int __devinit pci_scan_bridge(struct pci_bus *bus, struct pci_dev * dev, int max, int pass)
> {
>  ...
> 	/* Disable MasterAbortMode during probing to avoid reporting
> 	   of bus errors (in some architectures) */ 
> 	pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &bctl);
> 	pci_write_config_word(dev, PCI_BRIDGE_CONTROL,
> 			      bctl & ~PCI_BRIDGE_CTL_MASTER_ABORT);
>  ...
> 	if ((buses & 0xffff00) && !pcibios_assign_all_busses() && !is_cardbus) {
> 		unsigned int cmax, busnr;
> 		/*
> 		 * Bus already configured by firmware, process it in the first
> 		 * pass and just note the configuration.
> 		 */
> 		if (pass)
> 			return max;
>  ...
> 	}
> 
> 	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl);
>  ...
> 
> This doesn't seem intentional.

No it doesn't.  Ralf added that code, way back in 2.6.2-rc2, with a
patch description of "[PATCH] PCI: fix probing for some mips systems"

David, does this break some stuff on your boxes?

Ralf, any thoughts?  It was way back in January of 2004 that this change
went in.

thanks,

greg k-h

Re: PCI probe leaves master abort disabled in PCI_BRIDGE_CONTROL

[Ralf Baechle]

On Mon, Feb 13, 2006 at 09:17:00PM -0800, Greg KH wrote:
> Date:	Mon, 13 Feb 2006 21:17:00 -0800
> From:	Greg KH <gregkh@suse.de>
> To:	"David S. Miller" <davem@davemloft.net>
> Cc:	linux-kernel@vger.kernel.org, ralf@linux-mips.org,
> 	linux-pci@atrey.karlin.mff.cuni.cz
> Subject: Re: PCI probe leaves master abort disabled in PCI_BRIDGE_CONTROL
> Content-Type: text/plain; charset=us-ascii
> 
> On Mon, Feb 13, 2006 at 05:13:21PM -0800, David S. Miller wrote:
> > 
> > In drivers/pci/probe.c:pci_scan_bridge(), if this is not the first
> > pass (pass != 0) we don't restore the PCI_BRIDGE_CONTROL_REGISTER and
> > thus leave PCI_BRIDGE_CTL_MASTER_ABORT off:
> > 
> > int __devinit pci_scan_bridge(struct pci_bus *bus, struct pci_dev * dev, int max, int pass)
> > {
> >  ...
> > 	/* Disable MasterAbortMode during probing to avoid reporting
> > 	   of bus errors (in some architectures) */ 
> > 	pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &bctl);
> > 	pci_write_config_word(dev, PCI_BRIDGE_CONTROL,
> > 			      bctl & ~PCI_BRIDGE_CTL_MASTER_ABORT);
> >  ...
> > 	if ((buses & 0xffff00) && !pcibios_assign_all_busses() && !is_cardbus) {
> > 		unsigned int cmax, busnr;
> > 		/*
> > 		 * Bus already configured by firmware, process it in the first
> > 		 * pass and just note the configuration.
> > 		 */
> > 		if (pass)
> > 			return max;
> >  ...
> > 	}
> > 
> > 	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl);
> >  ...
> > 
> > This doesn't seem intentional.
> 
> No it doesn't.  Ralf added that code, way back in 2.6.2-rc2, with a
> patch description of "[PATCH] PCI: fix probing for some mips systems"
> 
> David, does this break some stuff on your boxes?
> 
> Ralf, any thoughts?  It was way back in January of 2004 that this change
> went in.

Agreed, looks like an accident.  The patch [1] originally came from Kip
Walker (Broadcom back then) between 2.6.0-test3 and 2.6.0-test4.  As I
recall it was supposed to fix an issue with with PCI aborts being
signalled by the PCI bridge of the Broadcom BCM1250 family of SOCs when
probing behind pci_scan_bridge.  It is undeseriable to disable
PCI_BRIDGE_CTL_MASTER_ABORT in pci_{read,write)_config_* and the
behaviour wasn't considered a bug in need of a workaround, so this was
put in probe.c.

I don't have an affected system at hand, so can't really test but I
propose something like the below patch.

  Ralf

[1] http://www.linux-mips.org/git?p=linux.git;a=commit;h=599457e0cb702a31a3247ea6a5d9c6c99c4cf195

[PCI] Avoid leaving MASTER_ABORT disabled permanently when returning from pci_scan_bridge.

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index adfad4f..6de7c59 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -456,7 +456,7 @@ int __devinit pci_scan_bridge(struct pci
 		 * pass and just note the configuration.
 		 */
 		if (pass)
-			return max;
+			goto out;
 		busnr = (buses >> 8) & 0xFF;
 
 		/*
@@ -466,12 +466,12 @@ int __devinit pci_scan_bridge(struct pci
 		if (pci_find_bus(pci_domain_nr(bus), busnr)) {
 			printk(KERN_INFO "PCI: Bus %04x:%02x already known\n",
 					pci_domain_nr(bus), busnr);
-			return max;
+			goto out;
 		}
 
 		child = pci_add_new_bus(bus, dev, busnr);
 		if (!child)
-			return max;
+			goto out;
 		child->primary = buses & 0xFF;
 		child->subordinate = (buses >> 16) & 0xFF;
 		child->bridge_ctl = bctl;
@@ -496,7 +496,7 @@ int __devinit pci_scan_bridge(struct pci
 				   bus ranges. */
 				pci_write_config_dword(dev, PCI_PRIMARY_BUS,
 						       buses & ~0xffffff);
-			return max;
+			goto out;
 		}
 
 		/* Clear errors */
@@ -505,7 +505,7 @@ int __devinit pci_scan_bridge(struct pci
 		/* Prevent assigning a bus number that already exists.
 		 * This can happen when a bridge is hot-plugged */
 		if (pci_find_bus(pci_domain_nr(bus), max+1))
-			return max;
+			goto out;
 		child = pci_add_new_bus(bus, dev, ++max);
 		buses = (buses & 0xff000000)
 		      | ((unsigned int)(child->primary)     <<  0)
@@ -576,8 +576,6 @@ int __devinit pci_scan_bridge(struct pci
 		pci_write_config_byte(dev, PCI_SUBORDINATE_BUS, max);
 	}
 
-	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl);
-
 	sprintf(child->name, (is_cardbus ? "PCI CardBus #%02x" : "PCI Bus #%02x"), child->number);
 
 	while (bus->parent) {
@@ -596,6 +594,9 @@ int __devinit pci_scan_bridge(struct pci
 		bus = bus->parent;
 	}
 
+out:
+	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl);
+
 	return max;
 }
 

Re: PCI probe leaves master abort disabled in PCI_BRIDGE_CONTROL

[David S. Miller]

From: Ralf Baechle <ralf@linux-mips.org>
Date: Tue, 14 Feb 2006 16:23:57 +0000

> I don't have an affected system at hand, so can't really test but I
> propose something like the below patch.

Looks good to me.