Re: [PATCH 10/12] xen-pcifront: this module is PV-only

[Bjorn Helgaas <helgaas@xxxxxxxxxx>]

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