Em Tue, 28 Dec 2021 16:06:44 +0100 Niklas Schnelle schnelle@linux.ibm.com escreveu:
(on a side note: the c/c list of this patch is too long. I would try to avoid using a too long list, as otherwise this e-mail may end being rejected by mail servers)
On Tue, 2021-12-28 at 13:54 +0100, Mauro Carvalho Chehab wrote:
---8<---
All you really care about is the "legacy" I/O spaces here, this isn't tied to PCI specifically at all, right?
So why not just have a OLD_STYLE_IO config option or something like that, to show that it's the i/o functions we care about here, not PCI at all?
And maybe not call it "old" or "legacy" as time constantly goes forward, just describe it as it is, "DIRECT_IO"?
Agreed. HAVE_PCI_DIRECT_IO (or something similar) seems a more appropriate name for it.
Thanks, Mauro
Hmm, I might be missing something here but that sounds a lot like the HAS_IOPORT option added in patch 02.
We add both LEGACY_PCI and HAS_IOPORT to differentiate between two cases. HAS_IOPORT is for PC-style devices that are not on a PCI card while LEGACY_PCI is for PCI drivers that require port I/O.
I didn't look at the other patches on this series, but why it is needed to deal with them on a separate way? Won't "PCI" and "HAS_IOPORT" be enough?
I mean, are there any architecture where HAVE_PCI=y and HAS_IOPORT=y where LEGACY_PCI shall be "n"?
In the current patch set LEGACY_PCI is not currently selected by architectures, though of course it could be if we know that an architecture requires it. We should probably also set it in any defconfig that has devices depending on it so as not to break these.
Other than that it would be set during kernel configuration if one wants/needs support for legacy PCI devices. For testing I ran with HAVE_PCI=y, HAS_IOPORT=y and LEGACY_PCI=n on both my local Ryzen 3990X based workstation and Raspberry Pi 4 (DT). I guess at the moment it would make most sense for special configs such as those tailored for vitualization guets but in the end that would be something for distributions to decide.
IMO, it makes sense to have a "default y" there, as on systems that support I/O space, disabling it will just randomly disable some drivers that could be required by some hardware. I won't doubt that some of those could be ported from using inb/outb to use, instead, readb/writeb.
Arnd described the options here: https://lore.kernel.org/lkml/CAK8P3a3HHeP+Gw_k2P7Qtig0OmErf0HN30G22+qHic_uZT...
Based on Arnd's description, LEGACY_PCI should depend on HAS_IOPORT. This is missing on patch 1. You should probably reorder your patch series to first create HAS_IOPORT and then add LEGACY_PCI with depends on, as otherwise it may cause randconfig build issues at robots and/or git bisect.
I would also suggest to first introduce such change and then send a per-subsystem LEGACY_PCI patch, as it would be a lot easier for maintainers to review.
This includes pre-PCIe devices as well as PCIe devices which require features like I/O spaces. The "legacy" naming is comes from the PCIe spec which in section 2.1.1.2 says "PCI Express supports I/O Space for compatibility with legacy devices which require their use. Future revisions of this specification may deprecate the use of I/O Space."
I would still avoid calling it LEGACY_PCI, as this sounds too generic.
I didn't read the PCI/PCIe specs, but I suspect that are a lot more features that were/will be deprecated on PCI specs as time goes by.
So, I would, instead, use something like PCI_LEGACY_IO_SPACE or HAVE_PCI_LEGACY_IO_SPACE, in order to let it clear what "legacy" means.
Hmm, I'd like to hear Bjorn's opinion on this. Personally I feel like LEGACY_PCI is pretty clear since most devices are either pre-PCIe devices or a compatibility feature allowing drivers for a pre-PCIe device to work with a PCIe device.
That's the main point: it is *not* disabling pre-PCIe devices or even legacy PCI drivers. It just disables a random set of drivers just because they use inb/outb instead of readb/writeb. It keeps several pure PCI drivers selected, and disables some PCIe for no real reason.
Just to give one example, this symbol:
diff --git a/drivers/media/cec/platform/Kconfig b/drivers/media/cec/platform/Kconfig index b672d3142eb7..5e92ece5b104 100644 --- a/drivers/media/cec/platform/Kconfig +++ b/drivers/media/cec/platform/Kconfig @@ -100,7 +100,7 @@ config CEC_TEGRA config CEC_SECO tristate "SECO Boards HDMI CEC driver" depends on (X86 || IA64) || COMPILE_TEST
- depends on PCI && DMI
- depends on LEGACY_PCI && DMI select CEC_CORE select CEC_NOTIFIER help
Disables HDMI CEC support on some Intel motherboards. Any distro meant to run on generic hardware should keep it selected.
I can see some value of a "PCI_LEGACY" option to disable all non-PCIe drivers, but this is not the case here.
Thanks, Mauro