> -----Original Message-----
> From: Jakub Kicinski <jakub.kicin...@netronome.com>
> Sent: Tuesday, July 2, 2019 11:17 PM
> To: Parav Pandit <pa...@mellanox.com>
> Cc: Jiri Pirko <j...@mellanox.com>; netdev@vger.kernel.org; Saeed
> Mahameed <sae...@mellanox.com>
> Subject: Re: [PATCH net-next 1/3] devlink: Introduce PCI PF port flavour and
> port attribute
>
> On Tue, 2 Jul 2019 04:26:47 +0000, Parav Pandit wrote:
> > > On Mon, 1 Jul 2019 07:27:32 -0500, Parav Pandit wrote:
> > > > In an eswitch, PCI PF may have port which is normally represented
> > > > using a representor netdevice.
> > > > To have better visibility of eswitch port, its association with
> > > > PF, a representor netdevice and port number, introduce a PCI PF
> > > > port flavour and port attriute.
> > > >
> > > > When devlink port flavour is PCI PF, fill up PCI PF attributes of
> > > > the port.
> > > >
> > > > Extend port name creation using PCI PF number on best effort basis.
> > > > So that vendor drivers can skip defining their own scheme.
> > > >
> > > > $ devlink port show
> > > > pci/0000:05:00.0/0: type eth netdev eth0 flavour pcipf pfnum 0
> > > >
> > > > Acked-by: Jiri Pirko <j...@mellanox.com>
> > > > Signed-off-by: Parav Pandit <pa...@mellanox.com> diff --git
> > > > a/include/net/devlink.h b/include/net/devlink.h index
> > > > 6625ea068d5e..8db9c0e83fb5 100644
> > > > --- a/include/net/devlink.h
> > > > +++ b/include/net/devlink.h
> > > > @@ -38,6 +38,10 @@ struct devlink {
> > > > char priv[0] __aligned(NETDEV_ALIGN); };
> > > >
> > > > +struct devlink_port_pci_pf_attrs {
> > >
> > > Why the named structure? Anonymous one should be just fine?
> > >
> > No specific reason for this patch. But named structure allows to
> > extend it more easily with code readability.
>
> I'd argue the readability - I hove to scroll up/look up the structure just to
> see
> it has a single member. But no big deal :)
>
Ok. :-)
> > Such as subsequently we want to add the peer_mac etc port attributes.
> > Named structure to store those attributes are helpful.
>
> It remains to be seen if peer attributes are flavour specific 🤔
> I'd imagine most port types would have some form of a peer (other than a
> network port, perhaps). But perhaps different peer attributes.
>
Few attributes may be common and few will be port specific.
So as it evolves, data structure will evolve.
Common attribute I can think of is - mac address.
> > > > diff --git a/net/core/devlink.c b/net/core/devlink.c index
> > > > 89c533778135..001f9e2c96f0 100644
> > > > --- a/net/core/devlink.c
> > > > +++ b/net/core/devlink.c
> > > > @@ -517,6 +517,11 @@ static int devlink_nl_port_attrs_put(struct
> sk_buff *msg,
> > > > return -EMSGSIZE;
> > > > if (nla_put_u32(msg, DEVLINK_ATTR_PORT_NUMBER, attrs-
> >port_number))
> > > > return -EMSGSIZE;
> > >
> > > Why would we report network port information for PF and VF port
> > > flavours?
> >
> > I didn't see any immediate need to report, at the same time didn't
> > find any reason to treat such port flavours differently than existing
> > one. It just gives a clear view of the device's eswitch. Might find it
> > useful during debugging while inspecting device internal tables..
>
> PFs and VFs ports are not tied to network ports in switchdev mode.
> You have only one network port under a devlink instance AFAIR, anyway.
>
I am not sure what do you mean by network port.
Do you intent to see a physical port that connects to physical network?
As I described in the comment of the PF and VF flavour, it is an eswitch port.
I have shown the diagram also of the eswitch in the cover letter.
Port_number doesn't have to a physical port. Flavour describe what port type is
and number says what is the eswitch port number.
Hope it clarifies.
> > > > + if (devlink_port->attrs.flavour == DEVLINK_PORT_FLAVOUR_PCI_PF)
> > > > {
> > > > + if (nla_put_u16(msg, DEVLINK_ATTR_PORT_PCI_PF_NUMBER,
> > > > + attrs->pci_pf.pf))
> > > > + return -EMSGSIZE;
> > > > + }
> > > > if (!attrs->split)
> > > > return 0;
> > > > if (nla_put_u32(msg, DEVLINK_ATTR_PORT_SPLIT_GROUP,
> > > > attrs->port_number))
