[PATCH] arch: powerpc: pci-common: fix wrong return value check on phd_id

[PATCH] arch: powerpc: pci-common: fix wrong return value check on phd_id

[Daniel Walker]

Cisco has a couple platforms which depend on the domain values getting
set a certain way. We discovered our machines not detecting the pci
devices, and traced it back to this commit,

63a7228 powerpc/pci: Assign fixed PHB number based on device-tree
properties

It seems that the code is expecting the return value of of_property_read_u64()
to be the opposite of what it actually is.. It returns zero on success, and a
negative return value on error. So if you only check when it's non-zero your
going to set Opal for all platforms but Opal, which I assume is not what was
expected.

Fix is just to negate the ret value.

Cc: xe-kernel@external.cisco.com
Cc: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
Cc: Gavin Shan <gwshan@linux.vnet.ibm.com>
Cc: Ian Munsie <imunsie@au1.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Fixes: 63a72284b159 ("powerpc/pci: Assign fixed PHB number based on device-tree properties")
Signed-off-by: Daniel Walker <danielwa@cisco.com>
---
 arch/powerpc/kernel/pci-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index fe9733f..0a1bcbe 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -89,7 +89,7 @@ static int get_phb_number(struct device_node *dn)
 	 * reading "ibm,opal-phbid", only present in OPAL environment.
 	 */
 	ret = of_property_read_u64(dn, "ibm,opal-phbid", &prop);
