[patch net-next v4 0/4] export device physical port id to userspace

[patch net-next v4 0/4] export device physical port id to userspace

[Jiri Pirko]

This patchset is based on patch by Narendra_K@Dell.com
Once device which can change phys port id during its lifetime adopts this,
NETDEV_CHANGEPHYSPORTID event will be added and driver will call
call_netdevice_notifiers(NETDEV_NETDEV_CHANGEPHYSPORTID, dev) to propagate
the change to userspace.

v1->v2: as suggested by Ben, handle -EOPNOTSUPP in rtnl code (wrapped up ndo call)
v2->v3: adjusted patch 1 commit message
v3->v4: used "%phN" for sysfs printf as suggested by DaveM
	added igb/igbvf implementation as requested by Or Gerlitz

Jiri Pirko (4):
  net: add ndo to get id of physical port of the device
  rtnl: export physical port id via RT netlink
  net: export physical port id via sysfs
  igb/igbvf: implement ndo_get_phys_port_id

 drivers/net/ethernet/intel/igb/e1000_mbx.h |  1 +
 drivers/net/ethernet/intel/igb/igb.h       |  2 ++
 drivers/net/ethernet/intel/igb/igb_main.c  | 43 +++++++++++++++++++++++++++-
 drivers/net/ethernet/intel/igbvf/igbvf.h   |  3 ++
 drivers/net/ethernet/intel/igbvf/mbx.h     |  1 +
 drivers/net/ethernet/intel/igbvf/netdev.c  | 45 ++++++++++++++++++++++++++++++
 drivers/net/ethernet/intel/igbvf/vf.c      | 34 ++++++++++++++++++++++
 drivers/net/ethernet/intel/igbvf/vf.h      |  1 +
 include/linux/netdevice.h                  | 20 +++++++++++++
 include/uapi/linux/if_link.h               |  1 +
 net/core/dev.c                             | 18 ++++++++++++
 net/core/net-sysfs.c                       | 22 +++++++++++++++
 net/core/rtnetlink.c                       | 25 ++++++++++++++++-
 13 files changed, 214 insertions(+), 2 deletions(-)

-- 
1.8.1.4

[patch net-next v4 1/4] net: add ndo to get id of physical port of the device

[Jiri Pirko]

This patch adds a ndo for getting physical port of the device. Driver
which is aware of being virtual function of some physical port should
implement this ndo. This is applicable not only for IOV, but for other
solutions (NPAR, multichannel) as well. Basically if there is possible
to have multiple netdevs on the single hw port.

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
Acked-by: Ben Hutchings <bhutchings@solarflare.com>
---
 include/linux/netdevice.h | 20 ++++++++++++++++++++
 net/core/dev.c            | 18 ++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 3ca60b0..875f869 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -728,6 +728,16 @@ struct netdev_fcoe_hbainfo {
 };
 #endif
 
+#define MAX_PHYS_PORT_ID_LEN 32
+
+/* This structure holds a unique identifier to identify the
+ * physical port used by a netdevice.
+ */
+struct netdev_phys_port_id {
+	unsigned char id[MAX_PHYS_PORT_ID_LEN];
+	unsigned char id_len;
+};
+
 /*
  * This structure defines the management hooks for network devices.
  * The following hooks can be defined; unless noted otherwise, they are
@@ -932,6 +942,12 @@ struct netdev_fcoe_hbainfo {
  *	that determine carrier state from physical hardware properties (eg
  *	network cables) or protocol-dependent mechanisms (eg
  *	USB_CDC_NOTIFY_NETWORK_CONNECTION) should NOT implement this function.
+ *
+ * int (*ndo_get_phys_port_id)(struct net_device *dev,
+ *			       struct netdev_phys_port_id *ppid);
+ *	Called to get ID of physical port of this device. If driver does
+ *	not implement this, it is assumed that the hw is not able to have
+ *	multiple net devices on single physical port.
  */
 struct net_device_ops {
 	int			(*ndo_init)(struct net_device *dev);
@@ -1060,6 +1076,8 @@ struct net_device_ops {
 						      struct nlmsghdr *nlh);
 	int			(*ndo_change_carrier)(struct net_device *dev,
 						      bool new_carrier);
+	int			(*ndo_get_phys_port_id)(struct net_device *dev,
+							struct netdev_phys_port_id *ppid);
 };
 
 /*
@@ -2315,6 +2333,8 @@ extern int		dev_set_mac_address(struct net_device *,
 					    struct sockaddr *);
 extern int		dev_change_carrier(struct net_device *,
 					   bool new_carrier);
+extern int		dev_get_phys_port_id(struct net_device *dev,
+					     struct netdev_phys_port_id *ppid);
 extern int		dev_hard_start_xmit(struct sk_buff *skb,
 					    struct net_device *dev,
 					    struct netdev_queue *txq);
diff --git a/net/core/dev.c b/net/core/dev.c
index dfd9f5d..58eb802 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4989,6 +4989,24 @@ int dev_change_carrier(struct net_device *dev, bool new_carrier)
 EXPORT_SYMBOL(dev_change_carrier);
 
 /**
+ *	dev_get_phys_port_id - Get device physical port ID
+ *	@dev: device
+ *	@ppid: port ID
+ *
+ *	Get device physical port ID
+ */
+int dev_get_phys_port_id(struct net_device *dev,
+			 struct netdev_phys_port_id *ppid)
+{
+	const struct net_device_ops *ops = dev->netdev_ops;
+
+	if (!ops->ndo_get_phys_port_id)
+		return -EOPNOTSUPP;
+	return ops->ndo_get_phys_port_id(dev, ppid);
+}
+EXPORT_SYMBOL(dev_get_phys_port_id);
+
+/**
  *	dev_new_index	-	allocate an ifindex
  *	@net: the applicable net namespace
  *
-- 
1.8.1.4

[patch net-next v4 2/4] rtnl: export physical port id via RT netlink

[Jiri Pirko]

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
Acked-by: Ben Hutchings <bhutchings@solarflare.com>
Signed-off-by: Narendra K <narendra_k@dell.com>
---
 include/uapi/linux/if_link.h |  1 +
 net/core/rtnetlink.c         | 25 ++++++++++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 03f6170..04c0e7a 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -143,6 +143,7 @@ enum {
 	IFLA_NUM_TX_QUEUES,
 	IFLA_NUM_RX_QUEUES,
 	IFLA_CARRIER,
+	IFLA_PHYS_PORT_ID,
 	__IFLA_MAX
 };
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 3de7408..0b2972c 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -767,7 +767,8 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
 	       + rtnl_vfinfo_size(dev, ext_filter_mask) /* IFLA_VFINFO_LIST */
 	       + rtnl_port_size(dev) /* IFLA_VF_PORTS + IFLA_PORT_SELF */
 	       + rtnl_link_get_size(dev) /* IFLA_LINKINFO */
-	       + rtnl_link_get_af_size(dev); /* IFLA_AF_SPEC */
+	       + rtnl_link_get_af_size(dev) /* IFLA_AF_SPEC */
+	       + nla_total_size(MAX_PHYS_PORT_ID_LEN); /* IFLA_PHYS_PORT_ID */
 }
 
 static int rtnl_vf_ports_fill(struct sk_buff *skb, struct net_device *dev)
@@ -846,6 +847,24 @@ static int rtnl_port_fill(struct sk_buff *skb, struct net_device *dev)
 	return 0;
 }
 
