Re: [PATCH] arch: powerpc: pci-common: fix wrong return value check on phd_id
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.
Re: [PATCH] arch: powerpc: pci-common: fix wrong return value check on phd_id
Hi Daniel, Sorry we broke things for you. Daniel Walker 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
> [...] > 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
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
On Mon, Jun 18, 2018 at 1:57 PM, Daniel Walker 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