-	if (ret) {
+	if (!ret) {
 		ret = of_property_read_u32_index(dn, "reg", 1, &prop_32);
 		prop = prop_32;
 	}
-- 
2.10.3.dirty

Re: [PATCH] arch: powerpc: pci-common: fix wrong return value check on phd_id

[Guilherme Piccoli]

On Mon, Jun 18, 2018 at 1:57 PM, Daniel Walker <danielwa@cisco.com> wrote:
> Cisco has a couple platforms which depend on the domain values getting
> set a certain way. We discovered our machines not detecting the pci
> devices, and traced it back to this commit,
>
> 63a7228 powerpc/pci: Assign fixed PHB number based on device-tree
> properties
>
> It seems that the code is expecting the return value of of_property_read_u64()
> to be the opposite of what it actually is.. It returns zero on success, and a
> negative return value on error. So if you only check when it's non-zero your
> going to set Opal for all platforms but Opal, which I assume is not what was
> expected.

Hi Daniel, thanks for exposing a potential issue and trying to fix.
(And thanks Mauro to point this message, since my email changed).

I disagree with your patch Daniel, I think it's wrong.
Since the logic of my original patch is kind of convoluted, for improve
(even my own) understanding, I tried to over-comment the logic here:
https://pastebin.ubuntu.com/p/4DNvd9b2bY

Please, take a look and see if you agree with the logic now.


Regarding your issue, I'm interested - I think perhaps you have some FW
flaw that might be exposing duplicated "reg" or "ibm,opal-phbid" properties.
Or maybe you somehow rely on the old incremental PHB setting and your
software stack is not working properly with the DT-based PHB values.

Please, elaborate the issue a bit more in order we can try to figure
out. And also, you could try reverting the below two patches and boot
the kernel to be sure it works:

61e8a0d5a02 powerpc/pci: Fix endian bug in fixed PHB numbering
63a72284b15 powerpc/pci: Assign fixed PHB number based on device-tree properties

*Notice* the first is a fix to my patch, and both should be tested together.
Be sure your current kernel has _both or neither_ these patches!

Cheers,


Guilherme

Re: [PATCH] arch: powerpc: pci-common: fix wrong return value check on phd_id

[Guilherme G. Piccoli]

> [...]
> What your doing is changing the phb_id to some transformation of "reg" for
> all platforms except which have "ibm,opal-phbid". This is what we observe.
> This is a radical altering of the prior phb_id selection before your patch.
>
> The question is, was that your intent ? We are expecting these numbers
> aren't getting altered in this way, this is why we have the patch. Your
> patch description appears to suggest you want this type of selection for
> "... pSeries and PowerNV cases)." , so I am assuming you did this by
> mistake. Also I don't see a reason to make this change for all platforms.

It was the intent - whenever we have device-tree information, the idea is to
use it to have more consistent numbering across reboots and PCI hotplugs.
The reason to change that originally is hotplugging a PCI device added
the device with a different PHB/Domain value, and network predictable naming
messed-up interfaces' names, leading to a machine without networking.


> [...]
>
> We have already done this, that is how we determined your patch was changing
> the domain values. There isn't much to tell w.r.t this issue on our side, we
> are expecting the domain numbers are set a specific way and they aren't
> getting set that way. The drivers which are looking for PCI devices are not
> in kernel space, and the PCI domain values are getting taken from userspace.
>

Oh okay, I imagined it was some crazy userspace-based PCI domain scheme
that led you to report the issue - confirmed heheh
So, you expect the old behavior right? Incremental PHB numbering.
I think we could have a kernel config option to allow that, this way
you could rebuild your kernel with that option and keep the old behavior.

This idea needs to be validated by the maintainers, or perhaps they could
propose another way to keep the old behavior to interested parties.

[Looping linux-pci too - original thread at:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-June/174760.html]

Re: [PATCH] arch: powerpc: pci-common: fix wrong return value check on phd_id

[Daniel Walker]

On 06/19/2018 09:26 AM, Guilherme G. Piccoli wrote:
>> [...]
>> What your doing is changing the phb_id to some transformation of "reg" for
>> all platforms except which have "ibm,opal-phbid". This is what we observe.
>> This is a radical altering of the prior phb_id selection before your patch.
>>
>> The question is, was that your intent ? We are expecting these numbers
>> aren't getting altered in this way, this is why we have the patch. Your
>> patch description appears to suggest you want this type of selection for
>> "... pSeries and PowerNV cases)." , so I am assuming you did this by
>> mistake. Also I don't see a reason to make this change for all platforms.
> It was the intent - whenever we have device-tree information, the idea is to
> use it to have more consistent numbering across reboots and PCI hotplugs.
> The reason to change that originally is hotplugging a PCI device added
> the device with a different PHB/Domain value, and network predictable naming
> messed-up interfaces' names, leading to a machine without networking.

Ok ..

>
>
>> [...]
>>
>> We have already done this, that is how we determined your patch was changing
>> the domain values. There isn't much to tell w.r.t this issue on our side, we
>> are expecting the domain numbers are set a specific way and they aren't
>> getting set that way. The drivers which are looking for PCI devices are not
>> in kernel space, and the PCI domain values are getting taken from userspace.
>>
> Oh okay, I imagined it was some crazy userspace-based PCI domain scheme
> that led you to report the issue - confirmed heheh
> So, you expect the old behavior right? Incremental PHB numbering.
> I think we could have a kernel config option to allow that, this way
> you could rebuild your kernel with that option and keep the old behavior.
>
> This idea needs to be validated by the maintainers, or perhaps they could
> propose another way to keep the old behavior to interested parties.
>
> [Looping linux-pci too - original thread at:
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-June/174760.html]

I didn't look into changing the behavior on our side because it didn't 
look like the intent of the patch was to make a global change. I can 
take a look at changing this behavior on our side , given that this was 
intended by your changes.

However, they're may be other platforms or drivers which depend on these 
numbers getting set a certain way, so there may be other userspace 
dependencies on these values.

Daniel

Re: [PATCH] arch: powerpc: pci-common: fix wrong return value check on phd_id

[Michael Ellerman]

Hi Daniel,

Sorry we broke things for you.

Daniel Walker <danielwa@cisco.com> writes:
> On 06/19/2018 09:26 AM, Guilherme G. Piccoli wrote:
>>> [...]
>>> What your doing is changing the phb_id to some transformation of "reg" for
>>> all platforms except which have "ibm,opal-phbid". This is what we observe.
>>> This is a radical altering of the prior phb_id selection before your patch.
...
>
> I didn't look into changing the behavior on our side because it didn't 
> look like the intent of the patch was to make a global change. I can 
> take a look at changing this behavior on our side , given that this was 
> intended by your changes.

You're right the change log and the patch are a bit out of sync, that
was probably my fault.

> However, they're may be other platforms or drivers which depend on these 
> numbers getting set a certain way, so there may be other userspace 
> dependencies on these values.

That's true, though I think yours is the first report we've had of
problems.

The old behaviour relied on device tree ordering in nearly all cases, so
you basically get whatever order your firmware happened to flatten the
device tree in.

That tends to be consistent on a single system or with a single firmware
version, but it's not stable in general. If your firmware changes, or
you kexec then the ordering can change.

So I'd definitely prefer we didn't go back to that behaviour, because
it's basically "random order".

If there's anything you can do on your end to cope with the new ordering
that would be great.

cheers

Re: [PATCH] arch: powerpc: pci-common: fix wrong return value check on phd_id

[Benjamin Herrenschmidt]

On Thu, 2018-06-21 at 10:28 +1000, Michael Ellerman wrote:
> 
> That's true, though I think yours is the first report we've had of
> problems.
> 
> The old behaviour relied on device tree ordering in nearly all cases, so
> you basically get whatever order your firmware happened to flatten the
> device tree in.
> 
> That tends to be consistent on a single system or with a single firmware
> version, but it's not stable in general. If your firmware changes, or
> you kexec then the ordering can change.
> 
> So I'd definitely prefer we didn't go back to that behaviour, because
> it's basically "random order".
> 
> If there's anything you can do on your end to cope with the ne

I think the numbering change has to be coped with. However:

The main issue I see is that it somewhat hard wires that "reg"
is a 64-bit property with the "interesting" bits in the bottom,
and that "interesting" part somewhat happens to fit in 16-bits.

It would have been better to get the full address out of reg (using the
appropriate size specified in the parent #address-cells) and hash it.

Cheers,
Ben.