[RFC/PATCH] remove gratuitous reads from maple pci config space methods

[RFC/PATCH] remove gratuitous reads from maple pci config space methods

[Nathan Lynch]

The maple pci configuration space write methods read the written
location immediately after the write is performed, presumably in order
to flush the write.  However, configuration space writes are not
allowed to be posted, making these reads gratuitous.  Furthermore,
this behavior potentially causes us to violate the PCI PM spec when
changing between e.g. D0 and D3 states, because a delay of up to 10ms
may be required before the OS accesses configuration space after the
write which initiates the transition.  It definitely causes a system
hang for me with a Broadcom 5721 PCIE network adapter, which is fixed
by this change.

Remove the gratuitous reads from u3_agp_write_config,
u3_ht_write_config, and u4_pcie_write_config.

Signed-off-by: Nathan Lynch <ntl@pobox.com>
---
 arch/powerpc/platforms/maple/pci.c |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/platforms/maple/pci.c b/arch/powerpc/platforms/maple/pci.c
index 2542403..b095eaa 100644
--- a/arch/powerpc/platforms/maple/pci.c
+++ b/arch/powerpc/platforms/maple/pci.c
@@ -169,15 +169,12 @@ static int u3_agp_write_config(struct pci_bus *bus, unsigned int devfn,
 	switch (len) {
 	case 1:
 		out_8(addr, val);
-		(void) in_8(addr);
 		break;
 	case 2:
 		out_le16(addr, val);
-		(void) in_le16(addr);
 		break;
 	default:
 		out_le32(addr, val);
-		(void) in_le32(addr);
 		break;
 	}
 	return PCIBIOS_SUCCESSFUL;
@@ -268,15 +265,12 @@ static int u3_ht_write_config(struct pci_bus *bus, unsigned int devfn,
 	switch (len) {
 	case 1:
 		out_8(addr, val);
-		(void) in_8(addr);
 		break;
 	case 2:
 		out_le16(addr, val);
-		(void) in_le16(addr);
 		break;
 	default:
 		out_le32(addr, val);
-		(void) in_le32(addr);
 		break;
 	}
 	return PCIBIOS_SUCCESSFUL;
@@ -376,15 +370,12 @@ static int u4_pcie_write_config(struct pci_bus *bus, unsigned int devfn,
         switch (len) {
         case 1:
                 out_8(addr, val);
-                (void) in_8(addr);
                 break;
         case 2:
                 out_le16(addr, val);
-                (void) in_le16(addr);
                 break;
         default:
                 out_le32(addr, val);
-                (void) in_le32(addr);
                 break;
         }
         return PCIBIOS_SUCCESSFUL;
-- 
1.5.2.4

Re: [RFC/PATCH] remove gratuitous reads from maple pci config space methods

[Benjamin Herrenschmidt]

On Wed, 2007-08-08 at 19:50 -0500, Nathan Lynch wrote:
> The maple pci configuration space write methods read the written
> location immediately after the write is performed, presumably in order
> to flush the write.  However, configuration space writes are not
> allowed to be posted, making these reads gratuitous.  Furthermore,
> this behavior potentially causes us to violate the PCI PM spec when
> changing between e.g. D0 and D3 states, because a delay of up to 10ms
> may be required before the OS accesses configuration space after the
> write which initiates the transition.  It definitely causes a system
> hang for me with a Broadcom 5721 PCIE network adapter, which is fixed
> by this change.
> 
> Remove the gratuitous reads from u3_agp_write_config,
> u3_ht_write_config, and u4_pcie_write_config.
> 
> Signed-off-by: Nathan Lynch <ntl@pobox.com>

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

Thanks ! Care to fix powermac too ? :-)

Cheers,
Ben.

> ---
>  arch/powerpc/platforms/maple/pci.c |    9 ---------
>  1 files changed, 0 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/maple/pci.c b/arch/powerpc/platforms/maple/pci.c
> index 2542403..b095eaa 100644
> --- a/arch/powerpc/platforms/maple/pci.c
> +++ b/arch/powerpc/platforms/maple/pci.c
> @@ -169,15 +169,12 @@ static int u3_agp_write_config(struct pci_bus *bus, unsigned int devfn,
>  	switch (len) {
>  	case 1:
>  		out_8(addr, val);
> -		(void) in_8(addr);
>  		break;
>  	case 2:
>  		out_le16(addr, val);
> -		(void) in_le16(addr);
>  		break;
>  	default:
>  		out_le32(addr, val);
> -		(void) in_le32(addr);
>  		break;
>  	}
>  	return PCIBIOS_SUCCESSFUL;
> @@ -268,15 +265,12 @@ static int u3_ht_write_config(struct pci_bus *bus, unsigned int devfn,
>  	switch (len) {
>  	case 1:
>  		out_8(addr, val);
> -		(void) in_8(addr);
>  		break;
>  	case 2:
>  		out_le16(addr, val);
> -		(void) in_le16(addr);
>  		break;
>  	default:
>  		out_le32(addr, val);
> -		(void) in_le32(addr);
>  		break;
>  	}
>  	return PCIBIOS_SUCCESSFUL;
> @@ -376,15 +370,12 @@ static int u4_pcie_write_config(struct pci_bus *bus, unsigned int devfn,
>          switch (len) {
>          case 1:
>                  out_8(addr, val);
> -                (void) in_8(addr);
>                  break;
>          case 2:
>                  out_le16(addr, val);
> -                (void) in_le16(addr);
>                  break;
>          default:
>                  out_le32(addr, val);
> -                (void) in_le32(addr);
>                  break;
>          }
>          return PCIBIOS_SUCCESSFUL;

Re: [RFC/PATCH] remove gratuitous reads from maple pci config space methods

[Nathan Lynch]

Benjamin Herrenschmidt wrote:
> On Wed, 2007-08-08 at 19:50 -0500, Nathan Lynch wrote:
> > 
> > Remove the gratuitous reads from u3_agp_write_config,
> > u3_ht_write_config, and u4_pcie_write_config.
> > 
> > Signed-off-by: Nathan Lynch <ntl@pobox.com>
> 
> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> 
> Thanks ! Care to fix powermac too ? :-)

Sure, I'll get it tomorrow... looks like pasemi cribbed the powermac
code too :)

Re: [RFC/PATCH] remove gratuitous reads from maple pci config space methods

[Benjamin Herrenschmidt]

On Wed, 2007-08-08 at 20:04 -0500, Nathan Lynch wrote:
> Benjamin Herrenschmidt wrote:
> > On Wed, 2007-08-08 at 19:50 -0500, Nathan Lynch wrote:
> > > 
> > > Remove the gratuitous reads from u3_agp_write_config,
> > > u3_ht_write_config, and u4_pcie_write_config.
> > > 
> > > Signed-off-by: Nathan Lynch <ntl@pobox.com>
> > 
> > Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> > 
> > Thanks ! Care to fix powermac too ? :-)
> 
> Sure, I'll get it tomorrow... looks like pasemi cribbed the powermac
> code too :)

Allright, thanks !

Cheersm
Ben.

Re: [RFC/PATCH] remove gratuitous reads from maple pci config space methods

[Olof Johansson]

On Wed, Aug 08, 2007 at 08:04:32PM -0500, Nathan Lynch wrote:
> Benjamin Herrenschmidt wrote:
> > On Wed, 2007-08-08 at 19:50 -0500, Nathan Lynch wrote:
> > > 
> > > Remove the gratuitous reads from u3_agp_write_config,
> > > u3_ht_write_config, and u4_pcie_write_config.
> > > 
> > > Signed-off-by: Nathan Lynch <ntl@pobox.com>
> > 
> > Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> > 
> > Thanks ! Care to fix powermac too ? :-)
> 
> Sure, I'll get it tomorrow... looks like pasemi cribbed the powermac
> code too :)