+static int rtnl_phys_port_id_fill(struct sk_buff *skb, struct net_device *dev)
+{
+	int err;
+	struct netdev_phys_port_id ppid;
+
+	err = dev_get_phys_port_id(dev, &ppid);
+	if (err) {
+		if (err == -EOPNOTSUPP)
+			return 0;
+		return err;
+	}
+
+	if (nla_put(skb, IFLA_PHYS_PORT_ID, ppid.id_len, ppid.id))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
 static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			    int type, u32 pid, u32 seq, u32 change,
 			    unsigned int flags, u32 ext_filter_mask)
@@ -913,6 +932,9 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			goto nla_put_failure;
 	}
 
+	if (rtnl_phys_port_id_fill(skb, dev))
+		goto nla_put_failure;
+
 	attr = nla_reserve(skb, IFLA_STATS,
 			sizeof(struct rtnl_link_stats));
 	if (attr == NULL)
@@ -1113,6 +1135,7 @@ const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 	[IFLA_PROMISCUITY]	= { .type = NLA_U32 },
 	[IFLA_NUM_TX_QUEUES]	= { .type = NLA_U32 },
 	[IFLA_NUM_RX_QUEUES]	= { .type = NLA_U32 },
+	[IFLA_PHYS_PORT_ID]	= { .type = NLA_BINARY, .len = MAX_PHYS_PORT_ID_LEN },
 };
 EXPORT_SYMBOL(ifla_policy);
 
-- 
1.8.1.4

[patch net-next v4 3/4] net: export physical port id via sysfs

[Jiri Pirko]

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
Acked-by: Ben Hutchings <bhutchings@solarflare.com>
Signed-off-by: Narendra K <narendra_k@dell.com>
---
 net/core/net-sysfs.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 981fed3..8826b0d 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -334,6 +334,27 @@ static ssize_t store_group(struct device *dev, struct device_attribute *attr,
 	return netdev_store(dev, attr, buf, len, change_group);
 }
 
+static ssize_t show_phys_port_id(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct net_device *netdev = to_net_dev(dev);
+	ssize_t ret = -EINVAL;
+
+	if (!rtnl_trylock())
+		return restart_syscall();
+
+	if (dev_isalive(netdev)) {
+		struct netdev_phys_port_id ppid;
+
+		ret = dev_get_phys_port_id(netdev, &ppid);
+		if (!ret)
+			ret = sprintf(buf, "%*phN\n", ppid.id_len, ppid.id);
+	}
+	rtnl_unlock();
+
+	return ret;
+}
+
 static struct device_attribute net_class_attributes[] = {
 	__ATTR(addr_assign_type, S_IRUGO, show_addr_assign_type, NULL),
 	__ATTR(addr_len, S_IRUGO, show_addr_len, NULL),
@@ -355,6 +376,7 @@ static struct device_attribute net_class_attributes[] = {
 	__ATTR(tx_queue_len, S_IRUGO | S_IWUSR, show_tx_queue_len,
 	       store_tx_queue_len),
 	__ATTR(netdev_group, S_IRUGO | S_IWUSR, show_group, store_group),
+	__ATTR(phys_port_id, S_IRUGO, show_phys_port_id, NULL),
 	{}
 };
 
-- 
1.8.1.4

[patch net-next v4 4/4] igb/igbvf: implement ndo_get_phys_port_id

[Jiri Pirko]

igb driver computes hash which will identify physical port. This hash is
available via ndo_get_phys_port_id directly on igb netdev. Also, this
hash is passed to igbvf using mailbox. After that, it is available via
ndo_get_phys_port_id on igbvf netdev as well.

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 drivers/net/ethernet/intel/igb/e1000_mbx.h |  1 +
 drivers/net/ethernet/intel/igb/igb.h       |  2 ++
 drivers/net/ethernet/intel/igb/igb_main.c  | 43 +++++++++++++++++++++++++++-
 drivers/net/ethernet/intel/igbvf/igbvf.h   |  3 ++
 drivers/net/ethernet/intel/igbvf/mbx.h     |  1 +
 drivers/net/ethernet/intel/igbvf/netdev.c  | 45 ++++++++++++++++++++++++++++++
 drivers/net/ethernet/intel/igbvf/vf.c      | 34 ++++++++++++++++++++++
 drivers/net/ethernet/intel/igbvf/vf.h      |  1 +
 8 files changed, 129 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_mbx.h b/drivers/net/ethernet/intel/igb/e1000_mbx.h
index de9bba4..1788480 100644
--- a/drivers/net/ethernet/intel/igb/e1000_mbx.h
+++ b/drivers/net/ethernet/intel/igb/e1000_mbx.h
@@ -64,6 +64,7 @@
 #define E1000_VF_SET_LPE	0x05 /* VF requests to set VMOLR.LPE */
 #define E1000_VF_SET_PROMISC	0x06 /*VF requests to clear VMOLR.ROPE/MPME*/
 #define E1000_VF_SET_PROMISC_MULTICAST	(0x02 << E1000_VT_MSGINFO_SHIFT)
+#define E1000_VF_GET_PHYS_PORT_ID 0x07 /* VF requests to get physical port id */
 
 #define E1000_PF_CONTROL_MSG	0x0100 /* PF control message */
 
diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 15ea8dc..12a25b1 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -444,6 +444,8 @@ struct igb_adapter {
 	struct i2c_algo_bit_data i2c_algo;
 	struct i2c_adapter i2c_adap;
 	struct i2c_client *i2c_client;
+
+	u32 phys_port_id;
 };
 
 #define IGB_FLAG_HAS_MSI		(1 << 0)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 6a0c1b6..4db3fde 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -1892,6 +1892,24 @@ static int igb_set_features(struct net_device *netdev,
 	return 0;
 }
 
+/**
+ * igb_get_phys_port_id - Get physical port ID
+ * @netdev: network interface device structure
+ * @ppid: pointer to a physical port id structure
+ *
+ * Returns 0 on success, negative on failure
+ **/
+static int igb_get_phys_port_id(struct net_device *netdev,
+				struct netdev_phys_port_id *ppid)
+{
+	struct igb_adapter *adapter = netdev_priv(netdev);
+
+	ppid->id_len = sizeof(adapter->phys_port_id);
+	memcpy(ppid->id, &adapter->phys_port_id, ppid->id_len);
+
+	return 0;
+}
+
 static const struct net_device_ops igb_netdev_ops = {
 	.ndo_open		= igb_open,
 	.ndo_stop		= igb_close,
@@ -1915,6 +1933,7 @@ static const struct net_device_ops igb_netdev_ops = {
 #endif
 	.ndo_fix_features	= igb_fix_features,
 	.ndo_set_features	= igb_set_features,
+	.ndo_get_phys_port_id	= igb_get_phys_port_id,
 };
 
 /**
@@ -1982,6 +2001,21 @@ static s32 igb_init_i2c(struct igb_adapter *adapter)
 	return status;
 }
 
+static void igb_compute_phys_port_id(struct igb_adapter *adapter)
+{
+	adapter->phys_port_id = *((u32 *) adapter->netdev->dev_addr);
+	adapter->phys_port_id ^= *((u32 *) adapter->netdev->dev_addr + 4);
+	adapter->phys_port_id ^= (long) adapter;
+	adapter->phys_port_id ^= (long) adapter->hw.hw_addr;
+	adapter->phys_port_id ^= (long) adapter->hw.flash_address;
+	adapter->phys_port_id ^= (u32) adapter->hw.io_base;
+	adapter->phys_port_id ^= adapter->hw.device_id;
+	adapter->phys_port_id ^= adapter->hw.subsystem_vendor_id;
+	adapter->phys_port_id ^= adapter->hw.subsystem_device_id;
+	adapter->phys_port_id ^= adapter->hw.vendor_id;
+	adapter->phys_port_id ^= adapter->hw.revision_id;
+}
+
 /**
  *  igb_probe - Device Initialization Routine
  *  @pdev: PCI device information struct
@@ -2287,6 +2321,8 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	 * driver. */
 	igb_get_hw_control(adapter);
 
+	igb_compute_phys_port_id(adapter);
+
 	strcpy(netdev->name, "eth%d");
 	err = register_netdev(netdev);
 	if (err)
@@ -5698,6 +5734,7 @@ static void igb_rcv_msg_from_vf(struct igb_adapter *adapter, u32 vf)
 	struct e1000_hw *hw = &adapter->hw;
 	struct vf_data_storage *vf_data = &adapter->vf_data[vf];
 	s32 retval;
+	u16 write_size = 1;
 
 	retval = igb_read_mbx(hw, msgbuf, E1000_VFMAILBOX_SIZE, vf);
 
@@ -5757,6 +5794,10 @@ static void igb_rcv_msg_from_vf(struct igb_adapter *adapter, u32 vf)
 		else
 			retval = igb_set_vf_vlan(adapter, msgbuf, vf);
 		break;
+	case E1000_VF_GET_PHYS_PORT_ID:
+		msgbuf[1] = adapter->phys_port_id;
+		write_size = 2;
+		break;
 	default:
 		dev_err(&pdev->dev, "Unhandled Msg %08x\n", msgbuf[0]);
 		retval = -1;
@@ -5771,7 +5812,7 @@ out:
 	else
 		msgbuf[0] |= E1000_VT_MSGTYPE_ACK;
 
-	igb_write_mbx(hw, msgbuf, 1, vf);
+	igb_write_mbx(hw, msgbuf, write_size, vf);
 }
 
 static void igb_msg_task(struct igb_adapter *adapter)
