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

[Bjorn Helgaas <helgaas@kernel.org>]

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
>