On 07.09.2021 17:30, Bjorn Helgaas wrote: > Update subject to follow conventions (use "git log --oneline > drivers/pci/Kconfig"). Should say what this patch does. I can change that; I don't think it'll carry any different information. > Commit log below should also say what this patch does. Currently it's > part of the rationale for the change, but doesn't say what the patch > does. "There's no point building ..." to me is as good as "Don't build ...". But oh well, I can adjust ... > On Tue, Sep 07, 2021 at 02:10:41PM +0200, Jan Beulich wrote: >> It's module init function does a xen_pv_domain() check first thing. >> Hence there's no point building it in non-PV configurations. > > s/It's/<name of function that calls xen_pv_domain()/ # pcifront_init()? I don't understand this - how is "module init function" not clear enough? > s/building it/building <name of module>/ # xen-pcifront.o? The driver name is already part of the subject; I didn't think I need to repeat that one here. > I see that CONFIG_XEN_PV is only mentioned in arch/x86, so > CONFIG_XEN_PV=y cannot be set on other arches. Is the current > "depends on X86" just a reflection of that, or is it because of some > other x86 dependency in the code? > > The connection between xen_pv_domain() and CONFIG_XEN_PV is not > completely obvious. > > If you only build xen-pcifront.o when CONFIG_XEN_PV=y, and > xen_pv_domain() is true if and only if CONFIG_XEN_PV=y, why bother > calling xen_pv_domain() at all? Because XEN_PV=y only _enables_ the kernel to run in PV mode. It may be enabled to also run in HVM and/or PVH modes. And it may then _run_ in any of the enabled modes. IOW xen_pv_domain() will always return false when !XEN_PV; no other implication is valid. I don't think this basic concept needs explaining in a simple patch like this. Instead I think the config option in question, despite living in drivers/pci/Kconfig, should be under "XEN HYPERVISOR INTERFACE" maintainership. I realize that's not even expressable in ./MAINTAINERS. I wonder why the option was put there in the first place, rather than in drivers/xen/Kconfig. Jan