Yeah, it originated either from powermac or maple so it's there too. Feel
free to fix it too while you're at it.


-Olof

Re: [RFC/PATCH] remove gratuitous reads from maple pci config space methods

[David Gibson]

On Wed, Aug 08, 2007 at 07:50:44PM -0500, Nathan Lynch wrote:
> The maple pci configuration space write methods read the written
> location immediately after the write is performed, presumably in order
> to flush the write.  However, configuration space writes are not
> allowed to be posted, making these reads gratuitous.

It might be worth checking that there isn't a particular reason for
these.  Just because posting writes are forbidden doesn't mean a
particular bridge won't screw it up...

> Furthermore,
> this behavior potentially causes us to violate the PCI PM spec when
> changing between e.g. D0 and D3 states, because a delay of up to 10ms
> may be required before the OS accesses configuration space after the
> write which initiates the transition.  It definitely causes a system
> hang for me with a Broadcom 5721 PCIE network adapter, which is fixed
> by this change.
> 
> Remove the gratuitous reads from u3_agp_write_config,
> u3_ht_write_config, and u4_pcie_write_config.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [RFC/PATCH] remove gratuitous reads from maple pci config space methods

[Nathan Lynch]

David Gibson wrote:
> On Wed, Aug 08, 2007 at 07:50:44PM -0500, Nathan Lynch wrote:
> > The maple pci configuration space write methods read the written
> > location immediately after the write is performed, presumably in order
> > to flush the write.  However, configuration space writes are not
> > allowed to be posted, making these reads gratuitous.
> 
> It might be worth checking that there isn't a particular reason for
> these.  Just because posting writes are forbidden doesn't mean a
> particular bridge won't screw it up...

