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. >+ * 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". >+ }; > 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. >+ 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. > 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. >+ 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 >