[PATCH] PCI: Add a comment for the is_physfn field

[PATCH] PCI: Add a comment for the is_physfn field

[Oliver O'Halloran]

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.
+	 */
 	unsigned int	is_physfn:1;
 	unsigned int	is_virtfn:1;
 	unsigned int	reset_fn:1;
-- 
2.20.1

Re: [PATCH] PCI: Add a comment for the is_physfn field

[Bjorn Helgaas]

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
> 

Re: [PATCH] PCI: Add a comment for the is_physfn field

[Oliver]

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
> >