| Message ID | 20210726180657.142727-5-boqun.feng@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: hv: Support host bridge probing on ARM64 | expand |
Catalin and Will, Appreciate it that you can have a look at this one and patch #4, note that there exists an alternative solution at[1]. The difference is the way used to pass the corresponding ACPI device pointers for PCI host bridges: currently pci_config_window->parent is used, and this patch and patch #4 allow the field to be NULL, because Hyper-V's PCI host bridges don't have ACPI devices, while [1] changes to use pci_host_bridge->private. And I'm OK with either way, I don't have a strong opinion here ;-) Looking forwards to your suggestion, Thanks! Regards, Boqun [1]: https://lore.kernel.org/lkml/20210811153619.88922-1-boqun.feng@gmail.com/ On Tue, Jul 27, 2021 at 02:06:53AM +0800, Boqun Feng wrote: > Currently at root bridge preparation, the corresponding ACPI device will > be set as the companion, however for a Hyper-V virtual PCI root bridge, > there is no corresponding ACPI device, because a Hyper-V virtual PCI > root bridge is discovered via VMBus rather than ACPI table. In order to > support this, we need to make pcibios_root_bridge_prepare() work with > cfg->parent being NULL. > > Use a NULL pointer as the ACPI device if there is no corresponding ACPI > device, and this is fine because: 1) ACPI_COMPANION_SET() can work with > the second parameter being NULL, 2) semantically, if a NULL pointer is > set via ACPI_COMPANION_SET(), ACPI_COMPANION() (the read API for this > field) will return NULL, and since ACPI_COMPANION() may return NULL, so > users must have handled the cases where it returns NULL, and 3) since > there is no corresponding ACPI device, it would be wrong to use any > other value here. > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > --- > arch/arm64/kernel/pci.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c > index 5148ae242780..2276689b5411 100644 > --- a/arch/arm64/kernel/pci.c > +++ b/arch/arm64/kernel/pci.c > @@ -90,7 +90,17 @@ int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) > return 0; > > cfg = bridge->bus->sysdata; > - adev = to_acpi_device(cfg->parent); > + > + /* > + * On Hyper-V there is no corresponding ACPI device for a root bridge, > + * therefore ->parent is set as NULL by the driver. And set 'adev' as > + * NULL in this case because there is no proper ACPI device. > + */ > + if (!cfg->parent) > + adev = NULL; > + else > + adev = to_acpi_device(cfg->parent); > + > bus_dev = &bridge->bus->dev; > > ACPI_COMPANION_SET(&bridge->dev, adev); > -- > 2.32.0 >
Hi Boqun, Sorry, I just got back from holiday and I'm still in the deleting emails mode. On Fri, Aug 20, 2021 at 12:13:22AM +0800, Boqun Feng wrote: > Appreciate it that you can have a look at this one and patch #4, note > that there exists an alternative solution at[1]. > > The difference is the way used to pass the corresponding ACPI device > pointers for PCI host bridges: currently pci_config_window->parent is > used, and this patch and patch #4 allow the field to be NULL, because > Hyper-V's PCI host bridges don't have ACPI devices, while [1] changes to > use pci_host_bridge->private. And I'm OK with either way, I don't have a > strong opinion here ;-) [...] > [1]: https://lore.kernel.org/lkml/20210811153619.88922-1-boqun.feng@gmail.com/ I'm ok with the arm64 bits in this series and the one you linked above. It's up to Lorenzo if he's happy with how pci-hyperv.c ends up looking, I'm not a PCIe expert. My preference would be for a combined series (this and [1] above). Happy to ack the arm64 patches in a combined series (if you are going to post one), the changes would look even simpler.
On Fri, Aug 20, 2021 at 06:49:47PM +0100, Catalin Marinas wrote: > Hi Boqun, > > Sorry, I just got back from holiday and I'm still in the deleting emails > mode. > > On Fri, Aug 20, 2021 at 12:13:22AM +0800, Boqun Feng wrote: > > Appreciate it that you can have a look at this one and patch #4, note > > that there exists an alternative solution at[1]. > > > > The difference is the way used to pass the corresponding ACPI device > > pointers for PCI host bridges: currently pci_config_window->parent is > > used, and this patch and patch #4 allow the field to be NULL, because > > Hyper-V's PCI host bridges don't have ACPI devices, while [1] changes to > > use pci_host_bridge->private. And I'm OK with either way, I don't have a > > strong opinion here ;-) > [...] > > [1]: https://lore.kernel.org/lkml/20210811153619.88922-1-boqun.feng@gmail.com/ > > I'm ok with the arm64 bits in this series and the one you linked above. > It's up to Lorenzo if he's happy with how pci-hyperv.c ends up looking, > I'm not a PCIe expert. My preference would be for a combined series > (this and [1] above). > > Happy to ack the arm64 patches in a combined series (if you are going to > post one), the changes would look even simpler. I believe [1] above is an experiment - therefore it is best to stick to this series as it is for the time being, pending refactoring that requires more time, I would not rush it. If you can ACK the arm64 patches (3,4) please I will pull the series into the PCI tree asap. Thanks, Lorenzo
On Tue, Jul 27, 2021 at 02:06:53AM +0800, Boqun Feng wrote: > Currently at root bridge preparation, the corresponding ACPI device will > be set as the companion, however for a Hyper-V virtual PCI root bridge, > there is no corresponding ACPI device, because a Hyper-V virtual PCI > root bridge is discovered via VMBus rather than ACPI table. In order to > support this, we need to make pcibios_root_bridge_prepare() work with > cfg->parent being NULL. > > Use a NULL pointer as the ACPI device if there is no corresponding ACPI > device, and this is fine because: 1) ACPI_COMPANION_SET() can work with > the second parameter being NULL, 2) semantically, if a NULL pointer is > set via ACPI_COMPANION_SET(), ACPI_COMPANION() (the read API for this > field) will return NULL, and since ACPI_COMPANION() may return NULL, so > users must have handled the cases where it returns NULL, and 3) since > there is no corresponding ACPI device, it would be wrong to use any > other value here. > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index 5148ae242780..2276689b5411 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -90,7 +90,17 @@ int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) return 0; cfg = bridge->bus->sysdata; - adev = to_acpi_device(cfg->parent); + + /* + * On Hyper-V there is no corresponding ACPI device for a root bridge, + * therefore ->parent is set as NULL by the driver. And set 'adev' as + * NULL in this case because there is no proper ACPI device. + */ + if (!cfg->parent) + adev = NULL; + else + adev = to_acpi_device(cfg->parent); + bus_dev = &bridge->bus->dev; ACPI_COMPANION_SET(&bridge->dev, adev);
Currently at root bridge preparation, the corresponding ACPI device will be set as the companion, however for a Hyper-V virtual PCI root bridge, there is no corresponding ACPI device, because a Hyper-V virtual PCI root bridge is discovered via VMBus rather than ACPI table. In order to support this, we need to make pcibios_root_bridge_prepare() work with cfg->parent being NULL. Use a NULL pointer as the ACPI device if there is no corresponding ACPI device, and this is fine because: 1) ACPI_COMPANION_SET() can work with the second parameter being NULL, 2) semantically, if a NULL pointer is set via ACPI_COMPANION_SET(), ACPI_COMPANION() (the read API for this field) will return NULL, and since ACPI_COMPANION() may return NULL, so users must have handled the cases where it returns NULL, and 3) since there is no corresponding ACPI device, it would be wrong to use any other value here. Signed-off-by: Boqun Feng <boqun.feng@gmail.com> --- arch/arm64/kernel/pci.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)