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

[Parav Pandit <parav@mellanox.com>]

> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Saturday, July 6, 2019 11:56 AM
> To: Parav Pandit <parav@mellanox.com>
> Cc: netdev@vger.kernel.org; Jiri Pirko <jiri@mellanox.com>; Saeed Mahameed
> <saeedm@mellanox.com>; jakub.kicinski@netronome.com
> Subject: Re: [PATCH net-next v3 1/3] devlink: Introduce PCI PF port flavour and
> port attribute
> 
> Sat, Jul 06, 2019 at 08:16:24AM CEST, parav@mellanox.com 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 and
> >a representor netdevice, 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
> >
> >Signed-off-by: Parav Pandit <parav@mellanox.com>
> >
> >---
> >Changelog:
> >v2->v3:
> > - Address comments from Jakub.
> > - Made port_number and split_port_number applicable only to
> >   physical port flavours by having in union.
> >v1->v2:
> > - Limited port_num attribute to physical ports
> > - Updated PCI PF attribute set API to not have port_number
> >---
> > include/net/devlink.h        | 21 +++++++-
> > include/uapi/linux/devlink.h |  5 ++
> > net/core/devlink.c           | 97 ++++++++++++++++++++++++++++--------
> > 3 files changed, 100 insertions(+), 23 deletions(-)
> >
> >diff --git a/include/net/devlink.h b/include/net/devlink.h index
> >6625ea068d5e..1455f60e4069 100644
> >--- a/include/net/devlink.h
> >+++ b/include/net/devlink.h
> >@@ -38,13 +38,27 @@ struct devlink {
> > 	char priv[0] __aligned(NETDEV_ALIGN);  };
> >
> >+struct devlink_port_phys_attrs {
> >+	u32 port_number; /* same value as "split group".
> 
> "Same" with capital letter.
> 
Done in v4.

> >+			  * A physical port which is visible to the user
> >+			  * for a given port flavour.
> >+			  */
> >+	u32 split_subport_number;
> >+};
> >+
> >+struct devlink_port_pci_pf_attrs {
> >+	u16 pf;	/* Associated PCI PF for this port. */
> >+};
> >+
> > struct devlink_port_attrs {
> > 	u8 set:1,
> > 	   split:1,
> > 	   switch_port:1;
> > 	enum devlink_port_flavour flavour;
> >-	u32 port_number; /* same value as "split group" */
> >-	u32 split_subport_number;
> >+	union {
> >+		struct devlink_port_phys_attrs phys_port;
> >+		struct devlink_port_pci_pf_attrs pci_pf;
> 
> Be consistent in naming: "phys", "pci_pf".
> 
Done in v4 as "physical".

> 
> >+	};
> > 	struct netdev_phys_item_id switch_id; };
> >
> >@@ -590,6 +604,9 @@ 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,
> >+				   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..9aa36104b471 100644
> >--- a/net/core/devlink.c
> >+++ b/net/core/devlink.c
> >@@ -506,6 +506,14 @@ static void devlink_notify(struct devlink *devlink,
> enum devlink_command cmd)
> > 				msg, 0, DEVLINK_MCGRP_CONFIG,
> GFP_KERNEL);  }
> >
> >+static bool
> >+is_devlink_phy_port_num_supported(const struct devlink_port *dl_port)
> >+{
> >+	return (dl_port->attrs.flavour == DEVLINK_PORT_FLAVOUR_PHYSICAL
> ||
> >+		dl_port->attrs.flavour == DEVLINK_PORT_FLAVOUR_CPU ||
> >+		dl_port->attrs.flavour == DEVLINK_PORT_FLAVOUR_DSA); }
> >+
> > static int devlink_nl_port_attrs_put(struct sk_buff *msg,
> > 				     struct devlink_port *devlink_port)  { @@ -
> 515,14 +523,23 @@
> >static int devlink_nl_port_attrs_put(struct sk_buff *msg,
> > 		return 0;
> > 	if (nla_put_u16(msg, DEVLINK_ATTR_PORT_FLAVOUR, attrs->flavour))
> > 		return -EMSGSIZE;
> >-	if (nla_put_u32(msg, DEVLINK_ATTR_PORT_NUMBER, attrs-
> >port_number))
> >+	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 (!is_devlink_phy_port_num_supported(devlink_port))
> 
> Please do the check here. No need for helper (the name with "is" and
> "supported" is weird anyway.
> 
Done in v4.
> 
> >+		return 0;
> >+	if (nla_put_u32(msg, DEVLINK_ATTR_PORT_NUMBER,
> >+			attrs->phys_port.port_number))
> > 		return -EMSGSIZE;
> > 	if (!attrs->split)
> > 		return 0;
> >-	if (nla_put_u32(msg, DEVLINK_ATTR_PORT_SPLIT_GROUP, attrs-
> >port_number))
> >+	if (nla_put_u32(msg, DEVLINK_ATTR_PORT_SPLIT_GROUP,
> >+			attrs->phys_port.port_number))
> 
> Better to split this into 2 patches. One pushing phys things into separate struct,
> the second the rest.
> 
Done in v4.
> 
> > 		return -EMSGSIZE;
> > 	if (nla_put_u32(msg, DEVLINK_ATTR_PORT_SPLIT_SUBPORT_NUMBER,
> >-			attrs->split_subport_number))
> >+			attrs->phys_port.split_subport_number))
> > 		return -EMSGSIZE;
> > 	return 0;
> > }
> >@@ -5738,6 +5755,30 @@ void devlink_port_type_clear(struct devlink_port
> >*devlink_port)  }  EXPORT_SYMBOL_GPL(devlink_port_type_clear);
> >
> >+static void __devlink_port_attrs_set(struct devlink_port *devlink_port,
> >+				     enum devlink_port_flavour flavour,
> >+				     u32 port_number,
> >+				     const unsigned char *switch_id,
> >+				     unsigned char switch_id_len)
> >+{
> >+	struct devlink_port_attrs *attrs = &devlink_port->attrs;
> >+
> >+	if (WARN_ON(devlink_port->registered))
> >+		return;
> >+	attrs->set = true;
> >+	attrs->flavour = flavour;
> >+	attrs->phys_port.port_number = port_number;
> >+	if (switch_id) {
> >+		attrs->switch_port = true;
> >+		if (WARN_ON(switch_id_len > MAX_PHYS_ITEM_ID_LEN))
> >+			switch_id_len = MAX_PHYS_ITEM_ID_LEN;
> >+		memcpy(attrs->switch_id.id, switch_id, switch_id_len);
> >+		attrs->switch_id.id_len = switch_id_len;
> >+	} else {
> >+		attrs->switch_port = false;
> >+	}
> >+}
> >+
> > /**
> >  *	devlink_port_attrs_set - Set port attributes
> >  *
> >@@ -5761,25 +5802,34 @@ void devlink_port_attrs_set(struct devlink_port
> >*devlink_port,  {
> > 	struct devlink_port_attrs *attrs = &devlink_port->attrs;
> >
> >-	if (WARN_ON(devlink_port->registered))
> >-		return;
> >-	attrs->set = true;
> >-	attrs->flavour = flavour;
> >-	attrs->port_number = port_number;
> >+	__devlink_port_attrs_set(devlink_port, flavour, port_number,
> >+				 switch_id, switch_id_len);
> > 	attrs->split = split;
> >-	attrs->split_subport_number = split_subport_number;
> >-	if (switch_id) {
> >-		attrs->switch_port = true;
> >-		if (WARN_ON(switch_id_len > MAX_PHYS_ITEM_ID_LEN))
> >-			switch_id_len = MAX_PHYS_ITEM_ID_LEN;
> >-		memcpy(attrs->switch_id.id, switch_id, switch_id_len);
> >-		attrs->switch_id.id_len = switch_id_len;
> >-	} else {
> >-		attrs->switch_port = false;
> >-	}
> >+	attrs->phys_port.split_subport_number = split_subport_number;
> > }
> > EXPORT_SYMBOL_GPL(devlink_port_attrs_set);
> >
> >+/**
> >+ *	devlink_port_attrs_pci_pf_set - Set PCI PF port attributes
> >+ *
> >+ *	@devlink_port: devlink port
> >+ *	@pf: associated PF for the devlink port instance
> >+ *	@switch_id: if the port is part of switch, this is buffer with ID,
> >+ *	            otwerwise this is NULL
> >+ *	@switch_id_len: length of the switch_id buffer
> >+ */
> >+void devlink_port_attrs_pci_pf_set(struct devlink_port *devlink_port,
> >+				   const unsigned char *switch_id,
> >+				   unsigned char switch_id_len, u16 pf) {
> >+	struct devlink_port_attrs *attrs = &devlink_port->attrs;
> >+
> >+	__devlink_port_attrs_set(devlink_port,
> DEVLINK_PORT_FLAVOUR_PCI_PF,
> >+				 0, switch_id, switch_id_len);
> 
> Please have this done differently. __devlink_port_attrs_set() sets
> attrs->phys_port.port_number which does not make sense there.
> 
Changed in v4.

> 
> >+	attrs->pci_pf.pf = pf;
> >+}
> >+EXPORT_SYMBOL_GPL(devlink_port_attrs_pci_pf_set);
> >+
> > static int __devlink_port_phys_port_name_get(struct devlink_port
> *devlink_port,
> > 					     char *name, size_t len)
> > {
> >@@ -5792,10 +5842,12 @@ static int
> __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
> > 	switch (attrs->flavour) {
> > 	case DEVLINK_PORT_FLAVOUR_PHYSICAL:
> > 		if (!attrs->split)
> >-			n = snprintf(name, len, "p%u", attrs->port_number);
> >+			n = snprintf(name, len, "p%u",
> >+				     attrs->phys_port.port_number);
> > 		else
> >-			n = snprintf(name, len, "p%us%u", attrs->port_number,
> >-				     attrs->split_subport_number);
> >+			n = snprintf(name, len, "p%us%u",
> >+				     attrs->phys_port.port_number,
> >+				     attrs->phys_port.split_subport_number);
> > 		break;
> > 	case DEVLINK_PORT_FLAVOUR_CPU:
> > 	case DEVLINK_PORT_FLAVOUR_DSA:
> >@@ -5804,6 +5856,9 @@ static int
> __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
> > 		 */
> > 		WARN_ON(1);
> > 		return -EINVAL;
> >+	case DEVLINK_PORT_FLAVOUR_PCI_PF:
> >+		n = snprintf(name, len, "pf%u", attrs->pci_pf.pf);
> >+		break;
> > 	}
> >
> > 	if (n >= len)
> >--
> >2.19.2
> >