Message-ID: <ZrX8IDdyfCrXaDv0@nanopsycho.orion>
Date: Fri, 9 Aug 2024 13:23:12 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Tony Nguyen <anthony.l.nguyen@...el.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
	edumazet@...gle.com, netdev@...r.kernel.org,
	Piotr Raczynski <piotr.raczynski@...el.com>,
	michal.swiatkowski@...ux.intel.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

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() :)

>+
>+	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...


>+	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