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