Hi Jakub,
> -----Original Message-----
> From: Jakub Kicinski <jakub.kicin...@netronome.com>
> Sent: Tuesday, July 2, 2019 4:57 AM
> To: Jiri Pirko <j...@mellanox.com>
> Cc: Parav Pandit <pa...@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 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>
> > ---
> > include/net/devlink.h | 11 ++++++
> > include/uapi/linux/devlink.h | 5 +++
> > net/core/devlink.c | 71 +++++++++++++++++++++++++++++-------
> > 3 files changed, 73 insertions(+), 14 deletions(-)
> >
> > 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.
Such as subsequently we want to add the peer_mac etc port attributes.
Named structure to store those attributes are helpful.
> > + u16 pf; /* Associated PCI PF for this port. */
> > +};
> > +
> > struct devlink_port_attrs {
> > u8 set:1,
> > split:1,
> > @@ -46,6 +50,9 @@ struct devlink_port_attrs {
> > u32 port_number; /* same value as "split group" */
> > u32 split_subport_number;
> > struct netdev_phys_item_id switch_id;
> > + union {
> > + struct devlink_port_pci_pf_attrs pci_pf;
> > + };
> > };
> >
> > struct devlink_port {
> > @@ -590,6 +597,10 @@ void devlink_port_attrs_set(struct devlink_port
> *devlink_port,
> > u32 split_subport_number,
> > const unsigned char *switch_id,
> > unsigned char switch_id_len);
> > +void devlink_port_attrs_pci_pf_set(struct devlink_port *devlink_port,
> > + u32 port_number,
> > + const unsigned char *switch_id,
> > + unsigned char switch_id_len, u16 pf);
> > int devlink_sb_register(struct devlink *devlink, unsigned int sb_index,
> > u32 size, u16 ingress_pools_count,
> > u16 egress_pools_count, u16 ingress_tc_count, diff --
> git
> > a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h index
> > 5287b42c181f..f7323884c3fe 100644
> > --- a/include/uapi/linux/devlink.h
> > +++ b/include/uapi/linux/devlink.h
> > @@ -169,6 +169,10 @@ enum devlink_port_flavour {
> > DEVLINK_PORT_FLAVOUR_DSA, /* Distributed switch architecture
> > * interconnect port.
> > */
> > + DEVLINK_PORT_FLAVOUR_PCI_PF, /* Represents eswitch port for
> > + * the PCI PF. It is an internal
> > + * port that faces the PCI PF.
> > + */
> > };
> >
> > enum devlink_param_cmode {
> > @@ -337,6 +341,7 @@ enum devlink_attr {
> > DEVLINK_ATTR_FLASH_UPDATE_STATUS_DONE, /* u64 */
> > DEVLINK_ATTR_FLASH_UPDATE_STATUS_TOTAL, /* u64 */
> >
> > + DEVLINK_ATTR_PORT_PCI_PF_NUMBER, /* u16 */
> > /* add new attributes above here, update the policy in devlink.c */
> >
> > __DEVLINK_ATTR_MAX,
> > 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..
>
> > + 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))
