Message-ID: <ZMt0D+T/FqQhhO4v@nanopsycho>
Date: Thu, 3 Aug 2023 11:31:59 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Jinjian Song <songjinjian@...mail.com>
Cc: netdev@...r.kernel.org, kuba@...nel.org, davem@...emloft.net,
	johannes@...solutions.net, ryazanov.s.a@...il.com,
	loic.poulain@...aro.org, ilpo.jarvinen@...ux.intel.com,
	ricardo.martinez@...ux.intel.com,
	chiranjeevi.rapolu@...ux.intel.com, haijun.liu@...iatek.com,
	edumazet@...gle.com, pabeni@...hat.com,
	chandrashekar.devegowda@...el.com, m.chetan.kumar@...ux.intel.com,
	linuxwwan@...el.com, linuxwwan_5g@...el.com,
	soumya.prakash.mishra@...el.com, jesse.brandeburg@...el.com,
	danielwinkler@...gle.com, Jinjian Song <jinjian.song@...ocom.com>
Subject: Re: [net-next 2/6] net: wwan: t7xx: Driver registers with Devlink
 framework

ehu, Aug 03, 2023 at 04:18:08AM CEST, songjinjian@...mail.com wrote:
>From: Jinjian Song <jinjian.song@...ocom.com>
>
>Adds support for t7xx wwan device firmware flashing using devlink.
>
>On early detection of wwan device in fastboot mode driver sets up CLDMA0 HW
>tx/rx queues for raw data transfer and then registers to devlink framework.
>
>Base on the v5 patch version of follow series:
>'net: wwan: t7xx: fw flashing & coredump support'
>(https://patchwork.kernel.org/project/netdevbpf/patch/fc8bbb0b66a5ff3a489ea9857d79b374508090ef.1674307425.git.m.chetan.kumar@linux.intel.com/)
>
>Signed-off-by: Jinjian Song <jinjian.song@...ocom.com>
>---
> drivers/net/wwan/Kconfig                  |   1 +
> drivers/net/wwan/t7xx/Makefile            |   3 +-
> drivers/net/wwan/t7xx/t7xx_pci.c          |  15 ++-
> drivers/net/wwan/t7xx/t7xx_pci.h          |   2 +
> drivers/net/wwan/t7xx/t7xx_port_devlink.c | 149 ++++++++++++++++++++++
> drivers/net/wwan/t7xx/t7xx_port_devlink.h |  29 +++++
> drivers/net/wwan/t7xx/t7xx_port_proxy.c   |  20 +++
> drivers/net/wwan/t7xx/t7xx_port_proxy.h   |   3 +
> 8 files changed, 218 insertions(+), 4 deletions(-)
> create mode 100644 drivers/net/wwan/t7xx/t7xx_port_devlink.c
> create mode 100644 drivers/net/wwan/t7xx/t7xx_port_devlink.h
>
>diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
>index 410b0245114e..dd7a9883c1ff 100644
>--- a/drivers/net/wwan/Kconfig
>+++ b/drivers/net/wwan/Kconfig
>@@ -108,6 +108,7 @@ config IOSM
> config MTK_T7XX
> 	tristate "MediaTek PCIe 5G WWAN modem T7xx device"
> 	depends on PCI
>+	select NET_DEVLINK
> 	select RELAY if WWAN_DEBUGFS
> 	help
> 	  Enables MediaTek PCIe based 5G WWAN modem (T7xx series) device.
>diff --git a/drivers/net/wwan/t7xx/Makefile b/drivers/net/wwan/t7xx/Makefile
>index 2652cd00504e..bb28e03eea68 100644
>--- a/drivers/net/wwan/t7xx/Makefile
>+++ b/drivers/net/wwan/t7xx/Makefile
>@@ -15,7 +15,8 @@ mtk_t7xx-y:=	t7xx_pci.o \
> 		t7xx_hif_dpmaif_tx.o \
> 		t7xx_hif_dpmaif_rx.o  \
> 		t7xx_dpmaif.o \
>-		t7xx_netdev.o
>+		t7xx_netdev.o \
>+		t7xx_port_devlink.o
> 
> mtk_t7xx-$(CONFIG_WWAN_DEBUGFS) += \
> 		t7xx_port_trace.o \
>diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c b/drivers/net/wwan/t7xx/t7xx_pci.c
>index 91256e005b84..831819267989 100644
>--- a/drivers/net/wwan/t7xx/t7xx_pci.c
>+++ b/drivers/net/wwan/t7xx/t7xx_pci.c
>@@ -39,6 +39,7 @@
> #include "t7xx_modem_ops.h"
> #include "t7xx_pci.h"
> #include "t7xx_pcie_mac.h"
>+#include "t7xx_port_devlink.h"
> #include "t7xx_reg.h"
> #include "t7xx_state_monitor.h"
> 
>@@ -108,7 +109,7 @@ static int t7xx_pci_pm_init(struct t7xx_pci_dev *t7xx_dev)
> 	pm_runtime_set_autosuspend_delay(&pdev->dev, PM_AUTOSUSPEND_MS);
> 	pm_runtime_use_autosuspend(&pdev->dev);
> 
>-	return t7xx_wait_pm_config(t7xx_dev);
>+	return 0;
> }
> 
> void t7xx_pci_pm_init_late(struct t7xx_pci_dev *t7xx_dev)
>@@ -723,22 +724,30 @@ static int t7xx_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> 	t7xx_pci_infracfg_ao_calc(t7xx_dev);
> 	t7xx_mhccif_init(t7xx_dev);
> 
>-	ret = t7xx_md_init(t7xx_dev);
>+	ret = t7xx_devlink_register(t7xx_dev);
> 	if (ret)
> 		return ret;
> 
>+	ret = t7xx_md_init(t7xx_dev);
>+	if (ret)
>+		goto err_devlink_unregister;
>+
> 	t7xx_pcie_mac_interrupts_dis(t7xx_dev);
> 
> 	ret = t7xx_interrupt_init(t7xx_dev);
> 	if (ret) {
> 		t7xx_md_exit(t7xx_dev);
>-		return ret;
>+		goto err_devlink_unregister;
> 	}
> 
> 	t7xx_pcie_mac_set_int(t7xx_dev, MHCCIF_INT);
> 	t7xx_pcie_mac_interrupts_en(t7xx_dev);
> 
> 	return 0;
>+
>+err_devlink_unregister:
>+	t7xx_devlink_unregister(t7xx_dev);
>+	return ret;
> }
> 
> static void t7xx_pci_remove(struct pci_dev *pdev)
>diff --git a/drivers/net/wwan/t7xx/t7xx_pci.h b/drivers/net/wwan/t7xx/t7xx_pci.h
>index f08f1ab74469..6777581cd540 100644
>--- a/drivers/net/wwan/t7xx/t7xx_pci.h
>+++ b/drivers/net/wwan/t7xx/t7xx_pci.h
>@@ -59,6 +59,7 @@ typedef irqreturn_t (*t7xx_intr_callback)(int irq, void *param);
>  * @md_pm_lock: protects PCIe sleep lock
>  * @sleep_disable_count: PCIe L1.2 lock counter
>  * @sleep_lock_acquire: indicates that sleep has been disabled
>+ * @dl: devlink struct
>  */
> struct t7xx_pci_dev {
> 	t7xx_intr_callback	intr_handler[EXT_INT_NUM];
>@@ -82,6 +83,7 @@ struct t7xx_pci_dev {
> #ifdef CONFIG_WWAN_DEBUGFS
> 	struct dentry		*debugfs_dir;
> #endif
>+	struct t7xx_devlink	*dl;
> };
> 
> enum t7xx_pm_id {
>diff --git a/drivers/net/wwan/t7xx/t7xx_port_devlink.c b/drivers/net/wwan/t7xx/t7xx_port_devlink.c
>new file mode 100644
>index 000000000000..9c09464b28ee
>--- /dev/null
>+++ b/drivers/net/wwan/t7xx/t7xx_port_devlink.c
>@@ -0,0 +1,149 @@
>+// SPDX-License-Identifier: GPL-2.0-only
>+/*
>+ * Copyright (c) 2022-2023, Intel Corporation.
>+ */
>+
>+#include <linux/vmalloc.h>
>+
>+#include "t7xx_port_proxy.h"
>+#include "t7xx_port_devlink.h"
>+
>+static int t7xx_devlink_flash_update(struct devlink *devlink,
>+				     struct devlink_flash_update_params *params,
>+				     struct netlink_ext_ack *extack)
>+{
>+	return 0;
>+}
>+
>+enum t7xx_devlink_param_id {
>+	T7XX_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
>+	T7XX_DEVLINK_PARAM_ID_FASTBOOT,
>+};
>+
>+static const struct devlink_param t7xx_devlink_params[] = {
>+	DEVLINK_PARAM_DRIVER(T7XX_DEVLINK_PARAM_ID_FASTBOOT,
>+			     "fastboot", DEVLINK_PARAM_TYPE_BOOL,
>+			     BIT(DEVLINK_PARAM_CMODE_DRIVERINIT),
>+			     NULL, NULL, NULL),
>+};

Please have the param introduction in a separate file.

Just to mention it right here, this param smells very oddly to me.


>+
>+bool t7xx_devlink_param_get_fastboot(struct devlink *devlink)
>+{
>+	union devlink_param_value saved_value;
>+
>+	devl_param_driverinit_value_get(devlink, T7XX_DEVLINK_PARAM_ID_FASTBOOT,
>+					&saved_value);
>+	return saved_value.vbool;
>+}
>+
>+static int t7xx_devlink_reload_down(struct devlink *devlink, bool netns_change,
>+				    enum devlink_reload_action action,
>+				    enum devlink_reload_limit limit,
>+				    struct netlink_ext_ack *extack)
>+{
>+	return 0;
>+}
>+
>+static int t7xx_devlink_reload_up(struct devlink *devlink,
>+				  enum devlink_reload_action action,
>+				  enum devlink_reload_limit limit,
>+				  u32 *actions_performed,
>+				  struct netlink_ext_ack *extack)
>+{
>+	return 0;
>+}
>+
>+static int t7xx_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
>+				 struct netlink_ext_ack *extack)
>+{
>+	return 0;
>+}

Don't put the stub functions here. Introduce them when you need them.


>+
>+/* Call back function for devlink ops */
>+static const struct devlink_ops devlink_flash_ops = {
>+	.supported_flash_update_params = DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK,
>+	.flash_update = t7xx_devlink_flash_update,
>+	.reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT) |
>+			  BIT(DEVLINK_RELOAD_ACTION_FW_ACTIVATE),
>+	.info_get = t7xx_devlink_info_get,
>+	.reload_down = t7xx_devlink_reload_down,
>+	.reload_up = t7xx_devlink_reload_up,
>+};
>+
>+int t7xx_devlink_register(struct t7xx_pci_dev *t7xx_dev)
>+{
>+	union devlink_param_value value;
>+	struct devlink *dl_ctx;
>+
>+	dl_ctx = devlink_alloc(&devlink_flash_ops, sizeof(struct t7xx_devlink),
>+			       &t7xx_dev->pdev->dev);
>+	if (!dl_ctx)
>+		return -ENOMEM;
>+
>+	t7xx_dev->dl = devlink_priv(dl_ctx);
>+	t7xx_dev->dl->ctx = dl_ctx;
>+	t7xx_dev->dl->t7xx_dev = t7xx_dev;
>+
>+	devl_lock(dl_ctx);
>+	devl_params_register(dl_ctx, t7xx_devlink_params, ARRAY_SIZE(t7xx_devlink_params));
>+	value.vbool = false;
>+	devl_param_driverinit_value_set(dl_ctx, T7XX_DEVLINK_PARAM_ID_FASTBOOT, value);
>+	devl_register(dl_ctx);
>+	devl_unlock(dl_ctx);
>+
>+	return 0;
>+}
>+
>+void t7xx_devlink_unregister(struct t7xx_pci_dev *t7xx_dev)
>+{
>+	struct devlink *dl_ctx = t7xx_dev->dl->ctx;
>+
>+	devl_lock(dl_ctx);
>+	devl_unregister(dl_ctx);
>+	devl_params_unregister(dl_ctx, t7xx_devlink_params, ARRAY_SIZE(t7xx_devlink_params));
>+	devl_unlock(dl_ctx);
>+	devlink_free(dl_ctx);
>+}
>+
>+/**
>+ * t7xx_devlink_init - Initialize devlink to t7xx driver
>+ * @port: Pointer to port structure
>+ *
>+ * Returns: 0 on success and error values on failure
>+ */
>+static int t7xx_devlink_init(struct t7xx_port *port)
>+{
>+	struct t7xx_devlink *dl = port->t7xx_dev->dl;
>+
>+	port->rx_length_th = T7XX_MAX_QUEUE_LENGTH;
>+
>+	dl->mode = T7XX_NORMAL_MODE;
>+	dl->status = T7XX_DEVLINK_IDLE;

? What is this supposed to mean? Looks quite wrong.


>+	dl->port = port;
>+
>+	return 0;
>+}
>+
>+static void t7xx_devlink_uninit(struct t7xx_port *port)
>+{
>+	struct t7xx_devlink *dl = port->t7xx_dev->dl;
>+
>+	dl->mode = T7XX_NORMAL_MODE;
>+
>+	skb_queue_purge(&port->rx_skb_list);
>+}
>+
>+static int t7xx_devlink_enable_chl(struct t7xx_port *port)
>+{
>+	t7xx_port_enable_chl(port);
>+
>+	return 0;
>+}
>+
>+struct port_ops devlink_port_ops = {

Could you reneame this to me less confusing? Looks like this should be
related to devlink_port, but actually it is not.

Could you please describe what exatly is the "port" entity for your
driver? What it represents?


>+	.init = &t7xx_devlink_init,
>+	.recv_skb = &t7xx_port_enqueue_skb,
>+	.uninit = &t7xx_devlink_uninit,
>+	.enable_chl = &t7xx_devlink_enable_chl,
>+	.disable_chl = &t7xx_port_disable_chl,
>+};
>diff --git a/drivers/net/wwan/t7xx/t7xx_port_devlink.h b/drivers/net/wwan/t7xx/t7xx_port_devlink.h
>new file mode 100644
>index 000000000000..12e5a63203af
>--- /dev/null
>+++ b/drivers/net/wwan/t7xx/t7xx_port_devlink.h
>@@ -0,0 +1,29 @@
>+/* SPDX-License-Identifier: GPL-2.0-only
>+ *
>+ * Copyright (c) 2022-2023, Intel Corporation.
>+ */
>+
>+#ifndef __T7XX_PORT_DEVLINK_H__
>+#define __T7XX_PORT_DEVLINK_H__
>+
>+#include <net/devlink.h>
>+#include <linux/string.h>
>+
>+#define T7XX_MAX_QUEUE_LENGTH 32
>+
>+#define T7XX_DEVLINK_IDLE   0
>+#define T7XX_NORMAL_MODE    0
>+
>+struct t7xx_devlink {
>+	struct t7xx_pci_dev *t7xx_dev;
>+	struct t7xx_port *port;
>+	struct devlink *ctx;
>+	unsigned long status;
>+	u8 mode;
>+};
>+
>+bool t7xx_devlink_param_get_fastboot(struct devlink *devlink);
>+int t7xx_devlink_register(struct t7xx_pci_dev *t7xx_dev);
>+void t7xx_devlink_unregister(struct t7xx_pci_dev *t7xx_dev);
>+
>+#endif /*__T7XX_PORT_DEVLINK_H__*/
>diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>index bdfeb10e0c51..f185a7fb0265 100644
>--- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>+++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>@@ -109,6 +109,8 @@ static struct t7xx_port_conf t7xx_early_port_conf[] = {
> 		.txq_exp_index = CLDMA_Q_IDX_DUMP,
> 		.rxq_exp_index = CLDMA_Q_IDX_DUMP,
> 		.path_id = CLDMA_ID_AP,
>+		.ops = &devlink_port_ops,
>+		.name = "devlink",
> 	},
> };
> 
>@@ -325,6 +327,24 @@ int t7xx_port_send_skb(struct t7xx_port *port, struct sk_buff *skb, unsigned int
> 	return t7xx_port_send_ccci_skb(port, skb, pkt_header, ex_msg);
> }
> 
>+int t7xx_port_enable_chl(struct t7xx_port *port)
>+{
>+	spin_lock(&port->port_update_lock);
>+	port->chan_enable = true;
>+	spin_unlock(&port->port_update_lock);
>+
>+	return 0;
>+}
>+
>+int t7xx_port_disable_chl(struct t7xx_port *port)
>+{
>+	spin_lock(&port->port_update_lock);
>+	port->chan_enable = false;
>+	spin_unlock(&port->port_update_lock);
>+
>+	return 0;
>+}
>+
> static void t7xx_proxy_setup_ch_mapping(struct port_proxy *port_prox)
> {
> 	struct t7xx_port *port;
>diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.h b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
>index 7f5706811445..c4cd1078ee92 100644
>--- a/drivers/net/wwan/t7xx/t7xx_port_proxy.h
>+++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
>@@ -93,6 +93,7 @@ struct ctrl_msg_header {
> /* Port operations mapping */
> extern struct port_ops wwan_sub_port_ops;
> extern struct port_ops ctl_port_ops;
>+extern struct port_ops devlink_port_ops;
> 
> #ifdef CONFIG_WWAN_DEBUGFS
> extern struct port_ops t7xx_trace_port_ops;
>@@ -108,5 +109,7 @@ int t7xx_port_proxy_chl_enable_disable(struct port_proxy *port_prox, unsigned in
> void t7xx_port_proxy_set_cfg(struct t7xx_modem *md, enum port_cfg_id cfg_id);
> int t7xx_port_proxy_recv_skb(struct cldma_queue *queue, struct sk_buff *skb);
> int t7xx_port_proxy_recv_skb_from_dedicated_queue(struct cldma_queue *queue, struct sk_buff *skb);
>+int t7xx_port_enable_chl(struct t7xx_port *port);
>+int t7xx_port_disable_chl(struct t7xx_port *port);
> 
> #endif /* __T7XX_PORT_PROXY_H__ */
>-- 
>2.34.1
>
>

Powered by blists - more mailing lists