On Wed, May 8, 2019 at 5:40 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> Hi Oliver,
>
> On Wed, Apr 10, 2019 at 05:44:55PM +1000, Oliver O'Halloran wrote:
> > The meaning of is_physfn and how it's different to is_virtfn really
> > isn't clear unless you do a bit of digging. Add a comment to help out
> > the unaware.
> >
> > Signed-off-by: Oliver O'Halloran <oohall@xxxxxxxxx>
> > ---
> > include/linux/pci.h | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 77448215ef5b..88bf71bfa757 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -393,6 +393,10 @@ struct pci_dev {
> > unsigned int is_managed:1;
> > unsigned int needs_freset:1; /* Requires fundamental reset */
> > unsigned int state_saved:1;
> > + /*
> > + * is_physfn indicates that the function can be used to host VFs.
> > + * It is only set when both the kernel and the device support IOV.
> > + */
>
> The comment is certainly accurate, no question there, but it sounds
> like the reason for adding it is because you stumbled over something
> in the confusing SR-IOV/PF/VF infrastructure. If we can, I'd really
> like to improve that infrastructure so it's less confusing in the
> first place.
>
> It seems like part of the problem is that "is_physfn" is telling us
> more than one thing: "CONFIG_PCI_IOV=y" and "pdev has an SR-IOV
> capability" and "pdev is a PF".
>
> Many of the uses of "is_physfn" are in powerpc code that tests
> "!pdev->is_physfn", and the negation of those multiple things makes it
> a little confusing to figure out what the real purpose it.
>
> Maybe we should cache the PCI_EXT_CAP_ID_SRIOV location in the
> pci_dev. That would simplify some drivers slightly, and if we had
> "pdev->sriov_cap" and "pdev->is_virtfn", I think we could drop
> "is_physfn". But I don't understand the powerpc uses well enough to
> know whether that would make things easier or harder.
The powerpc bits were what I was looking at. Some parts use !is_virtfn
and other parts use !is_physfn which threw me a bit. Caching the
SR-IOV cap offset would be an improvement since all the is_physfn
usages are just tests to determine if any platform specific SR-IOV
setup needs to be done for the device.
> > unsigned int is_physfn:1;
> > unsigned int is_virtfn:1;
> > unsigned int reset_fn:1;
> > --
> > 2.20.1
> >
