Retire non-NEW_PCIB code and remove config option

[jrtc27]

Summary

All architectures enable NEW_PCIB in DEFAULTS (arm being the most recent
to do so in 121be555997b (arm: Set NEW_PCIB in DEFAULTS rather than a
subset of kernel configs")), so it's time we removed the legacy code
that no longer sees much testing and has a significant maintenance
burden.

[emaste]

👍

[rpokala]

... leaving just arm (which still enables it for GENERIC and TEGRA124) and mips
(which still enables it for RT2880_FDT and various std.mediatek-using kernel
configs), ...

For everything that already had NEW_PCIB, this change is a no-op, obviously. But what about those configs which haven't had NEW_PCIB until now? Presumably the reason they didn't have it until now is because there was some incompatibility with NEW_PCIB, but I don't see anything which would resolve those issues.

[rpokala]

sys/dev/pci/pci_pci.c

131–132

This drops the PCI_HP check in addition to NEW_PCIB; is that what you intended?

1589–1590

These drop the PCI_HP check as well as the NEW_PCIB check; intentional?

[jrtc27]

In D32954#743964, @rpokala wrote:

... leaving just arm (which still enables it for GENERIC and TEGRA124) and mips
(which still enables it for RT2880_FDT and various std.mediatek-using kernel
configs), ...

For everything that already had NEW_PCIB, this change is a no-op, obviously. But what about those configs which haven't had NEW_PCIB until now? Presumably the reason they didn't have it until now is because there was some incompatibility with NEW_PCIB, but I don't see anything which would resolve those issues.

All the pcib drivers in sys/arm support NEW_PCIB as far as I can tell, by virtue of deriving from either ofw_pcib_driver or generic_pcie_fdt_driver, or implementing support themselves like mv_pci. And the only board not supposedly supported by GENERIC whose board-specific kernel config enables device pci is ALPINE, whose PCI controller is just a thin wrapper around generic_pcie_fdt_driver.

The only issue therefore is mips, which has some drivers that indeed appear to not be compatible. Testing a MALTA64 kernel in QEMU it's no longer able to attach to pci0 as it can't allocate a bus number; mips's resource.h unconditionally defines PCI_RES_BUS whenever NEW_PCIB is defined, but not all its pcib drivers support it. From what I can see, all of ar71xx_pci, ar724x_pci, qca955x_pci, octopci, gt_pci and xlp_pci will fail PCI_RES_BUS allocations, with just mtk_pcie supporting it, hence why that was the only config with it. So I guess we just wait until we're free from mips; I could fix gt_pci so MALTA64 keeps working (which, given it's what gets used for QEMU, is about the only kernel config anyone's actually using) but that would be a waste of time.

sys/dev/pci/pci_pci.c

131–132

Yes, because true || X is always true, it's only where it's an && that the RHS needs to stay

[jrtc27]

(the device pci but currently not NEW_PCIB arm kernel configs being just ALPINE, ARMADA38X and ARMADAXP)

[imp]

Now that sparc64 and mips are gone, what's the status of this?

[jrtc27]

I believe there's nothing blocking it, bar rebasing, but more scrutiny would probably be good to verify I'm not missing something when asserting that all drivers should be compatible, especially in the 32-bit Arm department

[manu]

In D32954#863796, @jrtc27 wrote:

I believe there's nothing blocking it, bar rebasing, but more scrutiny would probably be good to verify I'm not missing something when asserting that all drivers should be compatible, especially in the 32-bit Arm department

Maybe @mmel or @mw could confirm that everything is ok for 32 bits arm, I don't have any board with PCI

[jrtc27]

In D32954#863891, @manu wrote:

In D32954#863796, @jrtc27 wrote:

I believe there's nothing blocking it, bar rebasing, but more scrutiny would probably be good to verify I'm not missing something when asserting that all drivers should be compatible, especially in the 32-bit Arm department

Maybe @mmel or @mw could confirm that everything is ok for 32 bits arm, I don't have any board with PCI

Also happy to just do a scream test if pre-commit testing is hard to come by for the affected configs :)

[emaste]

Also happy to just do a scream test if pre-commit testing is hard to come by for the affected configs :)

That sounds reasonable to me. We could always implement missing functionality, fix bugs, or even revert the commit if necessary.

[imp]

Glancing over the arm parts of the change, and if it compiles, it looks like there's an excellent chance that things will just work. There weren't that many 32-bit ARM platforms with actual PCI busses that we supported.
And I have no reason to think that it won't just work on those platforms...
Call for testing might not be bad, though... It looks like mostly an unifdef run though for things that are now always defined.

sys/conf/config.mk

64

I wonder what the DEV_PCI options are needed for...

[kd]

I've applied this patch and it seems to work just fine with a ARMADA38X config, which didn't use the NEW_PCIB option before.
That is the system boots, but I don't have anything plugged into the pcie slot at the moment.
I'll look for a card that I can put in, and will let you know if it works.

[mmel]

IMHO NEW_PCIB is an essential feature for all arm/arm64 FDT based boards. Or more generally, it is essential for all boards where the bootloader/firmware does not fully initialize all PCI hardware, i.e. all systems without ACPI.

[jhb]

Everything else looks fine to me aside from the missing #else.

sys/dev/acpica/acpi_pcib_acpi.c

139

You still need the #else here for bus_generic_release_resource to be used in the !PCI_RES_BUS case.

[jrtc27]

sys/dev/acpica/acpi_pcib_acpi.c

139

I wonder how I screwed that up. Should probably double-check everything myself...

[andrew]

sys/dev/acpica/acpi_pcib_acpi.c

139

It looks like we could also drop the PCI_RES_BUS checks as it's defined on all architectures.

[jrtc27]

sys/dev/acpica/acpi_pcib_acpi.c

139

D32955

[jhb]

sys/dev/acpica/acpi_pcib_acpi.c

139

No worries, I read the whole patch and that was the only thing I found.

Re: removing the PCI_RES_BUS checks, I would suggest doing that in a separate followup commit. I do think if all arches define them then we should remove the checks and require it going forward.

[emaste]

sys/dev/pci/pci_pci.c

1809–1815

what's the story here?

[jrtc27]

sys/dev/pci/pci_pci.c

1809–1815

pcib_cfg_save is a no-op without NEW_PCIB so I just replaced the kobj method with bus_generic_suspend

[jrtc27]

Rebased. Build-tested with tinderbox -DWITHOUT_WORLDS -DMAKE_ALL_KERNELS.

[jrtc27]

Rebased