diff --git a/drivers/net/ethernet/intel/igbvf/igbvf.h b/drivers/net/ethernet/intel/igbvf/igbvf.h
index a1463e3..559ccaf 100644
--- a/drivers/net/ethernet/intel/igbvf/igbvf.h
+++ b/drivers/net/ethernet/intel/igbvf/igbvf.h
@@ -283,6 +283,9 @@ struct igbvf_adapter {
 
 	unsigned int flags;
 	unsigned long last_reset;
+
+	u32 phys_port_id;
+	bool phys_port_id_set;
 };
 
 struct igbvf_info {
diff --git a/drivers/net/ethernet/intel/igbvf/mbx.h b/drivers/net/ethernet/intel/igbvf/mbx.h
index 24370bc..1040d36 100644
--- a/drivers/net/ethernet/intel/igbvf/mbx.h
+++ b/drivers/net/ethernet/intel/igbvf/mbx.h
@@ -66,6 +66,7 @@
 #define E1000_VF_SET_MULTICAST    0x03 /* VF requests PF to set MC addr */
 #define E1000_VF_SET_VLAN         0x04 /* VF requests PF to set VLAN */
 #define E1000_VF_SET_LPE          0x05 /* VF requests PF to set VMOLR.LPE */
+#define E1000_VF_GET_PHYS_PORT_ID 0x07 /* VF requests to get physical port id */
 
 #define E1000_PF_CONTROL_MSG      0x0100 /* PF control message */
 
diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c
index 93eb7ee..475cccb 100644
--- a/drivers/net/ethernet/intel/igbvf/netdev.c
+++ b/drivers/net/ethernet/intel/igbvf/netdev.c
@@ -1444,6 +1444,21 @@ static void igbvf_configure(struct igbvf_adapter *adapter)
 	                       igbvf_desc_unused(adapter->rx_ring));
 }
 
+static void igbvf_refresh_ppid(struct igbvf_adapter *adapter)
+{
+	struct e1000_hw *hw = &adapter->hw;
+	int err;
+
+	err = hw->mac.ops.ppid_get_vf(hw, &adapter->phys_port_id);
+	if (err) {
+		adapter->phys_port_id_set = false;
+		dev_err(&adapter->pdev->dev,
+			"Failed to get physical port ID\n");
+	} else {
+		adapter->phys_port_id_set = true;
+	}
+}
+
 /* igbvf_reset - bring the hardware into a known good state
  *
  * This function boots the hardware and enables some settings that
@@ -1461,6 +1476,8 @@ static void igbvf_reset(struct igbvf_adapter *adapter)
 	if (mac->ops.reset_hw(hw))
 		dev_err(&adapter->pdev->dev, "PF still resetting\n");
 
+	igbvf_refresh_ppid(adapter);
+
 	mac->ops.init_hw(hw);
 
 	if (is_valid_ether_addr(adapter->hw.mac.addr)) {
@@ -1753,6 +1770,27 @@ static int igbvf_set_mac(struct net_device *netdev, void *p)
 	return 0;
 }
 
+/**
+ * igbvf_get_phys_port_id - Get physical port ID
+ * @netdev: network interface device structure
+ * @ppid: pointer to a physical port id structure
+ *
+ * Returns 0 on success, negative on failure
+ **/
+static int igbvf_get_phys_port_id(struct net_device *netdev,
+				  struct netdev_phys_port_id *ppid)
+{
+	struct igbvf_adapter *adapter = netdev_priv(netdev);
+
+	if (!adapter->phys_port_id_set)
+		return -EOPNOTSUPP;
+
+	ppid->id_len = sizeof(adapter->phys_port_id);
+	memcpy(ppid->id, &adapter->phys_port_id, ppid->id_len);
+
+	return 0;
+}
+
 #define UPDATE_VF_COUNTER(reg, name)                                    \
 	{                                                               \
 		u32 current_counter = er32(reg);                        \
@@ -2610,6 +2648,7 @@ static const struct net_device_ops igbvf_netdev_ops = {
 	.ndo_poll_controller            = igbvf_netpoll,
 #endif
 	.ndo_set_features               = igbvf_set_features,
+	.ndo_get_phys_port_id		= igbvf_get_phys_port_id,
 };
 
 /**
@@ -2750,6 +2789,10 @@ static int igbvf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			dev_info(&pdev->dev, "MAC address not assigned by administrator.\n");
 		memcpy(netdev->dev_addr, adapter->hw.mac.addr,
 		       netdev->addr_len);
+		err = hw->mac.ops.ppid_get_vf(hw, &adapter->phys_port_id);
+		if (err)
+			dev_err(&pdev->dev,
+				 "Failed to get physical port ID\n");
 	}
 
 	if (!is_valid_ether_addr(netdev->dev_addr)) {
@@ -2759,6 +2802,8 @@ static int igbvf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			netdev->addr_len);
 	}
 
+	igbvf_refresh_ppid(adapter);
+
 	setup_timer(&adapter->watchdog_timer, &igbvf_watchdog,
 	            (unsigned long) adapter);
 
diff --git a/drivers/net/ethernet/intel/igbvf/vf.c b/drivers/net/ethernet/intel/igbvf/vf.c
index eea0e10..1a4afa5 100644
--- a/drivers/net/ethernet/intel/igbvf/vf.c
+++ b/drivers/net/ethernet/intel/igbvf/vf.c
@@ -39,6 +39,7 @@ static void e1000_update_mc_addr_list_vf(struct e1000_hw *hw, u8 *,
 static void e1000_rar_set_vf(struct e1000_hw *, u8 *, u32);
 static s32 e1000_read_mac_addr_vf(struct e1000_hw *);
 static s32 e1000_set_vfta_vf(struct e1000_hw *, u16, bool);
+static s32 e1000_ppid_get_vf(struct e1000_hw *hw, u32 *id);
 
 /**
  *  e1000_init_mac_params_vf - Inits MAC params
@@ -70,6 +71,8 @@ static s32 e1000_init_mac_params_vf(struct e1000_hw *hw)
 	mac->ops.read_mac_addr = e1000_read_mac_addr_vf;
 	/* set vlan filter table array */
 	mac->ops.set_vfta = e1000_set_vfta_vf;
+	/* get physical port ID */
+	mac->ops.ppid_get_vf = e1000_ppid_get_vf;
 
 	return E1000_SUCCESS;
 }