Well, I had already checked with Ben, who wrote the code, and my
understanding is that the reads are intended to work around some
misbehaving Apple bridges, but that a sync after the write (implied by
releasing pci_lock in the generic pci code) should suffice for those.

Re: [RFC/PATCH] remove gratuitous reads from maple pci config space methods

[David Gibson]

On Wed, Aug 08, 2007 at 11:16:32PM -0500, Nathan Lynch wrote:
> David Gibson wrote:
> > On Wed, Aug 08, 2007 at 07:50:44PM -0500, Nathan Lynch wrote:
> > > The maple pci configuration space write methods read the written
> > > location immediately after the write is performed, presumably in order
> > > to flush the write.  However, configuration space writes are not
> > > allowed to be posted, making these reads gratuitous.
> > 
> > It might be worth checking that there isn't a particular reason for
> > these.  Just because posting writes are forbidden doesn't mean a
> > particular bridge won't screw it up...
> 
> Well, I had already checked with Ben, who wrote the code, and my
> understanding is that the reads are intended to work around some
> misbehaving Apple bridges, but that a sync after the write (implied by
> releasing pci_lock in the generic pci code) should suffice for
> those.

Ah, ok then.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [RFC/PATCH] remove gratuitous reads from maple pci config space methods

[Segher Boessenkool]

>> It might be worth checking that there isn't a particular reason for
>> these.  Just because posting writes are forbidden doesn't mean a
>> particular bridge won't screw it up...
>
> Well, I had already checked with Ben, who wrote the code, and my
> understanding is that the reads are intended to work around some
> misbehaving Apple bridges,

None of the PCI interfaces on the U3 or U4 bridges have that
problem as far as I know.  I think the workaround was copied
from code for older Apple bridges?

> but that a sync after the write (implied by
> releasing pci_lock in the generic pci code) should suffice for those.

I don't see how a sync could help here at all, not more than
an eieio anyway?


Segher

Re: [RFC/PATCH] remove gratuitous reads from maple pci config space methods

[Nathan Lynch]

Segher Boessenkool wrote:
> >>It might be worth checking that there isn't a particular reason for
> >>these.  Just because posting writes are forbidden doesn't mean a
> >>particular bridge won't screw it up...
> >
> >Well, I had already checked with Ben, who wrote the code, and my
> >understanding is that the reads are intended to work around some
> >misbehaving Apple bridges,
> 
> None of the PCI interfaces on the U3 or U4 bridges have that
> problem as far as I know.  I think the workaround was copied
> from code for older Apple bridges?

Okay, then the change should be fine for maple.

> >but that a sync after the write (implied by
> >releasing pci_lock in the generic pci code) should suffice for those.
> 
> I don't see how a sync could help here at all, not more than
> an eieio anyway?

Alright, well, maybe take it up with Ben when I post the patch for
powermac, since that's where it could actually matter.

Re: [RFC/PATCH] remove gratuitous reads from maple pci config space methods

[Segher Boessenkool]

>>> Well, I had already checked with Ben, who wrote the code, and my
>>> understanding is that the reads are intended to work around some
>>> misbehaving Apple bridges,
>>
>> None of the PCI interfaces on the U3 or U4 bridges have that
>> problem as far as I know.  I think the workaround was copied
>> from code for older Apple bridges?
>
> Okay, then the change should be fine for maple.

Yes.  Of course, as usual, testing is needed, yada yada.

>>> but that a sync after the write (implied by
>>> releasing pci_lock in the generic pci code) should suffice for those.
>>
>> I don't see how a sync could help here at all, not more than
>> an eieio anyway?
>
> Alright, well, maybe take it up with Ben when I post the patch for
> powermac, since that's where it could actually matter.

It should be fine on PowerMac as well -- all G5s use U3/U4,
the workaround is for certain older Apple bridge chips.


Segher

Re: [RFC/PATCH] remove gratuitous reads from maple pci config space methods

[Benjamin Herrenschmidt]

> It should be fine on PowerMac as well -- all G5s use U3/U4,
> the workaround is for certain older Apple bridge chips.

Yeah, remove them. The workaround that is needed afaik is really only
the one that reads back the -address- before accessing the data register
(and I think it's sill needed on U3), but it's unrelated to what that
patch changes.

As I said on IRC, I suspect those reads come from misguided attempts on
my part to avoid the processor itself posting those writes, since we
want config space access to be synchronous all the way. In that case,
however, a sync will do the job just fine to ensure the previous write
did hit the bridge.

Cheers,
Ben.