[PATCH v2 net-next 0/2] ionic: add sriov support

[PATCH v2 net-next 0/2] ionic: add sriov support

[Shannon Nelson]

Set up the basic support for enabling SR-IOV devices in the
ionic driver.  Since most of the management work happens in
the NIC firmware, the driver becomes mostly a pass-through
for the network stack commands that want to control and
configure the VFs.

v2:	use pci_num_vf() and kcalloc()
	add locking for the VF operations
	disable VFs in ionic_remove() if they are still running

Shannon Nelson (2):
  ionic: ionic_if bits for sr-iov support
  ionic: support sr-iov operations

 drivers/net/ethernet/pensando/ionic/ionic.h   |  15 +-
 .../ethernet/pensando/ionic/ionic_bus_pci.c   |  85 ++++++
 .../net/ethernet/pensando/ionic/ionic_dev.c   |  58 ++++
 .../net/ethernet/pensando/ionic/ionic_dev.h   |   7 +
 .../net/ethernet/pensando/ionic/ionic_if.h    |  97 +++++++
 .../net/ethernet/pensando/ionic/ionic_lif.c   | 254 +++++++++++++++++-
 .../net/ethernet/pensando/ionic/ionic_lif.h   |   7 +
 .../net/ethernet/pensando/ionic/ionic_main.c  |   4 +
 8 files changed, 519 insertions(+), 8 deletions(-)

-- 
2.17.1

[PATCH v2 net-next 1/2] ionic: ionic_if bits for sr-iov support

[Shannon Nelson]