@@ -398,3 +401,34 @@ out:
 	return ret_val;
 }
 
+/**
+ *  e1000_ppid_get_vf - get device MAC physical port ID
+ *  @hw: pointer to the HW structure
+ *  @id: pointer where port ID will be written
+ **/
+static s32 e1000_ppid_get_vf(struct e1000_hw *hw, u32 *id)
+{
+	struct e1000_mbx_info *mbx = &hw->mbx;
+	u32 msgbuf[2];
+	s32 ret_val;
+
+	memset(msgbuf, 0, sizeof(msgbuf));
+	msgbuf[0] = E1000_VF_GET_PHYS_PORT_ID;
+
+	ret_val = mbx->ops.write_posted(hw, msgbuf, 1);
+	if (ret_val)
+		goto out;
+
+	ret_val = mbx->ops.read_posted(hw, msgbuf, 2);
+	if (ret_val)
+		goto out;
+
+	if (msgbuf[0] & E1000_VT_MSGTYPE_NACK) {
+		ret_val = -E1000_ERR_MAC_INIT;
+		goto out;
+	}
+	*id = msgbuf[1];
+
+out:
+	return ret_val;
+}
diff --git a/drivers/net/ethernet/intel/igbvf/vf.h b/drivers/net/ethernet/intel/igbvf/vf.h
index 57db3c6..aa04f46 100644
--- a/drivers/net/ethernet/intel/igbvf/vf.h
+++ b/drivers/net/ethernet/intel/igbvf/vf.h
@@ -188,6 +188,7 @@ struct e1000_mac_operations {
 	void (*rar_set)(struct e1000_hw *, u8*, u32);
 	s32  (*read_mac_addr)(struct e1000_hw *);
 	s32  (*set_vfta)(struct e1000_hw *, u16, bool);
+	s32  (*ppid_get_vf)(struct e1000_hw *, u32 *);
 };
 
 struct e1000_mac_info {
-- 
1.8.1.4

Re: [patch net-next v4 4/4] igb/igbvf: implement ndo_get_phys_port_id

[Alexander Duyck]

On 07/25/2013 06:03 AM, Jiri Pirko wrote:
> @@ -1982,6 +2001,21 @@ static s32 igb_init_i2c(struct igb_adapter *adapter)
>  	return status;
>  }
>  
> +static void igb_compute_phys_port_id(struct igb_adapter *adapter)
> +{
> +	adapter->phys_port_id = *((u32 *) adapter->netdev->dev_addr);
> +	adapter->phys_port_id ^= *((u32 *) adapter->netdev->dev_addr + 4);
> +	adapter->phys_port_id ^= (long) adapter;
> +	adapter->phys_port_id ^= (long) adapter->hw.hw_addr;
> +	adapter->phys_port_id ^= (long) adapter->hw.flash_address;
> +	adapter->phys_port_id ^= (u32) adapter->hw.io_base;
> +	adapter->phys_port_id ^= adapter->hw.device_id;
> +	adapter->phys_port_id ^= adapter->hw.subsystem_vendor_id;
> +	adapter->phys_port_id ^= adapter->hw.subsystem_device_id;
> +	adapter->phys_port_id ^= adapter->hw.vendor_id;
> +	adapter->phys_port_id ^= adapter->hw.revision_id;
> +}
> +
>  /**
>   *  igb_probe - Device Initialization Routine
>   *  @pdev: PCI device information struct

I really think this bit here should be standardized and made available
to all drivers.  If nothing else maybe you should define a function that
takes in the netdev and the pci_dev and computes the hash based on those
values.  Otherwise you are going to end up with each driver doing its'
own version of this function and that will make things rather messy.

Also the port_id has a high likelihood of most fields canceling each
other out.  For example, I'm not sure if you really want to try and find
the offset of the hw_addr within the adapter structure but that is
likely what you are getting by casting the adapter pointer and he
hw_addr arrary as longs and XORing them together.  From what I can tell
the only fields that are providing any real value in distinguishing
between ports on a 4 port adapter would be the dev_addr, flash_address,
and io_base.

Thanks,

Alex

Re: [patch net-next v4 4/4] igb/igbvf: implement ndo_get_phys_port_id

[Ben Hutchings]

On Thu, 2013-07-25 at 09:17 -0700, Alexander Duyck wrote:
> On 07/25/2013 06:03 AM, Jiri Pirko wrote:
> > @@ -1982,6 +2001,21 @@ static s32 igb_init_i2c(struct igb_adapter *adapter)
> >  	return status;
> >  }
> >  
> > +static void igb_compute_phys_port_id(struct igb_adapter *adapter)
> > +{
> > +	adapter->phys_port_id = *((u32 *) adapter->netdev->dev_addr);
> > +	adapter->phys_port_id ^= *((u32 *) adapter->netdev->dev_addr + 4);
> > +	adapter->phys_port_id ^= (long) adapter;
> > +	adapter->phys_port_id ^= (long) adapter->hw.hw_addr;
> > +	adapter->phys_port_id ^= (long) adapter->hw.flash_address;
> > +	adapter->phys_port_id ^= (u32) adapter->hw.io_base;
> > +	adapter->phys_port_id ^= adapter->hw.device_id;
> > +	adapter->phys_port_id ^= adapter->hw.subsystem_vendor_id;
> > +	adapter->phys_port_id ^= adapter->hw.subsystem_device_id;
> > +	adapter->phys_port_id ^= adapter->hw.vendor_id;
> > +	adapter->phys_port_id ^= adapter->hw.revision_id;

I didn't look at Jiri's patch initially, but... what's wrong with using
the MAC address from NVRAM (should already be in netdev->perm_addr)?
That's what I'm expecting to do for the SFC9000 family in sfc.

> > +}
> > +
> >  /**
> >   *  igb_probe - Device Initialization Routine
> >   *  @pdev: PCI device information struct
> 
> I really think this bit here should be standardized and made available
> to all drivers.
[...]

I think it's a bad example and should not be used in any drivers!

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [patch net-next v4 4/4] igb/igbvf: implement ndo_get_phys_port_id

[Alexander Duyck]

On 07/25/2013 09:44 AM, Ben Hutchings wrote:
> On Thu, 2013-07-25 at 09:17 -0700, Alexander Duyck wrote:
>> On 07/25/2013 06:03 AM, Jiri Pirko wrote:
>>> @@ -1982,6 +2001,21 @@ static s32 igb_init_i2c(struct igb_adapter *adapter)
>>>  	return status;
>>>  }
>>>  
>>> +static void igb_compute_phys_port_id(struct igb_adapter *adapter)
>>> +{
>>> +	adapter->phys_port_id = *((u32 *) adapter->netdev->dev_addr);
>>> +	adapter->phys_port_id ^= *((u32 *) adapter->netdev->dev_addr + 4);
>>> +	adapter->phys_port_id ^= (long) adapter;
>>> +	adapter->phys_port_id ^= (long) adapter->hw.hw_addr;
>>> +	adapter->phys_port_id ^= (long) adapter->hw.flash_address;
>>> +	adapter->phys_port_id ^= (u32) adapter->hw.io_base;
>>> +	adapter->phys_port_id ^= adapter->hw.device_id;
>>> +	adapter->phys_port_id ^= adapter->hw.subsystem_vendor_id;
>>> +	adapter->phys_port_id ^= adapter->hw.subsystem_device_id;
>>> +	adapter->phys_port_id ^= adapter->hw.vendor_id;
>>> +	adapter->phys_port_id ^= adapter->hw.revision_id;
> I didn't look at Jiri's patch initially, but... what's wrong with using
> the MAC address from NVRAM (should already be in netdev->perm_addr)?
> That's what I'm expecting to do for the SFC9000 family in sfc.
>
>>> +}
>>> +
>>>  /**
>>>   *  igb_probe - Device Initialization Routine
>>>   *  @pdev: PCI device information struct
>> I really think this bit here should be standardized and made available
>> to all drivers.
> [...]
>
> I think it's a bad example and should not be used in any drivers!
>
> Ben.
>

I agree.  That is why the second paragraph started listing what was
wrong with this implementation.  What I said was meant in the more
general sense that whatever solution should be used should be the same
across multiple drivers, not up to each driver to compute.

Thanks,

Alex

Re: [patch net-next v4 4/4] igb/igbvf: implement ndo_get_phys_port_id

[Ben Hutchings]

On Thu, 2013-07-25 at 11:00 -0700, Alexander Duyck wrote:
> On 07/25/2013 09:44 AM, Ben Hutchings wrote:
> > On Thu, 2013-07-25 at 09:17 -0700, Alexander Duyck wrote:
> >> On 07/25/2013 06:03 AM, Jiri Pirko wrote:
> >>> @@ -1982,6 +2001,21 @@ static s32 igb_init_i2c(struct igb_adapter *adapter)
> >>>  	return status;
> >>>  }
> >>>  
> >>> +static void igb_compute_phys_port_id(struct igb_adapter *adapter)
> >>> +{
> >>> +	adapter->phys_port_id = *((u32 *) adapter->netdev->dev_addr);
> >>> +	adapter->phys_port_id ^= *((u32 *) adapter->netdev->dev_addr + 4);
> >>> +	adapter->phys_port_id ^= (long) adapter;
> >>> +	adapter->phys_port_id ^= (long) adapter->hw.hw_addr;
> >>> +	adapter->phys_port_id ^= (long) adapter->hw.flash_address;
> >>> +	adapter->phys_port_id ^= (u32) adapter->hw.io_base;
> >>> +	adapter->phys_port_id ^= adapter->hw.device_id;
> >>> +	adapter->phys_port_id ^= adapter->hw.subsystem_vendor_id;
> >>> +	adapter->phys_port_id ^= adapter->hw.subsystem_device_id;
> >>> +	adapter->phys_port_id ^= adapter->hw.vendor_id;
> >>> +	adapter->phys_port_id ^= adapter->hw.revision_id;
> > I didn't look at Jiri's patch initially, but... what's wrong with using
> > the MAC address from NVRAM (should already be in netdev->perm_addr)?
> > That's what I'm expecting to do for the SFC9000 family in sfc.
> >
> >>> +}
> >>> +
> >>>  /**
> >>>   *  igb_probe - Device Initialization Routine
> >>>   *  @pdev: PCI device information struct
> >> I really think this bit here should be standardized and made available
> >> to all drivers.
> > [...]
> >
> > I think it's a bad example and should not be used in any drivers!
> >
> > Ben.
> >
> 
> I agree.  That is why the second paragraph started listing what was
> wrong with this implementation.  What I said was meant in the more
> general sense that whatever solution should be used should be the same
> across multiple drivers, not up to each driver to compute.

I would love to know how you think this can be done generically.  If it
can then we don't need the driver operation at all.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [patch net-next v4 4/4] igb/igbvf: implement ndo_get_phys_port_id

[Alexander Duyck]

On 07/25/2013 11:51 AM, Ben Hutchings wrote:
> On Thu, 2013-07-25 at 11:00 -0700, Alexander Duyck wrote:
>> On 07/25/2013 09:44 AM, Ben Hutchings wrote:
>>> On Thu, 2013-07-25 at 09:17 -0700, Alexander Duyck wrote:
>>>> On 07/25/2013 06:03 AM, Jiri Pirko wrote:
>>>>> @@ -1982,6 +2001,21 @@ static s32 igb_init_i2c(struct igb_adapter *adapter)
>>>>>  	return status;
>>>>>  }
>>>>>  
>>>>> +static void igb_compute_phys_port_id(struct igb_adapter *adapter)
>>>>> +{
>>>>> +	adapter->phys_port_id = *((u32 *) adapter->netdev->dev_addr);
>>>>> +	adapter->phys_port_id ^= *((u32 *) adapter->netdev->dev_addr + 4);
>>>>> +	adapter->phys_port_id ^= (long) adapter;
>>>>> +	adapter->phys_port_id ^= (long) adapter->hw.hw_addr;
>>>>> +	adapter->phys_port_id ^= (long) adapter->hw.flash_address;
>>>>> +	adapter->phys_port_id ^= (u32) adapter->hw.io_base;
>>>>> +	adapter->phys_port_id ^= adapter->hw.device_id;
>>>>> +	adapter->phys_port_id ^= adapter->hw.subsystem_vendor_id;
>>>>> +	adapter->phys_port_id ^= adapter->hw.subsystem_device_id;
>>>>> +	adapter->phys_port_id ^= adapter->hw.vendor_id;
>>>>> +	adapter->phys_port_id ^= adapter->hw.revision_id;
>>> I didn't look at Jiri's patch initially, but... what's wrong with using
>>> the MAC address from NVRAM (should already be in netdev->perm_addr)?
>>> That's what I'm expecting to do for the SFC9000 family in sfc.
>>>
>>>>> +}
>>>>> +
>>>>>  /**
>>>>>   *  igb_probe - Device Initialization Routine
>>>>>   *  @pdev: PCI device information struct
>>>> I really think this bit here should be standardized and made available
>>>> to all drivers.
>>> [...]
>>>
>>> I think it's a bad example and should not be used in any drivers!
>>>
>>> Ben.
>>>
>> I agree.  That is why the second paragraph started listing what was
>> wrong with this implementation.  What I said was meant in the more
>> general sense that whatever solution should be used should be the same
>> across multiple drivers, not up to each driver to compute.
> I would love to know how you think this can be done generically.  If it
> can then we don't need the driver operation at all.
>
> Ben.
>

Well like you mentioned, you could just pull this out out of
netdev->perm_addr.  You don't need to have anything driver specific as
long as that is the field you are using generic netdev or pci_dev
attributes to do the identification.  All you need is something that
uniquely identifies the device in the system correct?  For that matter
it seems like you could probably just pull the domain, bus, device, and
function number out of the PCI device and that would probably work as
well as long as you cannot somehow have PFs running inside of guests.

Thanks,

Alex

Re: [patch net-next v4 4/4] igb/igbvf: implement ndo_get_phys_port_id

[Ben Hutchings]

On Thu, 2013-07-25 at 12:03 -0700, Alexander Duyck wrote:
> On 07/25/2013 11:51 AM, Ben Hutchings wrote:
> > On Thu, 2013-07-25 at 11:00 -0700, Alexander Duyck wrote:
> >> On 07/25/2013 09:44 AM, Ben Hutchings wrote:
> >>> On Thu, 2013-07-25 at 09:17 -0700, Alexander Duyck wrote:
> >>>> On 07/25/2013 06:03 AM, Jiri Pirko wrote:
> >>>>> @@ -1982,6 +2001,21 @@ static s32 igb_init_i2c(struct igb_adapter *adapter)
> >>>>>  	return status;
> >>>>>  }
> >>>>>  
> >>>>> +static void igb_compute_phys_port_id(struct igb_adapter *adapter)
> >>>>> +{
> >>>>> +	adapter->phys_port_id = *((u32 *) adapter->netdev->dev_addr);
> >>>>> +	adapter->phys_port_id ^= *((u32 *) adapter->netdev->dev_addr + 4);
> >>>>> +	adapter->phys_port_id ^= (long) adapter;
> >>>>> +	adapter->phys_port_id ^= (long) adapter->hw.hw_addr;
> >>>>> +	adapter->phys_port_id ^= (long) adapter->hw.flash_address;
> >>>>> +	adapter->phys_port_id ^= (u32) adapter->hw.io_base;
> >>>>> +	adapter->phys_port_id ^= adapter->hw.device_id;
> >>>>> +	adapter->phys_port_id ^= adapter->hw.subsystem_vendor_id;
> >>>>> +	adapter->phys_port_id ^= adapter->hw.subsystem_device_id;
> >>>>> +	adapter->phys_port_id ^= adapter->hw.vendor_id;
> >>>>> +	adapter->phys_port_id ^= adapter->hw.revision_id;
> >>> I didn't look at Jiri's patch initially, but... what's wrong with using
> >>> the MAC address from NVRAM (should already be in netdev->perm_addr)?
> >>> That's what I'm expecting to do for the SFC9000 family in sfc.
> >>>
> >>>>> +}
> >>>>> +
> >>>>>  /**
> >>>>>   *  igb_probe - Device Initialization Routine
> >>>>>   *  @pdev: PCI device information struct
> >>>> I really think this bit here should be standardized and made available
> >>>> to all drivers.
> >>> [...]
> >>>
> >>> I think it's a bad example and should not be used in any drivers!
> >>>
> >>> Ben.
> >>>
> >> I agree.  That is why the second paragraph started listing what was
> >> wrong with this implementation.  What I said was meant in the more
> >> general sense that whatever solution should be used should be the same
> >> across multiple drivers, not up to each driver to compute.
> > I would love to know how you think this can be done generically.  If it
> > can then we don't need the driver operation at all.
> >
> > Ben.
> >
> 
> Well like you mentioned, you could just pull this out out of
> netdev->perm_addr.  You don't need to have anything driver specific as
> long as that is the field you are using generic netdev or pci_dev
> attributes to do the identification.

For a PF driver that never shares the port with another PF, yes.  There
are a handful of those drivers so it might be worth sharing an
implementation that uses perm_addr, but it's so trivial...

> All you need is something that
> uniquely identifies the device in the system correct?

The spec is that it is universally unique.  I don't know who's going to
need that property but I think it's achievable since each physical port
normally has at least one globally unique MAC address assigned at
manufacturing time.

> For that matter
> it seems like you could probably just pull the domain, bus, device, and
> function number out of the PCI device and that would probably work as
> well as long as you cannot somehow have PFs running inside of guests.

Some NICs have multiple PFs for the same port, and those could be
assigned to guest VMs.

All VF drivers would need some way to get the port ID, and that can't be
done generically from inside a guest VM.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

RE: [patch net-next v4 4/4] igb/igbvf: implement ndo_get_phys_port_id

[Rose, Gregory V]

> -----Original Message-----
> From: Duyck, Alexander H
> Sent: Thursday, July 25, 2013 12:04 PM
> To: Ben Hutchings
> Cc: Jiri Pirko; netdev@vger.kernel.org; davem@davemloft.net;
> stephen@networkplumber.org; Narendra_K@Dell.com; Fastabend, John R;
> or.gerlitz@gmail.com; Kirsher, Jeffrey T; Brandeburg, Jesse; Allan, Bruce
> W; Wyborny, Carolyn; Skidmore, Donald C; Rose, Gregory V; Waskiewicz Jr,
> Peter P; Ronciak, John; Dave, Tushar N; Vick, Matthew; Williams, Mitch A;
> vyasevic@redhat.com; amwang@redhat.com; johannes@sipsolutions.net
> Subject: Re: [patch net-next v4 4/4] igb/igbvf: implement
> ndo_get_phys_port_id
> 
> On 07/25/2013 11:51 AM, Ben Hutchings wrote:
> > On Thu, 2013-07-25 at 11:00 -0700, Alexander Duyck wrote:
> >> On 07/25/2013 09:44 AM, Ben Hutchings wrote:
> >>> On Thu, 2013-07-25 at 09:17 -0700, Alexander Duyck wrote:
> >>>> On 07/25/2013 06:03 AM, Jiri Pirko wrote:
> >>>>> @@ -1982,6 +2001,21 @@ static s32 igb_init_i2c(struct igb_adapter
> *adapter)
> >>>>>  	return status;
> >>>>>  }
> >>>>>
> >>>>> +static void igb_compute_phys_port_id(struct igb_adapter *adapter)
> >>>>> +{
> >>>>> +	adapter->phys_port_id = *((u32 *) adapter->netdev->dev_addr);
> >>>>> +	adapter->phys_port_id ^= *((u32 *) adapter->netdev->dev_addr +
> 4);
> >>>>> +	adapter->phys_port_id ^= (long) adapter;
> >>>>> +	adapter->phys_port_id ^= (long) adapter->hw.hw_addr;
> >>>>> +	adapter->phys_port_id ^= (long) adapter->hw.flash_address;
> >>>>> +	adapter->phys_port_id ^= (u32) adapter->hw.io_base;
> >>>>> +	adapter->phys_port_id ^= adapter->hw.device_id;
> >>>>> +	adapter->phys_port_id ^= adapter->hw.subsystem_vendor_id;
> >>>>> +	adapter->phys_port_id ^= adapter->hw.subsystem_device_id;
> >>>>> +	adapter->phys_port_id ^= adapter->hw.vendor_id;
> >>>>> +	adapter->phys_port_id ^= adapter->hw.revision_id;
> >>> I didn't look at Jiri's patch initially, but... what's wrong with
> >>> using the MAC address from NVRAM (should already be in netdev-
> >perm_addr)?
> >>> That's what I'm expecting to do for the SFC9000 family in sfc.
> >>>
> >>>>> +}
> >>>>> +
> >>>>>  /**
> >>>>>   *  igb_probe - Device Initialization Routine
> >>>>>   *  @pdev: PCI device information struct
> >>>> I really think this bit here should be standardized and made
> >>>> available to all drivers.
> >>> [...]
> >>>
> >>> I think it's a bad example and should not be used in any drivers!
> >>>
> >>> Ben.
> >>>
> >> I agree.  That is why the second paragraph started listing what was
> >> wrong with this implementation.  What I said was meant in the more
> >> general sense that whatever solution should be used should be the
> >> same across multiple drivers, not up to each driver to compute.
> > I would love to know how you think this can be done generically.  If
> > it can then we don't need the driver operation at all.
> >
> > Ben.
> >
> 
> Well like you mentioned, you could just pull this out out of
> netdev->perm_addr.  You don't need to have anything driver specific as
> long as that is the field you are using generic netdev or pci_dev
> attributes to do the identification.  All you need is something that
> uniquely identifies the device in the system correct?  For that matter it
> seems like you could probably just pull the domain, bus, device, and
> function number out of the PCI device and that would probably work as well
> as long as you cannot somehow have PFs running inside of guests.

NACK any direct information to the VF guest device as to what its domain, bus, device and function numbers are.  This opens up various exploits.  Virtual functions are not guaranteed to always run in trusted guest VMs, they shouldn't be given anymore HW information than needed to function.

There's a reason we asked Jiri to use a hash value.

- Greg

> 
> Thanks,
> 
> Alex

Re: [patch net-next v4 4/4] igb/igbvf: implement ndo_get_phys_port_id

[John Fastabend]

On 07/25/2013 12:15 PM, Ben Hutchings wrote:
> On Thu, 2013-07-25 at 12:03 -0700, Alexander Duyck wrote:
>> On 07/25/2013 11:51 AM, Ben Hutchings wrote:
>>> On Thu, 2013-07-25 at 11:00 -0700, Alexander Duyck wrote:
>>>> On 07/25/2013 09:44 AM, Ben Hutchings wrote:
>>>>> On Thu, 2013-07-25 at 09:17 -0700, Alexander Duyck wrote:
>>>>>> On 07/25/2013 06:03 AM, Jiri Pirko wrote:
>>>>>>> @@ -1982,6 +2001,21 @@ static s32 igb_init_i2c(struct igb_adapter *adapter)
>>>>>>>   	return status;
>>>>>>>   }
>>>>>>>
>>>>>>> +static void igb_compute_phys_port_id(struct igb_adapter *adapter)
>>>>>>> +{
>>>>>>> +	adapter->phys_port_id = *((u32 *) adapter->netdev->dev_addr);
>>>>>>> +	adapter->phys_port_id ^= *((u32 *) adapter->netdev->dev_addr + 4);
>>>>>>> +	adapter->phys_port_id ^= (long) adapter;
>>>>>>> +	adapter->phys_port_id ^= (long) adapter->hw.hw_addr;
>>>>>>> +	adapter->phys_port_id ^= (long) adapter->hw.flash_address;
>>>>>>> +	adapter->phys_port_id ^= (u32) adapter->hw.io_base;
>>>>>>> +	adapter->phys_port_id ^= adapter->hw.device_id;
>>>>>>> +	adapter->phys_port_id ^= adapter->hw.subsystem_vendor_id;
>>>>>>> +	adapter->phys_port_id ^= adapter->hw.subsystem_device_id;
>>>>>>> +	adapter->phys_port_id ^= adapter->hw.vendor_id;
>>>>>>> +	adapter->phys_port_id ^= adapter->hw.revision_id;
>>>>> I didn't look at Jiri's patch initially, but... what's wrong with using
>>>>> the MAC address from NVRAM (should already be in netdev->perm_addr)?
>>>>> That's what I'm expecting to do for the SFC9000 family in sfc.
>>>>>
>>>>>>> +}
>>>>>>> +
>>>>>>>   /**
>>>>>>>    *  igb_probe - Device Initialization Routine
>>>>>>>    *  @pdev: PCI device information struct
>>>>>> I really think this bit here should be standardized and made available
>>>>>> to all drivers.
>>>>> [...]
>>>>>
>>>>> I think it's a bad example and should not be used in any drivers!
>>>>>
>>>>> Ben.
>>>>>
>>>> I agree.  That is why the second paragraph started listing what was
>>>> wrong with this implementation.  What I said was meant in the more
>>>> general sense that whatever solution should be used should be the same
>>>> across multiple drivers, not up to each driver to compute.
>>> I would love to know how you think this can be done generically.  If it
>>> can then we don't need the driver operation at all.
>>>
>>> Ben.
>>>
>>
>> Well like you mentioned, you could just pull this out out of
>> netdev->perm_addr.  You don't need to have anything driver specific as
>> long as that is the field you are using generic netdev or pci_dev
>> attributes to do the identification.
>
> For a PF driver that never shares the port with another PF, yes.  There
> are a handful of those drivers so it might be worth sharing an
> implementation that uses perm_addr, but it's so trivial...
>
>> All you need is something that
>> uniquely identifies the device in the system correct?
>
> The spec is that it is universally unique.  I don't know who's going to
> need that property but I think it's achievable since each physical port
> normally has at least one globally unique MAC address assigned at
> manufacturing time.
>
>> For that matter
>> it seems like you could probably just pull the domain, bus, device, and
>> function number out of the PCI device and that would probably work as
>> well as long as you cannot somehow have PFs running inside of guests.
>
> Some NICs have multiple PFs for the same port, and those could be
> assigned to guest VMs.
>
> All VF drivers would need some way to get the port ID, and that can't be
> done generically from inside a guest VM.
>
> Ben.
>

I doubt you'll be able to think up a way to do this generically as Ben
points out. But also no reason for the complicated hash just use the
perm address of the PF and if you have multiple PFs elect one of the
n perm address to be the stand-in for the unique one.

.John

-- 
John Fastabend         Intel Corporation

Re: [patch net-next v4 4/4] igb/igbvf: implement ndo_get_phys_port_id

[Alexander Duyck]

On 07/25/2013 12:58 PM, John Fastabend wrote:
> On 07/25/2013 12:15 PM, Ben Hutchings wrote:
>> On Thu, 2013-07-25 at 12:03 -0700, Alexander Duyck wrote:
>>> On 07/25/2013 11:51 AM, Ben Hutchings wrote:
>>>> On Thu, 2013-07-25 at 11:00 -0700, Alexander Duyck wrote:
>>>>> On 07/25/2013 09:44 AM, Ben Hutchings wrote:
>>>>>> On Thu, 2013-07-25 at 09:17 -0700, Alexander Duyck wrote:
>>>>>>> On 07/25/2013 06:03 AM, Jiri Pirko wrote:
>>>>>>>> @@ -1982,6 +2001,21 @@ static s32 igb_init_i2c(struct
>>>>>>>> igb_adapter *adapter)
>>>>>>>>       return status;
>>>>>>>>   }
>>>>>>>>
>>>>>>>> +static void igb_compute_phys_port_id(struct igb_adapter *adapter)
>>>>>>>> +{
>>>>>>>> +    adapter->phys_port_id = *((u32 *) adapter->netdev->dev_addr);
>>>>>>>> +    adapter->phys_port_id ^= *((u32 *)
>>>>>>>> adapter->netdev->dev_addr + 4);
>>>>>>>> +    adapter->phys_port_id ^= (long) adapter;
>>>>>>>> +    adapter->phys_port_id ^= (long) adapter->hw.hw_addr;
>>>>>>>> +    adapter->phys_port_id ^= (long) adapter->hw.flash_address;
>>>>>>>> +    adapter->phys_port_id ^= (u32) adapter->hw.io_base;
>>>>>>>> +    adapter->phys_port_id ^= adapter->hw.device_id;
>>>>>>>> +    adapter->phys_port_id ^= adapter->hw.subsystem_vendor_id;
>>>>>>>> +    adapter->phys_port_id ^= adapter->hw.subsystem_device_id;
>>>>>>>> +    adapter->phys_port_id ^= adapter->hw.vendor_id;
>>>>>>>> +    adapter->phys_port_id ^= adapter->hw.revision_id;
>>>>>> I didn't look at Jiri's patch initially, but... what's wrong with
>>>>>> using
>>>>>> the MAC address from NVRAM (should already be in netdev->perm_addr)?
>>>>>> That's what I'm expecting to do for the SFC9000 family in sfc.
>>>>>>
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>   /**
>>>>>>>>    *  igb_probe - Device Initialization Routine
>>>>>>>>    *  @pdev: PCI device information struct
>>>>>>> I really think this bit here should be standardized and made
>>>>>>> available
>>>>>>> to all drivers.
>>>>>> [...]
>>>>>>
>>>>>> I think it's a bad example and should not be used in any drivers!
>>>>>>
>>>>>> Ben.
>>>>>>
>>>>> I agree.  That is why the second paragraph started listing what was
>>>>> wrong with this implementation.  What I said was meant in the more
>>>>> general sense that whatever solution should be used should be the
>>>>> same
>>>>> across multiple drivers, not up to each driver to compute.
>>>> I would love to know how you think this can be done generically. 
>>>> If it
>>>> can then we don't need the driver operation at all.
>>>>
>>>> Ben.
>>>>
>>>
>>> Well like you mentioned, you could just pull this out out of
>>> netdev->perm_addr.  You don't need to have anything driver specific as
>>> long as that is the field you are using generic netdev or pci_dev
>>> attributes to do the identification.
>>
>> For a PF driver that never shares the port with another PF, yes.  There
>> are a handful of those drivers so it might be worth sharing an
>> implementation that uses perm_addr, but it's so trivial...
>>
>>> All you need is something that
>>> uniquely identifies the device in the system correct?
>>
>> The spec is that it is universally unique.  I don't know who's going to
>> need that property but I think it's achievable since each physical port
>> normally has at least one globally unique MAC address assigned at
>> manufacturing time.
>>
>>> For that matter
>>> it seems like you could probably just pull the domain, bus, device, and
>>> function number out of the PCI device and that would probably work as
>>> well as long as you cannot somehow have PFs running inside of guests.
>>
>> Some NICs have multiple PFs for the same port, and those could be
>> assigned to guest VMs.
>>
>> All VF drivers would need some way to get the port ID, and that can't be
>> done generically from inside a guest VM.
>>
>> Ben.
>>
>
> I doubt you'll be able to think up a way to do this generically as Ben
> points out. But also no reason for the complicated hash just use the
> perm address of the PF and if you have multiple PFs elect one of the
> n perm address to be the stand-in for the unique one.
>
> .John
>

I think there may have been some miscommunication.  I wasn't talking
about the VF drivers having a generic means of getting the ID.  I would
just want all of the drivers to be using a similar approach for
generating the port ID so that we reduce the risk of any kind of port ID
collision.  We need to make sure we are all stuffing the 6 bytes of
perm_addr into the 4 byte port ID using the same approach.

Thanks,

Alex

Re: [patch net-next v4 4/4] igb/igbvf: implement ndo_get_phys_port_id

[Ben Hutchings]

On Thu, 2013-07-25 at 15:27 -0700, Alexander Duyck wrote:
> On 07/25/2013 12:58 PM, John Fastabend wrote:
> > On 07/25/2013 12:15 PM, Ben Hutchings wrote:
> >> On Thu, 2013-07-25 at 12:03 -0700, Alexander Duyck wrote:
[...]
> >>> Well like you mentioned, you could just pull this out out of
> >>> netdev->perm_addr.  You don't need to have anything driver specific as
> >>> long as that is the field you are using generic netdev or pci_dev
> >>> attributes to do the identification.
> >>
> >> For a PF driver that never shares the port with another PF, yes.  There
> >> are a handful of those drivers so it might be worth sharing an
> >> implementation that uses perm_addr, but it's so trivial...
> >>
> >>> All you need is something that
> >>> uniquely identifies the device in the system correct?
> >>
> >> The spec is that it is universally unique.  I don't know who's going to
> >> need that property but I think it's achievable since each physical port
> >> normally has at least one globally unique MAC address assigned at
> >> manufacturing time.
> >>
> >>> For that matter
> >>> it seems like you could probably just pull the domain, bus, device, and
> >>> function number out of the PCI device and that would probably work as
> >>> well as long as you cannot somehow have PFs running inside of guests.
> >>
> >> Some NICs have multiple PFs for the same port, and those could be
> >> assigned to guest VMs.
> >>
> >> All VF drivers would need some way to get the port ID, and that can't be
> >> done generically from inside a guest VM.
> >>
> >> Ben.
> >>
> >
> > I doubt you'll be able to think up a way to do this generically as Ben
> > points out. But also no reason for the complicated hash just use the
> > perm address of the PF and if you have multiple PFs elect one of the
> > n perm address to be the stand-in for the unique one.
> >
> > .John
> >
> 
> I think there may have been some miscommunication.  I wasn't talking
> about the VF drivers having a generic means of getting the ID.  I would
> just want all of the drivers to be using a similar approach for
> generating the port ID so that we reduce the risk of any kind of port ID
> collision.  We need to make sure we are all stuffing the 6 bytes of
> perm_addr into the 4 byte port ID using the same approach.

But we started with an up-to-32-byte port ID; why do you say 4 bytes?

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [patch net-next v4 4/4] igb/igbvf: implement ndo_get_phys_port_id

[Alexander Duyck]

On 07/25/2013 03:44 PM, Ben Hutchings wrote:
> On Thu, 2013-07-25 at 15:27 -0700, Alexander Duyck wrote:
>> On 07/25/2013 12:58 PM, John Fastabend wrote:
>>> On 07/25/2013 12:15 PM, Ben Hutchings wrote:
>>>> On Thu, 2013-07-25 at 12:03 -0700, Alexander Duyck wrote:
> [...]
>>>>> Well like you mentioned, you could just pull this out out of
>>>>> netdev->perm_addr.  You don't need to have anything driver specific as
>>>>> long as that is the field you are using generic netdev or pci_dev
>>>>> attributes to do the identification.
>>>> For a PF driver that never shares the port with another PF, yes.  There
>>>> are a handful of those drivers so it might be worth sharing an
>>>> implementation that uses perm_addr, but it's so trivial...
>>>>
>>>>> All you need is something that
>>>>> uniquely identifies the device in the system correct?
>>>> The spec is that it is universally unique.  I don't know who's going to
>>>> need that property but I think it's achievable since each physical port
>>>> normally has at least one globally unique MAC address assigned at
>>>> manufacturing time.
>>>>
>>>>> For that matter
>>>>> it seems like you could probably just pull the domain, bus, device, and
>>>>> function number out of the PCI device and that would probably work as
>>>>> well as long as you cannot somehow have PFs running inside of guests.
>>>> Some NICs have multiple PFs for the same port, and those could be
>>>> assigned to guest VMs.
>>>>
>>>> All VF drivers would need some way to get the port ID, and that can't be
>>>> done generically from inside a guest VM.
>>>>
>>>> Ben.
>>>>
>>> I doubt you'll be able to think up a way to do this generically as Ben
>>> points out. But also no reason for the complicated hash just use the
>>> perm address of the PF and if you have multiple PFs elect one of the
>>> n perm address to be the stand-in for the unique one.
>>>
>>> .John
>>>
>> I think there may have been some miscommunication.  I wasn't talking
>> about the VF drivers having a generic means of getting the ID.  I would
>> just want all of the drivers to be using a similar approach for
>> generating the port ID so that we reduce the risk of any kind of port ID
>> collision.  We need to make sure we are all stuffing the 6 bytes of
>> perm_addr into the 4 byte port ID using the same approach.
> But we started with an up-to-32-byte port ID; why do you say 4 bytes?
>
> Ben.

Sorry I was looking at the driver specific implementation that was only
populating a u32.

Thanks,

Alex