[PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[Yijing Wang]

Commit d0751b98dfa3 ("PCI: Add dev->has_secondary_link to
track downstream PCIe links")assumed root port is always
the top device in pcie tree. But on sparc64 V245 and T2000
the pcie tree has no root port device in top level, the
upstream port device is connected directly to root bus.
So we may get NULL parent for this upstream port device.

Upstream port ------ Downstream port ------PCIe-PCI bridge

Reported-by: Meelis Roos <mroos@linux.ee>
Tested-by: Meelis Roos <mroos@linux.ee>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/probe.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 51ebb97..cd54298 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1003,7 +1003,10 @@ void set_pcie_port_type(struct pci_dev *pdev)
 	else if (type == PCI_EXP_TYPE_UPSTREAM ||
 		 type == PCI_EXP_TYPE_DOWNSTREAM) {
 		parent = pci_upstream_bridge(pdev);
-		if (!parent->has_secondary_link)
+		/* For sparc64 V245 and T2000, upstream port may be
+		 * the top level device, connect to root bus directly.
+		 */
+		if (parent && !parent->has_secondary_link)
 			pdev->has_secondary_link = 1;
 	}
 }
-- 
1.7.1

Re: [PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[Bjorn Helgaas]

[+cc Dave, Eric, Ben, sparclinux]

On Mon, Aug 17, 2015 at 06:47:58PM +0800, Yijing Wang wrote:
> Commit d0751b98dfa3 ("PCI: Add dev->has_secondary_link to
> track downstream PCIe links")assumed root port is always
> the top device in pcie tree. But on sparc64 V245 and T2000
> the pcie tree has no root port device in top level, the
> upstream port device is connected directly to root bus.
> So we may get NULL parent for this upstream port device.
> 
> Upstream port ------ Downstream port ------PCIe-PCI bridge

This is an unusual, possibly even illegal, PCIe topology because it
lacks a Root Port at the top of the hierarchy.  From lspci output [1]
collected by Meelis, the top-level devices are:

  0000:02:00.0 PCI bridge to [bus 03-0d]    Upstream Port
  0001:02:00.0 PCI bridge to [bus 03]       PCIe to PCI/PCI-X Bridge
  0001:02:00.2 PCI bridge to [bus 04]       PCIe to PCI/PCI-X Bridge

There *should* be two Root Ports, one leading to 0000:02 and another
leading to 0001:02.1

I suspect this is an artifact of the way sparc and powerpc use OF to
enumerate PCI.  I suspect there actually *is* a Root Port in the
hardware: why would the hardware designers go to the trouble of
working out special-case rules and behavior for a non-standard PCIe
hierarchy when it would be much easier to reuse existing hardware?

As long as the Root Port is configured correctly, Linux can pretend it
doesn't exist and things will mostly just work.  Except for a few
corner cases like this one.

I really hate to put special case workarounds like this patch in the
generic code.  It's likely that we'd trip over the same issue again in
other areas, e.g., MPS or other configuration.

Sparc & powerpc guys, any input on this?

Bjorn

[1] http://lkml.kernel.org/r/alpine.LRH.2.20.1508131111570.9039@math.ut.ee

> Reported-by: Meelis Roos <mroos@linux.ee>
> Tested-by: Meelis Roos <mroos@linux.ee>
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  drivers/pci/probe.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 51ebb97..cd54298 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1003,7 +1003,10 @@ void set_pcie_port_type(struct pci_dev *pdev)
>  	else if (type == PCI_EXP_TYPE_UPSTREAM ||
>  		 type == PCI_EXP_TYPE_DOWNSTREAM) {
>  		parent = pci_upstream_bridge(pdev);
> -		if (!parent->has_secondary_link)
> +		/* For sparc64 V245 and T2000, upstream port may be
> +		 * the top level device, connect to root bus directly.
> +		 */
> +		if (parent && !parent->has_secondary_link)
>  			pdev->has_secondary_link = 1;
>  	}
>  }
> -- 
> 1.7.1
> 

Re: [PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[David Miller]

From: Bjorn Helgaas <bhelgaas@google.com>
Date: Tue, 18 Aug 2015 13:49:43 -0500

> [+cc Dave, Eric, Ben, sparclinux]
> 
> On Mon, Aug 17, 2015 at 06:47:58PM +0800, Yijing Wang wrote:
>> Commit d0751b98dfa3 ("PCI: Add dev->has_secondary_link to
>> track downstream PCIe links")assumed root port is always
>> the top device in pcie tree. But on sparc64 V245 and T2000
>> the pcie tree has no root port device in top level, the
>> upstream port device is connected directly to root bus.
>> So we may get NULL parent for this upstream port device.
>> 
>> Upstream port ------ Downstream port ------PCIe-PCI bridge
> 
> This is an unusual, possibly even illegal, PCIe topology because it
> lacks a Root Port at the top of the hierarchy.  From lspci output [1]
> collected by Meelis, the top-level devices are:

The root port is in the PCI controller on sparc64, and PCI controllers
don't have actual real PCI configuration space available.

I used to put dummy PCI bus nodes into the tree and do provide dummy
software PCI config space methods, but that caused more harm than
good.

So you'll have to accomodate situations where the root port is not
there in the Linux kernel PCI bus hierarchy.

Re: [PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[Bjorn Helgaas]

On Tue, Aug 18, 2015 at 12:10:04PM -0700, David Miller wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> Date: Tue, 18 Aug 2015 13:49:43 -0500
> 
> > [+cc Dave, Eric, Ben, sparclinux]
> > 
> > On Mon, Aug 17, 2015 at 06:47:58PM +0800, Yijing Wang wrote:
> >> Commit d0751b98dfa3 ("PCI: Add dev->has_secondary_link to
> >> track downstream PCIe links")assumed root port is always
> >> the top device in pcie tree. But on sparc64 V245 and T2000
> >> the pcie tree has no root port device in top level, the
> >> upstream port device is connected directly to root bus.
> >> So we may get NULL parent for this upstream port device.
> >> 
> >> Upstream port ------ Downstream port ------PCIe-PCI bridge
> > 
> > This is an unusual, possibly even illegal, PCIe topology because it
> > lacks a Root Port at the top of the hierarchy.  From lspci output [1]
> > collected by Meelis, the top-level devices are:
> 
> The root port is in the PCI controller on sparc64, and PCI controllers
> don't have actual real PCI configuration space available.
> 
> I used to put dummy PCI bus nodes into the tree and do provide dummy
> software PCI config space methods, but that caused more harm than
> good.

I guess you're talking about what you removed with c26d3c013897
("sparc64: Stop creating dummy root PCI host controller devices."),
which talks about duplicate sysfs and procfs nodes.  Removing the
dummy devices avoids the duplicate node problem, but it doesn't fix
the root cause, so it's probably not the only possible resolution.

We'll probably have to do something ugly like Yijing's patch just
because v4.2 is imminent and I don't want it to be broken.

I don't like it because we have these PCIe links where we only know
about devices on one end of the link, and then we can't manage
ASPM/MPS/etc.  But we've had this situation for several years, so
I guess nobody cares too much about those things on the affected
mahines.

Bjorn

Re: [PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[Benjamin Herrenschmidt]

On Wed, 2015-08-19 at 17:16 -0500, Bjorn Helgaas wrote:
> On Tue, Aug 18, 2015 at 12:10:04PM -0700, David Miller wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > Date: Tue, 18 Aug 2015 13:49:43 -0500
> > 
> > > [+cc Dave, Eric, Ben, sparclinux]
> > > 
> > > On Mon, Aug 17, 2015 at 06:47:58PM +0800, Yijing Wang wrote:
> > > > Commit d0751b98dfa3 ("PCI: Add dev->has_secondary_link to
> > > > track downstream PCIe links")assumed root port is always
> > > > the top device in pcie tree. But on sparc64 V245 and T2000tthe
> > > > pcie tree has no root port device in top level, the
> > > > upstream port device is connected directly to root bus.
> > > > So we may get NULL parent for this upstream port device.

Picking up that thread half way through...

> > > > Upstream port ------ Downstream port ------PCIe-PCI bridge
> > > 
> > > This is an unusual, possibly even illegal, PCIe topology because 
> > > it lacks a Root Port at the top of the hierarchy.  From lspci
> > > output
> > > [1]
> > > collected by Meelis, the top-level devices are:

Nobody cares what is "illegal", what matters is what HW actually does :
-) Specs only matter as far as they get followed. In any case, it also
happens on powerpc, when running under a hypervisor such as IBM
PowerVM, PCIe devices appear directly under a virtual host bridge which
is not exposed as a PCIe root complex.

> > The root port is in the PCI controller on sparc64, and PCI
> > controllers
> > don't have actual real PCI configuration space available.
> > 
> > I used to put dummy PCI bus nodes into the tree and do provide
> > dummy
> > software PCI config space methods, but that caused more harm than
> > good.
> 
> I guess you're talking about what you removed with c26d3c013897
> ("sparc64: Stop creating dummy root PCI host controller devices."),
> which talks about duplicate sysfs and procfs nodes.  Removing the
> dummy devices avoids the duplicate node problem, but it doesn't fix
> the root cause, so it's probably not the only possible resolution.
> 
> We'll probably have to do something ugly like Yijing's patch just
> because v4.2 is imminent and I don't want it to be broken.
> 
> I don't like it because we have these PCIe links where we only know
> about devices on one end of the link, and then we can't manage
> ASPM/MPS/etc.  But we've had this situation for several years, so
> I guess nobody cares too much about those things on the affected
> mahines.

Correct, we have this situation on more than just sparc64 and we
shouldn't crash. Just don't touch ASPM/MPS/etc... in those cases.

Cheers,
Ben.

Re: [PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[Bjorn Helgaas]

On Wed, Aug 19, 2015 at 3:29 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2015-08-19 at 17:16 -0500, Bjorn Helgaas wrote:
>> On Tue, Aug 18, 2015 at 12:10:04PM -0700, David Miller wrote:
>> > From: Bjorn Helgaas <bhelgaas@google.com>
>> > Date: Tue, 18 Aug 2015 13:49:43 -0500
>> >
>> > > [+cc Dave, Eric, Ben, sparclinux]
>> > >
>> > > On Mon, Aug 17, 2015 at 06:47:58PM +0800, Yijing Wang wrote:
>> > > > Commit d0751b98dfa3 ("PCI: Add dev->has_secondary_link to
>> > > > track downstream PCIe links")assumed root port is always
>> > > > the top device in pcie tree. But on sparc64 V245 and T2000tthe
>> > > > pcie tree has no root port device in top level, the
>> > > > upstream port device is connected directly to root bus.
>> > > > So we may get NULL parent for this upstream port device.
>
> Picking up that thread half way through...
>
>> > > > Upstream port ------ Downstream port ------PCIe-PCI bridge
>> > >
>> > > This is an unusual, possibly even illegal, PCIe topology because
>> > > it lacks a Root Port at the top of the hierarchy.  From lspci
>> > > output
>> > > [1]
>> > > collected by Meelis, the top-level devices are:
>
> Nobody cares what is "illegal", what matters is what HW actually does :
> -) Specs only matter as far as they get followed.

I agree.  But nobody can predict all the ways platforms violate specs,
so platforms that don't follow the spec don't work as well, and they
cause a maintenance burden for everybody else.

> In any case, it also
> happens on powerpc, when running under a hypervisor such as IBM
> PowerVM, PCIe devices appear directly under a virtual host bridge which
> is not exposed as a PCIe root complex.

Thanks for the confirmation.

>> I don't like it because we have these PCIe links where we only know
>> about devices on one end of the link, and then we can't manage
>> ASPM/MPS/etc.  But we've had this situation for several years, so
>> I guess nobody cares too much about those things on the affected
>> mahines.
>
> Correct, we have this situation on more than just sparc64 and we
> shouldn't crash. Just don't touch ASPM/MPS/etc... in those cases.

Easier said than done.  AFAIK, we won't crash after we add Yijing's
patch.  But it's unreasonable to expect people writing to the spec to
know the peculiarities of sparc64/powerpc, so there may be future
things we discover the hard way.  We'll just have to deal with them as
we find them.

Bjorn

Re: [PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[Benjamin Herrenschmidt]

On Wed, 2015-08-19 at 23:01 -0700, Bjorn Helgaas wrote:
> Easier said than done.  AFAIK, we won't crash after we add Yijing's
> patch.  But it's unreasonable to expect people writing to the spec to
> know the peculiarities of sparc64/powerpc, so there may be future
> things we discover the hard way.  We'll just have to deal with them as
> we find them.

I agree in general, you can't plan for all possible spec violations but
it's also pretty standard defensive coding to now blow up if stuff
doesn't look expected :)

Anyway, this one is sorted.

Cheers,
Ben.

Re: [PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[Kjetil Oftedal]

On 20/08/2015, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Aug 19, 2015 at 3:29 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
>> On Wed, 2015-08-19 at 17:16 -0500, Bjorn Helgaas wrote:
>>> On Tue, Aug 18, 2015 at 12:10:04PM -0700, David Miller wrote:
>>> > From: Bjorn Helgaas <bhelgaas@google.com>
>>> > Date: Tue, 18 Aug 2015 13:49:43 -0500
>>> >
>>> > > [+cc Dave, Eric, Ben, sparclinux]
>>> > >
>>> > > On Mon, Aug 17, 2015 at 06:47:58PM +0800, Yijing Wang wrote:
>>> > > > Commit d0751b98dfa3 ("PCI: Add dev->has_secondary_link to
>>> > > > track downstream PCIe links")assumed root port is always
>>> > > > the top device in pcie tree. But on sparc64 V245 and T2000tthe
>>> > > > pcie tree has no root port device in top level, the
>>> > > > upstream port device is connected directly to root bus.
>>> > > > So we may get NULL parent for this upstream port device.
>>
>> Picking up that thread half way through...
>>
>>> > > > Upstream port ------ Downstream port ------PCIe-PCI bridge
>>> > >
>>> > > This is an unusual, possibly even illegal, PCIe topology because
>>> > > it lacks a Root Port at the top of the hierarchy.  From lspci
>>> > > output
>>> > > [1]
>>> > > collected by Meelis, the top-level devices are:
>>
>> Nobody cares what is "illegal", what matters is what HW actually does :
>> -) Specs only matter as far as they get followed.
>
> I agree.  But nobody can predict all the ways platforms violate specs,
> so platforms that don't follow the spec don't work as well, and they
> cause a maintenance burden for everybody else.
>
>> In any case, it also
>> happens on powerpc, when running under a hypervisor such as IBM
>> PowerVM, PCIe devices appear directly under a virtual host bridge which
>> is not exposed as a PCIe root complex.
>
> Thanks for the confirmation.
>
>>> I don't like it because we have these PCIe links where we only know
>>> about devices on one end of the link, and then we can't manage
>>> ASPM/MPS/etc.  But we've had this situation for several years, so
>>> I guess nobody cares too much about those things on the affected
>>> mahines.
>>
>> Correct, we have this situation on more than just sparc64 and we
>> shouldn't crash. Just don't touch ASPM/MPS/etc... in those cases.
>
> Easier said than done.  AFAIK, we won't crash after we add Yijing's
> patch.  But it's unreasonable to expect people writing to the spec to
> know the peculiarities of sparc64/powerpc, so there may be future
> things we discover the hard way.  We'll just have to deal with them as
> we find them.
>
> Bjorn

In this particular case it was adding support for at illegal PCIe hierarchy
that broke support for another strange PCIe hierarchy. So I guess writing
to the spec is not really an option if Linux is to support all the strange
variants out in the wild. Less assumptions made on the basis of the
PCIe spec might prevent some of this breakage?

-
Kjetil

Re: [PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[David Miller]

From: Kjetil Oftedal <oftedal@gmail.com>
Date: Thu, 20 Aug 2015 08:44:45 +0200

> Less assumptions made on the basis of the PCIe spec might prevent
> some of this breakage?

+1

Re: [PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[David Miller]

From: Bjorn Helgaas <bhelgaas@google.com>
Date: Wed, 19 Aug 2015 23:01:19 -0700

> Easier said than done.  AFAIK, we won't crash after we add Yijing's
> patch.  But it's unreasonable to expect people writing to the spec to
> know the peculiarities of sparc64/powerpc, so there may be future
> things we discover the hard way.  We'll just have to deal with them as
> we find them.

I think it is unreasonable to assume that the PCI root complex is
presented as a device in the full PCI bus hierarchy.

If one wanted to chop the system up into multiple physical components,
hiding the PCI root complex behind some controlled entity (hypervisor
or whatever) is the only way to accomplish that.

Therefore it is a very reasonable and perhaps even expected design
decision to do what both powerpc and sparc64 have done.

Re: [PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[Bjorn Helgaas]

[+cc Dave, Eric, Ben, sparclinux]

On Mon, Aug 17, 2015 at 06:47:58PM +0800, Yijing Wang wrote:
> Commit d0751b98dfa3 ("PCI: Add dev->has_secondary_link to
> track downstream PCIe links")assumed root port is always
> the top device in pcie tree. But on sparc64 V245 and T2000
> the pcie tree has no root port device in top level, the
> upstream port device is connected directly to root bus.
> So we may get NULL parent for this upstream port device.
> 
> Upstream port ------ Downstream port ------PCIe-PCI bridge
> 
> Reported-by: Meelis Roos <mroos@linux.ee>
> Tested-by: Meelis Roos <mroos@linux.ee>
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  drivers/pci/probe.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 51ebb97..cd54298 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1003,7 +1003,10 @@ void set_pcie_port_type(struct pci_dev *pdev)
>  	else if (type == PCI_EXP_TYPE_UPSTREAM ||
>  		 type == PCI_EXP_TYPE_DOWNSTREAM) {
>  		parent = pci_upstream_bridge(pdev);
> -		if (!parent->has_secondary_link)
> +		/* For sparc64 V245 and T2000, upstream port may be
> +		 * the top level device, connect to root bus directly.
> +		 */
> +		if (parent && !parent->has_secondary_link)
>  			pdev->has_secondary_link = 1;
>  	}
>  }
> -- 
> 1.7.1
> 

Re: [PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[David Miller]

From: Bjorn Helgaas <bhelgaas@google.com>
Date: Thu, 20 Aug 2015 00:48:00 -0500

> [+cc Dave, Eric, Ben, sparclinux]

I think the comment is terrible.

It is not limited to those two systems, it is a general problem with
how the code assumes the root PCI-E port is instantiated in the PCI
bus hierarchy which will not be true on any system which has a PCI
controller that doesn't actually provide a bonafide config space to
access.

Re: [PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[Bjorn Helgaas]

On Thu, Aug 20, 2015 at 10:47 AM, David Miller <davem@davemloft.net> wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> Date: Thu, 20 Aug 2015 00:48:00 -0500
>
>> [+cc Dave, Eric, Ben, sparclinux]
>
> I think the comment is terrible.

I'll update the comment if you suggest some text.  Do you object
mainly to the mention of the specific systems?

> It is not limited to those two systems, it is a general problem with
> how the code assumes the root PCI-E port is instantiated in the PCI
> bus hierarchy which will not be true on any system which has a PCI
> controller that doesn't actually provide a bonafide config space to
> access.

I agree, we should not crash if the hardware we find isn't what we expect.

Bjorn

Re: [PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[David Miller]

From: Bjorn Helgaas <bhelgaas@google.com>
Date: Thu, 20 Aug 2015 11:23:43 -0700

> On Thu, Aug 20, 2015 at 10:47 AM, David Miller <davem@davemloft.net> wrote:
>> From: Bjorn Helgaas <bhelgaas@google.com>
>> Date: Thu, 20 Aug 2015 00:48:00 -0500
>>
>>> [+cc Dave, Eric, Ben, sparclinux]
>>
>> I think the comment is terrible.
> 
> I'll update the comment if you suggest some text.  Do you object
> mainly to the mention of the specific systems?

"PCI root complex can not be assumed to be instantiated in the PCI bus
 hierarchy."
 

Re: [PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[Bjorn Helgaas]

On Thu, Aug 20, 2015 at 11:40:42AM -0700, David Miller wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> Date: Thu, 20 Aug 2015 11:23:43 -0700
> 
> > On Thu, Aug 20, 2015 at 10:47 AM, David Miller <davem@davemloft.net> wrote:
> >> From: Bjorn Helgaas <bhelgaas@google.com>
> >> Date: Thu, 20 Aug 2015 00:48:00 -0500
> >>
> >>> [+cc Dave, Eric, Ben, sparclinux]
> >>
> >> I think the comment is terrible.
> > 
> > I'll update the comment if you suggest some text.  Do you object
> > mainly to the mention of the specific systems?
> 
> "PCI root complex can not be assumed to be instantiated in the PCI bus
>  hierarchy."

AIUI, the root complex itself (as distinct from root ports) doesn't
normally appear in the PCI hierarchy, so I reworded it as follows.  I hope
this helps address your objection, but if not, I can try again.

    PCI: Tolerate hierarchies with no Root Port
    
    We should not assume any particular hardware topology.  Commit d0751b98dfa3
    ("PCI: Add dev->has_secondary_link to track downstream PCIe links") relied
    on the assumption that every PCIe hierarchy is rooted at a Root Port.  But
    we can't rely on any assumption about what hardware we will find; we just
    have to deal with the world as it is.
    
    On some platforms, PCIe devices (endpoints, switch upstream ports, etc.)
    appear directly on the root bus, and there is no Root Port in the PCI bus
    hierarchy.  For example, Meelis observed these top-level devices on a
    Sparc V245:
    
      0000:02:00.0 PCI bridge to [bus 03-0d]    Switch Upstream Port
      0001:02:00.0 PCI bridge to [bus 03]       PCIe to PCI/PCI-X Bridge
    
    These devices *look* like they have links going upstream, but there really
    are no upstream devices.
    
    In set_pcie_port_type(), we used the parent device to figure out which side
    of a switch port has a link, so if the parent device did not exist, we
    dereferenced a NULL parent pointer.
    
    Check whether the parent device exists before dereferencing it.
    
    Meelis observed this oops on Sparc V245 and T2000.  Ben Herrenschmidt says
    this is also possible on IBM PowerVM guests on PowerPC.

Re: [PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[David Miller]

From: Bjorn Helgaas <bhelgaas@google.com>
Date: Thu, 20 Aug 2015 15:21:35 -0500

> On Thu, Aug 20, 2015 at 11:40:42AM -0700, David Miller wrote:
>> From: Bjorn Helgaas <bhelgaas@google.com>
>> Date: Thu, 20 Aug 2015 11:23:43 -0700
>> 
>> > On Thu, Aug 20, 2015 at 10:47 AM, David Miller <davem@davemloft.net> wrote:
>> >> From: Bjorn Helgaas <bhelgaas@google.com>
>> >> Date: Thu, 20 Aug 2015 00:48:00 -0500
>> >>
>> >>> [+cc Dave, Eric, Ben, sparclinux]
>> >>
>> >> I think the comment is terrible.
>> > 
>> > I'll update the comment if you suggest some text.  Do you object
>> > mainly to the mention of the specific systems?
>> 
>> "PCI root complex can not be assumed to be instantiated in the PCI bus
>>  hierarchy."
> 
> AIUI, the root complex itself (as distinct from root ports) doesn't
> normally appear in the PCI hierarchy, so I reworded it as follows.  I hope
> this helps address your objection, but if not, I can try again.

This is fine, but it was the comment we were talking about adjusting not
the commit message :-)

Re: [PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[Bjorn Helgaas]

On Thu, Aug 20, 2015 at 01:58:32PM -0700, David Miller wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> Date: Thu, 20 Aug 2015 15:21:35 -0500
> 
> > On Thu, Aug 20, 2015 at 11:40:42AM -0700, David Miller wrote:
> >> From: Bjorn Helgaas <bhelgaas@google.com>
> >> Date: Thu, 20 Aug 2015 11:23:43 -0700
> >> 
> >> > On Thu, Aug 20, 2015 at 10:47 AM, David Miller <davem@davemloft.net> wrote:
> >> >> From: Bjorn Helgaas <bhelgaas@google.com>
> >> >> Date: Thu, 20 Aug 2015 00:48:00 -0500
> >> >>
> >> >>> [+cc Dave, Eric, Ben, sparclinux]
> >> >>
> >> >> I think the comment is terrible.
> >> > 
> >> > I'll update the comment if you suggest some text.  Do you object
> >> > mainly to the mention of the specific systems?
> >> 
> >> "PCI root complex can not be assumed to be instantiated in the PCI bus
> >>  hierarchy."
> > 
> > AIUI, the root complex itself (as distinct from root ports) doesn't
> > normally appear in the PCI hierarchy, so I reworded it as follows.  I hope
> > this helps address your objection, but if not, I can try again.
> 
> This is fine, but it was the comment we were talking about adjusting not
> the commit message :-)

Oh, sorry!  I totally misread that.  Here's another try.

commit 27ff9f3e0ab34a75a3420bffd3eb5316de2e4d92
Author: Yijing Wang <wangyijing@huawei.com>
Date:   Mon Aug 17 18:47:58 2015 +0800

    PCI: Tolerate hierarchies with no Root Port
    
    We should not assume any particular hardware topology.  Commit d0751b98dfa3
    ("PCI: Add dev->has_secondary_link to track downstream PCIe links") relied
    on the assumption that every PCIe hierarchy is rooted at a Root Port.  But
    we can't rely on any assumption about what hardware we will find; we just
    have to deal with the world as it is.
    
    On some platforms, PCIe devices (endpoints, switch upstream ports, etc.)
    appear directly on the root bus, and there is no Root Port in the PCI bus
    hierarchy.  For example, Meelis observed these top-level devices on a
    Sparc V245:
    
      0000:02:00.0 PCI bridge to [bus 03-0d]    Switch Upstream Port
      0001:02:00.0 PCI bridge to [bus 03]       PCIe to PCI/PCI-X Bridge
    
    These devices *look* like they have links going upstream, but there really
    are no upstream devices.
    
    In set_pcie_port_type(), we used the parent device to figure out which side
    of a switch port has a link, so if the parent device did not exist, we
    dereferenced a NULL parent pointer.
    
    Check whether the parent device exists before dereferencing it.
    
    Meelis observed this oops on Sparc V245 and T2000.  Ben Herrenschmidt says
    this is also possible on IBM PowerVM guests on PowerPC.
    
    [bhelgaas: changelog, comment]
    Link: http://lkml.kernel.org/r/alpine.LRH.2.20.1508122118210.18637@math.ut.ee
    Reported-by: Meelis Roos <mroos@linux.ee>
    Tested-by: Meelis Roos <mroos@linux.ee>
    Signed-off-by: Yijing Wang <wangyijing@huawei.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cefd636..b978bbf 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -997,7 +997,12 @@ void set_pcie_port_type(struct pci_dev *pdev)
 	else if (type == PCI_EXP_TYPE_UPSTREAM ||
 		 type == PCI_EXP_TYPE_DOWNSTREAM) {
 		parent = pci_upstream_bridge(pdev);
-		if (!parent->has_secondary_link)
+
+		/*
+		 * Usually there's a parent device (Root Port or Switch
+		 * Downstream Port), but we can't assume one exists.
+		 */
+		if (parent && !parent->has_secondary_link)
 			pdev->has_secondary_link = 1;
 	}
 }

Re: [PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[David Miller]

From: Bjorn Helgaas <bhelgaas@google.com>
Date: Thu, 20 Aug 2015 16:21:09 -0500

> On Thu, Aug 20, 2015 at 01:58:32PM -0700, David Miller wrote:
>> From: Bjorn Helgaas <bhelgaas@google.com>
>> Date: Thu, 20 Aug 2015 15:21:35 -0500
>> 
>> > On Thu, Aug 20, 2015 at 11:40:42AM -0700, David Miller wrote:
>> >> From: Bjorn Helgaas <bhelgaas@google.com>
>> >> Date: Thu, 20 Aug 2015 11:23:43 -0700
>> >> 
>> >> > On Thu, Aug 20, 2015 at 10:47 AM, David Miller <davem@davemloft.net> wrote:
>> >> >> From: Bjorn Helgaas <bhelgaas@google.com>
>> >> >> Date: Thu, 20 Aug 2015 00:48:00 -0500
>> >> >>
>> >> >>> [+cc Dave, Eric, Ben, sparclinux]
>> >> >>
>> >> >> I think the comment is terrible.
>> >> > 
>> >> > I'll update the comment if you suggest some text.  Do you object
>> >> > mainly to the mention of the specific systems?
>> >> 
>> >> "PCI root complex can not be assumed to be instantiated in the PCI bus
>> >>  hierarchy."
>> > 
>> > AIUI, the root complex itself (as distinct from root ports) doesn't
>> > normally appear in the PCI hierarchy, so I reworded it as follows.  I hope
>> > this helps address your objection, but if not, I can try again.
>> 
>> This is fine, but it was the comment we were talking about adjusting not
>> the commit message :-)
> 
> Oh, sorry!  I totally misread that.  Here's another try.

This also looks fine:

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH] PCI/ASPM: Fix a NULL pointer crash on sparc64

[wangyijing]

Hi Bjorn, David, thanks for your effort to improve the patch commit.

Thanks!
Yijing.

> commit 27ff9f3e0ab34a75a3420bffd3eb5316de2e4d92
> Author: Yijing Wang <wangyijing@huawei.com>
> Date:   Mon Aug 17 18:47:58 2015 +0800
> 
>     PCI: Tolerate hierarchies with no Root Port
>     
>     We should not assume any particular hardware topology.  Commit d0751b98dfa3
>     ("PCI: Add dev->has_secondary_link to track downstream PCIe links") relied
>     on the assumption that every PCIe hierarchy is rooted at a Root Port.  But
>     we can't rely on any assumption about what hardware we will find; we just
>     have to deal with the world as it is.
>     
>     On some platforms, PCIe devices (endpoints, switch upstream ports, etc.)
>     appear directly on the root bus, and there is no Root Port in the PCI bus
>     hierarchy.  For example, Meelis observed these top-level devices on a
>     Sparc V245:
>     
>       0000:02:00.0 PCI bridge to [bus 03-0d]    Switch Upstream Port
>       0001:02:00.0 PCI bridge to [bus 03]       PCIe to PCI/PCI-X Bridge
>     
>     These devices *look* like they have links going upstream, but there really
>     are no upstream devices.
>     
>     In set_pcie_port_type(), we used the parent device to figure out which side
>     of a switch port has a link, so if the parent device did not exist, we
>     dereferenced a NULL parent pointer.
>     
>     Check whether the parent device exists before dereferencing it.
>     
>     Meelis observed this oops on Sparc V245 and T2000.  Ben Herrenschmidt says
>     this is also possible on IBM PowerVM guests on PowerPC.
>     
>     [bhelgaas: changelog, comment]
>     Link: http://lkml.kernel.org/r/alpine.LRH.2.20.1508122118210.18637@math.ut.ee
>     Reported-by: Meelis Roos <mroos@linux.ee>
>     Tested-by: Meelis Roos <mroos@linux.ee>
>     Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index cefd636..b978bbf 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -997,7 +997,12 @@ void set_pcie_port_type(struct pci_dev *pdev)
>  	else if (type == PCI_EXP_TYPE_UPSTREAM ||
>  		 type == PCI_EXP_TYPE_DOWNSTREAM) {
>  		parent = pci_upstream_bridge(pdev);
> -		if (!parent->has_secondary_link)
> +
> +		/*
> +		 * Usually there's a parent device (Root Port or Switch
> +		 * Downstream Port), but we can't assume one exists.
> +		 */
> +		if (parent && !parent->has_secondary_link)
>  			pdev->has_secondary_link = 1;
>  	}
>  }
> 
> .
>