Message-ID: <ZrYCLr368aBxJBfW@mev-dev.igk.intel.com>
Date: Fri, 9 Aug 2024 13:49:02 +0200
From: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
To: Jiri Pirko <jiri@...nulli.us>
Cc: Tony Nguyen <anthony.l.nguyen@...el.com>, davem@...emloft.net,
	kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com,
	netdev@...r.kernel.org, Piotr Raczynski <piotr.raczynski@...el.com>,
	jiri@...dia.com, shayd@...dia.com, wojciech.drewek@...el.com,
	horms@...nel.org, sridhar.samudrala@...el.com,
	mateusz.polchlopek@...el.com, kalesh-anakkur.purayil@...adcom.com,
	michal.kubiak@...el.com, pio.raczynski@...il.com,
	przemyslaw.kitszel@...el.com, jacob.e.keller@...el.com,
	maciej.fijalkowski@...el.com,
	Rafal Romanowski <rafal.romanowski@...el.com>
Subject: Re: [PATCH net-next v3 06/15] ice: base subfunction aux driver

On Fri, Aug 09, 2024 at 01:23:12PM +0200, Jiri Pirko wrote:
> Thu, Aug 08, 2024 at 07:30:52PM CEST, anthony.l.nguyen@...el.com wrote:
> >From: Piotr Raczynski <piotr.raczynski@...el.com>
> >
> >Implement subfunction driver. It is probe when subfunction port is
> >activated.
> >
> >VSI is already created. During the probe VSI is being configured.
> >MAC unicast and broadcast filter is added to allow traffic to pass.
> >
> >Store subfunction pointer in VSI struct. The same is done for VF
> >pointer. Make union of subfunction and VF pointer as only one of them
> >can be set with one VSI.
> >
> >Reviewed-by: Simon Horman <horms@...nel.org>
> >Reviewed-by: Jiri Pirko <jiri@...dia.com>
> >Signed-off-by: Piotr Raczynski <piotr.raczynski@...el.com>
> >Signed-off-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
> >Tested-by: Rafal Romanowski <rafal.romanowski@...el.com>
> >Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
> >---
> > drivers/net/ethernet/intel/ice/Makefile       |   1 +
> > .../ethernet/intel/ice/devlink/devlink_port.c |  41 ++++++
> > .../ethernet/intel/ice/devlink/devlink_port.h |   3 +
> > drivers/net/ethernet/intel/ice/ice.h          |   7 +-
> > drivers/net/ethernet/intel/ice/ice_main.c     |  10 ++
> > drivers/net/ethernet/intel/ice/ice_sf_eth.c   | 139 ++++++++++++++++++
> > drivers/net/ethernet/intel/ice/ice_sf_eth.h   |  15 ++
> > 7 files changed, 215 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/net/ethernet/intel/ice/ice_sf_eth.c
> >
> >diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
> >index 03500e28ac99..4d987f5dcdc1 100644
> >--- a/drivers/net/ethernet/intel/ice/Makefile
> >+++ b/drivers/net/ethernet/intel/ice/Makefile
> >@@ -31,6 +31,7 @@ ice-y := ice_main.o	\
> > 	 ice_idc.o	\
> > 	 devlink/devlink.o	\
> > 	 devlink/devlink_port.o \
> >+	 ice_sf_eth.o	\
> > 	 ice_ddp.o	\
> > 	 ice_fw_update.o \
> > 	 ice_lag.o	\
> >diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink_port.c b/drivers/net/ethernet/intel/ice/devlink/devlink_port.c
> >index 5d1fe08e4bab..f06baabd0112 100644
> >--- a/drivers/net/ethernet/intel/ice/devlink/devlink_port.c
> >+++ b/drivers/net/ethernet/intel/ice/devlink/devlink_port.c
> >@@ -489,6 +489,47 @@ void ice_devlink_destroy_vf_port(struct ice_vf *vf)
> > 	devl_port_unregister(&vf->devlink_port);
> > }
> > 
> >+/**
> >+ * ice_devlink_create_sf_dev_port - Register virtual port for a subfunction
> >+ * @sf_dev: the subfunction device to create a devlink port for
> >+ *
> >+ * Register virtual flavour devlink port for the subfunction auxiliary device
> >+ * created after activating a dynamically added devlink port.
> >+ *
> >+ * Return: zero on success or an error code on failure.
> >+ */
> >+int ice_devlink_create_sf_dev_port(struct ice_sf_dev *sf_dev)
> >+{
> >+	struct devlink_port_attrs attrs = {};
> >+	struct ice_dynamic_port *dyn_port;
> >+	struct devlink_port *devlink_port;
> >+	struct devlink *devlink;
> >+	struct ice_vsi *vsi;
> >+
> >+	dyn_port = sf_dev->dyn_port;
> >+	vsi = dyn_port->vsi;
> >+
> >+	devlink_port = &sf_dev->priv->devlink_port;
> >+
> >+	attrs.flavour = DEVLINK_PORT_FLAVOUR_VIRTUAL;
> >+
> >+	devlink_port_attrs_set(devlink_port, &attrs);
> >+	devlink = priv_to_devlink(sf_dev->priv);
> >+
> >+	return devl_port_register(devlink, devlink_port, vsi->idx);
> >+}
> >+
> >+/**
> >+ * ice_devlink_destroy_sf_dev_port - Destroy virtual port for a subfunction
> >+ * @sf_dev: the subfunction device to create a devlink port for
> >+ *
> >+ * Unregisters the virtual port associated with this subfunction.
> >+ */
> >+void ice_devlink_destroy_sf_dev_port(struct ice_sf_dev *sf_dev)
> >+{
> >+	devl_port_unregister(&sf_dev->priv->devlink_port);
> >+}
> >+
> > /**
> >  * ice_dealloc_dynamic_port - Deallocate and remove a dynamic port
> >  * @dyn_port: dynamic port instance to deallocate
> >diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink_port.h b/drivers/net/ethernet/intel/ice/devlink/devlink_port.h
> >index 08ebf56664a5..97b21b58c300 100644
> >--- a/drivers/net/ethernet/intel/ice/devlink/devlink_port.h
> >+++ b/drivers/net/ethernet/intel/ice/devlink/devlink_port.h
> >@@ -5,6 +5,7 @@
> > #define _DEVLINK_PORT_H_
> > 
> > #include "../ice.h"
> >+#include "../ice_sf_eth.h"
> > 
> > /**
> >  * struct ice_dynamic_port - Track dynamically added devlink port instance
> >@@ -34,6 +35,8 @@ int ice_devlink_create_vf_port(struct ice_vf *vf);
> > void ice_devlink_destroy_vf_port(struct ice_vf *vf);
> > int ice_devlink_create_sf_port(struct ice_dynamic_port *dyn_port);
> > void ice_devlink_destroy_sf_port(struct ice_dynamic_port *dyn_port);
> >+int ice_devlink_create_sf_dev_port(struct ice_sf_dev *sf_dev);
> >+void ice_devlink_destroy_sf_dev_port(struct ice_sf_dev *sf_dev);
> > 
> > #define ice_devlink_port_to_dyn(port) \
> > 	container_of(port, struct ice_dynamic_port, devlink_port)
> >diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> >index d61f8e602cac..0046684004ff 100644
> >--- a/drivers/net/ethernet/intel/ice/ice.h
> >+++ b/drivers/net/ethernet/intel/ice/ice.h
> >@@ -449,7 +449,12 @@ struct ice_vsi {
> > 	struct_group_tagged(ice_vsi_cfg_params, params,
> > 		struct ice_port_info *port_info; /* back pointer to port_info */
> > 		struct ice_channel *ch; /* VSI's channel structure, may be NULL */
> >-		struct ice_vf *vf; /* VF associated with this VSI, may be NULL */
> >+		union {
> >+			/* VF associated with this VSI, may be NULL */
> >+			struct ice_vf *vf;
> >+			/* SF associated with this VSI, may be NULL */
> >+			struct ice_dynamic_port *sf;
> >+		};
> > 		u32 flags; /* VSI flags used for rebuild and configuration */
> > 		enum ice_vsi_type type; /* the type of the VSI */
> > 	);
> >diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> >index e10f10a21c95..bdd88617ec9b 100644
> >--- a/drivers/net/ethernet/intel/ice/ice_main.c
> >+++ b/drivers/net/ethernet/intel/ice/ice_main.c
> >@@ -15,6 +15,7 @@
> > #include "ice_dcb_nl.h"
> > #include "devlink/devlink.h"
> > #include "devlink/devlink_port.h"
> >+#include "ice_sf_eth.h"
> > #include "ice_hwmon.h"
> > /* Including ice_trace.h with CREATE_TRACE_POINTS defined will generate the
> >  * ice tracepoint functions. This must be done exactly once across the
> >@@ -5908,8 +5909,16 @@ static int __init ice_module_init(void)
> > 		goto err_dest_lag_wq;
> > 	}
> > 
> >+	status = ice_sf_driver_register();
> >+	if (status) {
> >+		pr_err("Failed to register SF driver, err %d\n", status);
> >+		goto err_sf_driver;
> >+	}
> >+
> > 	return 0;
> > 
> >+err_sf_driver:
> >+	pci_unregister_driver(&ice_driver);
> > err_dest_lag_wq:
> > 	destroy_workqueue(ice_lag_wq);
> > 	ice_debugfs_exit();
> >@@ -5927,6 +5936,7 @@ module_init(ice_module_init);
> >  */
> > static void __exit ice_module_exit(void)
> > {
> >+	ice_sf_driver_unregister();
> > 	pci_unregister_driver(&ice_driver);
> > 	ice_debugfs_exit();
> > 	destroy_workqueue(ice_wq);
> >diff --git a/drivers/net/ethernet/intel/ice/ice_sf_eth.c b/drivers/net/ethernet/intel/ice/ice_sf_eth.c
> >new file mode 100644
> >index 000000000000..abe495c2d033
> >--- /dev/null
> >+++ b/drivers/net/ethernet/intel/ice/ice_sf_eth.c
> >@@ -0,0 +1,139 @@
> >+// SPDX-License-Identifier: GPL-2.0
> >+/* Copyright (c) 2024, Intel Corporation. */
> >+#include "ice.h"
> >+#include "ice_lib.h"
> >+#include "ice_fltr.h"
> >+#include "ice_sf_eth.h"
> >+#include "devlink/devlink_port.h"
> >+#include "devlink/devlink.h"
> >+
> >+/**
> >+ * ice_sf_dev_probe - subfunction driver probe function
> >+ * @adev: pointer to the auxiliary device
> >+ * @id: pointer to the auxiliary_device id
> >+ *
> >+ * Configure VSI and netdev resources for the subfunction device.
> >+ *
> >+ * Return: zero on success or an error code on failure.
> >+ */
> >+static int ice_sf_dev_probe(struct auxiliary_device *adev,
> >+			    const struct auxiliary_device_id *id)
> >+{
> >+	struct ice_sf_dev *sf_dev = ice_adev_to_sf_dev(adev);
> >+	struct ice_dynamic_port *dyn_port = sf_dev->dyn_port;
> >+	struct ice_vsi *vsi = dyn_port->vsi;
> >+	struct ice_pf *pf = dyn_port->pf;
> >+	struct device *dev = &adev->dev;
> >+	struct ice_sf_priv *priv;
> >+	struct devlink *devlink;
> >+	int err;
> >+
> >+	vsi->type = ICE_VSI_SF;
> >+	vsi->port_info = pf->hw.port_info;
> >+	vsi->flags = ICE_VSI_FLAG_INIT;
> >+
> >+	priv = ice_allocate_sf(&adev->dev, pf);
> >+	if (!priv) {
> >+		dev_err(dev, "Subfunction devlink alloc failed");
> >+		return -ENOMEM;
> >+	}
> >+
> >+	priv->dev = sf_dev;
> >+	sf_dev->priv = priv;
> >+	devlink = priv_to_devlink(priv);
> >+
> >+	devl_lock(devlink);
> >+
> >+	err = ice_vsi_cfg(vsi);
> >+	if (err) {
> >+		dev_err(dev, "Subfunction vsi config failed");
> >+		goto err_free_devlink;
> >+	}
> >+	vsi->sf = dyn_port;
> >+
> >+	err = ice_devlink_create_sf_dev_port(sf_dev);
> >+	if (err) {
> >+		dev_err(dev, "Cannot add ice virtual devlink port for subfunction");
> >+		goto err_vsi_decfg;
> >+	}
> >+
> >+	err = devl_port_fn_devlink_set(&dyn_port->devlink_port, devlink);
> >+	if (err) {
> >+		dev_err(dev, "Can't link devlink instance to SF devlink port");
> >+		goto err_devlink_destroy;
> >+	}
> >+
> >+	ice_napi_add(vsi);
> >+	devl_unlock(devlink);
> >+
> >+	devlink_register(devlink);
> 
> Use devl_register() and do it before you call devl_unlock() :)
> 

Ok, I will

> >+
> >+	return 0;
> >+
> >+err_devlink_destroy:
> >+	ice_devlink_destroy_sf_dev_port(sf_dev);
> >+err_vsi_decfg:
> >+	ice_vsi_decfg(vsi);
> >+err_free_devlink:
> >+	devl_unlock(devlink);
> >+	devlink_free(devlink);
> >+	return err;
> >+}
> >+
> >+/**
> >+ * ice_sf_dev_remove - subfunction driver remove function
> >+ * @adev: pointer to the auxiliary device
> >+ *
> >+ * Deinitalize VSI and netdev resources for the subfunction device.
> >+ */
> >+static void ice_sf_dev_remove(struct auxiliary_device *adev)
> >+{
> >+	struct ice_sf_dev *sf_dev = ice_adev_to_sf_dev(adev);
> >+	struct ice_dynamic_port *dyn_port = sf_dev->dyn_port;
> >+	struct ice_vsi *vsi = dyn_port->vsi;
> >+	struct devlink *devlink;
> >+
> >+	devlink = priv_to_devlink(sf_dev->priv);
> >+	devl_lock(devlink);
> >+
> >+	ice_vsi_close(vsi);
> >+
> >+	ice_devlink_destroy_sf_dev_port(sf_dev);
> >+	devl_unregister(devlink);
> 
> Interesting, you call devl_unregister() here...

I forgot to move and use devl_register in probe.
> 
> 
> >+	devl_unlock(devlink);
> >+	devlink_free(devlink);
> >+	ice_vsi_decfg(vsi);
> >+}
> >+
> >+static const struct auxiliary_device_id ice_sf_dev_id_table[] = {
> >+	{ .name = "ice.sf", },
> >+	{ },
> >+};
> >+
> >+MODULE_DEVICE_TABLE(auxiliary, ice_sf_dev_id_table);
> >+
> >+static struct auxiliary_driver ice_sf_driver = {
> >+	.name = "sf",
> >+	.probe = ice_sf_dev_probe,
> >+	.remove = ice_sf_dev_remove,
> >+	.id_table = ice_sf_dev_id_table
> >+};
> >+
> >+/**
> >+ * ice_sf_driver_register - Register new auxiliary subfunction driver
> >+ *
> >+ * Return: zero on success or an error code on failure.
> >+ */
> >+int ice_sf_driver_register(void)
> >+{
> >+	return auxiliary_driver_register(&ice_sf_driver);
> >+}
> >+
> >+/**
> >+ * ice_sf_driver_unregister - Unregister new auxiliary subfunction driver
> >+ *
> >+ */
> >+void ice_sf_driver_unregister(void)
> >+{
> >+	auxiliary_driver_unregister(&ice_sf_driver);
> >+}
> >diff --git a/drivers/net/ethernet/intel/ice/ice_sf_eth.h b/drivers/net/ethernet/intel/ice/ice_sf_eth.h
> >index 267da33a0135..e972c50f96c9 100644
> >--- a/drivers/net/ethernet/intel/ice/ice_sf_eth.h
> >+++ b/drivers/net/ethernet/intel/ice/ice_sf_eth.h
> >@@ -7,9 +7,24 @@
> > #include <linux/auxiliary_bus.h>
> > #include "ice.h"
> > 
> >+struct ice_sf_dev {
> >+	struct auxiliary_device adev;
> >+	struct ice_dynamic_port *dyn_port;
> >+	struct ice_sf_priv *priv;
> >+};
> >+
> > struct ice_sf_priv {
> > 	struct ice_sf_dev *dev;
> > 	struct devlink_port devlink_port;
> > };
> > 
> >+static inline struct
> >+ice_sf_dev *ice_adev_to_sf_dev(struct auxiliary_device *adev)
> >+{
> >+	return container_of(adev, struct ice_sf_dev, adev);
> >+}
> >+
> >+int ice_sf_driver_register(void);
> >+void ice_sf_driver_unregister(void);
> >+
> > #endif /* _ICE_SF_ETH_H_ */
> >-- 
> >2.42.0
> >
> >

Powered by blists - more mailing lists