| Message ID | 20190410074455.26964-1-oohall@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Add a comment for the is_physfn field | expand |
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@gmail.com> > --- > 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. > unsigned int is_physfn:1; > unsigned int is_virtfn:1; > unsigned int reset_fn:1; > -- > 2.20.1 >
On Wed, May 8, 2019 at 5:40 AM Bjorn Helgaas <helgaas@kernel.org> 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@gmail.com> > > --- > > 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 > >
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. + */ unsigned int is_physfn:1; unsigned int is_virtfn:1; unsigned int reset_fn:1;
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@gmail.com> --- include/linux/pci.h | 4 ++++ 1 file changed, 4 insertions(+)