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

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.