Adds new AdminQ calls and their related structs for
supporting PF controls on VFs:
    CMD_OPCODE_VF_GETATTR
    CMD_OPCODE_VF_SETATTR

Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 .../net/ethernet/pensando/ionic/ionic_if.h    | 97 +++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_if.h b/drivers/net/ethernet/pensando/ionic/ionic_if.h
index dbdb7c5ae8f1..ea5c379824c1 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_if.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_if.h
@@ -51,6 +51,10 @@ enum ionic_cmd_opcode {
 	IONIC_CMD_RDMA_CREATE_CQ		= 52,
 	IONIC_CMD_RDMA_CREATE_ADMINQ		= 53,
 
+	/* SR/IOV commands */
+	IONIC_CMD_VF_GETATTR			= 60,
+	IONIC_CMD_VF_SETATTR			= 61,
+
 	/* QoS commands */
 	IONIC_CMD_QOS_CLASS_IDENTIFY		= 240,
 	IONIC_CMD_QOS_CLASS_INIT		= 241,
@@ -1639,6 +1643,93 @@ enum ionic_qos_sched_type {
 	IONIC_QOS_SCHED_TYPE_DWRR	= 1,	/* Deficit weighted round-robin */
 };
 
+enum ionic_vf_attr {
+	IONIC_VF_ATTR_SPOOFCHK	= 1,
+	IONIC_VF_ATTR_TRUST	= 2,
+	IONIC_VF_ATTR_MAC	= 3,
+	IONIC_VF_ATTR_LINKSTATE	= 4,
+	IONIC_VF_ATTR_VLAN	= 5,
+	IONIC_VF_ATTR_RATE	= 6,
+	IONIC_VF_ATTR_STATSADDR	= 7,
+};
+
+/**
+ * VF link status
+ */
+enum ionic_vf_link_status {
+	IONIC_VF_LINK_STATUS_AUTO = 0,	/* link state of the uplink */
+	IONIC_VF_LINK_STATUS_UP   = 1,	/* link is always up */
+	IONIC_VF_LINK_STATUS_DOWN = 2,	/* link is always down */
+};
+
+/**
+ * struct ionic_vf_setattr_cmd - Set VF attributes on the NIC
+ * @opcode:     Opcode
+ * @index:      VF index
+ * @attr:       Attribute type (enum ionic_vf_attr)
+ *	macaddr		mac address
+ *	vlanid		vlan ID
+ *	maxrate		max Tx rate in Mbps
+ *	spoofchk	enable address spoof checking
+ *	trust		enable VF trust
+ *	linkstate	set link up or down
+ *	stats_pa	set DMA address for VF stats
+ */
+struct ionic_vf_setattr_cmd {
+	u8     opcode;
+	u8     attr;
+	__le16 vf_index;
+	union {
+		u8     macaddr[6];
+		__le16 vlanid;
+		__le32 maxrate;
+		u8     spoofchk;
+		u8     trust;
+		u8     linkstate;
+		__le64 stats_pa;
+		u8     pad[60];
+	};
+};
+
+struct ionic_vf_setattr_comp {
+	u8     status;
+	u8     attr;
+	__le16 vf_index;
+	__le16 comp_index;
+	u8     rsvd[9];
+	u8     color;
+};
+
+/**
+ * struct ionic_vf_getattr_cmd - Get VF attributes from the NIC
+ * @opcode:     Opcode
+ * @index:      VF index
+ * @attr:       Attribute type (enum ionic_vf_attr)
+ */
+struct ionic_vf_getattr_cmd {
+	u8     opcode;
+	u8     attr;
+	__le16 vf_index;
+	u8     rsvd[60];
+};
+
+struct ionic_vf_getattr_comp {
+	u8     status;
+	u8     attr;
+	__le16 vf_index;
+	union {
+		u8     macaddr[6];
+		__le16 vlanid;
+		__le32 maxrate;
+		u8     spoofchk;
+		u8     trust;
+		u8     linkstate;
+		__le64 stats_pa;
+		u8     pad[11];
+	};
+	u8     color;
+};
+
 /**
  * union ionic_qos_config - Qos configuration structure
  * @flags:		Configuration flags
@@ -2289,6 +2380,9 @@ union ionic_dev_cmd {
 	struct ionic_port_getattr_cmd port_getattr;
 	struct ionic_port_setattr_cmd port_setattr;
 
+	struct ionic_vf_setattr_cmd vf_setattr;
+	struct ionic_vf_getattr_cmd vf_getattr;
+
 	struct ionic_lif_identify_cmd lif_identify;
 	struct ionic_lif_init_cmd lif_init;
 	struct ionic_lif_reset_cmd lif_reset;
@@ -2318,6 +2412,9 @@ union ionic_dev_cmd_comp {
 	struct ionic_port_getattr_comp port_getattr;
 	struct ionic_port_setattr_comp port_setattr;
 
+	struct ionic_vf_setattr_comp vf_setattr;
+	struct ionic_vf_getattr_comp vf_getattr;
+
 	struct ionic_lif_identify_comp lif_identify;
 	struct ionic_lif_init_comp lif_init;
 	ionic_lif_reset_comp lif_reset;
-- 
2.17.1

[PATCH v2 net-next 2/2] ionic: support sr-iov operations

[Shannon Nelson]

Add the netdev ops for managing VFs.  Since most of the
management work happens in the NIC firmware, the driver becomes
mostly a pass-through for the network stack commands that want
to control and configure the VFs.

We also tweak ionic_station_set() a little to allow for
the VFs that start off with a zero'd mac address.

Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 drivers/net/ethernet/pensando/ionic/ionic.h   |  15 +-
 .../ethernet/pensando/ionic/ionic_bus_pci.c   |  85 ++++++
 .../net/ethernet/pensando/ionic/ionic_dev.c   |  58 ++++
 .../net/ethernet/pensando/ionic/ionic_dev.h   |   7 +
 .../net/ethernet/pensando/ionic/ionic_lif.c   | 254 +++++++++++++++++-
 .../net/ethernet/pensando/ionic/ionic_lif.h   |   7 +
 .../net/ethernet/pensando/ionic/ionic_main.c  |   4 +
 7 files changed, 422 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h
index 98e102af7756..e5c9e4b0450b 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic.h
@@ -12,7 +12,7 @@ struct ionic_lif;
 
 #define IONIC_DRV_NAME		"ionic"
 #define IONIC_DRV_DESCRIPTION	"Pensando Ethernet NIC Driver"
-#define IONIC_DRV_VERSION	"0.18.0-k"
+#define IONIC_DRV_VERSION	"0.20.0-k"
 
 #define PCI_VENDOR_ID_PENSANDO			0x1dd8
 
@@ -25,6 +25,18 @@ struct ionic_lif;
 
 #define DEVCMD_TIMEOUT  10
 
+struct ionic_vf {
+	u16	 index;
+	u8	 macaddr[6];
+	__le32	 maxrate;
+	__le16	 vlanid;
+	u8	 spoofchk;
+	u8	 trusted;
+	u8	 linkstate;
+	dma_addr_t       stats_pa;
+	struct ionic_lif_stats stats;
+};
+
 struct ionic {
 	struct pci_dev *pdev;
 	struct device *dev;
@@ -46,6 +58,7 @@ struct ionic {
 	DECLARE_BITMAP(intrs, IONIC_INTR_CTRL_REGS_MAX);
 	struct work_struct nb_work;
 	struct notifier_block nb;
+	struct ionic_vf **vf;
 	struct timer_list watchdog_timer;
 	int watchdog_period;
 };
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
index 9a9ab8cb2cb3..057eb453dd11 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
@@ -250,6 +250,87 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	return err;
 }
 
+static int ionic_sriov_configure(struct pci_dev *pdev, int num_vfs)
+{
+	struct ionic *ionic = pci_get_drvdata(pdev);
+	struct device *dev = ionic->dev;
+	int i, ret = 0;
+	int nvfs = 0;
+
+	if (test_and_set_bit(IONIC_LIF_VF_OP, ionic->master_lif->state)) {
+		dev_warn(&pdev->dev, "Unable to configure VFs, other operation is pending.\n");
+		return -EAGAIN;
+	}
+
+	if (num_vfs > 0) {
+		ret = pci_enable_sriov(pdev, num_vfs);
+		if (ret) {
+			dev_err(&pdev->dev, "Cannot enable SRIOV: %d\n", ret);
+			goto out;
+		}
+
+		ionic->vf = kcalloc(num_vfs, sizeof(struct ionic_vf *),
+				    GFP_KERNEL);
+		if (!ionic->vf) {
+			pci_disable_sriov(pdev);
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		for (i = 0; i < num_vfs; i++) {
+			struct ionic_vf *v;
+
+			v = kzalloc(sizeof(*v), GFP_KERNEL);
+			if (!v) {
+				ret = -ENOMEM;
+				num_vfs = 0;
+				goto remove_vfs;
+			}
+
+			v->stats_pa = dma_map_single(dev, &v->stats,
+						     sizeof(v->stats),
+						     DMA_FROM_DEVICE);
+			if (dma_mapping_error(dev, v->stats_pa)) {
+				ret = -ENODEV;
+				kfree(v);
+				ionic->vf[i] = NULL;
+				num_vfs = 0;
+				goto remove_vfs;
+			}
+
+			ionic->vf[i] = v;
+			nvfs++;
+		}
+
+		ret = num_vfs;
+		goto out;
+	}
+
+remove_vfs:
+	if (num_vfs == 0) {
+		if (ret)
+			dev_err(&pdev->dev, "SRIOV setup failed: %d\n", ret);
+
+		pci_disable_sriov(pdev);
+
+		if (!nvfs)
+			nvfs = pci_num_vf(pdev);
+		for (i = 0; i < nvfs; i++) {
+			dma_unmap_single(dev, ionic->vf[i]->stats_pa,
+					 sizeof(struct ionic_lif_stats),
+					 DMA_FROM_DEVICE);
+			kfree(ionic->vf[i]);
+		}
+
+		kfree(ionic->vf);
+		ionic->vf = NULL;
+	}
+
+out:
+	clear_bit(IONIC_LIF_VF_OP, ionic->master_lif->state);
+	return ret;
+}
+
 static void ionic_remove(struct pci_dev *pdev)
 {
 	struct ionic *ionic = pci_get_drvdata(pdev);
@@ -257,6 +338,9 @@ static void ionic_remove(struct pci_dev *pdev)
 	if (!ionic)
 		return;
 
+	if (pci_num_vf(pdev))
+		ionic_sriov_configure(pdev, 0);
+
 	ionic_devlink_unregister(ionic);
 	ionic_lifs_unregister(ionic);
 	ionic_lifs_deinit(ionic);
@@ -279,6 +363,7 @@ static struct pci_driver ionic_driver = {
 	.id_table = ionic_id_table,
 	.probe = ionic_probe,
 	.remove = ionic_remove,
+	.sriov_configure = ionic_sriov_configure,
 };
 
 int ionic_bus_register_driver(void)
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.c b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
index 5f9d2ec70446..87f82f36812f 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
@@ -286,6 +286,64 @@ void ionic_dev_cmd_port_pause(struct ionic_dev *idev, u8 pause_type)
 	ionic_dev_cmd_go(idev, &cmd);
 }
 
+/* VF commands */
+int ionic_set_vf_config(struct ionic *ionic, int vf, u8 attr, u8 *data)
+{
+	union ionic_dev_cmd cmd = {
+		.vf_setattr.opcode = IONIC_CMD_VF_SETATTR,
+		.vf_setattr.attr = attr,
+		.vf_setattr.vf_index = vf,
+	};
+	int err;
+
+	switch (attr) {
+	case IONIC_VF_ATTR_SPOOFCHK:
+		cmd.vf_setattr.spoofchk = *data;
+		dev_dbg(ionic->dev, "%s: vf %d spoof %d\n",
+			__func__, vf, *data);
+		break;
+	case IONIC_VF_ATTR_TRUST:
+		cmd.vf_setattr.trust = *data;
+		dev_dbg(ionic->dev, "%s: vf %d trust %d\n",
+			__func__, vf, *data);
+		break;
+	case IONIC_VF_ATTR_LINKSTATE:
+		cmd.vf_setattr.linkstate = *data;
+		dev_dbg(ionic->dev, "%s: vf %d linkstate %d\n",
+			__func__, vf, *data);
+		break;
+	case IONIC_VF_ATTR_MAC:
+		ether_addr_copy(cmd.vf_setattr.macaddr, data);
+		dev_dbg(ionic->dev, "%s: vf %d macaddr %pM\n",
+			__func__, vf, data);
+		break;
+	case IONIC_VF_ATTR_VLAN:
+		cmd.vf_setattr.vlanid = cpu_to_le16(*(u16 *)data);
+		dev_dbg(ionic->dev, "%s: vf %d vlan %d\n",
+			__func__, vf, *(u16 *)data);
+		break;
+	case IONIC_VF_ATTR_RATE:
+		cmd.vf_setattr.maxrate = cpu_to_le32(*(u32 *)data);
+		dev_dbg(ionic->dev, "%s: vf %d maxrate %d\n",
+			__func__, vf, *(u32 *)data);
+		break;
+	case IONIC_VF_ATTR_STATSADDR:
+		cmd.vf_setattr.stats_pa = cpu_to_le64(*(u64 *)data);
+		dev_dbg(ionic->dev, "%s: vf %d stats_pa 0x%08llx\n",
+			__func__, vf, *(u64 *)data);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	mutex_lock(&ionic->dev_cmd_lock);
+	ionic_dev_cmd_go(&ionic->idev, &cmd);
+	err = ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT);
+	mutex_unlock(&ionic->dev_cmd_lock);
+
+	return err;
+}
+
 /* LIF commands */
 void ionic_dev_cmd_lif_identify(struct ionic_dev *idev, u8 type, u8 ver)
 {
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.h b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
index 4665c5dc5324..7838e342c4fd 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_dev.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
@@ -113,6 +113,12 @@ static_assert(sizeof(struct ionic_rxq_desc) == 16);
 static_assert(sizeof(struct ionic_rxq_sg_desc) == 128);
 static_assert(sizeof(struct ionic_rxq_comp) == 16);
 
+/* SR/IOV */
+static_assert(sizeof(struct ionic_vf_setattr_cmd) == 64);
+static_assert(sizeof(struct ionic_vf_setattr_comp) == 16);
+static_assert(sizeof(struct ionic_vf_getattr_cmd) == 64);
+static_assert(sizeof(struct ionic_vf_getattr_comp) == 16);
+
 struct ionic_devinfo {
 	u8 asic_type;
 	u8 asic_rev;
@@ -275,6 +281,7 @@ void ionic_dev_cmd_port_autoneg(struct ionic_dev *idev, u8 an_enable);
 void ionic_dev_cmd_port_fec(struct ionic_dev *idev, u8 fec_type);
 void ionic_dev_cmd_port_pause(struct ionic_dev *idev, u8 pause_type);
 
+int ionic_set_vf_config(struct ionic *ionic, int vf, u8 attr, u8 *data);
 void ionic_dev_cmd_lif_identify(struct ionic_dev *idev, u8 type, u8 ver);
 void ionic_dev_cmd_lif_init(struct ionic_dev *idev, u16 lif_index,
 			    dma_addr_t addr);
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index 60fd14df49d7..4d75d6c40c43 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -1616,6 +1616,234 @@ int ionic_stop(struct net_device *netdev)
 	return err;
 }
 
+static int ionic_get_vf_config(struct net_device *netdev,
+			       int vf, struct ifla_vf_info *ivf)
+{
+	struct ionic_lif *lif = netdev_priv(netdev);
+	struct ionic *ionic = lif->ionic;
+
+	if (vf >= pci_num_vf(lif->ionic->pdev))
+		return -EINVAL;
+
+	if (test_and_set_bit(IONIC_LIF_VF_OP, lif->state)) {
+		netdev_warn(netdev, "Unable to configure VFs, other operation is pending.\n");
+		return -EAGAIN;
+	}
+
+	ivf->vf           = vf;
+	ivf->vlan         = ionic->vf[vf]->vlanid;
+	ivf->qos	  = 0;
+	ivf->spoofchk     = ionic->vf[vf]->spoofchk;
+	ivf->linkstate    = ionic->vf[vf]->linkstate;
+	ivf->max_tx_rate  = ionic->vf[vf]->maxrate;
+	ivf->trusted      = ionic->vf[vf]->trusted;
+	ether_addr_copy(ivf->mac, ionic->vf[vf]->macaddr);
+
+	clear_bit(IONIC_LIF_VF_OP, lif->state);
+	return 0;
+}
+
+static int ionic_get_vf_stats(struct net_device *netdev, int vf,
+			      struct ifla_vf_stats *vf_stats)
+{
+	struct ionic_lif *lif = netdev_priv(netdev);
+	struct ionic *ionic = lif->ionic;
+	struct ionic_lif_stats *vs;
+
+	if (vf >= pci_num_vf(lif->ionic->pdev))
+		return -EINVAL;
+
+	if (test_and_set_bit(IONIC_LIF_VF_OP, lif->state)) {
+		netdev_warn(netdev, "Unable to configure VFs, other operation is pending.\n");
+		return -EAGAIN;
+	}
+
+	memset(vf_stats, 0, sizeof(*vf_stats));
+	vs = &ionic->vf[vf]->stats;
+
+	vf_stats->rx_packets = le64_to_cpu(vs->rx_ucast_packets);
+	vf_stats->tx_packets = le64_to_cpu(vs->tx_ucast_packets);
+	vf_stats->rx_bytes   = le64_to_cpu(vs->rx_ucast_bytes);
+	vf_stats->tx_bytes   = le64_to_cpu(vs->tx_ucast_bytes);
+	vf_stats->broadcast  = le64_to_cpu(vs->rx_bcast_packets);
+	vf_stats->multicast  = le64_to_cpu(vs->rx_mcast_packets);
+	vf_stats->rx_dropped = le64_to_cpu(vs->rx_ucast_drop_packets) +
+			       le64_to_cpu(vs->rx_mcast_drop_packets) +
+			       le64_to_cpu(vs->rx_bcast_drop_packets);
+	vf_stats->tx_dropped = le64_to_cpu(vs->tx_ucast_drop_packets) +
+			       le64_to_cpu(vs->tx_mcast_drop_packets) +
+			       le64_to_cpu(vs->tx_bcast_drop_packets);
+
+	clear_bit(IONIC_LIF_VF_OP, lif->state);
+	return 0;
+}
+
+static int ionic_set_vf_mac(struct net_device *netdev, int vf, u8 *mac)
+{
+	struct ionic_lif *lif = netdev_priv(netdev);
+	int ret;
+
+	if (vf >= pci_num_vf(lif->ionic->pdev))
+		return -EINVAL;
+
+	if (!(is_zero_ether_addr(mac) || is_valid_ether_addr(mac)))
+		return -EINVAL;
+
+	if (test_and_set_bit(IONIC_LIF_VF_OP, lif->state)) {
+		netdev_warn(netdev, "Unable to configure VFs, other operation is pending.\n");
+		return -EAGAIN;
+	}
+
+	ret = ionic_set_vf_config(lif->ionic, vf,
+				  IONIC_VF_ATTR_MAC, mac);
+	if (!ret)
+		ether_addr_copy(lif->ionic->vf[vf]->macaddr, mac);
+
+	clear_bit(IONIC_LIF_VF_OP, lif->state);
+	return ret;
+}
+
+static int ionic_set_vf_vlan(struct net_device *netdev, int vf, u16 vlan,
+			     u8 qos, __be16 proto)
+{
+	struct ionic_lif *lif = netdev_priv(netdev);
+	int ret;
+
+	if (vf >= pci_num_vf(lif->ionic->pdev))
+		return -EINVAL;
+
+	/* until someday when we support qos */
+	if (qos)
+		return -EINVAL;
+
+	if (vlan > 4095)
+		return -EINVAL;
+
+	if (proto != htons(ETH_P_8021Q))
+		return -EPROTONOSUPPORT;
+
+	if (test_and_set_bit(IONIC_LIF_VF_OP, lif->state)) {
+		netdev_warn(netdev, "Unable to configure VFs, other operation is pending.\n");
+		return -EAGAIN;
+	}
+
+	ret = ionic_set_vf_config(lif->ionic, vf,
+				  IONIC_VF_ATTR_VLAN, (u8 *)&vlan);
+	if (!ret)
+		lif->ionic->vf[vf]->vlanid = vlan;
+
+	clear_bit(IONIC_LIF_VF_OP, lif->state);
+	return ret;
+}
+
+static int ionic_set_vf_rate(struct net_device *netdev, int vf,
+			     int tx_min, int tx_max)
+{
+	struct ionic_lif *lif = netdev_priv(netdev);
+	int ret;
+
+	if (vf >= pci_num_vf(lif->ionic->pdev))
+		return -EINVAL;
+
+	/* setting the min just seems silly */
+	if (tx_min)
+		return -EINVAL;
+
+	if (test_and_set_bit(IONIC_LIF_VF_OP, lif->state)) {
+		netdev_warn(netdev, "Unable to configure VFs, other operation is pending.\n");
+		return -EAGAIN;
+	}
+
+	ret = ionic_set_vf_config(lif->ionic, vf,
+				  IONIC_VF_ATTR_RATE, (u8 *)&tx_max);
+	if (!ret)
+		lif->ionic->vf[vf]->maxrate = tx_max;
+
+	clear_bit(IONIC_LIF_VF_OP, lif->state);
+	return ret;
+}
+
+static int ionic_set_vf_spoofchk(struct net_device *netdev, int vf, bool set)
+{
+	struct ionic_lif *lif = netdev_priv(netdev);
+	u8 data = set;  /* convert to u8 for config */
+	int ret;
+
+	if (vf >= pci_num_vf(lif->ionic->pdev))
+		return -EINVAL;
+
+	if (test_and_set_bit(IONIC_LIF_VF_OP, lif->state)) {
+		netdev_warn(netdev, "Unable to configure VFs, other operation is pending.\n");
+		return -EAGAIN;
+	}
+
+	ret = ionic_set_vf_config(lif->ionic, vf,
+				  IONIC_VF_ATTR_SPOOFCHK, &data);
+	if (!ret)
+		lif->ionic->vf[vf]->spoofchk = data;
+
+	clear_bit(IONIC_LIF_VF_OP, lif->state);
+	return ret;
+}
+
+static int ionic_set_vf_trust(struct net_device *netdev, int vf, bool set)
+{
+	struct ionic_lif *lif = netdev_priv(netdev);
+	u8 data = set;  /* convert to u8 for config */
+	int ret;
+
+	if (vf >= pci_num_vf(lif->ionic->pdev))
+		return -EINVAL;
+
+	if (test_and_set_bit(IONIC_LIF_VF_OP, lif->state)) {
+		netdev_warn(netdev, "Unable to configure VFs, other operation is pending.\n");
+		return -EAGAIN;
+	}
+
+	ret = ionic_set_vf_config(lif->ionic, vf,
+				  IONIC_VF_ATTR_TRUST, &data);
+	if (!ret)
+		lif->ionic->vf[vf]->trusted = data;
+
+	clear_bit(IONIC_LIF_VF_OP, lif->state);
+	return ret;
+}
+
+static int ionic_set_vf_link_state(struct net_device *netdev, int vf, int set)
+{
+	struct ionic_lif *lif = netdev_priv(netdev);
+	u8 data;
+	int ret;
+
+	if (vf >= pci_num_vf(lif->ionic->pdev))
+		return -EINVAL;
+
+	if (test_and_set_bit(IONIC_LIF_VF_OP, lif->state)) {
+		netdev_warn(netdev, "Unable to configure VFs, other operation is pending.\n");
+		return -EAGAIN;
+	}
+
+	switch (set) {
+	case IFLA_VF_LINK_STATE_AUTO:
+		data = IONIC_VF_LINK_STATUS_AUTO;
+		break;
+	case IFLA_VF_LINK_STATE_ENABLE:
+		data = IONIC_VF_LINK_STATUS_UP;
+		break;
+	case IFLA_VF_LINK_STATE_DISABLE:
+		data = IONIC_VF_LINK_STATUS_DOWN;
+		break;
+	}
+
+	ret = ionic_set_vf_config(lif->ionic, vf,
+				  IONIC_VF_ATTR_LINKSTATE, &data);
+	if (!ret)
+		lif->ionic->vf[vf]->linkstate = set;
+
+	clear_bit(IONIC_LIF_VF_OP, lif->state);
+	return ret;
+}
+
 static const struct net_device_ops ionic_netdev_ops = {
 	.ndo_open               = ionic_open,
 	.ndo_stop               = ionic_stop,
@@ -1629,6 +1857,14 @@ static const struct net_device_ops ionic_netdev_ops = {
 	.ndo_change_mtu         = ionic_change_mtu,
 	.ndo_vlan_rx_add_vid    = ionic_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid   = ionic_vlan_rx_kill_vid,
+	.ndo_set_vf_vlan	= ionic_set_vf_vlan,
+	.ndo_set_vf_trust	= ionic_set_vf_trust,
+	.ndo_set_vf_mac		= ionic_set_vf_mac,
+	.ndo_set_vf_rate	= ionic_set_vf_rate,
+	.ndo_set_vf_spoofchk	= ionic_set_vf_spoofchk,
+	.ndo_get_vf_config	= ionic_get_vf_config,
+	.ndo_set_vf_link_state	= ionic_set_vf_link_state,
+	.ndo_get_vf_stats       = ionic_get_vf_stats,
 };
 
 int ionic_reset_queues(struct ionic_lif *lif)
@@ -1961,18 +2197,22 @@ static int ionic_station_set(struct ionic_lif *lif)
 	if (err)
 		return err;
 
+	if (is_zero_ether_addr(ctx.comp.lif_getattr.mac))
+		return 0;
+
 	memcpy(addr.sa_data, ctx.comp.lif_getattr.mac, netdev->addr_len);
 	addr.sa_family = AF_INET;
 	err = eth_prepare_mac_addr_change(netdev, &addr);
-	if (err)
-		return err;
-
-	if (!is_zero_ether_addr(netdev->dev_addr)) {
-		netdev_dbg(lif->netdev, "deleting station MAC addr %pM\n",
-			   netdev->dev_addr);
-		ionic_lif_addr(lif, netdev->dev_addr, false);
+	if (err) {
+		netdev_warn(lif->netdev, "ignoring bad MAC addr from NIC %pM\n",
+			    addr.sa_data);
+		return 0;
 	}
 
+	netdev_dbg(lif->netdev, "deleting station MAC addr %pM\n",
+		   netdev->dev_addr);
+	ionic_lif_addr(lif, netdev->dev_addr, false);
+
 	eth_commit_mac_addr_change(netdev, &addr);
 	netdev_dbg(lif->netdev, "adding station MAC addr %pM\n",
 		   netdev->dev_addr);
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
index a55fd1f8c31b..0657e00ab85f 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
@@ -125,6 +125,7 @@ enum ionic_lif_state_flags {
 	IONIC_LIF_UP,
 	IONIC_LIF_LINK_CHECK_REQUESTED,
 	IONIC_LIF_QUEUE_RESET,
+	IONIC_LIF_VF_OP,
 
 	/* leave this as last */
 	IONIC_LIF_STATE_SIZE
@@ -195,6 +196,12 @@ static inline int ionic_wait_for_bit(struct ionic_lif *lif, int bitname)
 	return wait_on_bit_lock(lif->state, bitname, TASK_INTERRUPTIBLE);
 }
 
+static inline bool ionic_is_pf(struct ionic *ionic)
+{
+	return ionic->pdev &&
+	       ionic->pdev->device == PCI_DEVICE_ID_PENSANDO_IONIC_ETH_PF;
+}
+
 static inline u32 ionic_coal_usec_to_hw(struct ionic *ionic, u32 usecs)
 {
 	u32 mult = le32_to_cpu(ionic->ident.dev.intr_coal_mult);
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_main.c b/drivers/net/ethernet/pensando/ionic/ionic_main.c
index 3590ea7fd88a..837b85f2fed9 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_main.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_main.c
@@ -165,6 +165,10 @@ static const char *ionic_opcode_to_str(enum ionic_cmd_opcode opcode)
 		return "IONIC_CMD_FW_DOWNLOAD";
 	case IONIC_CMD_FW_CONTROL:
 		return "IONIC_CMD_FW_CONTROL";
+	case IONIC_CMD_VF_GETATTR:
+		return "IONIC_CMD_VF_GETATTR";
+	case IONIC_CMD_VF_SETATTR:
+		return "IONIC_CMD_VF_SETATTR";
 	default:
 		return "DEVCMD_UNKNOWN";
 	}
-- 
2.17.1

Re: [PATCH v2 net-next 2/2] ionic: support sr-iov operations

[Parav Pandit]

On 12/11/2019 6:33 PM, Shannon Nelson wrote:
> Add the netdev ops for managing VFs.  Since most of the
> management work happens in the NIC firmware, the driver becomes
> mostly a pass-through for the network stack commands that want
> to control and configure the VFs.
> 
> We also tweak ionic_station_set() a little to allow for
> the VFs that start off with a zero'd mac address.
> 
> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> ---
>  drivers/net/ethernet/pensando/ionic/ionic.h   |  15 +-
>  .../ethernet/pensando/ionic/ionic_bus_pci.c   |  85 ++++++
>  .../net/ethernet/pensando/ionic/ionic_dev.c   |  58 ++++
>  .../net/ethernet/pensando/ionic/ionic_dev.h   |   7 +
>  .../net/ethernet/pensando/ionic/ionic_lif.c   | 254 +++++++++++++++++-
>  .../net/ethernet/pensando/ionic/ionic_lif.h   |   7 +
>  .../net/ethernet/pensando/ionic/ionic_main.c  |   4 +
>  7 files changed, 422 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h
> index 98e102af7756..e5c9e4b0450b 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic.h
> +++ b/drivers/net/ethernet/pensando/ionic/ionic.h
> @@ -12,7 +12,7 @@ struct ionic_lif;
>  
>  #define IONIC_DRV_NAME		"ionic"
>  #define IONIC_DRV_DESCRIPTION	"Pensando Ethernet NIC Driver"
> -#define IONIC_DRV_VERSION	"0.18.0-k"
> +#define IONIC_DRV_VERSION	"0.20.0-k"
>  
>  #define PCI_VENDOR_ID_PENSANDO			0x1dd8
>  
> @@ -25,6 +25,18 @@ struct ionic_lif;
>  
>  #define DEVCMD_TIMEOUT  10
>  
> +struct ionic_vf {
> +	u16	 index;
> +	u8	 macaddr[6];
> +	__le32	 maxrate;
> +	__le16	 vlanid;
> +	u8	 spoofchk;
> +	u8	 trusted;
> +	u8	 linkstate;
> +	dma_addr_t       stats_pa;
> +	struct ionic_lif_stats stats;
> +};
> +
>  struct ionic {
>  	struct pci_dev *pdev;
>  	struct device *dev;
> @@ -46,6 +58,7 @@ struct ionic {
>  	DECLARE_BITMAP(intrs, IONIC_INTR_CTRL_REGS_MAX);
>  	struct work_struct nb_work;
>  	struct notifier_block nb;
> +	struct ionic_vf **vf;
>  	struct timer_list watchdog_timer;
>  	int watchdog_period;
>  };
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> index 9a9ab8cb2cb3..057eb453dd11 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> @@ -250,6 +250,87 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	return err;
>  }
>  
> +static int ionic_sriov_configure(struct pci_dev *pdev, int num_vfs)
> +{
> +	struct ionic *ionic = pci_get_drvdata(pdev);
> +	struct device *dev = ionic->dev;
> +	int i, ret = 0;
> +	int nvfs = 0;
> +
> +	if (test_and_set_bit(IONIC_LIF_VF_OP, ionic->master_lif->state)) {
Nop. This is not correct.
User space doesn't retry the command when you throw an error as EAGAIN.
These are not file system calls.
As I told in v1, you need rwsem here without below warning messages all
over the ops.

> +		dev_warn(&pdev->dev, "Unable to configure VFs, other operation is pending.\n");
> +		return -EAGAIN;
> +	}
> +
> +	if (num_vfs > 0) {
> +		ret = pci_enable_sriov(pdev, num_vfs);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Cannot enable SRIOV: %d\n", ret);
> +			goto out;
> +		}
> +
> +		ionic->vf = kcalloc(num_vfs, sizeof(struct ionic_vf *),
> +				    GFP_KERNEL);
> +		if (!ionic->vf) {
> +			pci_disable_sriov(pdev);
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		for (i = 0; i < num_vfs; i++) {
> +			struct ionic_vf *v;
> +
> +			v = kzalloc(sizeof(*v), GFP_KERNEL);
> +			if (!v) {
> +				ret = -ENOMEM;
> +				num_vfs = 0;
> +				goto remove_vfs;
> +			}
> +
> +			v->stats_pa = dma_map_single(dev, &v->stats,
> +						     sizeof(v->stats),
> +						     DMA_FROM_DEVICE);
> +			if (dma_mapping_error(dev, v->stats_pa)) {
> +				ret = -ENODEV;
> +				kfree(v);
> +				ionic->vf[i] = NULL;
> +				num_vfs = 0;
> +				goto remove_vfs;
> +			}
> +
> +			ionic->vf[i] = v;
> +			nvfs++;
No need for this extra nvfs. Run the loop from i to 0 in reverse order
in error unwinding.

> +		}
> +
> +		ret = num_vfs;
> +		goto out;
> +	}
> +
> +remove_vfs:
> +	if (num_vfs == 0) {
> +		if (ret)
> +			dev_err(&pdev->dev, "SRIOV setup failed: %d\n", ret);
> +
> +		pci_disable_sriov(pdev);
> +
> +		if (!nvfs)
> +			nvfs = pci_num_vf(pdev);
> +		for (i = 0; i < nvfs; i++) {
error unwinding should be reverse of setup. It should run from i to 0.

> +			dma_unmap_single(dev, ionic->vf[i]->stats_pa,
> +					 sizeof(struct ionic_lif_stats),
Please be consistent with dma_map_single which uses sizeof(v->stats).

> +					 DMA_FROM_DEVICE);
> +			kfree(ionic->vf[i]);
> +		}
> +
> +		kfree(ionic->vf);
> +		ionic->vf = NULL;
> +	}
> +
> +out:
> +	clear_bit(IONIC_LIF_VF_OP, ionic->master_lif->state);
> +	return ret;
> +}
> +
>  static void ionic_remove(struct pci_dev *pdev)
>  {
>  	struct ionic *ionic = pci_get_drvdata(pdev);
> @@ -257,6 +338,9 @@ static void ionic_remove(struct pci_dev *pdev)
>  	if (!ionic)
>  		return;
>  
> +	if (pci_num_vf(pdev))
> +		ionic_sriov_configure(pdev, 0);
> +
Usually sriov is left enabled while removing PF.
It is not the role of the pci PF removal to disable it sriov.

Re: [PATCH v2 net-next 2/2] ionic: support sr-iov operations

[Shannon Nelson]

On 12/11/19 10:53 PM, Parav Pandit wrote:
> On 12/11/2019 6:33 PM, Shannon Nelson wrote:
>> Add the netdev ops for managing VFs.  Since most of the
>> management work happens in the NIC firmware, the driver becomes
>> mostly a pass-through for the network stack commands that want
>> to control and configure the VFs.
>>
>> We also tweak ionic_station_set() a little to allow for
>> the VFs that start off with a zero'd mac address.
>>
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
>> ---
>>   drivers/net/ethernet/pensando/ionic/ionic.h   |  15 +-
>>   .../ethernet/pensando/ionic/ionic_bus_pci.c   |  85 ++++++
>>   .../net/ethernet/pensando/ionic/ionic_dev.c   |  58 ++++
>>   .../net/ethernet/pensando/ionic/ionic_dev.h   |   7 +
>>   .../net/ethernet/pensando/ionic/ionic_lif.c   | 254 +++++++++++++++++-
>>   .../net/ethernet/pensando/ionic/ionic_lif.h   |   7 +
>>   .../net/ethernet/pensando/ionic/ionic_main.c  |   4 +
>>   7 files changed, 422 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h
>> index 98e102af7756..e5c9e4b0450b 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic.h
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic.h
>> @@ -12,7 +12,7 @@ struct ionic_lif;
>>   
>>   #define IONIC_DRV_NAME		"ionic"
>>   #define IONIC_DRV_DESCRIPTION	"Pensando Ethernet NIC Driver"
>> -#define IONIC_DRV_VERSION	"0.18.0-k"
>> +#define IONIC_DRV_VERSION	"0.20.0-k"
>>   
>>   #define PCI_VENDOR_ID_PENSANDO			0x1dd8
>>   
>> @@ -25,6 +25,18 @@ struct ionic_lif;
>>   
>>   #define DEVCMD_TIMEOUT  10
>>   
>> +struct ionic_vf {
>> +	u16	 index;
>> +	u8	 macaddr[6];
>> +	__le32	 maxrate;
>> +	__le16	 vlanid;
>> +	u8	 spoofchk;
>> +	u8	 trusted;
>> +	u8	 linkstate;
>> +	dma_addr_t       stats_pa;
>> +	struct ionic_lif_stats stats;
>> +};
>> +
>>   struct ionic {
>>   	struct pci_dev *pdev;
>>   	struct device *dev;
>> @@ -46,6 +58,7 @@ struct ionic {
>>   	DECLARE_BITMAP(intrs, IONIC_INTR_CTRL_REGS_MAX);
>>   	struct work_struct nb_work;
>>   	struct notifier_block nb;
>> +	struct ionic_vf **vf;
>>   	struct timer_list watchdog_timer;
>>   	int watchdog_period;
>>   };
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
>> index 9a9ab8cb2cb3..057eb453dd11 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
>> @@ -250,6 +250,87 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>   	return err;
>>   }
>>   
>> +static int ionic_sriov_configure(struct pci_dev *pdev, int num_vfs)
>> +{
>> +	struct ionic *ionic = pci_get_drvdata(pdev);
>> +	struct device *dev = ionic->dev;
>> +	int i, ret = 0;
>> +	int nvfs = 0;
>> +
>> +	if (test_and_set_bit(IONIC_LIF_VF_OP, ionic->master_lif->state)) {
> Nop. This is not correct.
> User space doesn't retry the command when you throw an error as EAGAIN.
> These are not file system calls.
> As I told in v1, you need rwsem here without below warning messages all
> over the ops.

Hmmm... I was following how i40e solved this a year ago, which seems 
valid enough, but I see how using an rwsem could be seen as more 
correct, and probably a lot kinder on reads with a device that can 
support a large number of VFs.  I'll take another run at this.

>
>> +		dev_warn(&pdev->dev, "Unable to configure VFs, other operation is pending.\n");
>> +		return -EAGAIN;
>> +	}
>> +
>> +	if (num_vfs > 0) {
>> +		ret = pci_enable_sriov(pdev, num_vfs);
>> +		if (ret) {
>> +			dev_err(&pdev->dev, "Cannot enable SRIOV: %d\n", ret);
>> +			goto out;
>> +		}
>> +
>> +		ionic->vf = kcalloc(num_vfs, sizeof(struct ionic_vf *),
>> +				    GFP_KERNEL);
>> +		if (!ionic->vf) {
>> +			pci_disable_sriov(pdev);
>> +			ret = -ENOMEM;
>> +			goto out;
>> +		}
>> +
>> +		for (i = 0; i < num_vfs; i++) {
>> +			struct ionic_vf *v;
>> +
>> +			v = kzalloc(sizeof(*v), GFP_KERNEL);
>> +			if (!v) {
>> +				ret = -ENOMEM;
>> +				num_vfs = 0;
>> +				goto remove_vfs;
>> +			}
>> +
>> +			v->stats_pa = dma_map_single(dev, &v->stats,
>> +						     sizeof(v->stats),
>> +						     DMA_FROM_DEVICE);
>> +			if (dma_mapping_error(dev, v->stats_pa)) {
>> +				ret = -ENODEV;
>> +				kfree(v);
>> +				ionic->vf[i] = NULL;
>> +				num_vfs = 0;
>> +				goto remove_vfs;
>> +			}
>> +
>> +			ionic->vf[i] = v;
>> +			nvfs++;
> No need for this extra nvfs. Run the loop from i to 0 in reverse order
> in error unwinding.

Sure

>
>> +		}
>> +
>> +		ret = num_vfs;
>> +		goto out;
>> +	}
>> +
>> +remove_vfs:
>> +	if (num_vfs == 0) {
>> +		if (ret)
>> +			dev_err(&pdev->dev, "SRIOV setup failed: %d\n", ret);
>> +
>> +		pci_disable_sriov(pdev);
>> +
>> +		if (!nvfs)
>> +			nvfs = pci_num_vf(pdev);
>> +		for (i = 0; i < nvfs; i++) {
> error unwinding should be reverse of setup. It should run from i to 0.

Yes, this follows if I ditch nvfs.

>
>> +			dma_unmap_single(dev, ionic->vf[i]->stats_pa,
>> +					 sizeof(struct ionic_lif_stats),
> Please be consistent with dma_map_single which uses sizeof(v->stats).

Yep

>
>> +					 DMA_FROM_DEVICE);
>> +			kfree(ionic->vf[i]);
>> +		}
>> +
>> +		kfree(ionic->vf);
>> +		ionic->vf = NULL;
>> +	}
>> +
>> +out:
>> +	clear_bit(IONIC_LIF_VF_OP, ionic->master_lif->state);
>> +	return ret;
>> +}
>> +
>>   static void ionic_remove(struct pci_dev *pdev)
>>   {
>>   	struct ionic *ionic = pci_get_drvdata(pdev);
>> @@ -257,6 +338,9 @@ static void ionic_remove(struct pci_dev *pdev)
>>   	if (!ionic)
>>   		return;
>>   
>> +	if (pci_num_vf(pdev))
>> +		ionic_sriov_configure(pdev, 0);
>> +
> Usually sriov is left enabled while removing PF.
> It is not the role of the pci PF removal to disable it sriov.

Looks like I'll need to split out the vf[] allocation into a separate 
routine (and perhaps the teardown too) so I can do it from
probe.

Version three coming soon to a mailing list near you...

sln

Re: [PATCH v2 net-next 2/2] ionic: support sr-iov operations

[Jakub Kicinski]

On Thu, 12 Dec 2019 06:53:42 +0000, Parav Pandit wrote:
> >  static void ionic_remove(struct pci_dev *pdev)
> >  {
> >  	struct ionic *ionic = pci_get_drvdata(pdev);
> > @@ -257,6 +338,9 @@ static void ionic_remove(struct pci_dev *pdev)
> >  	if (!ionic)
> >  		return;
> >  
> > +	if (pci_num_vf(pdev))
> > +		ionic_sriov_configure(pdev, 0);
> > +  
> Usually sriov is left enabled while removing PF.
> It is not the role of the pci PF removal to disable it sriov.

I don't think that's true. I consider igb and ixgbe to set the standard
for legacy SR-IOV handling since they were one of the first (the first?)
and Alex Duyck wrote them.

mlx4, bnxt and nfp all disable SR-IOV on remove.

Re: [PATCH v2 net-next 2/2] ionic: support sr-iov operations

[Shannon Nelson]

On 12/12/19 11:52 AM, Jakub Kicinski wrote:
> On Thu, 12 Dec 2019 06:53:42 +0000, Parav Pandit wrote:
>>>   static void ionic_remove(struct pci_dev *pdev)
>>>   {
>>>   	struct ionic *ionic = pci_get_drvdata(pdev);
>>> @@ -257,6 +338,9 @@ static void ionic_remove(struct pci_dev *pdev)
>>>   	if (!ionic)
>>>   		return;
>>>   
>>> +	if (pci_num_vf(pdev))
>>> +		ionic_sriov_configure(pdev, 0);
>>> +
>> Usually sriov is left enabled while removing PF.
>> It is not the role of the pci PF removal to disable it sriov.
> I don't think that's true. I consider igb and ixgbe to set the standard
> for legacy SR-IOV handling since they were one of the first (the first?)
> and Alex Duyck wrote them.
>
> mlx4, bnxt and nfp all disable SR-IOV on remove.

This was my understanding as well, but now I can see that ixgbe and i40e 
are both checking for existing VFs in probe and setting up to use them, 
as well as the newer ice driver.  I found this today by looking for 
where they use pci_num_vf().

sln

Re: [PATCH v2 net-next 2/2] ionic: support sr-iov operations

[Jakub Kicinski]

On Thu, 12 Dec 2019 11:59:50 -0800, Shannon Nelson wrote:
> On 12/12/19 11:52 AM, Jakub Kicinski wrote:
> > On Thu, 12 Dec 2019 06:53:42 +0000, Parav Pandit wrote:  
> >>>   static void ionic_remove(struct pci_dev *pdev)
> >>>   {
> >>>   	struct ionic *ionic = pci_get_drvdata(pdev);
> >>> @@ -257,6 +338,9 @@ static void ionic_remove(struct pci_dev *pdev)
> >>>   	if (!ionic)
> >>>   		return;
> >>>   
> >>> +	if (pci_num_vf(pdev))
> >>> +		ionic_sriov_configure(pdev, 0);
> >>> +  
> >> Usually sriov is left enabled while removing PF.
> >> It is not the role of the pci PF removal to disable it sriov.  
> > I don't think that's true. I consider igb and ixgbe to set the standard
> > for legacy SR-IOV handling since they were one of the first (the first?)
> > and Alex Duyck wrote them.
> >
> > mlx4, bnxt and nfp all disable SR-IOV on remove.  
> 
> This was my understanding as well, but now I can see that ixgbe and i40e 
> are both checking for existing VFs in probe and setting up to use them, 
> as well as the newer ice driver.  I found this today by looking for 
> where they use pci_num_vf().

Right, if the VFs very already enabled on probe they are set up.

It's a bit of a asymmetric design, in case some other driver left
SR-IOV on, I guess.

Re: [PATCH v2 net-next 2/2] ionic: support sr-iov operations

[Parav Pandit]

On 12/12/2019 3:35 PM, Jakub Kicinski wrote:
> On Thu, 12 Dec 2019 11:59:50 -0800, Shannon Nelson wrote:
>> On 12/12/19 11:52 AM, Jakub Kicinski wrote:
>>> On Thu, 12 Dec 2019 06:53:42 +0000, Parav Pandit wrote:  
>>>>>   static void ionic_remove(struct pci_dev *pdev)
>>>>>   {
>>>>>   	struct ionic *ionic = pci_get_drvdata(pdev);
>>>>> @@ -257,6 +338,9 @@ static void ionic_remove(struct pci_dev *pdev)
>>>>>   	if (!ionic)
>>>>>   		return;
>>>>>   
>>>>> +	if (pci_num_vf(pdev))
>>>>> +		ionic_sriov_configure(pdev, 0);
>>>>> +  
>>>> Usually sriov is left enabled while removing PF.
>>>> It is not the role of the pci PF removal to disable it sriov.  
>>> I don't think that's true. I consider igb and ixgbe to set the standard
>>> for legacy SR-IOV handling since they were one of the first (the first?)
>>> and Alex Duyck wrote them.
>>>
>>> mlx4, bnxt and nfp all disable SR-IOV on remove.  
>>
>> This was my understanding as well, but now I can see that ixgbe and i40e 
>> are both checking for existing VFs in probe and setting up to use them, 
>> as well as the newer ice driver.  I found this today by looking for 
>> where they use pci_num_vf().
> 
> Right, if the VFs very already enabled on probe they are set up.
> 
> It's a bit of a asymmetric design, in case some other driver left
> SR-IOV on, I guess.
> 

I remember on one email thread on netdev list from someone that in one
use case, they upgrade the PF driver while VFs are still bound and
SR-IOV kept enabled.
I am not sure how much it is used in practice/or practical.
Such use case may be the reason to keep SR-IOV enabled.

Re: [PATCH v2 net-next 2/2] ionic: support sr-iov operations

[Shannon Nelson]

On 12/12/19 2:24 PM, Parav Pandit wrote:
> On 12/12/2019 3:35 PM, Jakub Kicinski wrote:
>> On Thu, 12 Dec 2019 11:59:50 -0800, Shannon Nelson wrote:
>>> On 12/12/19 11:52 AM, Jakub Kicinski wrote:
>>>> On Thu, 12 Dec 2019 06:53:42 +0000, Parav Pandit wrote:
>>>>>>    static void ionic_remove(struct pci_dev *pdev)
>>>>>>    {
>>>>>>    	struct ionic *ionic = pci_get_drvdata(pdev);
>>>>>> @@ -257,6 +338,9 @@ static void ionic_remove(struct pci_dev *pdev)
>>>>>>    	if (!ionic)
>>>>>>    		return;
>>>>>>    
>>>>>> +	if (pci_num_vf(pdev))
>>>>>> +		ionic_sriov_configure(pdev, 0);
>>>>>> +
>>>>> Usually sriov is left enabled while removing PF.
>>>>> It is not the role of the pci PF removal to disable it sriov.
>>>> I don't think that's true. I consider igb and ixgbe to set the standard
>>>> for legacy SR-IOV handling since they were one of the first (the first?)
>>>> and Alex Duyck wrote them.
>>>>
>>>> mlx4, bnxt and nfp all disable SR-IOV on remove.
>>> This was my understanding as well, but now I can see that ixgbe and i40e
>>> are both checking for existing VFs in probe and setting up to use them,
>>> as well as the newer ice driver.  I found this today by looking for
>>> where they use pci_num_vf().
>> Right, if the VFs very already enabled on probe they are set up.
>>
>> It's a bit of a asymmetric design, in case some other driver left
>> SR-IOV on, I guess.
>>
> I remember on one email thread on netdev list from someone that in one
> use case, they upgrade the PF driver while VFs are still bound and
> SR-IOV kept enabled.
> I am not sure how much it is used in practice/or practical.
> Such use case may be the reason to keep SR-IOV enabled.

This brings up a potential corner case where it would be better for the 
driver to use its own num_vfs value rather than relying on the 
pci_num_vf() when answering the ndo_get_vf_*() callbacks, and at least 
the igb may be susceptible.  If the driver hasn't set up its vf[] data 
arrays because there was an error in setting them up in the probe(), and 
later someone tries to get VF statistics, the ndo_get_vf_stats callback 
could end up dereferencing bad pointers because vf is less than 
pci_num_vf() but more than the number of vf[] structs set up by the driver.

I suppose the argument could be made that PF's probe should if the VF 
config fails, but it might be nice to have the PF driver running to help 
fix up whatever when sideways in the VF configuration.

sln

Re: [PATCH v2 net-next 2/2] ionic: support sr-iov operations

[Jakub Kicinski]

On Thu, 12 Dec 2019 14:40:01 -0800, Shannon Nelson wrote:
> I suppose the argument could be made that PF's probe should if the VF 
> config fails, but it might be nice to have the PF driver running to help 
> fix up whatever when sideways in the VF configuration.

If probe fails driver should probe in a degraded mode, where only
subset of devlink functionality needed for debug is available.

Re: [PATCH v2 net-next 2/2] ionic: support sr-iov operations

[Parav Pandit]

On 12/13/2019 4:10 AM, Shannon Nelson wrote:
> On 12/12/19 2:24 PM, Parav Pandit wrote:
>> On 12/12/2019 3:35 PM, Jakub Kicinski wrote:
>>> On Thu, 12 Dec 2019 11:59:50 -0800, Shannon Nelson wrote:
>>>> On 12/12/19 11:52 AM, Jakub Kicinski wrote:
>>>>> On Thu, 12 Dec 2019 06:53:42 +0000, Parav Pandit wrote:
>>>>>>>    static void ionic_remove(struct pci_dev *pdev)
>>>>>>>    {
>>>>>>>        struct ionic *ionic = pci_get_drvdata(pdev);
>>>>>>> @@ -257,6 +338,9 @@ static void ionic_remove(struct pci_dev *pdev)
>>>>>>>        if (!ionic)
>>>>>>>            return;
>>>>>>>    +    if (pci_num_vf(pdev))
>>>>>>> +        ionic_sriov_configure(pdev, 0);
>>>>>>> +
>>>>>> Usually sriov is left enabled while removing PF.
>>>>>> It is not the role of the pci PF removal to disable it sriov.
>>>>> I don't think that's true. I consider igb and ixgbe to set the
>>>>> standard
>>>>> for legacy SR-IOV handling since they were one of the first (the
>>>>> first?)
>>>>> and Alex Duyck wrote them.
>>>>>
>>>>> mlx4, bnxt and nfp all disable SR-IOV on remove.
>>>> This was my understanding as well, but now I can see that ixgbe and
>>>> i40e
>>>> are both checking for existing VFs in probe and setting up to use them,
>>>> as well as the newer ice driver.  I found this today by looking for
>>>> where they use pci_num_vf().
>>> Right, if the VFs very already enabled on probe they are set up.
>>>
>>> It's a bit of a asymmetric design, in case some other driver left
>>> SR-IOV on, I guess.
>>>
>> I remember on one email thread on netdev list from someone that in one
>> use case, they upgrade the PF driver while VFs are still bound and
>> SR-IOV kept enabled.
>> I am not sure how much it is used in practice/or practical.
>> Such use case may be the reason to keep SR-IOV enabled.
> 
> This brings up a potential corner case where it would be better for the
> driver to use its own num_vfs value rather than relying on the
> pci_num_vf() when answering the ndo_get_vf_*() callbacks, and at least
> the igb may be susceptible.  
Please do not cache num_vfs in driver. Use the pci core's pci_num_vf()
in the new code that you are adding.
More below.
> If the driver hasn't set up its vf[] data
> arrays because there was an error in setting them up in the probe(), and
> later someone tries to get VF statistics, the ndo_get_vf_stats callback
> could end up dereferencing bad pointers because vf is less than
> pci_num_vf() but more than the number of vf[] structs set up by the driver.
> 
> I suppose the argument could be made that PF's probe should if the VF
> config fails, but it might be nice to have the PF driver running to help
> fix up whatever when sideways in the VF configuration.
> 
> sln
> 
I not have strong opinion on letting sriov enabled/disabled on PF device
removal.
But it should be symmetric on probe() and remove() for PF.
If you want to keep it enabled on PF removal, you need to check on probe
and allocate VF metadata you have by using helper function in
sriov_configure() and in probe().
This is followed by mlx5 driver.

Re: [PATCH v2 net-next 2/2] ionic: support sr-iov operations

[Shannon Nelson]

On 12/15/19 8:47 PM, Parav Pandit wrote:
> On 12/13/2019 4:10 AM, Shannon Nelson wrote:
>> On 12/12/19 2:24 PM, Parav Pandit wrote:
>>> On 12/12/2019 3:35 PM, Jakub Kicinski wrote:
>>>> On Thu, 12 Dec 2019 11:59:50 -0800, Shannon Nelson wrote:
>>>>> On 12/12/19 11:52 AM, Jakub Kicinski wrote:
>>>>>> On Thu, 12 Dec 2019 06:53:42 +0000, Parav Pandit wrote:
>>>>>>>>     static void ionic_remove(struct pci_dev *pdev)
>>>>>>>>     {
>>>>>>>>         struct ionic *ionic = pci_get_drvdata(pdev);
>>>>>>>> @@ -257,6 +338,9 @@ static void ionic_remove(struct pci_dev *pdev)
>>>>>>>>         if (!ionic)
>>>>>>>>             return;
>>>>>>>>     +    if (pci_num_vf(pdev))
>>>>>>>> +        ionic_sriov_configure(pdev, 0);
>>>>>>>> +
>>>>>>> Usually sriov is left enabled while removing PF.
>>>>>>> It is not the role of the pci PF removal to disable it sriov.
>>>>>> I don't think that's true. I consider igb and ixgbe to set the
>>>>>> standard
>>>>>> for legacy SR-IOV handling since they were one of the first (the
>>>>>> first?)
>>>>>> and Alex Duyck wrote them.
>>>>>>
>>>>>> mlx4, bnxt and nfp all disable SR-IOV on remove.
>>>>> This was my understanding as well, but now I can see that ixgbe and
>>>>> i40e
>>>>> are both checking for existing VFs in probe and setting up to use them,
>>>>> as well as the newer ice driver.  I found this today by looking for
>>>>> where they use pci_num_vf().
>>>> Right, if the VFs very already enabled on probe they are set up.
>>>>
>>>> It's a bit of a asymmetric design, in case some other driver left
>>>> SR-IOV on, I guess.
>>>>
>>> I remember on one email thread on netdev list from someone that in one
>>> use case, they upgrade the PF driver while VFs are still bound and
>>> SR-IOV kept enabled.
>>> I am not sure how much it is used in practice/or practical.
>>> Such use case may be the reason to keep SR-IOV enabled.
>> This brings up a potential corner case where it would be better for the
>> driver to use its own num_vfs value rather than relying on the
>> pci_num_vf() when answering the ndo_get_vf_*() callbacks, and at least
>> the igb may be susceptible.
> Please do not cache num_vfs in driver. Use the pci core's pci_num_vf()
> in the new code that you are adding.

I disagree.  The pci_num_vf() tells us what the kernel has set up for 
VFs running, while the driver's num_vfs tracks how many resources the 
driver has set up for handling VFs: these are two different numbers, and 
there are times in the life of the driver when these numbers are 
different.  Yes, these are small windows of time, but they are different 
and need to be treated differently.

> More below.
>> If the driver hasn't set up its vf[] data
>> arrays because there was an error in setting them up in the probe(), and
>> later someone tries to get VF statistics, the ndo_get_vf_stats callback
>> could end up dereferencing bad pointers because vf is less than
>> pci_num_vf() but more than the number of vf[] structs set up by the driver.
>>
>> I suppose the argument could be made that PF's probe should if the VF
>> config fails, but it might be nice to have the PF driver running to help
>> fix up whatever when sideways in the VF configuration.
>>
>> sln
>>
> I not have strong opinion on letting sriov enabled/disabled on PF device
> removal.
> But it should be symmetric on probe() and remove() for PF.
> If you want to keep it enabled on PF removal, you need to check on probe
> and allocate VF metadata you have by using helper function in
> sriov_configure() and in probe().
> This is followed by mlx5 driver.

Agreed, and this check at probe time is included in the v3 patch that I 
sent out on Friday.

sln

Re: [PATCH v2 net-next 2/2] ionic: support sr-iov operations

[Parav Pandit]

On 12/16/2019 11:32 AM, Shannon Nelson wrote:
> On 12/15/19 8:47 PM, Parav Pandit wrote:
>> On 12/13/2019 4:10 AM, Shannon Nelson wrote:
>>> On 12/12/19 2:24 PM, Parav Pandit wrote:
>>>> On 12/12/2019 3:35 PM, Jakub Kicinski wrote:
>>>>> On Thu, 12 Dec 2019 11:59:50 -0800, Shannon Nelson wrote:
>>>>>> On 12/12/19 11:52 AM, Jakub Kicinski wrote:
>>>>>>> On Thu, 12 Dec 2019 06:53:42 +0000, Parav Pandit wrote:
>>>>>>>>>     static void ionic_remove(struct pci_dev *pdev)
>>>>>>>>>     {
>>>>>>>>>         struct ionic *ionic = pci_get_drvdata(pdev);
>>>>>>>>> @@ -257,6 +338,9 @@ static void ionic_remove(struct pci_dev *pdev)
>>>>>>>>>         if (!ionic)
>>>>>>>>>             return;
>>>>>>>>>     +    if (pci_num_vf(pdev))
>>>>>>>>> +        ionic_sriov_configure(pdev, 0);
>>>>>>>>> +
>>>>>>>> Usually sriov is left enabled while removing PF.
>>>>>>>> It is not the role of the pci PF removal to disable it sriov.
>>>>>>> I don't think that's true. I consider igb and ixgbe to set the
>>>>>>> standard
>>>>>>> for legacy SR-IOV handling since they were one of the first (the
>>>>>>> first?)
>>>>>>> and Alex Duyck wrote them.
>>>>>>>
>>>>>>> mlx4, bnxt and nfp all disable SR-IOV on remove.
>>>>>> This was my understanding as well, but now I can see that ixgbe and
>>>>>> i40e
>>>>>> are both checking for existing VFs in probe and setting up to use
>>>>>> them,
>>>>>> as well as the newer ice driver.  I found this today by looking for
>>>>>> where they use pci_num_vf().
>>>>> Right, if the VFs very already enabled on probe they are set up.
>>>>>
>>>>> It's a bit of a asymmetric design, in case some other driver left
>>>>> SR-IOV on, I guess.
>>>>>
>>>> I remember on one email thread on netdev list from someone that in one
>>>> use case, they upgrade the PF driver while VFs are still bound and
>>>> SR-IOV kept enabled.
>>>> I am not sure how much it is used in practice/or practical.
>>>> Such use case may be the reason to keep SR-IOV enabled.
>>> This brings up a potential corner case where it would be better for the
>>> driver to use its own num_vfs value rather than relying on the
>>> pci_num_vf() when answering the ndo_get_vf_*() callbacks, and at least
>>> the igb may be susceptible.
>> Please do not cache num_vfs in driver. Use the pci core's pci_num_vf()
>> in the new code that you are adding.
> 
> I disagree.  The pci_num_vf() tells us what the kernel has set up for
> VFs running, while the driver's num_vfs tracks how many resources the
> driver has set up for handling VFs: these are two different numbers, and
> there are times in the life of the driver when these numbers are
> different.  Yes, these are small windows of time, but they are different
> and need to be treated differently.
> 
They shouldn't be different. Why are they different?

>> More below.
>>> If the driver hasn't set up its vf[] data
>>> arrays because there was an error in setting them up in the probe(), and
>>> later someone tries to get VF statistics, the ndo_get_vf_stats callback
>>> could end up dereferencing bad pointers because vf is less than
>>> pci_num_vf() but more than the number of vf[] structs set up by the
>>> driver.
>>>
>>> I suppose the argument could be made that PF's probe should if the VF
>>> config fails, but it might be nice to have the PF driver running to help
>>> fix up whatever when sideways in the VF configuration.
>>>
>>> sln
>>>
>> I not have strong opinion on letting sriov enabled/disabled on PF device
>> removal.
>> But it should be symmetric on probe() and remove() for PF.
>> If you want to keep it enabled on PF removal, you need to check on probe
>> and allocate VF metadata you have by using helper function in
>> sriov_configure() and in probe().
>> This is followed by mlx5 driver.
> 
> Agreed, and this check at probe time is included in the v3 patch that I
> sent out on Friday.
> 
ok. Will review. Thanks.

> sln
> 
> 

Re: [PATCH v2 net-next 2/2] ionic: support sr-iov operations

[Shannon Nelson]

On 12/16/19 12:46 AM, Parav Pandit wrote:
> On 12/16/2019 11:32 AM, Shannon Nelson wrote:
>> On 12/15/19 8:47 PM, Parav Pandit wrote:
>>> On 12/13/2019 4:10 AM, Shannon Nelson wrote:
>>>> On 12/12/19 2:24 PM, Parav Pandit wrote:
>>>>> On 12/12/2019 3:35 PM, Jakub Kicinski wrote:
>>>>>> On Thu, 12 Dec 2019 11:59:50 -0800, Shannon Nelson wrote:
>>>>>>> On 12/12/19 11:52 AM, Jakub Kicinski wrote:
>>>>>>>> On Thu, 12 Dec 2019 06:53:42 +0000, Parav Pandit wrote:
>>>>>>>>>>      static void ionic_remove(struct pci_dev *pdev)
>>>>>>>>>>      {
>>>>>>>>>>          struct ionic *ionic = pci_get_drvdata(pdev);
>>>>>>>>>> @@ -257,6 +338,9 @@ static void ionic_remove(struct pci_dev *pdev)
>>>>>>>>>>          if (!ionic)
>>>>>>>>>>              return;
>>>>>>>>>>      +    if (pci_num_vf(pdev))
>>>>>>>>>> +        ionic_sriov_configure(pdev, 0);
>>>>>>>>>> +
>>>>>>>>> Usually sriov is left enabled while removing PF.
>>>>>>>>> It is not the role of the pci PF removal to disable it sriov.
>>>>>>>> I don't think that's true. I consider igb and ixgbe to set the
>>>>>>>> standard
>>>>>>>> for legacy SR-IOV handling since they were one of the first (the
>>>>>>>> first?)
>>>>>>>> and Alex Duyck wrote them.
>>>>>>>>
>>>>>>>> mlx4, bnxt and nfp all disable SR-IOV on remove.
>>>>>>> This was my understanding as well, but now I can see that ixgbe and
>>>>>>> i40e
>>>>>>> are both checking for existing VFs in probe and setting up to use
>>>>>>> them,
>>>>>>> as well as the newer ice driver.  I found this today by looking for
>>>>>>> where they use pci_num_vf().
>>>>>> Right, if the VFs very already enabled on probe they are set up.
>>>>>>
>>>>>> It's a bit of a asymmetric design, in case some other driver left
>>>>>> SR-IOV on, I guess.
>>>>>>
>>>>> I remember on one email thread on netdev list from someone that in one
>>>>> use case, they upgrade the PF driver while VFs are still bound and
>>>>> SR-IOV kept enabled.
>>>>> I am not sure how much it is used in practice/or practical.
>>>>> Such use case may be the reason to keep SR-IOV enabled.
>>>> This brings up a potential corner case where it would be better for the
>>>> driver to use its own num_vfs value rather than relying on the
>>>> pci_num_vf() when answering the ndo_get_vf_*() callbacks, and at least
>>>> the igb may be susceptible.
>>> Please do not cache num_vfs in driver. Use the pci core's pci_num_vf()
>>> in the new code that you are adding.
>> I disagree.  The pci_num_vf() tells us what the kernel has set up for
>> VFs running, while the driver's num_vfs tracks how many resources the
>> driver has set up for handling VFs: these are two different numbers, and
>> there are times in the life of the driver when these numbers are
>> different.  Yes, these are small windows of time, but they are different
>> and need to be treated differently.
>>
> They shouldn't be different. Why are they different?

One simple case where they are different is when .sriov_config is first 
called and the driver hasn't set anything up for handling the VFs.  Once 
the driver has set up whatever resources it needs, then they definitely 
should be the same.  This works in reverse when tearing down the VFs: 
pci_num_vf() says 0, but the driver thinks it has N VFs configured until 
it has torn down its resources and decremented its own counter.  I'm 
sure there are some drivers that don't need to allocate any resources, 
but if they do, these need to be tracked for tearing down later.

In this ionic driver, after working through this and fixing all the 
places where ionic->num_vfs can/should be replaced by pci_num_vf(), I'm 
still left with this issue: since pci_num_vf() returns 0 when 
.sriov_configure() is called when disabling sr-iov, the driver doesn't 
know how many ionic->vf[] had been set up earlier and can't properly 
unmap the DMA stats memory set up for each VF.

I think the driver needs to keep its own resource count.

sln