> -----Original Message-----
> From: Jiri Pirko <j...@resnulli.us>
> Sent: Saturday, July 6, 2019 11:56 AM
> To: Parav Pandit <pa...@mellanox.com>
> Cc: netdev@vger.kernel.org; Jiri Pirko <j...@mellanox.com>; Saeed Mahameed
> <sae...@mellanox.com>; jakub.kicin...@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, pa...@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 <pa...@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
> >
