| Message ID | bbfb4191-9e34-53da-f179-4549b10dcfb3@suse.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | None | expand |
Update subject to follow conventions (use "git log --oneline drivers/pci/Kconfig"). Should say what this patch does. 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. 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()? s/building it/building <name of module>/ # xen-pcifront.o? 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? > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/drivers/pci/Kconfig > +++ b/drivers/pci/Kconfig > @@ -110,7 +110,7 @@ config PCI_PF_STUB > > config XEN_PCIDEV_FRONTEND > tristate "Xen PCI Frontend" > - depends on X86 && XEN > + depends on XEN_PV > select PCI_XEN > select XEN_XENBUS_FRONTEND > default y >
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
On Tue, Sep 07, 2021 at 06:14:16PM +0200, Jan Beulich wrote: > 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. It might not be different information, but if you use the same sentence structure and formatting as all the other subject lines, they're easier to read as a group. > > 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? Saying "module init function" makes the reader do extra work to figure out what function you are referring to. I had to look at drivers/pci/Makefile to find the module name, then look at drivers/pci/xen-pcifront.c to look for the init function. Saying pcifront_init() makes it trivial to look *there*. > > 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. Why be vague when it's so easy to be explicit and save everybody else the effort? Part of the disconnect here is that the subject line is not *part* of the commit log, so the commit log should make sense even if you can't see the subject line. It's like an essay that should make sense without its title. Most of this *is* trivial, I agree. Just minor hiccups in the process of reading. > > 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. No doubt it's obvious to Xen experts. Unfortunately I am not one. Evidently there's no real dependency on the X86 arch, which makes me wonder why the "depends on X86" was there in the first place. Bjorn
On Tue, 7 Sep 2021, 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. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- a/drivers/pci/Kconfig > +++ b/drivers/pci/Kconfig > @@ -110,7 +110,7 @@ config PCI_PF_STUB > > config XEN_PCIDEV_FRONTEND > tristate "Xen PCI Frontend" > - depends on X86 && XEN > + depends on XEN_PV > select PCI_XEN > select XEN_XENBUS_FRONTEND > default y >
--- a/drivers/pci/Kconfig +++ b/drivers/pci/Kconfig @@ -110,7 +110,7 @@ config PCI_PF_STUB config XEN_PCIDEV_FRONTEND tristate "Xen PCI Frontend" - depends on X86 && XEN + depends on XEN_PV select PCI_XEN select XEN_XENBUS_FRONTEND default y
It's module init function does a xen_pv_domain() check first thing. Hence there's no point building it in non-PV configurations. Signed-off-by: Jan Beulich <jbeulich@suse.com>