RE: [PATCH net-next 1/3] devlink: Introduce PCI PF port flavour and port attribute

[Parav Pandit <parav@mellanox.com>]

Hi Jakub,

> -----Original Message-----
> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> Sent: Tuesday, July 2, 2019 4:57 AM
> To: Jiri Pirko <jiri@mellanox.com>
> Cc: Parav Pandit <parav@mellanox.com>; netdev@vger.kernel.org; Saeed
> Mahameed <saeedm@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 <jiri@mellanox.com>
> > Signed-off-by: Parav Pandit <parav@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))