* [PATCH net-next 4/5] net: wwan: t7xx: Enable devlink based fw flashing and coredump collection
@ 2022-08-16 4:24 m.chetan.kumar
2022-08-18 14:33 ` Ilpo Järvinen
2022-08-30 2:21 ` Sergey Ryazanov
0 siblings, 2 replies; 10+ messages in thread
From: m.chetan.kumar @ 2022-08-16 4:24 UTC (permalink / raw)
To: netdev
Cc: kuba, davem, johannes, ryazanov.s.a, loic.poulain,
krishna.c.sudi, m.chetan.kumar, linuxwwan,
Devegowda Chandrashekar, Mishra Soumya Prakash
From: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
This patch brings-in support for t7xx wwan device firmware flashing &
coredump collection using devlink.
Driver Registers with Devlink framework.
Implements devlink ops flash_update callback that programs modem firmware.
Creates region & snapshot required for device coredump log collection.
On early detection of wwan device in fastboot mode driver sets up CLDMA0 HW
tx/rx queues for raw data transfer then registers with devlink framework.
Upon receiving firmware image & partition details driver sends fastboot
commands for flashing the firmware.
In this flow the fastboot command & response gets exchanged between driver
and device. Once firmware flashing is success completion status is reported
to user space application.
Below is the devlink command usage for firmware flashing
$devlink dev flash pci/$BDF file ABC.img component ABC
Note: ABC.img is the firmware to be programmed to "ABC" partition.
In case of coredump collection when wwan device encounters an exception
it reboots & stays in fastboot mode for coredump collection by host driver.
On detecting exception state driver collects the core dump, creates the
devlink region & reports an event to user space application for dump
collection. The user space application invokes devlink region read command
for dump collection.
Below are the devlink commands used for coredump collection.
devlink region new pci/$BDF/mr_dump
devlink region read pci/$BDF/mr_dump snapshot $ID address $ADD length $LEN
devlink region del pci/$BDF/mr_dump snapshot $ID
Signed-off-by: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
Signed-off-by: Devegowda Chandrashekar <chandrashekar.devegowda@intel.com>
Signed-off-by: Mishra Soumya Prakash <soumya.prakash.mishra@intel.com>
---
drivers/net/wwan/Kconfig | 1 +
drivers/net/wwan/t7xx/Makefile | 4 +-
drivers/net/wwan/t7xx/t7xx_pci.c | 14 +-
drivers/net/wwan/t7xx/t7xx_pci.h | 2 +
drivers/net/wwan/t7xx/t7xx_port.h | 1 +
drivers/net/wwan/t7xx/t7xx_port_devlink.c | 705 +++++++++++++++++++++
drivers/net/wwan/t7xx/t7xx_port_devlink.h | 85 +++
drivers/net/wwan/t7xx/t7xx_port_proxy.c | 2 +
drivers/net/wwan/t7xx/t7xx_port_proxy.h | 1 +
drivers/net/wwan/t7xx/t7xx_reg.h | 6 +
drivers/net/wwan/t7xx/t7xx_state_monitor.c | 36 +-
drivers/net/wwan/t7xx/t7xx_uevent.c | 41 ++
drivers/net/wwan/t7xx/t7xx_uevent.h | 39 ++
13 files changed, 932 insertions(+), 5 deletions(-)
create mode 100644 drivers/net/wwan/t7xx/t7xx_port_devlink.c
create mode 100644 drivers/net/wwan/t7xx/t7xx_port_devlink.h
create mode 100644 drivers/net/wwan/t7xx/t7xx_uevent.c
create mode 100644 drivers/net/wwan/t7xx/t7xx_uevent.h
diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
index 3486ffe94ac4..73b8cc1db0bd 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
help
Enables MediaTek PCIe based 5G WWAN modem (T7xx series) device.
Adapts WWAN framework and provides network interface like wwan0
diff --git a/drivers/net/wwan/t7xx/Makefile b/drivers/net/wwan/t7xx/Makefile
index df42764b3df8..91ecabf29dd1 100644
--- a/drivers/net/wwan/t7xx/Makefile
+++ b/drivers/net/wwan/t7xx/Makefile
@@ -18,4 +18,6 @@ mtk_t7xx-y:= t7xx_pci.o \
t7xx_hif_dpmaif_rx.o \
t7xx_dpmaif.o \
t7xx_netdev.o \
- t7xx_pci_rescan.o
+ t7xx_pci_rescan.o \
+ t7xx_uevent.o \
+ t7xx_port_devlink.o
diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c b/drivers/net/wwan/t7xx/t7xx_pci.c
index 2f5c6fbe601e..14cdf00cac8e 100644
--- a/drivers/net/wwan/t7xx/t7xx_pci.c
+++ b/drivers/net/wwan/t7xx/t7xx_pci.c
@@ -40,6 +40,7 @@
#include "t7xx_pci.h"
#include "t7xx_pci_rescan.h"
#include "t7xx_pcie_mac.h"
+#include "t7xx_port_devlink.h"
#include "t7xx_reg.h"
#include "t7xx_state_monitor.h"
@@ -704,16 +705,20 @@ 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_rescan_done();
@@ -723,6 +728,10 @@ static int t7xx_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
pci_ignore_hotplug(pdev);
return 0;
+
+err_devlink_unregister:
+ t7xx_devlink_unregister(t7xx_dev);
+ return ret;
}
static void t7xx_pci_remove(struct pci_dev *pdev)
@@ -732,6 +741,7 @@ static void t7xx_pci_remove(struct pci_dev *pdev)
t7xx_dev = pci_get_drvdata(pdev);
t7xx_md_exit(t7xx_dev);
+ t7xx_devlink_unregister(t7xx_dev);
for (i = 0; i < EXT_INT_NUM; i++) {
if (!t7xx_dev->intr_handler[i])
diff --git a/drivers/net/wwan/t7xx/t7xx_pci.h b/drivers/net/wwan/t7xx/t7xx_pci.h
index a87c4cae94ef..1017d21aad59 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];
@@ -79,6 +80,7 @@ struct t7xx_pci_dev {
spinlock_t md_pm_lock; /* Protects PCI resource lock */
unsigned int sleep_disable_count;
struct completion sleep_lock_acquire;
+ struct t7xx_devlink *dl;
};
enum t7xx_pm_id {
diff --git a/drivers/net/wwan/t7xx/t7xx_port.h b/drivers/net/wwan/t7xx/t7xx_port.h
index 6a96ee6d9449..070097a658d1 100644
--- a/drivers/net/wwan/t7xx/t7xx_port.h
+++ b/drivers/net/wwan/t7xx/t7xx_port.h
@@ -129,6 +129,7 @@ struct t7xx_port {
int rx_length_th;
bool chan_enable;
struct task_struct *thread;
+ struct t7xx_devlink *dl;
};
int t7xx_get_port_mtu(struct t7xx_port *port);
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..026a1db42f69
--- /dev/null
+++ b/drivers/net/wwan/t7xx/t7xx_port_devlink.c
@@ -0,0 +1,705 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022, Intel Corporation.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/debugfs.h>
+#include <linux/vmalloc.h>
+
+#include "t7xx_hif_cldma.h"
+#include "t7xx_pci_rescan.h"
+#include "t7xx_port_devlink.h"
+#include "t7xx_port_proxy.h"
+#include "t7xx_state_monitor.h"
+#include "t7xx_uevent.h"
+
+static struct t7xx_devlink_region_info t7xx_devlink_region_list[T7XX_TOTAL_REGIONS] = {
+ {"mr_dump", T7XX_MRDUMP_SIZE},
+ {"lk_dump", T7XX_LKDUMP_SIZE},
+};
+
+static int t7xx_devlink_port_read(struct t7xx_port *port, char *buf, size_t count)
+{
+ int ret = 0, read_len;
+ struct sk_buff *skb;
+
+ spin_lock_irq(&port->rx_wq.lock);
+ if (skb_queue_empty(&port->rx_skb_list)) {
+ ret = wait_event_interruptible_locked_irq(port->rx_wq,
+ !skb_queue_empty(&port->rx_skb_list));
+ if (ret == -ERESTARTSYS) {
+ spin_unlock_irq(&port->rx_wq.lock);
+ return -EINTR;
+ }
+ }
+ skb = skb_dequeue(&port->rx_skb_list);
+ spin_unlock_irq(&port->rx_wq.lock);
+
+ read_len = count > skb->len ? skb->len : count;
+ memcpy(buf, skb->data, read_len);
+ dev_kfree_skb(skb);
+
+ return ret ? ret : read_len;
+}
+
+static int t7xx_devlink_port_write(struct t7xx_port *port, const char *buf, size_t count)
+{
+ const struct t7xx_port_conf *port_conf = port->port_conf;
+ size_t actual_count;
+ struct sk_buff *skb;
+ int ret, txq_mtu;
+
+ txq_mtu = t7xx_get_port_mtu(port);
+ if (txq_mtu < 0)
+ return -EINVAL;
+
+ actual_count = count > txq_mtu ? txq_mtu : count;
+ skb = __dev_alloc_skb(actual_count, GFP_KERNEL);
+ if (!skb)
+ return -ENOMEM;
+
+ skb_put_data(skb, buf, actual_count);
+ ret = t7xx_port_send_raw_skb(port, skb);
+ if (ret) {
+ dev_err(port->dev, "write error on %s, size: %zu, ret: %d\n",
+ port_conf->name, actual_count, ret);
+ dev_kfree_skb(skb);
+ return ret;
+ }
+
+ return actual_count;
+}
+
+static int t7xx_devlink_fb_handle_response(struct t7xx_port *port, int *data)
+{
+ int ret = 0, index = 0, return_data = 0, read_bytes;
+ char status[T7XX_FB_RESPONSE_SIZE + 1];
+
+ while (index < T7XX_FB_RESP_COUNT) {
+ index++;
+ read_bytes = t7xx_devlink_port_read(port, status, T7XX_FB_RESPONSE_SIZE);
+ if (read_bytes < 0) {
+ dev_err(port->dev, "status read failed");
+ ret = -EIO;
+ break;
+ }
+
+ status[read_bytes] = '\0';
+ if (!strncmp(status, T7XX_FB_RESP_INFO, strlen(T7XX_FB_RESP_INFO))) {
+ break;
+ } else if (!strncmp(status, T7XX_FB_RESP_OKAY, strlen(T7XX_FB_RESP_OKAY))) {
+ break;
+ } else if (!strncmp(status, T7XX_FB_RESP_FAIL, strlen(T7XX_FB_RESP_FAIL))) {
+ ret = -EPROTO;
+ break;
+ } else if (!strncmp(status, T7XX_FB_RESP_DATA, strlen(T7XX_FB_RESP_DATA))) {
+ if (data) {
+ if (!kstrtoint(status + strlen(T7XX_FB_RESP_DATA), 16,
+ &return_data)) {
+ *data = return_data;
+ } else {
+ dev_err(port->dev, "kstrtoint error!\n");
+ ret = -EPROTO;
+ }
+ }
+ break;
+ }
+ }
+
+ return ret;
+}
+
+static int t7xx_devlink_fb_raw_command(char *cmd, struct t7xx_port *port, int *data)
+{
+ int ret, cmd_size = strlen(cmd);
+
+ if (cmd_size > T7XX_FB_COMMAND_SIZE) {
+ dev_err(port->dev, "command length %d is long\n", cmd_size);
+ return -EINVAL;
+ }
+
+ if (cmd_size != t7xx_devlink_port_write(port, cmd, cmd_size)) {
+ dev_err(port->dev, "raw command = %s write failed\n", cmd);
+ return -EIO;
+ }
+
+ dev_dbg(port->dev, "raw command = %s written to the device\n", cmd);
+ ret = t7xx_devlink_fb_handle_response(port, data);
+ if (ret)
+ dev_err(port->dev, "raw command = %s response FAILURE:%d\n", cmd, ret);
+
+ return ret;
+}
+
+static int t7xx_devlink_fb_send_buffer(struct t7xx_port *port, const u8 *buf, size_t size)
+{
+ size_t remaining = size, offset = 0, len;
+ int write_done;
+
+ if (!size)
+ return -EINVAL;
+
+ while (remaining) {
+ len = min_t(size_t, remaining, CLDMA_DEDICATED_Q_BUFF_SZ);
+ write_done = t7xx_devlink_port_write(port, buf + offset, len);
+
+ if (write_done < 0) {
+ dev_err(port->dev, "write to device failed in %s", __func__);
+ return -EIO;
+ } else if (write_done != len) {
+ dev_err(port->dev, "write Error. Only %d/%zu bytes written",
+ write_done, len);
+ return -EIO;
+ }
+
+ remaining -= len;
+ offset += len;
+ }
+
+ return 0;
+}
+
+static int t7xx_devlink_fb_download_command(struct t7xx_port *port, size_t size)
+{
+ char download_command[T7XX_FB_COMMAND_SIZE];
+
+ snprintf(download_command, sizeof(download_command), "%s:%08zx",
+ T7XX_FB_CMD_DOWNLOAD, size);
+ return t7xx_devlink_fb_raw_command(download_command, port, NULL);
+}
+
+static int t7xx_devlink_fb_download(struct t7xx_port *port, const u8 *buf, size_t size)
+{
+ int ret;
+
+ if (size <= 0 || size > SIZE_MAX) {
+ dev_err(port->dev, "file is too large to download");
+ return -EINVAL;
+ }
+
+ ret = t7xx_devlink_fb_download_command(port, size);
+ if (ret)
+ return ret;
+
+ ret = t7xx_devlink_fb_send_buffer(port, buf, size);
+ if (ret)
+ return ret;
+
+ return t7xx_devlink_fb_handle_response(port, NULL);
+}
+
+static int t7xx_devlink_fb_flash(const char *cmd, struct t7xx_port *port)
+{
+ char flash_command[T7XX_FB_COMMAND_SIZE];
+
+ snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd);
+ return t7xx_devlink_fb_raw_command(flash_command, port, NULL);
+}
+
+static int t7xx_devlink_fb_flash_partition(const char *partition, const u8 *buf,
+ struct t7xx_port *port, size_t size)
+{
+ int ret;
+
+ ret = t7xx_devlink_fb_download(port, buf, size);
+ if (ret)
+ return ret;
+
+ return t7xx_devlink_fb_flash(partition, port);
+}
+
+static int t7xx_devlink_fb_get_core(struct t7xx_port *port)
+{
+ struct t7xx_devlink_region_info *mrdump_region;
+ char mrdump_complete_event[T7XX_FB_EVENT_SIZE];
+ u32 mrd_mb = T7XX_MRDUMP_SIZE / (1024 * 1024);
+ struct t7xx_devlink *dl = port->dl;
+ int clen, dlen = 0, result = 0;
+ unsigned long long zipsize = 0;
+ char mcmd[T7XX_FB_MCMD_SIZE];
+ size_t offset_dlen = 0;
+ char *mdata;
+
+ set_bit(T7XX_MRDUMP_STATUS, &dl->status);
+ mdata = kmalloc(T7XX_FB_MDATA_SIZE, GFP_KERNEL);
+ if (!mdata) {
+ result = -ENOMEM;
+ goto get_core_exit;
+ }
+
+ mrdump_region = dl->dl_region_info[T7XX_MRDUMP_INDEX];
+ mrdump_region->dump = vmalloc(mrdump_region->default_size);
+ if (!mrdump_region->dump) {
+ kfree(mdata);
+ result = -ENOMEM;
+ goto get_core_exit;
+ }
+
+ result = t7xx_devlink_fb_raw_command(T7XX_FB_CMD_OEM_MRDUMP, port, NULL);
+ if (result) {
+ dev_err(port->dev, "%s command failed\n", T7XX_FB_CMD_OEM_MRDUMP);
+ vfree(mrdump_region->dump);
+ kfree(mdata);
+ goto get_core_exit;
+ }
+
+ while (mrdump_region->default_size > offset_dlen) {
+ clen = t7xx_devlink_port_read(port, mcmd, sizeof(mcmd));
+ if (clen == strlen(T7XX_FB_CMD_RTS) &&
+ (!strncmp(mcmd, T7XX_FB_CMD_RTS, strlen(T7XX_FB_CMD_RTS)))) {
+ memset(mdata, 0, T7XX_FB_MDATA_SIZE);
+ dlen = 0;
+ memset(mcmd, 0, sizeof(mcmd));
+ clen = snprintf(mcmd, sizeof(mcmd), "%s", T7XX_FB_CMD_CTS);
+
+ if (t7xx_devlink_port_write(port, mcmd, clen) != clen) {
+ dev_err(port->dev, "write for _CTS failed:%d\n", clen);
+ goto get_core_free_mem;
+ }
+
+ dlen = t7xx_devlink_port_read(port, mdata, T7XX_FB_MDATA_SIZE);
+ if (dlen <= 0) {
+ dev_err(port->dev, "read data error(%d)\n", dlen);
+ goto get_core_free_mem;
+ }
+
+ zipsize += (unsigned long long)(dlen);
+ memcpy(mrdump_region->dump + offset_dlen, mdata, dlen);
+ offset_dlen += dlen;
+ memset(mcmd, 0, sizeof(mcmd));
+ clen = snprintf(mcmd, sizeof(mcmd), "%s", T7XX_FB_CMD_FIN);
+ if (t7xx_devlink_port_write(port, mcmd, clen) != clen) {
+ dev_err(port->dev, "%s: _FIN failed, (Read %05d:%05llu)\n",
+ __func__, clen, zipsize);
+ goto get_core_free_mem;
+ }
+ } else if ((clen == strlen(T7XX_FB_RESP_MRDUMP_DONE)) &&
+ (!strncmp(mcmd, T7XX_FB_RESP_MRDUMP_DONE,
+ strlen(T7XX_FB_RESP_MRDUMP_DONE)))) {
+ dev_dbg(port->dev, "%s! size:%zd\n", T7XX_FB_RESP_MRDUMP_DONE, offset_dlen);
+ mrdump_region->actual_size = offset_dlen;
+ snprintf(mrdump_complete_event, sizeof(mrdump_complete_event),
+ "%s size=%zu", T7XX_UEVENT_MRDUMP_READY, offset_dlen);
+ t7xx_uevent_send(dl->dev, mrdump_complete_event);
+ kfree(mdata);
+ result = 0;
+ goto get_core_exit;
+ } else {
+ dev_err(port->dev, "getcore protocol error (read len %05d)\n", clen);
+ goto get_core_free_mem;
+ }
+ }
+
+ dev_err(port->dev, "mrdump exceeds %uMB size. Discarded!", mrd_mb);
+ t7xx_uevent_send(port->dev, T7XX_UEVENT_MRD_DISCD);
+
+get_core_free_mem:
+ kfree(mdata);
+ vfree(mrdump_region->dump);
+ clear_bit(T7XX_MRDUMP_STATUS, &dl->status);
+ return -EPROTO;
+
+get_core_exit:
+ clear_bit(T7XX_MRDUMP_STATUS, &dl->status);
+ return result;
+}
+
+static int t7xx_devlink_fb_dump_log(struct t7xx_port *port)
+{
+ struct t7xx_devlink_region_info *lkdump_region;
+ char lkdump_complete_event[T7XX_FB_EVENT_SIZE];
+ struct t7xx_devlink *dl = port->dl;
+ int dlen, datasize = 0, result;
+ size_t offset_dlen = 0;
+ u8 *data;
+
+ set_bit(T7XX_LKDUMP_STATUS, &dl->status);
+ result = t7xx_devlink_fb_raw_command(T7XX_FB_CMD_OEM_LKDUMP, port, &datasize);
+ if (result) {
+ dev_err(port->dev, "%s command returns failure\n", T7XX_FB_CMD_OEM_LKDUMP);
+ goto lkdump_exit;
+ }
+
+ lkdump_region = dl->dl_region_info[T7XX_LKDUMP_INDEX];
+ if (datasize > lkdump_region->default_size) {
+ dev_err(port->dev, "lkdump size is more than %dKB. Discarded!",
+ T7XX_LKDUMP_SIZE / 1024);
+ t7xx_uevent_send(dl->dev, T7XX_UEVENT_LKD_DISCD);
+ result = -EPROTO;
+ goto lkdump_exit;
+ }
+
+ data = kzalloc(datasize, GFP_KERNEL);
+ if (!data) {
+ result = -ENOMEM;
+ goto lkdump_exit;
+ }
+
+ lkdump_region->dump = vmalloc(lkdump_region->default_size);
+ if (!lkdump_region->dump) {
+ kfree(data);
+ result = -ENOMEM;
+ goto lkdump_exit;
+ }
+
+ while (datasize > 0) {
+ dlen = t7xx_devlink_port_read(port, data, datasize);
+ if (dlen <= 0) {
+ dev_err(port->dev, "lkdump read error ret = %d", dlen);
+ kfree(data);
+ result = -EPROTO;
+ goto lkdump_exit;
+ }
+
+ memcpy(lkdump_region->dump + offset_dlen, data, dlen);
+ datasize -= dlen;
+ offset_dlen += dlen;
+ }
+
+ dev_dbg(port->dev, "LKDUMP DONE! size:%zd\n", offset_dlen);
+ lkdump_region->actual_size = offset_dlen;
+ snprintf(lkdump_complete_event, sizeof(lkdump_complete_event), "%s size=%zu",
+ T7XX_UEVENT_LKDUMP_READY, offset_dlen);
+ t7xx_uevent_send(dl->dev, lkdump_complete_event);
+ kfree(data);
+ clear_bit(T7XX_LKDUMP_STATUS, &dl->status);
+ return t7xx_devlink_fb_handle_response(port, NULL);
+
+lkdump_exit:
+ clear_bit(T7XX_LKDUMP_STATUS, &dl->status);
+ return result;
+}
+
+static int t7xx_devlink_flash_update(struct devlink *devlink,
+ struct devlink_flash_update_params *params,
+ struct netlink_ext_ack *extack)
+{
+ struct t7xx_devlink *dl = devlink_priv(devlink);
+ const char *component = params->component;
+ const struct firmware *fw = params->fw;
+ char flash_event[T7XX_FB_EVENT_SIZE];
+ struct t7xx_port *port;
+ int ret;
+
+ port = dl->port;
+ if (port->dl->mode != T7XX_FB_DL_MODE) {
+ dev_err(port->dev, "Modem is not in fastboot download mode!");
+ ret = -EPERM;
+ goto err_out;
+ }
+
+ if (dl->status != T7XX_DEVLINK_IDLE) {
+ dev_err(port->dev, "Modem is busy!");
+ ret = -EBUSY;
+ goto err_out;
+ }
+
+ if (!component || !fw->data) {
+ ret = -EINVAL;
+ goto err_out;
+ }
+
+ set_bit(T7XX_FLASH_STATUS, &dl->status);
+ dev_dbg(port->dev, "flash partition name:%s binary size:%zu\n", component, fw->size);
+ ret = t7xx_devlink_fb_flash_partition(component, fw->data, port, fw->size);
+ if (ret) {
+ devlink_flash_update_status_notify(devlink, "flashing failure!",
+ params->component, 0, 0);
+ snprintf(flash_event, sizeof(flash_event), "%s for [%s]",
+ T7XX_UEVENT_FLASHING_FAILURE, params->component);
+ } else {
+ devlink_flash_update_status_notify(devlink, "flashing success!",
+ params->component, 0, 0);
+ snprintf(flash_event, sizeof(flash_event), "%s for [%s]",
+ T7XX_UEVENT_FLASHING_SUCCESS, params->component);
+ }
+
+ t7xx_uevent_send(dl->dev, flash_event);
+
+err_out:
+ clear_bit(T7XX_FLASH_STATUS, &dl->status);
+ return ret;
+}
+
+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)
+{
+ struct t7xx_devlink *dl = devlink_priv(devlink);
+
+ switch (action) {
+ case DEVLINK_RELOAD_ACTION_DRIVER_REINIT:
+ dl->set_fastboot_dl = 1;
+ return 0;
+ case DEVLINK_RELOAD_ACTION_FW_ACTIVATE:
+ return t7xx_devlink_fb_raw_command(T7XX_FB_CMD_REBOOT, dl->port, NULL);
+ default:
+ /* Unsupported action should not get to this function */
+ return -EOPNOTSUPP;
+ }
+}
+
+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)
+{
+ struct t7xx_devlink *dl = devlink_priv(devlink);
+ *actions_performed = BIT(action);
+ switch (action) {
+ case DEVLINK_RELOAD_ACTION_DRIVER_REINIT:
+ case DEVLINK_RELOAD_ACTION_FW_ACTIVATE:
+ t7xx_rescan_queue_work(dl->mtk_dev->pdev);
+ return 0;
+ default:
+ /* Unsupported action should not get to this function */
+ return -EOPNOTSUPP;
+ }
+}
+
+/* Call back function for devlink ops */
+static const struct devlink_ops devlink_flash_ops = {
+ .supported_flash_update_params = DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT,
+ .flash_update = t7xx_devlink_flash_update,
+ .reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT) |
+ BIT(DEVLINK_RELOAD_ACTION_FW_ACTIVATE),
+ .reload_down = t7xx_devlink_reload_down,
+ .reload_up = t7xx_devlink_reload_up,
+};
+
+static int t7xx_devlink_region_snapshot(struct devlink *dl, const struct devlink_region_ops *ops,
+ struct netlink_ext_ack *extack, u8 **data)
+{
+ struct t7xx_devlink_region_info *region_info = ops->priv;
+ struct t7xx_devlink *t7xx_dl = devlink_priv(dl);
+ u8 *snapshot_mem;
+
+ if (t7xx_dl->status != T7XX_DEVLINK_IDLE) {
+ dev_err(t7xx_dl->dev, "Modem is busy!");
+ return -EBUSY;
+ }
+
+ dev_dbg(t7xx_dl->dev, "accessed devlink region:%s index:%d", ops->name, region_info->entry);
+ if (!strncmp(ops->name, "mr_dump", strlen("mr_dump"))) {
+ if (!region_info->dump) {
+ dev_err(t7xx_dl->dev, "devlink region:%s dump memory is not valid!",
+ region_info->region_name);
+ return -ENOMEM;
+ }
+
+ snapshot_mem = vmalloc(region_info->default_size);
+ if (!snapshot_mem)
+ return -ENOMEM;
+
+ memcpy(snapshot_mem, region_info->dump, region_info->default_size);
+ *data = snapshot_mem;
+ } else if (!strncmp(ops->name, "lk_dump", strlen("lk_dump"))) {
+ int ret;
+
+ ret = t7xx_devlink_fb_dump_log(t7xx_dl->port);
+ if (ret)
+ return ret;
+
+ *data = region_info->dump;
+ }
+
+ return 0;
+}
+
+/* To create regions for dump files */
+static int t7xx_devlink_create_region(struct t7xx_devlink *dl)
+{
+ struct devlink_region_ops *region_ops;
+ int rc, i;
+
+ region_ops = dl->dl_region_ops;
+ for (i = 0; i < T7XX_TOTAL_REGIONS; i++) {
+ region_ops[i].name = t7xx_devlink_region_list[i].region_name;
+ region_ops[i].snapshot = t7xx_devlink_region_snapshot;
+ region_ops[i].destructor = vfree;
+ dl->dl_region[i] =
+ devlink_region_create(dl->dl_ctx, ®ion_ops[i], T7XX_MAX_SNAPSHOTS,
+ t7xx_devlink_region_list[i].default_size);
+
+ if (IS_ERR(dl->dl_region[i])) {
+ rc = PTR_ERR(dl->dl_region[i]);
+ dev_err(dl->dev, "devlink region fail,err %d", rc);
+ for ( ; i >= 0; i--)
+ devlink_region_destroy(dl->dl_region[i]);
+
+ return rc;
+ }
+
+ t7xx_devlink_region_list[i].entry = i;
+ region_ops[i].priv = t7xx_devlink_region_list + i;
+ }
+
+ return 0;
+}
+
+/* To Destroy devlink regions */
+static void t7xx_devlink_destroy_region(struct t7xx_devlink *dl)
+{
+ u8 i;
+
+ for (i = 0; i < T7XX_TOTAL_REGIONS; i++)
+ devlink_region_destroy(dl->dl_region[i]);
+}
+
+int t7xx_devlink_register(struct t7xx_pci_dev *t7xx_dev)
+{
+ struct devlink *dl_ctx;
+
+ dl_ctx = devlink_alloc(&devlink_flash_ops, sizeof(struct t7xx_devlink),
+ &t7xx_dev->pdev->dev);
+ if (!dl_ctx)
+ return -ENOMEM;
+
+ devlink_set_features(dl_ctx, DEVLINK_F_RELOAD);
+ devlink_register(dl_ctx);
+ t7xx_dev->dl = devlink_priv(dl_ctx);
+ t7xx_dev->dl->dl_ctx = dl_ctx;
+
+ return 0;
+}
+
+void t7xx_devlink_unregister(struct t7xx_pci_dev *t7xx_dev)
+{
+ struct devlink *dl_ctx = priv_to_devlink(t7xx_dev->dl);
+
+ devlink_unregister(dl_ctx);
+ devlink_free(dl_ctx);
+}
+
+/**
+ * t7xx_devlink_region_init - Initialize/register devlink to t7xx driver
+ * @port: Pointer to port structure
+ * @dw: Pointer to devlink work structure
+ * @wq: Pointer to devlink workqueue structure
+ *
+ * Returns: Pointer to t7xx_devlink on success and NULL on failure
+ */
+static struct t7xx_devlink *t7xx_devlink_region_init(struct t7xx_port *port,
+ struct t7xx_devlink_work *dw,
+ struct workqueue_struct *wq)
+{
+ struct t7xx_pci_dev *mtk_dev = port->t7xx_dev;
+ struct t7xx_devlink *dl = mtk_dev->dl;
+ int rc, i;
+
+ dl->dl_ctx = mtk_dev->dl->dl_ctx;
+ dl->mtk_dev = mtk_dev;
+ dl->dev = &mtk_dev->pdev->dev;
+ dl->mode = T7XX_FB_NO_MODE;
+ dl->status = T7XX_DEVLINK_IDLE;
+ dl->dl_work = dw;
+ dl->dl_wq = wq;
+ for (i = 0; i < T7XX_TOTAL_REGIONS; i++) {
+ dl->dl_region_info[i] = &t7xx_devlink_region_list[i];
+ dl->dl_region_info[i]->dump = NULL;
+ }
+ dl->port = port;
+ port->dl = dl;
+
+ rc = t7xx_devlink_create_region(dl);
+ if (rc) {
+ dev_err(dl->dev, "devlink region creation failed, rc %d", rc);
+ return NULL;
+ }
+
+ return dl;
+}
+
+/**
+ * t7xx_devlink_region_deinit - To unintialize the devlink from T7XX driver.
+ * @dl: Devlink instance
+ */
+static void t7xx_devlink_region_deinit(struct t7xx_devlink *dl)
+{
+ dl->mode = T7XX_FB_NO_MODE;
+ t7xx_devlink_destroy_region(dl);
+}
+
+static void t7xx_devlink_work_handler(struct work_struct *data)
+{
+ struct t7xx_devlink_work *dl_work;
+
+ dl_work = container_of(data, struct t7xx_devlink_work, work);
+ t7xx_devlink_fb_get_core(dl_work->port);
+}
+
+static int t7xx_devlink_init(struct t7xx_port *port)
+{
+ struct t7xx_devlink_work *dl_work;
+ struct workqueue_struct *wq;
+
+ dl_work = kmalloc(sizeof(*dl_work), GFP_KERNEL);
+ if (!dl_work)
+ return -ENOMEM;
+
+ wq = create_workqueue("t7xx_devlink");
+ if (!wq) {
+ kfree(dl_work);
+ dev_err(port->dev, "create_workqueue failed\n");
+ return -ENODATA;
+ }
+
+ INIT_WORK(&dl_work->work, t7xx_devlink_work_handler);
+ dl_work->port = port;
+ port->rx_length_th = T7XX_MAX_QUEUE_LENGTH;
+
+ if (!t7xx_devlink_region_init(port, dl_work, wq))
+ return -ENOMEM;
+
+ return 0;
+}
+
+static void t7xx_devlink_uninit(struct t7xx_port *port)
+{
+ struct t7xx_devlink *dl = port->dl;
+ struct sk_buff *skb;
+ unsigned long flags;
+
+ vfree(dl->dl_region_info[T7XX_MRDUMP_INDEX]->dump);
+ if (dl->dl_wq)
+ destroy_workqueue(dl->dl_wq);
+ kfree(dl->dl_work);
+
+ t7xx_devlink_region_deinit(port->dl);
+ spin_lock_irqsave(&port->rx_skb_list.lock, flags);
+ while ((skb = __skb_dequeue(&port->rx_skb_list)) != NULL)
+ dev_kfree_skb(skb);
+ spin_unlock_irqrestore(&port->rx_skb_list.lock, flags);
+}
+
+static int t7xx_devlink_enable_chl(struct t7xx_port *port)
+{
+ spin_lock(&port->port_update_lock);
+ port->chan_enable = true;
+ spin_unlock(&port->port_update_lock);
+
+ if (port->dl->dl_wq && port->dl->mode == T7XX_FB_DUMP_MODE)
+ queue_work(port->dl->dl_wq, &port->dl->dl_work->work);
+
+ return 0;
+}
+
+static int t7xx_devlink_disable_chl(struct t7xx_port *port)
+{
+ spin_lock(&port->port_update_lock);
+ port->chan_enable = false;
+ spin_unlock(&port->port_update_lock);
+
+ return 0;
+}
+
+struct port_ops devlink_port_ops = {
+ .init = &t7xx_devlink_init,
+ .recv_skb = &t7xx_port_enqueue_skb,
+ .uninit = &t7xx_devlink_uninit,
+ .enable_chl = &t7xx_devlink_enable_chl,
+ .disable_chl = &t7xx_devlink_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..85384e40519e
--- /dev/null
+++ b/drivers/net/wwan/t7xx/t7xx_port_devlink.h
@@ -0,0 +1,85 @@
+/* SPDX-License-Identifier: GPL-2.0-only
+ *
+ * Copyright (c) 2022, Intel Corporation.
+ */
+
+#ifndef __T7XX_PORT_DEVLINK_H__
+#define __T7XX_PORT_DEVLINK_H__
+
+#include <net/devlink.h>
+
+#include "t7xx_pci.h"
+
+#define T7XX_MAX_QUEUE_LENGTH 32
+#define T7XX_FB_COMMAND_SIZE 64
+#define T7XX_FB_RESPONSE_SIZE 64
+#define T7XX_FB_MCMD_SIZE 64
+#define T7XX_FB_MDATA_SIZE 1024
+#define T7XX_FB_RESP_COUNT 30
+
+#define T7XX_FB_CMD_RTS "_RTS"
+#define T7XX_FB_CMD_CTS "_CTS"
+#define T7XX_FB_CMD_FIN "_FIN"
+#define T7XX_FB_CMD_OEM_MRDUMP "oem mrdump"
+#define T7XX_FB_CMD_OEM_LKDUMP "oem dump_pllk_log"
+#define T7XX_FB_CMD_DOWNLOAD "download"
+#define T7XX_FB_CMD_FLASH "flash"
+#define T7XX_FB_CMD_REBOOT "reboot"
+#define T7XX_FB_RESP_MRDUMP_DONE "MRDUMP08_DONE"
+#define T7XX_FB_RESP_OKAY "OKAY"
+#define T7XX_FB_RESP_FAIL "FAIL"
+#define T7XX_FB_RESP_DATA "DATA"
+#define T7XX_FB_RESP_INFO "INFO"
+
+#define T7XX_FB_EVENT_SIZE 50
+
+#define T7XX_MAX_SNAPSHOTS 1
+#define T7XX_MAX_REGION_NAME_LENGTH 20
+#define T7XX_MRDUMP_SIZE (160 * 1024 * 1024)
+#define T7XX_LKDUMP_SIZE (256 * 1024)
+#define T7XX_TOTAL_REGIONS 2
+
+#define T7XX_FLASH_STATUS 0
+#define T7XX_MRDUMP_STATUS 1
+#define T7XX_LKDUMP_STATUS 2
+#define T7XX_DEVLINK_IDLE 0
+
+#define T7XX_FB_NO_MODE 0
+#define T7XX_FB_DL_MODE 1
+#define T7XX_FB_DUMP_MODE 2
+
+#define T7XX_MRDUMP_INDEX 0
+#define T7XX_LKDUMP_INDEX 1
+
+struct t7xx_devlink_work {
+ struct work_struct work;
+ struct t7xx_port *port;
+};
+
+struct t7xx_devlink_region_info {
+ char region_name[T7XX_MAX_REGION_NAME_LENGTH];
+ u32 default_size;
+ u32 actual_size;
+ u32 entry;
+ u8 *dump;
+};
+
+struct t7xx_devlink {
+ struct t7xx_pci_dev *mtk_dev;
+ struct t7xx_port *port;
+ struct device *dev;
+ struct devlink *dl_ctx;
+ struct t7xx_devlink_work *dl_work;
+ struct workqueue_struct *dl_wq;
+ struct t7xx_devlink_region_info *dl_region_info[T7XX_TOTAL_REGIONS];
+ struct devlink_region_ops dl_region_ops[T7XX_TOTAL_REGIONS];
+ struct devlink_region *dl_region[T7XX_TOTAL_REGIONS];
+ u8 mode;
+ unsigned long status;
+ int set_fastboot_dl;
+};
+
+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 7582777cf94d..fdf0c6e5ed6d 100644
--- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
+++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
@@ -98,6 +98,7 @@ static struct t7xx_port_conf t7xx_early_port_conf[] = {
.rxq_exp_index = 1,
.path_id = CLDMA_ID_AP,
.is_early_port = true,
+ .ops = &devlink_port_ops,
.name = "ttyDUMP",
},
};
@@ -493,6 +494,7 @@ static void t7xx_proxy_init_all_ports(struct t7xx_modem *md)
port->t7xx_dev = md->t7xx_dev;
port->dev = &md->t7xx_dev->pdev->dev;
+ port->dl = md->t7xx_dev->dl;
spin_lock_init(&port->port_update_lock);
port->chan_enable = false;
diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.h b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
index 33caf85f718a..7298a2d09fa0 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;
void t7xx_port_proxy_reset(struct port_proxy *port_prox);
void t7xx_port_proxy_uninit(struct port_proxy *port_prox);
diff --git a/drivers/net/wwan/t7xx/t7xx_reg.h b/drivers/net/wwan/t7xx/t7xx_reg.h
index 60e025e57baa..3a758bf79a4e 100644
--- a/drivers/net/wwan/t7xx/t7xx_reg.h
+++ b/drivers/net/wwan/t7xx/t7xx_reg.h
@@ -101,11 +101,17 @@ enum t7xx_pm_resume_state {
PM_RESUME_REG_STATE_L2_EXP,
};
+enum host_event_e {
+ HOST_EVENT_INIT = 0,
+ FASTBOOT_DL_NOTY = 0x3,
+};
+
#define T7XX_PCIE_MISC_DEV_STATUS 0x0d1c
#define MISC_RESET_TYPE_FLDR BIT(27)
#define MISC_RESET_TYPE_PLDR BIT(26)
#define MISC_DEV_STATUS_MASK GENMASK(15, 0)
#define LK_EVENT_MASK GENMASK(11, 8)
+#define HOST_EVENT_MASK GENMASK(31, 28)
enum lk_event_id {
LK_EVENT_NORMAL = 0,
diff --git a/drivers/net/wwan/t7xx/t7xx_state_monitor.c b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
index 9c222809371b..00e143c8d568 100644
--- a/drivers/net/wwan/t7xx/t7xx_state_monitor.c
+++ b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
@@ -38,10 +38,12 @@
#include "t7xx_netdev.h"
#include "t7xx_pci.h"
#include "t7xx_pcie_mac.h"
+#include "t7xx_port_devlink.h"
#include "t7xx_port_proxy.h"
#include "t7xx_pci_rescan.h"
#include "t7xx_reg.h"
#include "t7xx_state_monitor.h"
+#include "t7xx_uevent.h"
#define FSM_DRM_DISABLE_DELAY_MS 200
#define FSM_EVENT_POLL_INTERVAL_MS 20
@@ -212,6 +214,16 @@ static void fsm_routine_exception(struct t7xx_fsm_ctl *ctl, struct t7xx_fsm_comm
fsm_finish_command(ctl, cmd, 0);
}
+static void t7xx_host_event_notify(struct t7xx_modem *md, unsigned int event_id)
+{
+ u32 value;
+
+ value = ioread32(IREG_BASE(md->t7xx_dev) + T7XX_PCIE_MISC_DEV_STATUS);
+ value &= ~HOST_EVENT_MASK;
+ value |= FIELD_PREP(HOST_EVENT_MASK, event_id);
+ iowrite32(value, IREG_BASE(md->t7xx_dev) + T7XX_PCIE_MISC_DEV_STATUS);
+}
+
static void t7xx_lk_stage_event_handling(struct t7xx_fsm_ctl *ctl, unsigned int dev_status)
{
struct t7xx_modem *md = ctl->md;
@@ -228,6 +240,7 @@ static void t7xx_lk_stage_event_handling(struct t7xx_fsm_ctl *ctl, unsigned int
break;
case LK_EVENT_CREATE_PD_PORT:
+ case LK_EVENT_CREATE_POST_DL_PORT:
md_ctrl = md->md_ctrl[CLDMA_ID_AP];
t7xx_cldma_hif_hw_init(md_ctrl);
t7xx_cldma_stop(md_ctrl);
@@ -239,8 +252,16 @@ static void t7xx_lk_stage_event_handling(struct t7xx_fsm_ctl *ctl, unsigned int
return;
}
+ if (lk_event == LK_EVENT_CREATE_PD_PORT)
+ port->dl->mode = T7XX_FB_DUMP_MODE;
+ else
+ port->dl->mode = T7XX_FB_DL_MODE;
port->port_conf->ops->enable_chl(port);
t7xx_cldma_start(md_ctrl);
+ if (lk_event == LK_EVENT_CREATE_PD_PORT)
+ t7xx_uevent_send(dev, T7XX_UEVENT_MODEM_FASTBOOT_DUMP_MODE);
+ else
+ t7xx_uevent_send(dev, T7XX_UEVENT_MODEM_FASTBOOT_DL_MODE);
break;
case LK_EVENT_RESET:
@@ -289,13 +310,23 @@ static void fsm_routine_stopping(struct t7xx_fsm_ctl *ctl, struct t7xx_fsm_comma
t7xx_cldma_stop(md_ctrl);
if (!ctl->md->rgu_irq_asserted) {
+ if (t7xx_dev->dl->set_fastboot_dl)
+ t7xx_host_event_notify(ctl->md, FASTBOOT_DL_NOTY);
+
t7xx_mhccif_h2d_swint_trigger(t7xx_dev, H2D_CH_DRM_DISABLE_AP);
/* Wait for the DRM disable to take effect */
msleep(FSM_DRM_DISABLE_DELAY_MS);
- err = t7xx_acpi_fldr_func(t7xx_dev);
- if (err)
+ if (t7xx_dev->dl->set_fastboot_dl) {
+ /* Do not try fldr because device will always wait for
+ * MHCCIF bit 13 in fastboot download flow.
+ */
t7xx_mhccif_h2d_swint_trigger(t7xx_dev, H2D_CH_DEVICE_RESET);
+ } else {
+ err = t7xx_acpi_fldr_func(t7xx_dev);
+ if (err)
+ t7xx_mhccif_h2d_swint_trigger(t7xx_dev, H2D_CH_DEVICE_RESET);
+ }
}
fsm_finish_command(ctl, cmd, fsm_stopped_handler(ctl));
@@ -318,6 +349,7 @@ static void fsm_routine_ready(struct t7xx_fsm_ctl *ctl)
ctl->curr_state = FSM_STATE_READY;
t7xx_fsm_broadcast_ready_state(ctl);
+ t7xx_uevent_send(&md->t7xx_dev->pdev->dev, T7XX_UEVENT_MODEM_READY);
t7xx_md_event_notify(md, FSM_READY);
}
diff --git a/drivers/net/wwan/t7xx/t7xx_uevent.c b/drivers/net/wwan/t7xx/t7xx_uevent.c
new file mode 100644
index 000000000000..5a320cf3f94b
--- /dev/null
+++ b/drivers/net/wwan/t7xx/t7xx_uevent.c
@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022, Intel Corporation.
+ */
+
+#include <linux/slab.h>
+
+#include "t7xx_uevent.h"
+
+/* Update the uevent in work queue context */
+static void t7xx_uevent_work(struct work_struct *data)
+{
+ struct t7xx_uevent_info *info;
+ char *envp[2] = { NULL, NULL };
+
+ info = container_of(data, struct t7xx_uevent_info, work);
+ envp[0] = info->uevent;
+
+ if (kobject_uevent_env(&info->dev->kobj, KOBJ_CHANGE, envp))
+ pr_err("uevent %s failed to sent", info->uevent);
+
+ kfree(info);
+}
+
+/**
+ * t7xx_uevent_send - Send modem event to user space.
+ * @dev: Generic device pointer
+ * @uevent: Uevent information
+ */
+void t7xx_uevent_send(struct device *dev, char *uevent)
+{
+ struct t7xx_uevent_info *info = kzalloc(sizeof(*info), GFP_ATOMIC);
+
+ if (!info)
+ return;
+
+ INIT_WORK(&info->work, t7xx_uevent_work);
+ info->dev = dev;
+ snprintf(info->uevent, T7XX_MAX_UEVENT_LEN, "T7XX_EVENT=%s", uevent);
+ schedule_work(&info->work);
+}
diff --git a/drivers/net/wwan/t7xx/t7xx_uevent.h b/drivers/net/wwan/t7xx/t7xx_uevent.h
new file mode 100644
index 000000000000..e871dc0e9444
--- /dev/null
+++ b/drivers/net/wwan/t7xx/t7xx_uevent.h
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0-only
+ *
+ * Copyright (c) 2022, Intel Corporation.
+ */
+
+#ifndef __T7XX_UEVENT_H__
+#define __T7XX_UEVENT_H__
+
+#include <linux/device.h>
+#include <linux/kobject.h>
+
+/* Maximum length of user events */
+#define T7XX_MAX_UEVENT_LEN 64
+
+/* T7XX Host driver uevents */
+#define T7XX_UEVENT_MODEM_READY "T7XX_MODEM_READY"
+#define T7XX_UEVENT_MODEM_FASTBOOT_DL_MODE "T7XX_MODEM_FASTBOOT_DL_MODE"
+#define T7XX_UEVENT_MODEM_FASTBOOT_DUMP_MODE "T7XX_MODEM_FASTBOOT_DUMP_MODE"
+#define T7XX_UEVENT_MRDUMP_READY "T7XX_MRDUMP_READY"
+#define T7XX_UEVENT_LKDUMP_READY "T7XX_LKDUMP_READY"
+#define T7XX_UEVENT_MRD_DISCD "T7XX_MRDUMP_DISCARDED"
+#define T7XX_UEVENT_LKD_DISCD "T7XX_LKDUMP_DISCARDED"
+#define T7XX_UEVENT_FLASHING_SUCCESS "T7XX_FLASHING_SUCCESS"
+#define T7XX_UEVENT_FLASHING_FAILURE "T7XX_FLASHING_FAILURE"
+
+/**
+ * struct t7xx_uevent_info - Uevent information structure.
+ * @dev: Pointer to device structure
+ * @uevent: Uevent information
+ * @work: Uevent work struct
+ */
+struct t7xx_uevent_info {
+ struct device *dev;
+ char uevent[T7XX_MAX_UEVENT_LEN];
+ struct work_struct work;
+};
+
+void t7xx_uevent_send(struct device *dev, char *uevent);
+#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 4/5] net: wwan: t7xx: Enable devlink based fw flashing and coredump collection
2022-08-16 4:24 [PATCH net-next 4/5] net: wwan: t7xx: Enable devlink based fw flashing and coredump collection m.chetan.kumar
@ 2022-08-18 14:33 ` Ilpo Järvinen
2022-09-06 10:15 ` Kumar, M Chetan
2022-08-30 2:21 ` Sergey Ryazanov
1 sibling, 1 reply; 10+ messages in thread
From: Ilpo Järvinen @ 2022-08-18 14:33 UTC (permalink / raw)
To: m.chetan.kumar
Cc: Netdev, kuba, davem, johannes, ryazanov.s.a, loic.poulain,
krishna.c.sudi, m.chetan.kumar, linuxwwan,
Devegowda Chandrashekar, Mishra Soumya Prakash
On Tue, 16 Aug 2022, m.chetan.kumar@intel.com wrote:
> From: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
>
> This patch brings-in support for t7xx wwan device firmware flashing &
Just say "Add support for ..."
> coredump collection using devlink.
>
> Driver Registers with Devlink framework.
> Implements devlink ops flash_update callback that programs modem firmware.
> Creates region & snapshot required for device coredump log collection.
> On early detection of wwan device in fastboot mode driver sets up CLDMA0 HW
> tx/rx queues for raw data transfer then registers with devlink framework.
> Upon receiving firmware image & partition details driver sends fastboot
> commands for flashing the firmware.
Things don't seem to connect well between sentences in this paragraph.
> In this flow the fastboot command & response gets exchanged between driver
> and device. Once firmware flashing is success completion status is reported
> to user space application.
>
> Below is the devlink command usage for firmware flashing
>
> $devlink dev flash pci/$BDF file ABC.img component ABC
>
> Note: ABC.img is the firmware to be programmed to "ABC" partition.
>
> In case of coredump collection when wwan device encounters an exception
> it reboots & stays in fastboot mode for coredump collection by host driver.
> On detecting exception state driver collects the core dump, creates the
> devlink region & reports an event to user space application for dump
> collection. The user space application invokes devlink region read command
> for dump collection.
>
> Below are the devlink commands used for coredump collection.
>
> devlink region new pci/$BDF/mr_dump
> devlink region read pci/$BDF/mr_dump snapshot $ID address $ADD length $LEN
> devlink region del pci/$BDF/mr_dump snapshot $ID
>
> Signed-off-by: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
> Signed-off-by: Devegowda Chandrashekar <chandrashekar.devegowda@intel.com>
> Signed-off-by: Mishra Soumya Prakash <soumya.prakash.mishra@intel.com>
> ---
> +static int t7xx_devlink_port_read(struct t7xx_port *port, char *buf, size_t count)
> +{
> + int ret = 0, read_len;
> + struct sk_buff *skb;
> +
> + spin_lock_irq(&port->rx_wq.lock);
> + if (skb_queue_empty(&port->rx_skb_list)) {
> + ret = wait_event_interruptible_locked_irq(port->rx_wq,
> + !skb_queue_empty(&port->rx_skb_list));
> + if (ret == -ERESTARTSYS) {
> + spin_unlock_irq(&port->rx_wq.lock);
> + return -EINTR;
> + }
> + }
> + skb = skb_dequeue(&port->rx_skb_list);
> + spin_unlock_irq(&port->rx_wq.lock);
> +
> + read_len = count > skb->len ? skb->len : count;
max_t()
> + memcpy(buf, skb->data, read_len);
> + dev_kfree_skb(skb);
> +
> + return ret ? ret : read_len;
Can ret actually be non-zero here since -ERESTARTSYS is covered above?
> +}
> +
> +static int t7xx_devlink_port_write(struct t7xx_port *port, const char *buf, size_t count)
> +{
> + const struct t7xx_port_conf *port_conf = port->port_conf;
> + size_t actual_count;
> + struct sk_buff *skb;
> + int ret, txq_mtu;
> +
> + txq_mtu = t7xx_get_port_mtu(port);
> + if (txq_mtu < 0)
> + return -EINVAL;
> +
> + actual_count = count > txq_mtu ? txq_mtu : count;
max_t()
> + skb = __dev_alloc_skb(actual_count, GFP_KERNEL);
> + if (!skb)
> + return -ENOMEM;
> +
> + skb_put_data(skb, buf, actual_count);
> + ret = t7xx_port_send_raw_skb(port, skb);
> + if (ret) {
> + dev_err(port->dev, "write error on %s, size: %zu, ret: %d\n",
> + port_conf->name, actual_count, ret);
> + dev_kfree_skb(skb);
> + return ret;
> + }
> +
> + return actual_count;
> +}
> +
> +static int t7xx_devlink_fb_handle_response(struct t7xx_port *port, int *data)
> +{
> + int ret = 0, index = 0, return_data = 0, read_bytes;
I'd move return_data declaration into block where it is used. I don't
think it needs to be initialized or does the compiler complain about it?
> + char status[T7XX_FB_RESPONSE_SIZE + 1];
> +
> + while (index < T7XX_FB_RESP_COUNT) {
> + index++;
for (index = 0; index < T7XX_FB_RESP_COUNT; index++) {
> + read_bytes = t7xx_devlink_port_read(port, status, T7XX_FB_RESPONSE_SIZE);
> + if (read_bytes < 0) {
> + dev_err(port->dev, "status read failed");
Printing "read failed" for -ERESTARTSYS/-EINTR??
> +static int t7xx_devlink_fb_download(struct t7xx_port *port, const u8 *buf, size_t size)
> +{
> + int ret;
> +
> + if (size <= 0 || size > SIZE_MAX) {
> + dev_err(port->dev, "file is too large to download");
> + return -EINVAL;
The if condition is effectively if (!size) because unsigned cannot be <0
nor can size_t be > (~(size_t)0). Given that, the error message is pretty
bogus. I'd tend to think returning just -EINVAL in case of !size w/o any
printingis sufficient here.
> +static int t7xx_devlink_fb_get_core(struct t7xx_port *port)
> +{
> + struct t7xx_devlink_region_info *mrdump_region;
> + char mrdump_complete_event[T7XX_FB_EVENT_SIZE];
> + u32 mrd_mb = T7XX_MRDUMP_SIZE / (1024 * 1024);
> + struct t7xx_devlink *dl = port->dl;
> + int clen, dlen = 0, result = 0;
No need to init dlen.
> + unsigned long long zipsize = 0;
> + char mcmd[T7XX_FB_MCMD_SIZE];
> + size_t offset_dlen = 0;
> + char *mdata;
> +
> + set_bit(T7XX_MRDUMP_STATUS, &dl->status);
> + mdata = kmalloc(T7XX_FB_MDATA_SIZE, GFP_KERNEL);
> + if (!mdata) {
> + result = -ENOMEM;
> + goto get_core_exit;
> + }
> +
> + mrdump_region = dl->dl_region_info[T7XX_MRDUMP_INDEX];
> + mrdump_region->dump = vmalloc(mrdump_region->default_size);
> + if (!mrdump_region->dump) {
> + kfree(mdata);
> + result = -ENOMEM;
> + goto get_core_exit;
> + }
Error handling/cleanup paths need to be cleaned up here.
> + result = t7xx_devlink_fb_raw_command(T7XX_FB_CMD_OEM_MRDUMP, port, NULL);
> + if (result) {
> + dev_err(port->dev, "%s command failed\n", T7XX_FB_CMD_OEM_MRDUMP);
> + vfree(mrdump_region->dump);
> + kfree(mdata);
> + goto get_core_exit;
> + }
Ditto.
> + while (mrdump_region->default_size > offset_dlen) {
> + clen = t7xx_devlink_port_read(port, mcmd, sizeof(mcmd));
> + if (clen == strlen(T7XX_FB_CMD_RTS) &&
> + (!strncmp(mcmd, T7XX_FB_CMD_RTS, strlen(T7XX_FB_CMD_RTS)))) {
> + memset(mdata, 0, T7XX_FB_MDATA_SIZE);
> + dlen = 0;
Unnecessary assignment.
> + memset(mcmd, 0, sizeof(mcmd));
> + clen = snprintf(mcmd, sizeof(mcmd), "%s", T7XX_FB_CMD_CTS);
> +
> + if (t7xx_devlink_port_write(port, mcmd, clen) != clen) {
> + dev_err(port->dev, "write for _CTS failed:%d\n", clen);
> + goto get_core_free_mem;
> + }
> +
> + dlen = t7xx_devlink_port_read(port, mdata, T7XX_FB_MDATA_SIZE);
> + if (dlen <= 0) {
> + dev_err(port->dev, "read data error(%d)\n", dlen);
> + goto get_core_free_mem;
> + }
> +
> + zipsize += (unsigned long long)(dlen);
> + memcpy(mrdump_region->dump + offset_dlen, mdata, dlen);
> + offset_dlen += dlen;
Why both offset_dlen and zipsize???
> + memset(mcmd, 0, sizeof(mcmd));
> + clen = snprintf(mcmd, sizeof(mcmd), "%s", T7XX_FB_CMD_FIN);
> + if (t7xx_devlink_port_write(port, mcmd, clen) != clen) {
> + dev_err(port->dev, "%s: _FIN failed, (Read %05d:%05llu)\n",
> + __func__, clen, zipsize);
Printing __func__ probably isn't that helpful here.
> + goto get_core_free_mem;
> + }
> + } else if ((clen == strlen(T7XX_FB_RESP_MRDUMP_DONE)) &&
> + (!strncmp(mcmd, T7XX_FB_RESP_MRDUMP_DONE,
> + strlen(T7XX_FB_RESP_MRDUMP_DONE)))) {
strcmp()
> + dev_dbg(port->dev, "%s! size:%zd\n", T7XX_FB_RESP_MRDUMP_DONE, offset_dlen);
> + mrdump_region->actual_size = offset_dlen;
> + snprintf(mrdump_complete_event, sizeof(mrdump_complete_event),
> + "%s size=%zu", T7XX_UEVENT_MRDUMP_READY, offset_dlen);
> + t7xx_uevent_send(dl->dev, mrdump_complete_event);
> + kfree(mdata);
> + result = 0;
> + goto get_core_exit;
> + } else {
> + dev_err(port->dev, "getcore protocol error (read len %05d)\n", clen);
> + goto get_core_free_mem;
> + }
> + }
> +
> + dev_err(port->dev, "mrdump exceeds %uMB size. Discarded!", mrd_mb);
> + t7xx_uevent_send(port->dev, T7XX_UEVENT_MRD_DISCD);
> +
> +get_core_free_mem:
> + kfree(mdata);
> + vfree(mrdump_region->dump);
> + clear_bit(T7XX_MRDUMP_STATUS, &dl->status);
> + return -EPROTO;
> +
> +get_core_exit:
> + clear_bit(T7XX_MRDUMP_STATUS, &dl->status);
> + return result;
Error handling/cleanup paths need to be cleaned up here.
> +}
> +
> +static int t7xx_devlink_fb_dump_log(struct t7xx_port *port)
> +{
> + struct t7xx_devlink_region_info *lkdump_region;
> + char lkdump_complete_event[T7XX_FB_EVENT_SIZE];
> + struct t7xx_devlink *dl = port->dl;
> + int dlen, datasize = 0, result;
> + size_t offset_dlen = 0;
> + u8 *data;
> +
> + set_bit(T7XX_LKDUMP_STATUS, &dl->status);
> + result = t7xx_devlink_fb_raw_command(T7XX_FB_CMD_OEM_LKDUMP, port, &datasize);
> + if (result) {
> + dev_err(port->dev, "%s command returns failure\n", T7XX_FB_CMD_OEM_LKDUMP);
> + goto lkdump_exit;
> + }
> +
> + lkdump_region = dl->dl_region_info[T7XX_LKDUMP_INDEX];
> + if (datasize > lkdump_region->default_size) {
> + dev_err(port->dev, "lkdump size is more than %dKB. Discarded!",
> + T7XX_LKDUMP_SIZE / 1024);
> + t7xx_uevent_send(dl->dev, T7XX_UEVENT_LKD_DISCD);
> + result = -EPROTO;
> + goto lkdump_exit;
> + }
> +
> + data = kzalloc(datasize, GFP_KERNEL);
> + if (!data) {
> + result = -ENOMEM;
> + goto lkdump_exit;
> + }
> +
> + lkdump_region->dump = vmalloc(lkdump_region->default_size);
> + if (!lkdump_region->dump) {
> + kfree(data);
> + result = -ENOMEM;
> + goto lkdump_exit;
Ditto.
> + }
> +
> + while (datasize > 0) {
> + dlen = t7xx_devlink_port_read(port, data, datasize);
> + if (dlen <= 0) {
> + dev_err(port->dev, "lkdump read error ret = %d", dlen);
> + kfree(data);
> + result = -EPROTO;
> + goto lkdump_exit;
Ditto.
> + }
> +
> + memcpy(lkdump_region->dump + offset_dlen, data, dlen);
> + datasize -= dlen;
> + offset_dlen += dlen;
> + }
> +
> + dev_dbg(port->dev, "LKDUMP DONE! size:%zd\n", offset_dlen);
> + lkdump_region->actual_size = offset_dlen;
> + snprintf(lkdump_complete_event, sizeof(lkdump_complete_event), "%s size=%zu",
> + T7XX_UEVENT_LKDUMP_READY, offset_dlen);
> + t7xx_uevent_send(dl->dev, lkdump_complete_event);
> + kfree(data);
> + clear_bit(T7XX_LKDUMP_STATUS, &dl->status);
> + return t7xx_devlink_fb_handle_response(port, NULL);
> +
> +lkdump_exit:
> + clear_bit(T7XX_LKDUMP_STATUS, &dl->status);
> + return result;
> +}
> +/* To create regions for dump files */
> +static int t7xx_devlink_create_region(struct t7xx_devlink *dl)
> +{
> + struct devlink_region_ops *region_ops;
> + int rc, i;
> +
> + region_ops = dl->dl_region_ops;
> + for (i = 0; i < T7XX_TOTAL_REGIONS; i++) {
Perhaps this is a matter of taste thing but I'd use
ARRAY_SIZE(t7xx_devlink_region_list).
> + region_ops[i].name = t7xx_devlink_region_list[i].region_name;
> + region_ops[i].snapshot = t7xx_devlink_region_snapshot;
> + region_ops[i].destructor = vfree;
> + dl->dl_region[i] =
> + devlink_region_create(dl->dl_ctx, ®ion_ops[i], T7XX_MAX_SNAPSHOTS,
> + t7xx_devlink_region_list[i].default_size);
Indentation.
> + if (IS_ERR(dl->dl_region[i])) {
> + rc = PTR_ERR(dl->dl_region[i]);
> + dev_err(dl->dev, "devlink region fail,err %d", rc);
Please fix message formatting.
> + rc = t7xx_devlink_create_region(dl);
> + if (rc) {
> + dev_err(dl->dev, "devlink region creation failed, rc %d", rc);
Since this is user visible error, "rc" should be replaced with something
meaningful.
> +static int t7xx_devlink_init(struct t7xx_port *port)
> +{
> + struct t7xx_devlink_work *dl_work;
> + struct workqueue_struct *wq;
> +
> + dl_work = kmalloc(sizeof(*dl_work), GFP_KERNEL);
> + if (!dl_work)
> + return -ENOMEM;
> +
> + wq = create_workqueue("t7xx_devlink");
> + if (!wq) {
> + kfree(dl_work);
> + dev_err(port->dev, "create_workqueue failed\n");
> + return -ENODATA;
> + }
> +
> + INIT_WORK(&dl_work->work, t7xx_devlink_work_handler);
> + dl_work->port = port;
> + port->rx_length_th = T7XX_MAX_QUEUE_LENGTH;
> +
> + if (!t7xx_devlink_region_init(port, dl_work, wq))
> + return -ENOMEM;
Leaks?
> +static int t7xx_devlink_disable_chl(struct t7xx_port *port)
> +{
> + spin_lock(&port->port_update_lock);
> + port->chan_enable = false;
> + spin_unlock(&port->port_update_lock);
> +
> + return 0;
> +}
This is identical to t7xx_port_wwan_disable_chl().
> +struct t7xx_devlink_region_info {
> + char region_name[T7XX_MAX_REGION_NAME_LENGTH];
> + u32 default_size;
> + u32 actual_size;
> + u32 entry;
entry seems pretty useless, used only to show an index within a debug
message.
> diff --git a/drivers/net/wwan/t7xx/t7xx_reg.h b/drivers/net/wwan/t7xx/t7xx_reg.h
> index 60e025e57baa..3a758bf79a4e 100644
> --- a/drivers/net/wwan/t7xx/t7xx_reg.h
> +++ b/drivers/net/wwan/t7xx/t7xx_reg.h
> @@ -101,11 +101,17 @@ enum t7xx_pm_resume_state {
> PM_RESUME_REG_STATE_L2_EXP,
> };
>
> +enum host_event_e {
> + HOST_EVENT_INIT = 0,
> + FASTBOOT_DL_NOTY = 0x3,
NOTIFY?
--
i.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 4/5] net: wwan: t7xx: Enable devlink based fw flashing and coredump collection
2022-08-16 4:24 [PATCH net-next 4/5] net: wwan: t7xx: Enable devlink based fw flashing and coredump collection m.chetan.kumar
2022-08-18 14:33 ` Ilpo Järvinen
@ 2022-08-30 2:21 ` Sergey Ryazanov
2022-09-03 8:31 ` Kumar, M Chetan
1 sibling, 1 reply; 10+ messages in thread
From: Sergey Ryazanov @ 2022-08-30 2:21 UTC (permalink / raw)
To: M Chetan Kumar
Cc: netdev, Jakub Kicinski, David Miller, Johannes Berg,
Loic Poulain, Sudi, Krishna C, M Chetan Kumar, Intel Corporation,
Devegowda Chandrashekar, Mishra Soumya Prakash
On Tue, Aug 16, 2022 at 7:12 AM <m.chetan.kumar@intel.com> wrote:
> From: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
>
> This patch brings-in support for t7xx wwan device firmware flashing &
> coredump collection using devlink.
>
> Driver Registers with Devlink framework.
> Implements devlink ops flash_update callback that programs modem firmware.
> Creates region & snapshot required for device coredump log collection.
> On early detection of wwan device in fastboot mode driver sets up CLDMA0 HW
> tx/rx queues for raw data transfer then registers with devlink framework.
> Upon receiving firmware image & partition details driver sends fastboot
> commands for flashing the firmware.
>
> In this flow the fastboot command & response gets exchanged between driver
> and device. Once firmware flashing is success completion status is reported
> to user space application.
>
> Below is the devlink command usage for firmware flashing
>
> $devlink dev flash pci/$BDF file ABC.img component ABC
>
> Note: ABC.img is the firmware to be programmed to "ABC" partition.
>
> In case of coredump collection when wwan device encounters an exception
> it reboots & stays in fastboot mode for coredump collection by host driver.
> On detecting exception state driver collects the core dump, creates the
> devlink region & reports an event to user space application for dump
> collection. The user space application invokes devlink region read command
> for dump collection.
>
> Below are the devlink commands used for coredump collection.
>
> devlink region new pci/$BDF/mr_dump
> devlink region read pci/$BDF/mr_dump snapshot $ID address $ADD length $LEN
> devlink region del pci/$BDF/mr_dump snapshot $ID
>
> Signed-off-by: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
> Signed-off-by: Devegowda Chandrashekar <chandrashekar.devegowda@intel.com>
> Signed-off-by: Mishra Soumya Prakash <soumya.prakash.mishra@intel.com>
[skipped]
> diff --git a/drivers/net/wwan/t7xx/t7xx_pci.h b/drivers/net/wwan/t7xx/t7xx_pci.h
> index a87c4cae94ef..1017d21aad59 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];
> @@ -79,6 +80,7 @@ struct t7xx_pci_dev {
> spinlock_t md_pm_lock; /* Protects PCI resource lock */
> unsigned int sleep_disable_count;
> struct completion sleep_lock_acquire;
> + struct t7xx_devlink *dl;
> };
>
> enum t7xx_pm_id {
> diff --git a/drivers/net/wwan/t7xx/t7xx_port.h b/drivers/net/wwan/t7xx/t7xx_port.h
> index 6a96ee6d9449..070097a658d1 100644
> --- a/drivers/net/wwan/t7xx/t7xx_port.h
> +++ b/drivers/net/wwan/t7xx/t7xx_port.h
> @@ -129,6 +129,7 @@ struct t7xx_port {
> int rx_length_th;
> bool chan_enable;
> struct task_struct *thread;
> + struct t7xx_devlink *dl;
The devlink state container is the device wide entity, and the device
state container already carries a pointer to it. So why do we need a
pointer copy inside the port state container?
> };
>
> int t7xx_get_port_mtu(struct t7xx_port *port);
> 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..026a1db42f69
> --- /dev/null
> +++ b/drivers/net/wwan/t7xx/t7xx_port_devlink.c
> @@ -0,0 +1,705 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022, Intel Corporation.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/debugfs.h>
> +#include <linux/vmalloc.h>
> +
> +#include "t7xx_hif_cldma.h"
> +#include "t7xx_pci_rescan.h"
> +#include "t7xx_port_devlink.h"
> +#include "t7xx_port_proxy.h"
> +#include "t7xx_state_monitor.h"
> +#include "t7xx_uevent.h"
> +
> +static struct t7xx_devlink_region_info t7xx_devlink_region_list[T7XX_TOTAL_REGIONS] = {
> + {"mr_dump", T7XX_MRDUMP_SIZE},
> + {"lk_dump", T7XX_LKDUMP_SIZE},
> +};
This array probably should be const.
Also, region indexes can be used in the array initialization to
clearly state element relations with other arrays:
static const t7xx_devlink_region_info t7xx_devlink_region_infos[] = {
[T7XX_MRDUMP_INDEX] = {"mr_dump", T7XX_MRDURM_SIZE},
[T7XX_LDDUMP_INDEX] = {"ld_dump", T7XX_LKDUMP_SIZE},
};
> +static int t7xx_devlink_port_read(struct t7xx_port *port, char *buf, size_t count)
> +{
> + int ret = 0, read_len;
> + struct sk_buff *skb;
> +
> + spin_lock_irq(&port->rx_wq.lock);
> + if (skb_queue_empty(&port->rx_skb_list)) {
> + ret = wait_event_interruptible_locked_irq(port->rx_wq,
> + !skb_queue_empty(&port->rx_skb_list));
> + if (ret == -ERESTARTSYS) {
> + spin_unlock_irq(&port->rx_wq.lock);
> + return -EINTR;
> + }
> + }
> + skb = skb_dequeue(&port->rx_skb_list);
> + spin_unlock_irq(&port->rx_wq.lock);
> +
> + read_len = count > skb->len ? skb->len : count;
> + memcpy(buf, skb->data, read_len);
> + dev_kfree_skb(skb);
Here the call will lose the remaining packet data if the buffer is
less than the skb data. Should the driver keep the skb leftover data
for subsequent port read calls? E.g.
if (read_len < skb->len) {
skb_pull(skb, read_len);
skb_queue_head(&port->rx_skb_list, skb);
} else {
consume_skb(skb);
}
> +
> + return ret ? ret : read_len;
> +}
> +
> +static int t7xx_devlink_port_write(struct t7xx_port *port, const char *buf, size_t count)
> +{
> + const struct t7xx_port_conf *port_conf = port->port_conf;
> + size_t actual_count;
> + struct sk_buff *skb;
> + int ret, txq_mtu;
> +
> + txq_mtu = t7xx_get_port_mtu(port);
> + if (txq_mtu < 0)
> + return -EINVAL;
> +
> + actual_count = count > txq_mtu ? txq_mtu : count;
> + skb = __dev_alloc_skb(actual_count, GFP_KERNEL);
This way the function will lose data past the MTU boundary. Should the
fragmentation code be implemented here and should the
t7xx_devlink_fb_send_buffer() wrapper be dropped? Or maybe place
WARN_ON() here at least?
> + if (!skb)
> + return -ENOMEM;
> +
> + skb_put_data(skb, buf, actual_count);
> + ret = t7xx_port_send_raw_skb(port, skb);
> + if (ret) {
> + dev_err(port->dev, "write error on %s, size: %zu, ret: %d\n",
> + port_conf->name, actual_count, ret);
> + dev_kfree_skb(skb);
> + return ret;
> + }
> +
> + return actual_count;
> +}
> +
> +static int t7xx_devlink_fb_handle_response(struct t7xx_port *port, int *data)
> +{
> + int ret = 0, index = 0, return_data = 0, read_bytes;
> + char status[T7XX_FB_RESPONSE_SIZE + 1];
> +
> + while (index < T7XX_FB_RESP_COUNT) {
> + index++;
> + read_bytes = t7xx_devlink_port_read(port, status, T7XX_FB_RESPONSE_SIZE);
> + if (read_bytes < 0) {
> + dev_err(port->dev, "status read failed");
> + ret = -EIO;
> + break;
> + }
> +
> + status[read_bytes] = '\0';
> + if (!strncmp(status, T7XX_FB_RESP_INFO, strlen(T7XX_FB_RESP_INFO))) {
> + break;
> + } else if (!strncmp(status, T7XX_FB_RESP_OKAY, strlen(T7XX_FB_RESP_OKAY))) {
> + break;
> + } else if (!strncmp(status, T7XX_FB_RESP_FAIL, strlen(T7XX_FB_RESP_FAIL))) {
> + ret = -EPROTO;
> + break;
> + } else if (!strncmp(status, T7XX_FB_RESP_DATA, strlen(T7XX_FB_RESP_DATA))) {
> + if (data) {
> + if (!kstrtoint(status + strlen(T7XX_FB_RESP_DATA), 16,
> + &return_data)) {
> + *data = return_data;
> + } else {
> + dev_err(port->dev, "kstrtoint error!\n");
> + ret = -EPROTO;
> + }
> + }
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int t7xx_devlink_fb_raw_command(char *cmd, struct t7xx_port *port, int *data)
> +{
> + int ret, cmd_size = strlen(cmd);
> +
> + if (cmd_size > T7XX_FB_COMMAND_SIZE) {
Just curious, is T7XX_FB_COMMAND_SIZE a real hardware limitation or is
this just-in-case check?
> + dev_err(port->dev, "command length %d is long\n", cmd_size);
> + return -EINVAL;
> + }
> +
> + if (cmd_size != t7xx_devlink_port_write(port, cmd, cmd_size)) {
> + dev_err(port->dev, "raw command = %s write failed\n", cmd);
> + return -EIO;
> + }
> +
> + dev_dbg(port->dev, "raw command = %s written to the device\n", cmd);
> + ret = t7xx_devlink_fb_handle_response(port, data);
> + if (ret)
> + dev_err(port->dev, "raw command = %s response FAILURE:%d\n", cmd, ret);
> +
> + return ret;
> +}
> +
> +static int t7xx_devlink_fb_send_buffer(struct t7xx_port *port, const u8 *buf, size_t size)
> +{
> + size_t remaining = size, offset = 0, len;
> + int write_done;
> +
> + if (!size)
> + return -EINVAL;
> +
> + while (remaining) {
> + len = min_t(size_t, remaining, CLDMA_DEDICATED_Q_BUFF_SZ);
> + write_done = t7xx_devlink_port_write(port, buf + offset, len);
> +
> + if (write_done < 0) {
> + dev_err(port->dev, "write to device failed in %s", __func__);
> + return -EIO;
> + } else if (write_done != len) {
> + dev_err(port->dev, "write Error. Only %d/%zu bytes written",
> + write_done, len);
> + return -EIO;
> + }
> +
> + remaining -= len;
> + offset += len;
> + }
> +
> + return 0;
> +}
> +
> +static int t7xx_devlink_fb_download_command(struct t7xx_port *port, size_t size)
> +{
> + char download_command[T7XX_FB_COMMAND_SIZE];
> +
> + snprintf(download_command, sizeof(download_command), "%s:%08zx",
> + T7XX_FB_CMD_DOWNLOAD, size);
> + return t7xx_devlink_fb_raw_command(download_command, port, NULL);
> +}
> +
> +static int t7xx_devlink_fb_download(struct t7xx_port *port, const u8 *buf, size_t size)
> +{
> + int ret;
> +
> + if (size <= 0 || size > SIZE_MAX) {
> + dev_err(port->dev, "file is too large to download");
> + return -EINVAL;
> + }
> +
> + ret = t7xx_devlink_fb_download_command(port, size);
> + if (ret)
> + return ret;
> +
> + ret = t7xx_devlink_fb_send_buffer(port, buf, size);
> + if (ret)
> + return ret;
> +
> + return t7xx_devlink_fb_handle_response(port, NULL);
> +}
> +
> +static int t7xx_devlink_fb_flash(const char *cmd, struct t7xx_port *port)
> +{
> + char flash_command[T7XX_FB_COMMAND_SIZE];
> +
> + snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd);
> + return t7xx_devlink_fb_raw_command(flash_command, port, NULL);
> +}
> +
> +static int t7xx_devlink_fb_flash_partition(const char *partition, const u8 *buf,
> + struct t7xx_port *port, size_t size)
> +{
> + int ret;
> +
> + ret = t7xx_devlink_fb_download(port, buf, size);
> + if (ret)
> + return ret;
> +
> + return t7xx_devlink_fb_flash(partition, port);
> +}
> +
> +static int t7xx_devlink_fb_get_core(struct t7xx_port *port)
> +{
> + struct t7xx_devlink_region_info *mrdump_region;
> + char mrdump_complete_event[T7XX_FB_EVENT_SIZE];
> + u32 mrd_mb = T7XX_MRDUMP_SIZE / (1024 * 1024);
> + struct t7xx_devlink *dl = port->dl;
> + int clen, dlen = 0, result = 0;
> + unsigned long long zipsize = 0;
> + char mcmd[T7XX_FB_MCMD_SIZE];
> + size_t offset_dlen = 0;
> + char *mdata;
> +
> + set_bit(T7XX_MRDUMP_STATUS, &dl->status);
> + mdata = kmalloc(T7XX_FB_MDATA_SIZE, GFP_KERNEL);
> + if (!mdata) {
> + result = -ENOMEM;
> + goto get_core_exit;
> + }
> +
> + mrdump_region = dl->dl_region_info[T7XX_MRDUMP_INDEX];
> + mrdump_region->dump = vmalloc(mrdump_region->default_size);
Maybe move this allocation to the devlink initialization function to
make it symmetrical to the buffer freeing on devlink deinitialization?
> + if (!mrdump_region->dump) {
> + kfree(mdata);
> + result = -ENOMEM;
> + goto get_core_exit;
> + }
> +
> + result = t7xx_devlink_fb_raw_command(T7XX_FB_CMD_OEM_MRDUMP, port, NULL);
> + if (result) {
> + dev_err(port->dev, "%s command failed\n", T7XX_FB_CMD_OEM_MRDUMP);
> + vfree(mrdump_region->dump);
> + kfree(mdata);
> + goto get_core_exit;
> + }
> +
> + while (mrdump_region->default_size > offset_dlen) {
> + clen = t7xx_devlink_port_read(port, mcmd, sizeof(mcmd));
Just terminate the response string and you can use strcmp() below. E.g.
clen = t7xx_devlink_port_read(port, mcmd, sizeof(mcmd) - 1);
mcmd[clen] = '\0';
if (strcmp(mcmd, ....) != 0) {
...
} else if (strcmp(mcmd, ...) != 0) {
....
}
> + if (clen == strlen(T7XX_FB_CMD_RTS) &&
> + (!strncmp(mcmd, T7XX_FB_CMD_RTS, strlen(T7XX_FB_CMD_RTS)))) {
> + memset(mdata, 0, T7XX_FB_MDATA_SIZE);
> + dlen = 0;
> + memset(mcmd, 0, sizeof(mcmd));
> + clen = snprintf(mcmd, sizeof(mcmd), "%s", T7XX_FB_CMD_CTS);
> +
> + if (t7xx_devlink_port_write(port, mcmd, clen) != clen) {
This command string copying and sending can be simplified to:
t7xx_devlink_fb_raw_command(T7XX_FB_CMD_CTS, port, NULL)
> + dev_err(port->dev, "write for _CTS failed:%d\n", clen);
> + goto get_core_free_mem;
> + }
> +
> + dlen = t7xx_devlink_port_read(port, mdata, T7XX_FB_MDATA_SIZE);
> + if (dlen <= 0) {
> + dev_err(port->dev, "read data error(%d)\n", dlen);
> + goto get_core_free_mem;
> + }
> +
> + zipsize += (unsigned long long)(dlen);
> + memcpy(mrdump_region->dump + offset_dlen, mdata, dlen);
Why is this reading into the intermediate buffer needed?
t7xx_devlink_port_read() will copy the data from an skb to the buffer
using memcpy(). So why not just use the dump buffer with a proper
offer? E.g.
t7xx_devlink_port_read(..., mrdump_region->dump + offset_dlen,
mrdump_region->default_size - offset_dlen);
BTW, copying memory between the buffers without the dlen check can
potentially cause a buffer overflow.
> + offset_dlen += dlen;
> + memset(mcmd, 0, sizeof(mcmd));
> + clen = snprintf(mcmd, sizeof(mcmd), "%s", T7XX_FB_CMD_FIN);
> + if (t7xx_devlink_port_write(port, mcmd, clen) != clen) {
t7xx_devlink_fb_raw_command(T7XX_FB_CMD_FIN, port, NULL) ?
> + dev_err(port->dev, "%s: _FIN failed, (Read %05d:%05llu)\n",
> + __func__, clen, zipsize);
> + goto get_core_free_mem;
> + }
> + } else if ((clen == strlen(T7XX_FB_RESP_MRDUMP_DONE)) &&
> + (!strncmp(mcmd, T7XX_FB_RESP_MRDUMP_DONE,
> + strlen(T7XX_FB_RESP_MRDUMP_DONE)))) {
> + dev_dbg(port->dev, "%s! size:%zd\n", T7XX_FB_RESP_MRDUMP_DONE, offset_dlen);
> + mrdump_region->actual_size = offset_dlen;
> + snprintf(mrdump_complete_event, sizeof(mrdump_complete_event),
> + "%s size=%zu", T7XX_UEVENT_MRDUMP_READY, offset_dlen);
> + t7xx_uevent_send(dl->dev, mrdump_complete_event);
> + kfree(mdata);
> + result = 0;
> + goto get_core_exit;
> + } else {
> + dev_err(port->dev, "getcore protocol error (read len %05d)\n", clen);
> + goto get_core_free_mem;
> + }
> + }
> +
> + dev_err(port->dev, "mrdump exceeds %uMB size. Discarded!", mrd_mb);
> + t7xx_uevent_send(port->dev, T7XX_UEVENT_MRD_DISCD);
> +
> +get_core_free_mem:
> + kfree(mdata);
> + vfree(mrdump_region->dump);
> + clear_bit(T7XX_MRDUMP_STATUS, &dl->status);
> + return -EPROTO;
> +
> +get_core_exit:
> + clear_bit(T7XX_MRDUMP_STATUS, &dl->status);
> + return result;
> +}
> +
> +static int t7xx_devlink_fb_dump_log(struct t7xx_port *port)
> +{
> + struct t7xx_devlink_region_info *lkdump_region;
> + char lkdump_complete_event[T7XX_FB_EVENT_SIZE];
> + struct t7xx_devlink *dl = port->dl;
> + int dlen, datasize = 0, result;
> + size_t offset_dlen = 0;
> + u8 *data;
> +
> + set_bit(T7XX_LKDUMP_STATUS, &dl->status);
> + result = t7xx_devlink_fb_raw_command(T7XX_FB_CMD_OEM_LKDUMP, port, &datasize);
> + if (result) {
> + dev_err(port->dev, "%s command returns failure\n", T7XX_FB_CMD_OEM_LKDUMP);
> + goto lkdump_exit;
> + }
> +
> + lkdump_region = dl->dl_region_info[T7XX_LKDUMP_INDEX];
> + if (datasize > lkdump_region->default_size) {
> + dev_err(port->dev, "lkdump size is more than %dKB. Discarded!",
> + T7XX_LKDUMP_SIZE / 1024);
> + t7xx_uevent_send(dl->dev, T7XX_UEVENT_LKD_DISCD);
> + result = -EPROTO;
> + goto lkdump_exit;
> + }
> +
> + data = kzalloc(datasize, GFP_KERNEL);
> + if (!data) {
> + result = -ENOMEM;
> + goto lkdump_exit;
> + }
> +
> + lkdump_region->dump = vmalloc(lkdump_region->default_size);
> + if (!lkdump_region->dump) {
> + kfree(data);
> + result = -ENOMEM;
> + goto lkdump_exit;
> + }
> +
> + while (datasize > 0) {
> + dlen = t7xx_devlink_port_read(port, data, datasize);
> + if (dlen <= 0) {
> + dev_err(port->dev, "lkdump read error ret = %d", dlen);
> + kfree(data);
> + result = -EPROTO;
> + goto lkdump_exit;
> + }
> +
> + memcpy(lkdump_region->dump + offset_dlen, data, dlen);
> + datasize -= dlen;
> + offset_dlen += dlen;
> + }
> +
> + dev_dbg(port->dev, "LKDUMP DONE! size:%zd\n", offset_dlen);
> + lkdump_region->actual_size = offset_dlen;
> + snprintf(lkdump_complete_event, sizeof(lkdump_complete_event), "%s size=%zu",
> + T7XX_UEVENT_LKDUMP_READY, offset_dlen);
> + t7xx_uevent_send(dl->dev, lkdump_complete_event);
> + kfree(data);
> + clear_bit(T7XX_LKDUMP_STATUS, &dl->status);
> + return t7xx_devlink_fb_handle_response(port, NULL);
> +
> +lkdump_exit:
> + clear_bit(T7XX_LKDUMP_STATUS, &dl->status);
> + return result;
> +}
> +
> +static int t7xx_devlink_flash_update(struct devlink *devlink,
> + struct devlink_flash_update_params *params,
> + struct netlink_ext_ack *extack)
> +{
> + struct t7xx_devlink *dl = devlink_priv(devlink);
> + const char *component = params->component;
> + const struct firmware *fw = params->fw;
> + char flash_event[T7XX_FB_EVENT_SIZE];
> + struct t7xx_port *port;
> + int ret;
> +
> + port = dl->port;
> + if (port->dl->mode != T7XX_FB_DL_MODE) {
> + dev_err(port->dev, "Modem is not in fastboot download mode!");
> + ret = -EPERM;
> + goto err_out;
> + }
> +
> + if (dl->status != T7XX_DEVLINK_IDLE) {
> + dev_err(port->dev, "Modem is busy!");
> + ret = -EBUSY;
> + goto err_out;
> + }
> +
> + if (!component || !fw->data) {
> + ret = -EINVAL;
> + goto err_out;
> + }
> +
> + set_bit(T7XX_FLASH_STATUS, &dl->status);
> + dev_dbg(port->dev, "flash partition name:%s binary size:%zu\n", component, fw->size);
> + ret = t7xx_devlink_fb_flash_partition(component, fw->data, port, fw->size);
> + if (ret) {
> + devlink_flash_update_status_notify(devlink, "flashing failure!",
> + params->component, 0, 0);
> + snprintf(flash_event, sizeof(flash_event), "%s for [%s]",
> + T7XX_UEVENT_FLASHING_FAILURE, params->component);
> + } else {
> + devlink_flash_update_status_notify(devlink, "flashing success!",
> + params->component, 0, 0);
> + snprintf(flash_event, sizeof(flash_event), "%s for [%s]",
> + T7XX_UEVENT_FLASHING_SUCCESS, params->component);
> + }
> +
> + t7xx_uevent_send(dl->dev, flash_event);
> +
> +err_out:
> + clear_bit(T7XX_FLASH_STATUS, &dl->status);
> + return ret;
> +}
> +
> +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)
> +{
> + struct t7xx_devlink *dl = devlink_priv(devlink);
> +
> + switch (action) {
> + case DEVLINK_RELOAD_ACTION_DRIVER_REINIT:
> + dl->set_fastboot_dl = 1;
A devlink expert may correct me, but this use of the driver reload
action to implicitly switch to fastboot mode looks like an incorrect
API use (see the reload action description in
Documentation/networking/devlink/devlink-reload.rst).
Most probably, the driver should implement a devlink param that
controls the device operation mode: normal or fastboot. Or just one
boolean param 'fastboot' that enables/disables fastboot mode. And only
after explicitly switching the device mode, the user should fire the
driver/firmware reload command.
To me, this looks like a less surprising way for the user to switch
between normal and flashing modes.
> + return 0;
> + case DEVLINK_RELOAD_ACTION_FW_ACTIVATE:
> + return t7xx_devlink_fb_raw_command(T7XX_FB_CMD_REBOOT, dl->port, NULL);
> + default:
> + /* Unsupported action should not get to this function */
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +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)
> +{
> + struct t7xx_devlink *dl = devlink_priv(devlink);
> + *actions_performed = BIT(action);
> + switch (action) {
> + case DEVLINK_RELOAD_ACTION_DRIVER_REINIT:
> + case DEVLINK_RELOAD_ACTION_FW_ACTIVATE:
> + t7xx_rescan_queue_work(dl->mtk_dev->pdev);
> + return 0;
> + default:
> + /* Unsupported action should not get to this function */
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +/* Call back function for devlink ops */
> +static const struct devlink_ops devlink_flash_ops = {
> + .supported_flash_update_params = DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT,
There is no such flag since f94b606325c1 ("net: devlink: limit flash
component name to match version returned by info_get()").
> + .flash_update = t7xx_devlink_flash_update,
> + .reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT) |
> + BIT(DEVLINK_RELOAD_ACTION_FW_ACTIVATE),
> + .reload_down = t7xx_devlink_reload_down,
> + .reload_up = t7xx_devlink_reload_up,
> +};
> +
> +static int t7xx_devlink_region_snapshot(struct devlink *dl, const struct devlink_region_ops *ops,
> + struct netlink_ext_ack *extack, u8 **data)
> +{
> + struct t7xx_devlink_region_info *region_info = ops->priv;
> + struct t7xx_devlink *t7xx_dl = devlink_priv(dl);
> + u8 *snapshot_mem;
> +
> + if (t7xx_dl->status != T7XX_DEVLINK_IDLE) {
> + dev_err(t7xx_dl->dev, "Modem is busy!");
> + return -EBUSY;
> + }
> +
> + dev_dbg(t7xx_dl->dev, "accessed devlink region:%s index:%d", ops->name, region_info->entry);
Since the region info pointer is stored inside the ops private data
pointer, the region index can be evaluated using pointer arithmetic:
int idx = region_info - t7xx_devlink_region_list;
if (idx == T7XX_MRDUMP_INDEX) {
...
} else if (idx == T7XX_LKDUMP_INDEX) {
...
} else {
return -ENOENT;
}
> + if (!strncmp(ops->name, "mr_dump", strlen("mr_dump"))) {
> + if (!region_info->dump) {
> + dev_err(t7xx_dl->dev, "devlink region:%s dump memory is not valid!",
> + region_info->region_name);
> + return -ENOMEM;
> + }
> +
> + snapshot_mem = vmalloc(region_info->default_size);
> + if (!snapshot_mem)
> + return -ENOMEM;
> +
> + memcpy(snapshot_mem, region_info->dump, region_info->default_size);
> + *data = snapshot_mem;
> + } else if (!strncmp(ops->name, "lk_dump", strlen("lk_dump"))) {
> + int ret;
> +
> + ret = t7xx_devlink_fb_dump_log(t7xx_dl->port);
> + if (ret)
> + return ret;
> +
> + *data = region_info->dump;
> + }
> +
> + return 0;
> +}
> +
> +/* To create regions for dump files */
> +static int t7xx_devlink_create_region(struct t7xx_devlink *dl)
This function is entitled 'create_region', but it creates multiple
regionS at once. It is better to rename it to 'create_regionS'.
Am I right if I say that this code was copied from the iosm driver? I
am asking because the iosm devlink integration was merged too quickly
without proper review. My bad. And now I see that it suffers from the
same issues as noted below.
> +{
> + struct devlink_region_ops *region_ops;
> + int rc, i;
> +
> + region_ops = dl->dl_region_ops;
> + for (i = 0; i < T7XX_TOTAL_REGIONS; i++) {
> + region_ops[i].name = t7xx_devlink_region_list[i].region_name;
As Ilpo already said, it is a matter of taste how to design the loops,
but I had construct it like this:
BUILD_BUG_ON(ARRAY_SIZE(t7xx_devlink_region_list) > ARRAY_SIZE(dl->regions));
for (i = 0; i < ARRAY_SIZE(t7xx_devlink_region_list); ++i) {
region_ops = &dl->dl_region_ops[i];
region_ops->name = t7xx_devlink_region_list[i].name;
Please note the BUILD_BUG_ON() use: checking the sizes of related
arrays may save a lot of time in the future and helps to document this
relationship.
> + region_ops[i].snapshot = t7xx_devlink_region_snapshot;
> + region_ops[i].destructor = vfree;
> + dl->dl_region[i] =
> + devlink_region_create(dl->dl_ctx, ®ion_ops[i], T7XX_MAX_SNAPSHOTS,
> + t7xx_devlink_region_list[i].default_size);
indentation
> +
Odd empty line between the region creation call and the result check.
> + if (IS_ERR(dl->dl_region[i])) {
> + rc = PTR_ERR(dl->dl_region[i]);
> + dev_err(dl->dev, "devlink region fail,err %d", rc);
> + for ( ; i >= 0; i--)
> + devlink_region_destroy(dl->dl_region[i]);
> +
> + return rc;
> + }
> +
> + t7xx_devlink_region_list[i].entry = i;
> + region_ops[i].priv = t7xx_devlink_region_list + i;
> + }
> +
> + return 0;
> +}
> +
> +/* To Destroy devlink regions */
> +static void t7xx_devlink_destroy_region(struct t7xx_devlink *dl)
> +{
> + u8 i;
> +
> + for (i = 0; i < T7XX_TOTAL_REGIONS; i++)
> + devlink_region_destroy(dl->dl_region[i]);
> +}
> +
> +int t7xx_devlink_register(struct t7xx_pci_dev *t7xx_dev)
> +{
> + struct devlink *dl_ctx;
> +
> + dl_ctx = devlink_alloc(&devlink_flash_ops, sizeof(struct t7xx_devlink),
> + &t7xx_dev->pdev->dev);
> + if (!dl_ctx)
> + return -ENOMEM;
> +
> + devlink_set_features(dl_ctx, DEVLINK_F_RELOAD);
> + devlink_register(dl_ctx);
> + t7xx_dev->dl = devlink_priv(dl_ctx);
> + t7xx_dev->dl->dl_ctx = dl_ctx;
> +
> + return 0;
> +}
> +
> +void t7xx_devlink_unregister(struct t7xx_pci_dev *t7xx_dev)
> +{
> + struct devlink *dl_ctx = priv_to_devlink(t7xx_dev->dl);
> +
> + devlink_unregister(dl_ctx);
> + devlink_free(dl_ctx);
> +}
> +
> +/**
> + * t7xx_devlink_region_init - Initialize/register devlink to t7xx driver
> + * @port: Pointer to port structure
> + * @dw: Pointer to devlink work structure
> + * @wq: Pointer to devlink workqueue structure
> + *
> + * Returns: Pointer to t7xx_devlink on success and NULL on failure
> + */
> +static struct t7xx_devlink *t7xx_devlink_region_init(struct t7xx_port *port,
> + struct t7xx_devlink_work *dw,
> + struct workqueue_struct *wq)
This function is entitled 'region_init', but it contains the common
devlink initialization code. Probably some or all of its contents
should be moved to the caller function (t7xx_devlink_init) to
consolidate the initialization code.
> +{
> + struct t7xx_pci_dev *mtk_dev = port->t7xx_dev;
> + struct t7xx_devlink *dl = mtk_dev->dl;
> + int rc, i;
> +
> + dl->dl_ctx = mtk_dev->dl->dl_ctx;
> + dl->mtk_dev = mtk_dev;
> + dl->dev = &mtk_dev->pdev->dev;
> + dl->mode = T7XX_FB_NO_MODE;
> + dl->status = T7XX_DEVLINK_IDLE;
> + dl->dl_work = dw;
> + dl->dl_wq = wq;
> + for (i = 0; i < T7XX_TOTAL_REGIONS; i++) {
> + dl->dl_region_info[i] = &t7xx_devlink_region_list[i];
This assignment will lead to various hard-to-investigate issues once a
user connects a couple of modems to a host. Since the region_info
structure contains the run-time modified fields. See also comments
near the structure definition above.
> + dl->dl_region_info[i]->dump = NULL;
> + }
> + dl->port = port;
> + port->dl = dl;
> +
> + rc = t7xx_devlink_create_region(dl);
> + if (rc) {
> + dev_err(dl->dev, "devlink region creation failed, rc %d", rc);
> + return NULL;
> + }
> +
> + return dl;
> +}
> +
> +/**
> + * t7xx_devlink_region_deinit - To unintialize the devlink from T7XX driver.
> + * @dl: Devlink instance
> + */
> +static void t7xx_devlink_region_deinit(struct t7xx_devlink *dl)
> +{
> + dl->mode = T7XX_FB_NO_MODE;
> + t7xx_devlink_destroy_region(dl);
> +}
> +
> +static void t7xx_devlink_work_handler(struct work_struct *data)
> +{
> + struct t7xx_devlink_work *dl_work;
> +
> + dl_work = container_of(data, struct t7xx_devlink_work, work);
> + t7xx_devlink_fb_get_core(dl_work->port);
> +}
> +
> +static int t7xx_devlink_init(struct t7xx_port *port)
> +{
> + struct t7xx_devlink_work *dl_work;
> + struct workqueue_struct *wq;
> +
> + dl_work = kmalloc(sizeof(*dl_work), GFP_KERNEL);
> + if (!dl_work)
> + return -ENOMEM;
> +
> + wq = create_workqueue("t7xx_devlink");
> + if (!wq) {
> + kfree(dl_work);
> + dev_err(port->dev, "create_workqueue failed\n");
> + return -ENODATA;
> + }
> +
> + INIT_WORK(&dl_work->work, t7xx_devlink_work_handler);
> + dl_work->port = port;
> + port->rx_length_th = T7XX_MAX_QUEUE_LENGTH;
> +
> + if (!t7xx_devlink_region_init(port, dl_work, wq))
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static void t7xx_devlink_uninit(struct t7xx_port *port)
> +{
> + struct t7xx_devlink *dl = port->dl;
> + struct sk_buff *skb;
> + unsigned long flags;
> +
> + vfree(dl->dl_region_info[T7XX_MRDUMP_INDEX]->dump);
> + if (dl->dl_wq)
> + destroy_workqueue(dl->dl_wq);
> + kfree(dl->dl_work);
> +
> + t7xx_devlink_region_deinit(port->dl);
> + spin_lock_irqsave(&port->rx_skb_list.lock, flags);
> + while ((skb = __skb_dequeue(&port->rx_skb_list)) != NULL)
> + dev_kfree_skb(skb);
> + spin_unlock_irqrestore(&port->rx_skb_list.lock, flags);
skb_queue_purge(&port->rx_skb_list) ?
> +}
[skipped]
> 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..85384e40519e
> --- /dev/null
> +++ b/drivers/net/wwan/t7xx/t7xx_port_devlink.h
> @@ -0,0 +1,85 @@
> +/* SPDX-License-Identifier: GPL-2.0-only
> + *
> + * Copyright (c) 2022, Intel Corporation.
> + */
> +
> +#ifndef __T7XX_PORT_DEVLINK_H__
> +#define __T7XX_PORT_DEVLINK_H__
> +
> +#include <net/devlink.h>
> +
> +#include "t7xx_pci.h"
> +
> +#define T7XX_MAX_QUEUE_LENGTH 32
> +#define T7XX_FB_COMMAND_SIZE 64
> +#define T7XX_FB_RESPONSE_SIZE 64
> +#define T7XX_FB_MCMD_SIZE 64
> +#define T7XX_FB_MDATA_SIZE 1024
> +#define T7XX_FB_RESP_COUNT 30
> +
> +#define T7XX_FB_CMD_RTS "_RTS"
> +#define T7XX_FB_CMD_CTS "_CTS"
> +#define T7XX_FB_CMD_FIN "_FIN"
> +#define T7XX_FB_CMD_OEM_MRDUMP "oem mrdump"
> +#define T7XX_FB_CMD_OEM_LKDUMP "oem dump_pllk_log"
> +#define T7XX_FB_CMD_DOWNLOAD "download"
> +#define T7XX_FB_CMD_FLASH "flash"
> +#define T7XX_FB_CMD_REBOOT "reboot"
> +#define T7XX_FB_RESP_MRDUMP_DONE "MRDUMP08_DONE"
> +#define T7XX_FB_RESP_OKAY "OKAY"
> +#define T7XX_FB_RESP_FAIL "FAIL"
> +#define T7XX_FB_RESP_DATA "DATA"
> +#define T7XX_FB_RESP_INFO "INFO"
> +
> +#define T7XX_FB_EVENT_SIZE 50
> +
> +#define T7XX_MAX_SNAPSHOTS 1
> +#define T7XX_MAX_REGION_NAME_LENGTH 20
> +#define T7XX_MRDUMP_SIZE (160 * 1024 * 1024)
> +#define T7XX_LKDUMP_SIZE (256 * 1024)
> +#define T7XX_TOTAL_REGIONS 2
> +
> +#define T7XX_FLASH_STATUS 0
> +#define T7XX_MRDUMP_STATUS 1
> +#define T7XX_LKDUMP_STATUS 2
> +#define T7XX_DEVLINK_IDLE 0
> +
> +#define T7XX_FB_NO_MODE 0
> +#define T7XX_FB_DL_MODE 1
> +#define T7XX_FB_DUMP_MODE 2
> +
> +#define T7XX_MRDUMP_INDEX 0
> +#define T7XX_LKDUMP_INDEX 1
Maybe convert these macros to enum and use them more actively? E.g.
/* Internal region indexes */
enum t7xx_regions {
T7XX_REGION_MRDUMP,
T7XX_REGION_LKDUMP,
T7XX_REGIONS_NUM
};
> +struct t7xx_devlink_work {
> + struct work_struct work;
> + struct t7xx_port *port;
> +};
You can embed the _work_ structure into the t7xx_devlink structure, so
you do not need this ad hoc structure with all the dynamic memory
allocation and pointers juggling associated with it.
> +struct t7xx_devlink_region_info {
> + char region_name[T7XX_MAX_REGION_NAME_LENGTH];
> + u32 default_size;
> + u32 actual_size;
> + u32 entry;
> + u8 *dump;
> +};
This structure mixes static initialization data and run-time state.
Also, the set of arrays inside the t7xx_devlink structure makes the
code harder to read. What if we split this structure into a static
configuration structure and a runtime state container structure? And
place all runtime data (e.g. ops, devlink region pointer, etc.) into
this common state container.
struct t7xx_devlink_region_info {
const char *name;
size_t size;
};
struct t7xx_devlink_region {
const struct t7xx_devlink_region_info *info;
struct devlink_region_ops ops;
struct devlink_region *dlreg;
size_t data_len;
void *buf;
};
struct t7xx_devlink {
...
struct t7xx_devlink_region regions[T7XX_TOTAL_REGIONS];
};
So the initialization will become:
for (...) {
dl->region[i].info = &t7xx_devlink_regions[i];
dl->region[i].ops.name = dl->region[i].info->name;
dl->region[i].ops.priv = &dl->region[i];
...
}
And the region index always can be evaluated from the info pointer:
idx = ((struct t7xx_devlink_region *)ops->priv)->info - t7xx_devlink_regions;
> +struct t7xx_devlink {
> + struct t7xx_pci_dev *mtk_dev;
> + struct t7xx_port *port;
> + struct device *dev;
This field is unused.
> + struct devlink *dl_ctx;
> + struct t7xx_devlink_work *dl_work;
> + struct workqueue_struct *dl_wq;
> + struct t7xx_devlink_region_info *dl_region_info[T7XX_TOTAL_REGIONS];
> + struct devlink_region_ops dl_region_ops[T7XX_TOTAL_REGIONS];
> + struct devlink_region *dl_region[T7XX_TOTAL_REGIONS];
> + u8 mode;
> + unsigned long status;
> + int set_fastboot_dl;
> +};
> +
> +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__*/
[skipped]
> diff --git a/drivers/net/wwan/t7xx/t7xx_state_monitor.c b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
> index 9c222809371b..00e143c8d568 100644
> --- a/drivers/net/wwan/t7xx/t7xx_state_monitor.c
> +++ b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
[skipped]
> @@ -239,8 +252,16 @@ static void t7xx_lk_stage_event_handling(struct t7xx_fsm_ctl *ctl, unsigned int
> return;
> }
>
> + if (lk_event == LK_EVENT_CREATE_PD_PORT)
> + port->dl->mode = T7XX_FB_DUMP_MODE;
> + else
> + port->dl->mode = T7XX_FB_DL_MODE;
> port->port_conf->ops->enable_chl(port);
> t7xx_cldma_start(md_ctrl);
> + if (lk_event == LK_EVENT_CREATE_PD_PORT)
> + t7xx_uevent_send(dev, T7XX_UEVENT_MODEM_FASTBOOT_DUMP_MODE);
> + else
> + t7xx_uevent_send(dev, T7XX_UEVENT_MODEM_FASTBOOT_DL_MODE);
> break;
>
> case LK_EVENT_RESET:
[skipped]
> @@ -318,6 +349,7 @@ static void fsm_routine_ready(struct t7xx_fsm_ctl *ctl)
>
> ctl->curr_state = FSM_STATE_READY;
> t7xx_fsm_broadcast_ready_state(ctl);
> + t7xx_uevent_send(&md->t7xx_dev->pdev->dev, T7XX_UEVENT_MODEM_READY);
> t7xx_md_event_notify(md, FSM_READY);
> }
These UEVENT things look at least unrelated to the patch. If the
deriver is really need it, please factor out it into a separate patch
with a comment describing why userspace wants to see these events.
On the other hand, this looks like a custom tracing implementation. It
might be better to use simple debug messages instead or even the
tracing API, which is much more powerful than any uevent.
--
Sergey
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 4/5] net: wwan: t7xx: Enable devlink based fw flashing and coredump collection
2022-08-30 2:21 ` Sergey Ryazanov
@ 2022-09-03 8:31 ` Kumar, M Chetan
2022-09-06 23:24 ` Sergey Ryazanov
0 siblings, 1 reply; 10+ messages in thread
From: Kumar, M Chetan @ 2022-09-03 8:31 UTC (permalink / raw)
To: Sergey Ryazanov
Cc: netdev, Jakub Kicinski, David Miller, Johannes Berg,
Loic Poulain, Sudi, Krishna C, Intel Corporation,
Devegowda Chandrashekar, Mishra Soumya Prakash
On 8/30/2022 7:51 AM, Sergey Ryazanov wrote:
> On Tue, Aug 16, 2022 at 7:12 AM <m.chetan.kumar@intel.com> wrote:
>> From: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
>>
>> This patch brings-in support for t7xx wwan device firmware flashing &
>> coredump collection using devlink.
>>
>> Driver Registers with Devlink framework.
>> Implements devlink ops flash_update callback that programs modem firmware.
>> Creates region & snapshot required for device coredump log collection.
>> On early detection of wwan device in fastboot mode driver sets up CLDMA0 HW
>> tx/rx queues for raw data transfer then registers with devlink framework.
>> Upon receiving firmware image & partition details driver sends fastboot
>> commands for flashing the firmware.
>>
>> In this flow the fastboot command & response gets exchanged between driver
>> and device. Once firmware flashing is success completion status is reported
>> to user space application.
>>
>> Below is the devlink command usage for firmware flashing
>>
>> $devlink dev flash pci/$BDF file ABC.img component ABC
>>
>> Note: ABC.img is the firmware to be programmed to "ABC" partition.
>>
>> In case of coredump collection when wwan device encounters an exception
>> it reboots & stays in fastboot mode for coredump collection by host driver.
>> On detecting exception state driver collects the core dump, creates the
>> devlink region & reports an event to user space application for dump
>> collection. The user space application invokes devlink region read command
>> for dump collection.
>>
>> Below are the devlink commands used for coredump collection.
>>
>> devlink region new pci/$BDF/mr_dump
>> devlink region read pci/$BDF/mr_dump snapshot $ID address $ADD length $LEN
>> devlink region del pci/$BDF/mr_dump snapshot $ID
>>
>> Signed-off-by: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
>> Signed-off-by: Devegowda Chandrashekar <chandrashekar.devegowda@intel.com>
>> Signed-off-by: Mishra Soumya Prakash <soumya.prakash.mishra@intel.com>
>
> [skipped]
>
>> diff --git a/drivers/net/wwan/t7xx/t7xx_pci.h b/drivers/net/wwan/t7xx/t7xx_pci.h
>> index a87c4cae94ef..1017d21aad59 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];
>> @@ -79,6 +80,7 @@ struct t7xx_pci_dev {
>> spinlock_t md_pm_lock; /* Protects PCI resource lock */
>> unsigned int sleep_disable_count;
>> struct completion sleep_lock_acquire;
>> + struct t7xx_devlink *dl;
>> };
>>
>> enum t7xx_pm_id {
>> diff --git a/drivers/net/wwan/t7xx/t7xx_port.h b/drivers/net/wwan/t7xx/t7xx_port.h
>> index 6a96ee6d9449..070097a658d1 100644
>> --- a/drivers/net/wwan/t7xx/t7xx_port.h
>> +++ b/drivers/net/wwan/t7xx/t7xx_port.h
>> @@ -129,6 +129,7 @@ struct t7xx_port {
>> int rx_length_th;
>> bool chan_enable;
>> struct task_struct *thread;
>> + struct t7xx_devlink *dl;
>
> The devlink state container is the device wide entity, and the device
> state container already carries a pointer to it. So why do we need a
> pointer copy inside the port state container?
Will drop it.
>
>> };
>>
>> int t7xx_get_port_mtu(struct t7xx_port *port);
>> 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..026a1db42f69
>> --- /dev/null
>> +++ b/drivers/net/wwan/t7xx/t7xx_port_devlink.c
>> @@ -0,0 +1,705 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2022, Intel Corporation.
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/vmalloc.h>
>> +
>> +#include "t7xx_hif_cldma.h"
>> +#include "t7xx_pci_rescan.h"
>> +#include "t7xx_port_devlink.h"
>> +#include "t7xx_port_proxy.h"
>> +#include "t7xx_state_monitor.h"
>> +#include "t7xx_uevent.h"
>> +
>> +static struct t7xx_devlink_region_info t7xx_devlink_region_list[T7XX_TOTAL_REGIONS] = {
>> + {"mr_dump", T7XX_MRDUMP_SIZE},
>> + {"lk_dump", T7XX_LKDUMP_SIZE},
>> +};
>
> This array probably should be const.
>
> Also, region indexes can be used in the array initialization to
> clearly state element relations with other arrays:
>
> static const t7xx_devlink_region_info t7xx_devlink_region_infos[] = {
> [T7XX_MRDUMP_INDEX] = {"mr_dump", T7XX_MRDURM_SIZE},
> [T7XX_LDDUMP_INDEX] = {"ld_dump", T7XX_LKDUMP_SIZE},
> };
Ok. will change it as per your suggestion.
>
>> +static int t7xx_devlink_port_read(struct t7xx_port *port, char *buf, size_t count)
>> +{
>> + int ret = 0, read_len;
>> + struct sk_buff *skb;
>> +
>> + spin_lock_irq(&port->rx_wq.lock);
>> + if (skb_queue_empty(&port->rx_skb_list)) {
>> + ret = wait_event_interruptible_locked_irq(port->rx_wq,
>> + !skb_queue_empty(&port->rx_skb_list));
>> + if (ret == -ERESTARTSYS) {
>> + spin_unlock_irq(&port->rx_wq.lock);
>> + return -EINTR;
>> + }
>> + }
>> + skb = skb_dequeue(&port->rx_skb_list);
>> + spin_unlock_irq(&port->rx_wq.lock);
>> +
>> + read_len = count > skb->len ? skb->len : count;
>> + memcpy(buf, skb->data, read_len);
>> + dev_kfree_skb(skb);
>
> Here the call will lose the remaining packet data if the buffer is
> less than the skb data. Should the driver keep the skb leftover data
> for subsequent port read calls? E.g.
>
> if (read_len < skb->len) {
> skb_pull(skb, read_len);
> skb_queue_head(&port->rx_skb_list, skb);
> } else {
> consume_skb(skb);
> }
We should not discard leftover data.
Will consider your changes.
>
>> +
>> + return ret ? ret : read_len;
>> +}
>> +
>> +static int t7xx_devlink_port_write(struct t7xx_port *port, const char *buf, size_t count)
>> +{
>> + const struct t7xx_port_conf *port_conf = port->port_conf;
>> + size_t actual_count;
>> + struct sk_buff *skb;
>> + int ret, txq_mtu;
>> +
>> + txq_mtu = t7xx_get_port_mtu(port);
>> + if (txq_mtu < 0)
>> + return -EINVAL;
>> +
>> + actual_count = count > txq_mtu ? txq_mtu : count;
>> + skb = __dev_alloc_skb(actual_count, GFP_KERNEL);
>
> This way the function will lose data past the MTU boundary. Should the
> fragmentation code be implemented here and should the
> t7xx_devlink_fb_send_buffer() wrapper be dropped? Or maybe place
> WARN_ON() here at least?
Ok. Will drop t7xx_devlink_fb_send_buffer() and implement the
fragementation logic here.
>> + if (!skb)
>> + return -ENOMEM;
>> +
>> + skb_put_data(skb, buf, actual_count);
>> + ret = t7xx_port_send_raw_skb(port, skb);
>> + if (ret) {
>> + dev_err(port->dev, "write error on %s, size: %zu, ret: %d\n",
>> + port_conf->name, actual_count, ret);
>> + dev_kfree_skb(skb);
>> + return ret;
>> + }
>> +
>> + return actual_count;
>> +}
>> +
>> +static int t7xx_devlink_fb_handle_response(struct t7xx_port *port, int *data)
>> +{
>> + int ret = 0, index = 0, return_data = 0, read_bytes;
>> + char status[T7XX_FB_RESPONSE_SIZE + 1];
>> +
>> + while (index < T7XX_FB_RESP_COUNT) {
>> + index++;
>> + read_bytes = t7xx_devlink_port_read(port, status, T7XX_FB_RESPONSE_SIZE);
>> + if (read_bytes < 0) {
>> + dev_err(port->dev, "status read failed");
>> + ret = -EIO;
>> + break;
>> + }
>> +
>> + status[read_bytes] = '\0';
>> + if (!strncmp(status, T7XX_FB_RESP_INFO, strlen(T7XX_FB_RESP_INFO))) {
>> + break;
>> + } else if (!strncmp(status, T7XX_FB_RESP_OKAY, strlen(T7XX_FB_RESP_OKAY))) {
>> + break;
>> + } else if (!strncmp(status, T7XX_FB_RESP_FAIL, strlen(T7XX_FB_RESP_FAIL))) {
>> + ret = -EPROTO;
>> + break;
>> + } else if (!strncmp(status, T7XX_FB_RESP_DATA, strlen(T7XX_FB_RESP_DATA))) {
>> + if (data) {
>> + if (!kstrtoint(status + strlen(T7XX_FB_RESP_DATA), 16,
>> + &return_data)) {
>> + *data = return_data;
>> + } else {
>> + dev_err(port->dev, "kstrtoint error!\n");
>> + ret = -EPROTO;
>> + }
>> + }
>> + break;
>> + }
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int t7xx_devlink_fb_raw_command(char *cmd, struct t7xx_port *port, int *data)
>> +{
>> + int ret, cmd_size = strlen(cmd);
>> +
>> + if (cmd_size > T7XX_FB_COMMAND_SIZE) {
>
> Just curious, is T7XX_FB_COMMAND_SIZE a real hardware limitation or is
> this just-in-case check?
It is a just-in-case check.
So far fastboot commands supported are with in this len.
>
>> + dev_err(port->dev, "command length %d is long\n", cmd_size);
>> + return -EINVAL;
>> + }
>> +
>> + if (cmd_size != t7xx_devlink_port_write(port, cmd, cmd_size)) {
>> + dev_err(port->dev, "raw command = %s write failed\n", cmd);
>> + return -EIO;
>> + }
>> +
>> + dev_dbg(port->dev, "raw command = %s written to the device\n", cmd);
>> + ret = t7xx_devlink_fb_handle_response(port, data);
>> + if (ret)
>> + dev_err(port->dev, "raw command = %s response FAILURE:%d\n", cmd, ret);
>> +
>> + return ret;
>> +}
>> +
>> +static int t7xx_devlink_fb_send_buffer(struct t7xx_port *port, const u8 *buf, size_t size)
>> +{
>> + size_t remaining = size, offset = 0, len;
>> + int write_done;
>> +
>> + if (!size)
>> + return -EINVAL;
>> +
>> + while (remaining) {
>> + len = min_t(size_t, remaining, CLDMA_DEDICATED_Q_BUFF_SZ);
>> + write_done = t7xx_devlink_port_write(port, buf + offset, len);
>> +
>> + if (write_done < 0) {
>> + dev_err(port->dev, "write to device failed in %s", __func__);
>> + return -EIO;
>> + } else if (write_done != len) {
>> + dev_err(port->dev, "write Error. Only %d/%zu bytes written",
>> + write_done, len);
>> + return -EIO;
>> + }
>> +
>> + remaining -= len;
>> + offset += len;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int t7xx_devlink_fb_download_command(struct t7xx_port *port, size_t size)
>> +{
>> + char download_command[T7XX_FB_COMMAND_SIZE];
>> +
>> + snprintf(download_command, sizeof(download_command), "%s:%08zx",
>> + T7XX_FB_CMD_DOWNLOAD, size);
>> + return t7xx_devlink_fb_raw_command(download_command, port, NULL);
>> +}
>> +
>> +static int t7xx_devlink_fb_download(struct t7xx_port *port, const u8 *buf, size_t size)
>> +{
>> + int ret;
>> +
>> + if (size <= 0 || size > SIZE_MAX) {
>> + dev_err(port->dev, "file is too large to download");
>> + return -EINVAL;
>> + }
>> +
>> + ret = t7xx_devlink_fb_download_command(port, size);
>> + if (ret)
>> + return ret;
>> +
>> + ret = t7xx_devlink_fb_send_buffer(port, buf, size);
>> + if (ret)
>> + return ret;
>> +
>> + return t7xx_devlink_fb_handle_response(port, NULL);
>> +}
>> +
>> +static int t7xx_devlink_fb_flash(const char *cmd, struct t7xx_port *port)
>> +{
>> + char flash_command[T7XX_FB_COMMAND_SIZE];
>> +
>> + snprintf(flash_command, sizeof(flash_command), "%s:%s", T7XX_FB_CMD_FLASH, cmd);
>> + return t7xx_devlink_fb_raw_command(flash_command, port, NULL);
>> +}
>> +
>> +static int t7xx_devlink_fb_flash_partition(const char *partition, const u8 *buf,
>> + struct t7xx_port *port, size_t size)
>> +{
>> + int ret;
>> +
>> + ret = t7xx_devlink_fb_download(port, buf, size);
>> + if (ret)
>> + return ret;
>> +
>> + return t7xx_devlink_fb_flash(partition, port);
>> +}
>> +
>> +static int t7xx_devlink_fb_get_core(struct t7xx_port *port)
>> +{
>> + struct t7xx_devlink_region_info *mrdump_region;
>> + char mrdump_complete_event[T7XX_FB_EVENT_SIZE];
>> + u32 mrd_mb = T7XX_MRDUMP_SIZE / (1024 * 1024);
>> + struct t7xx_devlink *dl = port->dl;
>> + int clen, dlen = 0, result = 0;
>> + unsigned long long zipsize = 0;
>> + char mcmd[T7XX_FB_MCMD_SIZE];
>> + size_t offset_dlen = 0;
>> + char *mdata;
>> +
>> + set_bit(T7XX_MRDUMP_STATUS, &dl->status);
>> + mdata = kmalloc(T7XX_FB_MDATA_SIZE, GFP_KERNEL);
>> + if (!mdata) {
>> + result = -ENOMEM;
>> + goto get_core_exit;
>> + }
>> +
>> + mrdump_region = dl->dl_region_info[T7XX_MRDUMP_INDEX];
>> + mrdump_region->dump = vmalloc(mrdump_region->default_size);
>
> Maybe move this allocation to the devlink initialization function to
> make it symmetrical to the buffer freeing on devlink deinitialization?
Sure. Will move ->dump allocation to devlink init.
>
>> + if (!mrdump_region->dump) {
>> + kfree(mdata);
>> + result = -ENOMEM;
>> + goto get_core_exit;
>> + }
>> +
>> + result = t7xx_devlink_fb_raw_command(T7XX_FB_CMD_OEM_MRDUMP, port, NULL);
>> + if (result) {
>> + dev_err(port->dev, "%s command failed\n", T7XX_FB_CMD_OEM_MRDUMP);
>> + vfree(mrdump_region->dump);
>> + kfree(mdata);
>> + goto get_core_exit;
>> + }
>> +
>> + while (mrdump_region->default_size > offset_dlen) {
>> + clen = t7xx_devlink_port_read(port, mcmd, sizeof(mcmd));
>
> Just terminate the response string and you can use strcmp() below. E.g.
>
> clen = t7xx_devlink_port_read(port, mcmd, sizeof(mcmd) - 1);
> mcmd[clen] = '\0';
> if (strcmp(mcmd, ....) != 0) {
> ...
> } else if (strcmp(mcmd, ...) != 0) {
> ....
> }
Ok. Will change it as per your suggestion.
>
>> + if (clen == strlen(T7XX_FB_CMD_RTS) &&
>> + (!strncmp(mcmd, T7XX_FB_CMD_RTS, strlen(T7XX_FB_CMD_RTS)))) {
>> + memset(mdata, 0, T7XX_FB_MDATA_SIZE);
>> + dlen = 0;
>> + memset(mcmd, 0, sizeof(mcmd));
>> + clen = snprintf(mcmd, sizeof(mcmd), "%s", T7XX_FB_CMD_CTS);
>> +
>> + if (t7xx_devlink_port_write(port, mcmd, clen) != clen) {
>
> This command string copying and sending can be simplified to:
>
> t7xx_devlink_fb_raw_command(T7XX_FB_CMD_CTS, port, NULL)
Ok. Will change it as per your suggestion.
>
>> + dev_err(port->dev, "write for _CTS failed:%d\n", clen);
>> + goto get_core_free_mem;
>> + }
>> +
>> + dlen = t7xx_devlink_port_read(port, mdata, T7XX_FB_MDATA_SIZE);
>> + if (dlen <= 0) {
>> + dev_err(port->dev, "read data error(%d)\n", dlen);
>> + goto get_core_free_mem;
>> + }
>> +
>> + zipsize += (unsigned long long)(dlen);
>> + memcpy(mrdump_region->dump + offset_dlen, mdata, dlen);
>
> Why is this reading into the intermediate buffer needed?
> t7xx_devlink_port_read() will copy the data from an skb to the buffer
> using memcpy(). So why not just use the dump buffer with a proper
> offer? E.g.
>
> t7xx_devlink_port_read(..., mrdump_region->dump + offset_dlen,
> mrdump_region->default_size - offset_dlen);
>
> BTW, copying memory between the buffers without the dlen check can
> potentially cause a buffer overflow.
Its not required.
Will make changes as per your suggestion.
>
>> + offset_dlen += dlen;
>> + memset(mcmd, 0, sizeof(mcmd));
>> + clen = snprintf(mcmd, sizeof(mcmd), "%s", T7XX_FB_CMD_FIN);
>> + if (t7xx_devlink_port_write(port, mcmd, clen) != clen) {
>
> t7xx_devlink_fb_raw_command(T7XX_FB_CMD_FIN, port, NULL) ?
>
>> + dev_err(port->dev, "%s: _FIN failed, (Read %05d:%05llu)\n",
>> + __func__, clen, zipsize);
>> + goto get_core_free_mem;
>> + }
>> + } else if ((clen == strlen(T7XX_FB_RESP_MRDUMP_DONE)) &&
>> + (!strncmp(mcmd, T7XX_FB_RESP_MRDUMP_DONE,
>> + strlen(T7XX_FB_RESP_MRDUMP_DONE)))) {
>> + dev_dbg(port->dev, "%s! size:%zd\n", T7XX_FB_RESP_MRDUMP_DONE, offset_dlen);
>> + mrdump_region->actual_size = offset_dlen;
>> + snprintf(mrdump_complete_event, sizeof(mrdump_complete_event),
>> + "%s size=%zu", T7XX_UEVENT_MRDUMP_READY, offset_dlen);
>> + t7xx_uevent_send(dl->dev, mrdump_complete_event);
>> + kfree(mdata);
>> + result = 0;
>> + goto get_core_exit;
>> + } else {
>> + dev_err(port->dev, "getcore protocol error (read len %05d)\n", clen);
>> + goto get_core_free_mem;
>> + }
>> + }
>> +
>> + dev_err(port->dev, "mrdump exceeds %uMB size. Discarded!", mrd_mb);
>> + t7xx_uevent_send(port->dev, T7XX_UEVENT_MRD_DISCD);
>> +
>> +get_core_free_mem:
>> + kfree(mdata);
>> + vfree(mrdump_region->dump);
>> + clear_bit(T7XX_MRDUMP_STATUS, &dl->status);
>> + return -EPROTO;
>> +
>> +get_core_exit:
>> + clear_bit(T7XX_MRDUMP_STATUS, &dl->status);
>> + return result;
>> +}
>> +
>> +static int t7xx_devlink_fb_dump_log(struct t7xx_port *port)
>> +{
>> + struct t7xx_devlink_region_info *lkdump_region;
>> + char lkdump_complete_event[T7XX_FB_EVENT_SIZE];
>> + struct t7xx_devlink *dl = port->dl;
>> + int dlen, datasize = 0, result;
>> + size_t offset_dlen = 0;
>> + u8 *data;
>> +
>> + set_bit(T7XX_LKDUMP_STATUS, &dl->status);
>> + result = t7xx_devlink_fb_raw_command(T7XX_FB_CMD_OEM_LKDUMP, port, &datasize);
>> + if (result) {
>> + dev_err(port->dev, "%s command returns failure\n", T7XX_FB_CMD_OEM_LKDUMP);
>> + goto lkdump_exit;
>> + }
>> +
>> + lkdump_region = dl->dl_region_info[T7XX_LKDUMP_INDEX];
>> + if (datasize > lkdump_region->default_size) {
>> + dev_err(port->dev, "lkdump size is more than %dKB. Discarded!",
>> + T7XX_LKDUMP_SIZE / 1024);
>> + t7xx_uevent_send(dl->dev, T7XX_UEVENT_LKD_DISCD);
>> + result = -EPROTO;
>> + goto lkdump_exit;
>> + }
>> +
>> + data = kzalloc(datasize, GFP_KERNEL);
>> + if (!data) {
>> + result = -ENOMEM;
>> + goto lkdump_exit;
>> + }
>> +
>> + lkdump_region->dump = vmalloc(lkdump_region->default_size);
>> + if (!lkdump_region->dump) {
>> + kfree(data);
>> + result = -ENOMEM;
>> + goto lkdump_exit;
>> + }
>> +
>> + while (datasize > 0) {
>> + dlen = t7xx_devlink_port_read(port, data, datasize);
>> + if (dlen <= 0) {
>> + dev_err(port->dev, "lkdump read error ret = %d", dlen);
>> + kfree(data);
>> + result = -EPROTO;
>> + goto lkdump_exit;
>> + }
>> +
>> + memcpy(lkdump_region->dump + offset_dlen, data, dlen);
>> + datasize -= dlen;
>> + offset_dlen += dlen;
>> + }
>> +
>> + dev_dbg(port->dev, "LKDUMP DONE! size:%zd\n", offset_dlen);
>> + lkdump_region->actual_size = offset_dlen;
>> + snprintf(lkdump_complete_event, sizeof(lkdump_complete_event), "%s size=%zu",
>> + T7XX_UEVENT_LKDUMP_READY, offset_dlen);
>> + t7xx_uevent_send(dl->dev, lkdump_complete_event);
>> + kfree(data);
>> + clear_bit(T7XX_LKDUMP_STATUS, &dl->status);
>> + return t7xx_devlink_fb_handle_response(port, NULL);
>> +
>> +lkdump_exit:
>> + clear_bit(T7XX_LKDUMP_STATUS, &dl->status);
>> + return result;
>> +}
>> +
>> +static int t7xx_devlink_flash_update(struct devlink *devlink,
>> + struct devlink_flash_update_params *params,
>> + struct netlink_ext_ack *extack)
>> +{
>> + struct t7xx_devlink *dl = devlink_priv(devlink);
>> + const char *component = params->component;
>> + const struct firmware *fw = params->fw;
>> + char flash_event[T7XX_FB_EVENT_SIZE];
>> + struct t7xx_port *port;
>> + int ret;
>> +
>> + port = dl->port;
>> + if (port->dl->mode != T7XX_FB_DL_MODE) {
>> + dev_err(port->dev, "Modem is not in fastboot download mode!");
>> + ret = -EPERM;
>> + goto err_out;
>> + }
>> +
>> + if (dl->status != T7XX_DEVLINK_IDLE) {
>> + dev_err(port->dev, "Modem is busy!");
>> + ret = -EBUSY;
>> + goto err_out;
>> + }
>> +
>> + if (!component || !fw->data) {
>> + ret = -EINVAL;
>> + goto err_out;
>> + }
>> +
>> + set_bit(T7XX_FLASH_STATUS, &dl->status);
>> + dev_dbg(port->dev, "flash partition name:%s binary size:%zu\n", component, fw->size);
>> + ret = t7xx_devlink_fb_flash_partition(component, fw->data, port, fw->size);
>> + if (ret) {
>> + devlink_flash_update_status_notify(devlink, "flashing failure!",
>> + params->component, 0, 0);
>> + snprintf(flash_event, sizeof(flash_event), "%s for [%s]",
>> + T7XX_UEVENT_FLASHING_FAILURE, params->component);
>> + } else {
>> + devlink_flash_update_status_notify(devlink, "flashing success!",
>> + params->component, 0, 0);
>> + snprintf(flash_event, sizeof(flash_event), "%s for [%s]",
>> + T7XX_UEVENT_FLASHING_SUCCESS, params->component);
>> + }
>> +
>> + t7xx_uevent_send(dl->dev, flash_event);
>> +
>> +err_out:
>> + clear_bit(T7XX_FLASH_STATUS, &dl->status);
>> + return ret;
>> +}
>> +
>> +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)
>> +{
>> + struct t7xx_devlink *dl = devlink_priv(devlink);
>> +
>> + switch (action) {
>> + case DEVLINK_RELOAD_ACTION_DRIVER_REINIT:
>> + dl->set_fastboot_dl = 1;
>
> A devlink expert may correct me, but this use of the driver reload
> action to implicitly switch to fastboot mode looks like an incorrect
> API use (see the reload action description in
> Documentation/networking/devlink/devlink-reload.rst).
>
> Most probably, the driver should implement a devlink param that
> controls the device operation mode: normal or fastboot. Or just one
> boolean param 'fastboot' that enables/disables fastboot mode. And only
> after explicitly switching the device mode, the user should fire the
> driver/firmware reload command.
Ok. will implement new devlink 'fastboot' param to control device
operational mode.
>
> To me, this looks like a less surprising way for the user to switch
> between normal and flashing modes.
>
>> + return 0;
>> + case DEVLINK_RELOAD_ACTION_FW_ACTIVATE:
>> + return t7xx_devlink_fb_raw_command(T7XX_FB_CMD_REBOOT, dl->port, NULL);
>> + default:
>> + /* Unsupported action should not get to this function */
>> + return -EOPNOTSUPP;
>> + }
>> +}
>> +
>> +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)
>> +{
>> + struct t7xx_devlink *dl = devlink_priv(devlink);
>> + *actions_performed = BIT(action);
>> + switch (action) {
>> + case DEVLINK_RELOAD_ACTION_DRIVER_REINIT:
>> + case DEVLINK_RELOAD_ACTION_FW_ACTIVATE:
>> + t7xx_rescan_queue_work(dl->mtk_dev->pdev);
>> + return 0;
>> + default:
>> + /* Unsupported action should not get to this function */
>> + return -EOPNOTSUPP;
>> + }
>> +}
>> +
>> +/* Call back function for devlink ops */
>> +static const struct devlink_ops devlink_flash_ops = {
>> + .supported_flash_update_params = DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT,
>
> There is no such flag since f94b606325c1 ("net: devlink: limit flash
> component name to match version returned by info_get()").
Thanks for pointing.
Author gave heads-up on this changeset. We are working on it.
>
>> + .flash_update = t7xx_devlink_flash_update,
>> + .reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT) |
>> + BIT(DEVLINK_RELOAD_ACTION_FW_ACTIVATE),
>> + .reload_down = t7xx_devlink_reload_down,
>> + .reload_up = t7xx_devlink_reload_up,
>> +};
>> +
>> +static int t7xx_devlink_region_snapshot(struct devlink *dl, const struct devlink_region_ops *ops,
>> + struct netlink_ext_ack *extack, u8 **data)
>> +{
>> + struct t7xx_devlink_region_info *region_info = ops->priv;
>> + struct t7xx_devlink *t7xx_dl = devlink_priv(dl);
>> + u8 *snapshot_mem;
>> +
>> + if (t7xx_dl->status != T7XX_DEVLINK_IDLE) {
>> + dev_err(t7xx_dl->dev, "Modem is busy!");
>> + return -EBUSY;
>> + }
>> +
>> + dev_dbg(t7xx_dl->dev, "accessed devlink region:%s index:%d", ops->name, region_info->entry);
>
> Since the region info pointer is stored inside the ops private data
> pointer, the region index can be evaluated using pointer arithmetic:
>
> int idx = region_info - t7xx_devlink_region_list;
>
> if (idx == T7XX_MRDUMP_INDEX) {
> ...
> } else if (idx == T7XX_LKDUMP_INDEX) {
> ...
> } else {
> return -ENOENT;
> }
Will drop entry from region list.
>
>> + if (!strncmp(ops->name, "mr_dump", strlen("mr_dump"))) {
>> + if (!region_info->dump) {
>> + dev_err(t7xx_dl->dev, "devlink region:%s dump memory is not valid!",
>> + region_info->region_name);
>> + return -ENOMEM;
>> + }
>> +
>> + snapshot_mem = vmalloc(region_info->default_size);
>> + if (!snapshot_mem)
>> + return -ENOMEM;
>> +
>> + memcpy(snapshot_mem, region_info->dump, region_info->default_size);
>> + *data = snapshot_mem;
>> + } else if (!strncmp(ops->name, "lk_dump", strlen("lk_dump"))) {
>> + int ret;
>> +
>> + ret = t7xx_devlink_fb_dump_log(t7xx_dl->port);
>> + if (ret)
>> + return ret;
>> +
>> + *data = region_info->dump;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/* To create regions for dump files */
>> +static int t7xx_devlink_create_region(struct t7xx_devlink *dl)
>
> This function is entitled 'create_region', but it creates multiple
> regionS at once. It is better to rename it to 'create_regionS'.
Ok. will rename it to create_regions.
>
> Am I right if I say that this code was copied from the iosm driver? I
> am asking because the iosm devlink integration was merged too quickly
> without proper review. My bad. And now I see that it suffers from the
> same issues as noted below.
>
>> +{
>> + struct devlink_region_ops *region_ops;
>> + int rc, i;
>> +
>> + region_ops = dl->dl_region_ops;
>> + for (i = 0; i < T7XX_TOTAL_REGIONS; i++) {
>> + region_ops[i].name = t7xx_devlink_region_list[i].region_name;
>
> As Ilpo already said, it is a matter of taste how to design the loops,
> but I had construct it like this:
>
> BUILD_BUG_ON(ARRAY_SIZE(t7xx_devlink_region_list) > ARRAY_SIZE(dl->regions));
> for (i = 0; i < ARRAY_SIZE(t7xx_devlink_region_list); ++i) {
> region_ops = &dl->dl_region_ops[i];
> region_ops->name = t7xx_devlink_region_list[i].name;
>
> Please note the BUILD_BUG_ON() use: checking the sizes of related
> arrays may save a lot of time in the future and helps to document this
> relationship.
Sure. will consider it.
>
>> + region_ops[i].snapshot = t7xx_devlink_region_snapshot;
>> + region_ops[i].destructor = vfree;
>> + dl->dl_region[i] =
>> + devlink_region_create(dl->dl_ctx, ®ion_ops[i], T7XX_MAX_SNAPSHOTS,
>> + t7xx_devlink_region_list[i].default_size);
>
> indentation
>
>> +
>
> Odd empty line between the region creation call and the result check.
Will fix it.
>
>> + if (IS_ERR(dl->dl_region[i])) {
>> + rc = PTR_ERR(dl->dl_region[i]);
>> + dev_err(dl->dev, "devlink region fail,err %d", rc);
>> + for ( ; i >= 0; i--)
>> + devlink_region_destroy(dl->dl_region[i]);
>> +
>> + return rc;
>> + }
>> +
>> + t7xx_devlink_region_list[i].entry = i;
>> + region_ops[i].priv = t7xx_devlink_region_list + i;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/* To Destroy devlink regions */
>> +static void t7xx_devlink_destroy_region(struct t7xx_devlink *dl)
>> +{
>> + u8 i;
>> +
>> + for (i = 0; i < T7XX_TOTAL_REGIONS; i++)
>> + devlink_region_destroy(dl->dl_region[i]);
>> +}
>> +
>> +int t7xx_devlink_register(struct t7xx_pci_dev *t7xx_dev)
>> +{
>> + struct devlink *dl_ctx;
>> +
>> + dl_ctx = devlink_alloc(&devlink_flash_ops, sizeof(struct t7xx_devlink),
>> + &t7xx_dev->pdev->dev);
>> + if (!dl_ctx)
>> + return -ENOMEM;
>> +
>> + devlink_set_features(dl_ctx, DEVLINK_F_RELOAD);
>> + devlink_register(dl_ctx);
>> + t7xx_dev->dl = devlink_priv(dl_ctx);
>> + t7xx_dev->dl->dl_ctx = dl_ctx;
>> +
>> + return 0;
>> +}
>> +
>> +void t7xx_devlink_unregister(struct t7xx_pci_dev *t7xx_dev)
>> +{
>> + struct devlink *dl_ctx = priv_to_devlink(t7xx_dev->dl);
>> +
>> + devlink_unregister(dl_ctx);
>> + devlink_free(dl_ctx);
>> +}
>> +
>> +/**
>> + * t7xx_devlink_region_init - Initialize/register devlink to t7xx driver
>> + * @port: Pointer to port structure
>> + * @dw: Pointer to devlink work structure
>> + * @wq: Pointer to devlink workqueue structure
>> + *
>> + * Returns: Pointer to t7xx_devlink on success and NULL on failure
>> + */
>> +static struct t7xx_devlink *t7xx_devlink_region_init(struct t7xx_port *port,
>> + struct t7xx_devlink_work *dw,
>> + struct workqueue_struct *wq)
>
> This function is entitled 'region_init', but it contains the common
> devlink initialization code. Probably some or all of its contents
> should be moved to the caller function (t7xx_devlink_init) to
> consolidate the initialization code.
Sure. will refactor it.
>
>> +{
>> + struct t7xx_pci_dev *mtk_dev = port->t7xx_dev;
>> + struct t7xx_devlink *dl = mtk_dev->dl;
>> + int rc, i;
>> +
>> + dl->dl_ctx = mtk_dev->dl->dl_ctx;
>> + dl->mtk_dev = mtk_dev;
>> + dl->dev = &mtk_dev->pdev->dev;
>> + dl->mode = T7XX_FB_NO_MODE;
>> + dl->status = T7XX_DEVLINK_IDLE;
>> + dl->dl_work = dw;
>> + dl->dl_wq = wq;
>> + for (i = 0; i < T7XX_TOTAL_REGIONS; i++) {
>> + dl->dl_region_info[i] = &t7xx_devlink_region_list[i];
>
> This assignment will lead to various hard-to-investigate issues once a
> user connects a couple of modems to a host. Since the region_info
> structure contains the run-time modified fields. See also comments
> near the structure definition above.
Sure. will refactor code as per your suggestion.
>
>> + dl->dl_region_info[i]->dump = NULL;
>> + }
>> + dl->port = port;
>> + port->dl = dl;
>> +
>> + rc = t7xx_devlink_create_region(dl);
>> + if (rc) {
>> + dev_err(dl->dev, "devlink region creation failed, rc %d", rc);
>> + return NULL;
>> + }
>> +
>> + return dl;
>> +}
>> +
>> +/**
>> + * t7xx_devlink_region_deinit - To unintialize the devlink from T7XX driver.
>> + * @dl: Devlink instance
>> + */
>> +static void t7xx_devlink_region_deinit(struct t7xx_devlink *dl)
>> +{
>> + dl->mode = T7XX_FB_NO_MODE;
>> + t7xx_devlink_destroy_region(dl);
>> +}
>> +
>> +static void t7xx_devlink_work_handler(struct work_struct *data)
>> +{
>> + struct t7xx_devlink_work *dl_work;
>> +
>> + dl_work = container_of(data, struct t7xx_devlink_work, work);
>> + t7xx_devlink_fb_get_core(dl_work->port);
>> +}
>> +
>> +static int t7xx_devlink_init(struct t7xx_port *port)
>> +{
>> + struct t7xx_devlink_work *dl_work;
>> + struct workqueue_struct *wq;
>> +
>> + dl_work = kmalloc(sizeof(*dl_work), GFP_KERNEL);
>> + if (!dl_work)
>> + return -ENOMEM;
>> +
>> + wq = create_workqueue("t7xx_devlink");
>> + if (!wq) {
>> + kfree(dl_work);
>> + dev_err(port->dev, "create_workqueue failed\n");
>> + return -ENODATA;
>> + }
>> +
>> + INIT_WORK(&dl_work->work, t7xx_devlink_work_handler);
>> + dl_work->port = port;
>> + port->rx_length_th = T7XX_MAX_QUEUE_LENGTH;
>> +
>> + if (!t7xx_devlink_region_init(port, dl_work, wq))
>> + return -ENOMEM;
>> +
>> + return 0;
>> +}
>> +
>> +static void t7xx_devlink_uninit(struct t7xx_port *port)
>> +{
>> + struct t7xx_devlink *dl = port->dl;
>> + struct sk_buff *skb;
>> + unsigned long flags;
>> +
>> + vfree(dl->dl_region_info[T7XX_MRDUMP_INDEX]->dump);
>> + if (dl->dl_wq)
>> + destroy_workqueue(dl->dl_wq);
>> + kfree(dl->dl_work);
>> +
>> + t7xx_devlink_region_deinit(port->dl);
>> + spin_lock_irqsave(&port->rx_skb_list.lock, flags);
>> + while ((skb = __skb_dequeue(&port->rx_skb_list)) != NULL)
>> + dev_kfree_skb(skb);
>> + spin_unlock_irqrestore(&port->rx_skb_list.lock, flags);
>
> skb_queue_purge(&port->rx_skb_list) ?
Ok. Will replace it.
>
>> +}
>
> [skipped]
>
>> 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..85384e40519e
>> --- /dev/null
>> +++ b/drivers/net/wwan/t7xx/t7xx_port_devlink.h
>> @@ -0,0 +1,85 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only
>> + *
>> + * Copyright (c) 2022, Intel Corporation.
>> + */
>> +
>> +#ifndef __T7XX_PORT_DEVLINK_H__
>> +#define __T7XX_PORT_DEVLINK_H__
>> +
>> +#include <net/devlink.h>
>> +
>> +#include "t7xx_pci.h"
>> +
>> +#define T7XX_MAX_QUEUE_LENGTH 32
>> +#define T7XX_FB_COMMAND_SIZE 64
>> +#define T7XX_FB_RESPONSE_SIZE 64
>> +#define T7XX_FB_MCMD_SIZE 64
>> +#define T7XX_FB_MDATA_SIZE 1024
>> +#define T7XX_FB_RESP_COUNT 30
>> +
>> +#define T7XX_FB_CMD_RTS "_RTS"
>> +#define T7XX_FB_CMD_CTS "_CTS"
>> +#define T7XX_FB_CMD_FIN "_FIN"
>> +#define T7XX_FB_CMD_OEM_MRDUMP "oem mrdump"
>> +#define T7XX_FB_CMD_OEM_LKDUMP "oem dump_pllk_log"
>> +#define T7XX_FB_CMD_DOWNLOAD "download"
>> +#define T7XX_FB_CMD_FLASH "flash"
>> +#define T7XX_FB_CMD_REBOOT "reboot"
>> +#define T7XX_FB_RESP_MRDUMP_DONE "MRDUMP08_DONE"
>> +#define T7XX_FB_RESP_OKAY "OKAY"
>> +#define T7XX_FB_RESP_FAIL "FAIL"
>> +#define T7XX_FB_RESP_DATA "DATA"
>> +#define T7XX_FB_RESP_INFO "INFO"
>> +
>> +#define T7XX_FB_EVENT_SIZE 50
>> +
>> +#define T7XX_MAX_SNAPSHOTS 1
>> +#define T7XX_MAX_REGION_NAME_LENGTH 20
>> +#define T7XX_MRDUMP_SIZE (160 * 1024 * 1024)
>> +#define T7XX_LKDUMP_SIZE (256 * 1024)
>> +#define T7XX_TOTAL_REGIONS 2
>> +
>> +#define T7XX_FLASH_STATUS 0
>> +#define T7XX_MRDUMP_STATUS 1
>> +#define T7XX_LKDUMP_STATUS 2
>> +#define T7XX_DEVLINK_IDLE 0
>> +
>> +#define T7XX_FB_NO_MODE 0
>> +#define T7XX_FB_DL_MODE 1
>> +#define T7XX_FB_DUMP_MODE 2
>> +
>> +#define T7XX_MRDUMP_INDEX 0
>> +#define T7XX_LKDUMP_INDEX 1
>
> Maybe convert these macros to enum and use them more actively? E.g.
>
> /* Internal region indexes */
> enum t7xx_regions {
> T7XX_REGION_MRDUMP,
> T7XX_REGION_LKDUMP,
> T7XX_REGIONS_NUM
> };
>
>> +struct t7xx_devlink_work {
>> + struct work_struct work;
>> + struct t7xx_port *port;
>> +};
>
> You can embed the _work_ structure into the t7xx_devlink structure, so
> you do not need this ad hoc structure with all the dynamic memory
> allocation and pointers juggling associated with it.
>
>> +struct t7xx_devlink_region_info {
>> + char region_name[T7XX_MAX_REGION_NAME_LENGTH];
>> + u32 default_size;
>> + u32 actual_size;
>> + u32 entry;
>> + u8 *dump;
>> +};
>
> This structure mixes static initialization data and run-time state.
> Also, the set of arrays inside the t7xx_devlink structure makes the
> code harder to read. What if we split this structure into a static
> configuration structure and a runtime state container structure? And
> place all runtime data (e.g. ops, devlink region pointer, etc.) into
> this common state container.
>
> struct t7xx_devlink_region_info {
> const char *name;
> size_t size;
> };
>
> struct t7xx_devlink_region {
> const struct t7xx_devlink_region_info *info;
> struct devlink_region_ops ops;
> struct devlink_region *dlreg;
> size_t data_len;
> void *buf;
> };
>
> struct t7xx_devlink {
> ...
> struct t7xx_devlink_region regions[T7XX_TOTAL_REGIONS];
> };
>
> So the initialization will become:
>
> for (...) {
> dl->region[i].info = &t7xx_devlink_regions[i];
> dl->region[i].ops.name = dl->region[i].info->name;
> dl->region[i].ops.priv = &dl->region[i];
> ...
> }
>
> And the region index always can be evaluated from the info pointer:
>
> idx = ((struct t7xx_devlink_region *)ops->priv)->info - t7xx_devlink_regions;
Thank you for the details.
Will refactor code as per your suggestion.
>
>> +struct t7xx_devlink {
>> + struct t7xx_pci_dev *mtk_dev;
>> + struct t7xx_port *port;
>> + struct device *dev;
>
> This field is unused.
devlink.c is using it in few place. But in many places we are referring
dev from port container.
Will drop it and refer to port container instead.
>
>> + struct devlink *dl_ctx;
>> + struct t7xx_devlink_work *dl_work;
>> + struct workqueue_struct *dl_wq;
>> + struct t7xx_devlink_region_info *dl_region_info[T7XX_TOTAL_REGIONS];
>> + struct devlink_region_ops dl_region_ops[T7XX_TOTAL_REGIONS];
>> + struct devlink_region *dl_region[T7XX_TOTAL_REGIONS];
>> + u8 mode;
>> + unsigned long status;
>> + int set_fastboot_dl;
>> +};
>> +
>> +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__*/
>
> [skipped]
>
>> diff --git a/drivers/net/wwan/t7xx/t7xx_state_monitor.c b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>> index 9c222809371b..00e143c8d568 100644
>> --- a/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>> +++ b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>
> [skipped]
>
>> @@ -239,8 +252,16 @@ static void t7xx_lk_stage_event_handling(struct t7xx_fsm_ctl *ctl, unsigned int
>> return;
>> }
>>
>> + if (lk_event == LK_EVENT_CREATE_PD_PORT)
>> + port->dl->mode = T7XX_FB_DUMP_MODE;
>> + else
>> + port->dl->mode = T7XX_FB_DL_MODE;
>> port->port_conf->ops->enable_chl(port);
>> t7xx_cldma_start(md_ctrl);
>> + if (lk_event == LK_EVENT_CREATE_PD_PORT)
>> + t7xx_uevent_send(dev, T7XX_UEVENT_MODEM_FASTBOOT_DUMP_MODE);
>> + else
>> + t7xx_uevent_send(dev, T7XX_UEVENT_MODEM_FASTBOOT_DL_MODE);
>> break;
>>
>> case LK_EVENT_RESET:
>
> [skipped]
>
>> @@ -318,6 +349,7 @@ static void fsm_routine_ready(struct t7xx_fsm_ctl *ctl)
>>
>> ctl->curr_state = FSM_STATE_READY;
>> t7xx_fsm_broadcast_ready_state(ctl);
>> + t7xx_uevent_send(&md->t7xx_dev->pdev->dev, T7XX_UEVENT_MODEM_READY);
>> t7xx_md_event_notify(md, FSM_READY);
>> }
>
> These UEVENT things look at least unrelated to the patch. If the
> deriver is really need it, please factor out it into a separate patch
> with a comment describing why userspace wants to see these events.
>
> On the other hand, this looks like a custom tracing implementation. It
> might be better to use simple debug messages instead or even the
> tracing API, which is much more powerful than any uevent.
Driver is reporting modem status (up, down, exception, etc) via uevent.
The wwan user space services use these states for taking some action.
So we have choose uevent for reporting modem status to user space.
Is it ok we retain this logic ? I will drop it from this patch and send
it as a separate patch for review.
--
Chetan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 4/5] net: wwan: t7xx: Enable devlink based fw flashing and coredump collection
2022-08-18 14:33 ` Ilpo Järvinen
@ 2022-09-06 10:15 ` Kumar, M Chetan
2022-09-06 10:31 ` Ilpo Järvinen
0 siblings, 1 reply; 10+ messages in thread
From: Kumar, M Chetan @ 2022-09-06 10:15 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Netdev, kuba, davem, johannes, ryazanov.s.a, loic.poulain,
krishna.c.sudi, linuxwwan, Devegowda Chandrashekar,
Mishra Soumya Prakash
Thank you Ilpo for reviewing patches.
Will address patch4 & patch5 review comments in v2.
On 8/18/2022 8:03 PM, Ilpo Järvinen wrote:
> On Tue, 16 Aug 2022, m.chetan.kumar@intel.com wrote:
>
>> From: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
>>
>> This patch brings-in support for t7xx wwan device firmware flashing &
>
> Just say "Add support for ..."
>
>> coredump collection using devlink.
>>
>> Driver Registers with Devlink framework.
>> Implements devlink ops flash_update callback that programs modem firmware.
>> Creates region & snapshot required for device coredump log collection.
>> On early detection of wwan device in fastboot mode driver sets up CLDMA0 HW
>> tx/rx queues for raw data transfer then registers with devlink framework.
>> Upon receiving firmware image & partition details driver sends fastboot
>> commands for flashing the firmware.
>
> Things don't seem to connect well between sentences in this paragraph.
>
>> In this flow the fastboot command & response gets exchanged between driver
>> and device. Once firmware flashing is success completion status is reported
>> to user space application.
>>
>> Below is the devlink command usage for firmware flashing
>>
>> $devlink dev flash pci/$BDF file ABC.img component ABC
>>
>> Note: ABC.img is the firmware to be programmed to "ABC" partition.
>>
>> In case of coredump collection when wwan device encounters an exception
>> it reboots & stays in fastboot mode for coredump collection by host driver.
>> On detecting exception state driver collects the core dump, creates the
>> devlink region & reports an event to user space application for dump
>> collection. The user space application invokes devlink region read command
>> for dump collection.
>>
>> Below are the devlink commands used for coredump collection.
>>
>> devlink region new pci/$BDF/mr_dump
>> devlink region read pci/$BDF/mr_dump snapshot $ID address $ADD length $LEN
>> devlink region del pci/$BDF/mr_dump snapshot $ID
>>
>> Signed-off-by: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
>> Signed-off-by: Devegowda Chandrashekar <chandrashekar.devegowda@intel.com>
>> Signed-off-by: Mishra Soumya Prakash <soumya.prakash.mishra@intel.com>
>> ---
>
>> +static int t7xx_devlink_port_read(struct t7xx_port *port, char *buf, size_t count)
>> +{
>> + int ret = 0, read_len;
>> + struct sk_buff *skb;
>> +
>> + spin_lock_irq(&port->rx_wq.lock);
>> + if (skb_queue_empty(&port->rx_skb_list)) {
>> + ret = wait_event_interruptible_locked_irq(port->rx_wq,
>> + !skb_queue_empty(&port->rx_skb_list));
>> + if (ret == -ERESTARTSYS) {
>> + spin_unlock_irq(&port->rx_wq.lock);
>> + return -EINTR;
>> + }
>> + }
>> + skb = skb_dequeue(&port->rx_skb_list);
>> + spin_unlock_irq(&port->rx_wq.lock);
>> +
>> + read_len = count > skb->len ? skb->len : count;
>
> max_t()
>
>> + memcpy(buf, skb->data, read_len);
>> + dev_kfree_skb(skb);
>> +
>> + return ret ? ret : read_len;
>
> Can ret actually be non-zero here since -ERESTARTSYS is covered above?
>
>> +}
>> +
>> +static int t7xx_devlink_port_write(struct t7xx_port *port, const char *buf, size_t count)
>> +{
>> + const struct t7xx_port_conf *port_conf = port->port_conf;
>> + size_t actual_count;
>> + struct sk_buff *skb;
>> + int ret, txq_mtu;
>> +
>> + txq_mtu = t7xx_get_port_mtu(port);
>> + if (txq_mtu < 0)
>> + return -EINVAL;
>> +
>> + actual_count = count > txq_mtu ? txq_mtu : count;
>
> max_t()
>
>> + skb = __dev_alloc_skb(actual_count, GFP_KERNEL);
>> + if (!skb)
>> + return -ENOMEM;
>> +
>> + skb_put_data(skb, buf, actual_count);
>> + ret = t7xx_port_send_raw_skb(port, skb);
>> + if (ret) {
>> + dev_err(port->dev, "write error on %s, size: %zu, ret: %d\n",
>> + port_conf->name, actual_count, ret);
>> + dev_kfree_skb(skb);
>> + return ret;
>> + }
>> +
>> + return actual_count;
>> +}
>> +
>> +static int t7xx_devlink_fb_handle_response(struct t7xx_port *port, int *data)
>> +{
>> + int ret = 0, index = 0, return_data = 0, read_bytes;
>
> I'd move return_data declaration into block where it is used. I don't
> think it needs to be initialized or does the compiler complain about it?
>
>
>> + char status[T7XX_FB_RESPONSE_SIZE + 1];
>> +
>> + while (index < T7XX_FB_RESP_COUNT) {
>> + index++;
>
> for (index = 0; index < T7XX_FB_RESP_COUNT; index++) {
>
>> + read_bytes = t7xx_devlink_port_read(port, status, T7XX_FB_RESPONSE_SIZE);
>> + if (read_bytes < 0) {
>> + dev_err(port->dev, "status read failed");
>
> Printing "read failed" for -ERESTARTSYS/-EINTR??
>
>> +static int t7xx_devlink_fb_download(struct t7xx_port *port, const u8 *buf, size_t size)
>> +{
>> + int ret;
>> +
>> + if (size <= 0 || size > SIZE_MAX) {
>> + dev_err(port->dev, "file is too large to download");
>> + return -EINVAL;
>
> The if condition is effectively if (!size) because unsigned cannot be <0
> nor can size_t be > (~(size_t)0). Given that, the error message is pretty
> bogus. I'd tend to think returning just -EINVAL in case of !size w/o any
> printingis sufficient here.
>
>> +static int t7xx_devlink_fb_get_core(struct t7xx_port *port)
>> +{
>> + struct t7xx_devlink_region_info *mrdump_region;
>> + char mrdump_complete_event[T7XX_FB_EVENT_SIZE];
>> + u32 mrd_mb = T7XX_MRDUMP_SIZE / (1024 * 1024);
>> + struct t7xx_devlink *dl = port->dl;
>> + int clen, dlen = 0, result = 0;
>
> No need to init dlen.
>
>> + unsigned long long zipsize = 0;
>> + char mcmd[T7XX_FB_MCMD_SIZE];
>> + size_t offset_dlen = 0;
>> + char *mdata;
>> +
>> + set_bit(T7XX_MRDUMP_STATUS, &dl->status);
>> + mdata = kmalloc(T7XX_FB_MDATA_SIZE, GFP_KERNEL);
>> + if (!mdata) {
>> + result = -ENOMEM;
>> + goto get_core_exit;
>> + }
>> +
>> + mrdump_region = dl->dl_region_info[T7XX_MRDUMP_INDEX];
>> + mrdump_region->dump = vmalloc(mrdump_region->default_size);
>> + if (!mrdump_region->dump) {
>> + kfree(mdata);
>> + result = -ENOMEM;
>> + goto get_core_exit;
>> + }
>
> Error handling/cleanup paths need to be cleaned up here.
>
>> + result = t7xx_devlink_fb_raw_command(T7XX_FB_CMD_OEM_MRDUMP, port, NULL);
>> + if (result) {
>> + dev_err(port->dev, "%s command failed\n", T7XX_FB_CMD_OEM_MRDUMP);
>> + vfree(mrdump_region->dump);
>> + kfree(mdata);
>> + goto get_core_exit;
>> + }
>
> Ditto.
>
>> + while (mrdump_region->default_size > offset_dlen) {
>> + clen = t7xx_devlink_port_read(port, mcmd, sizeof(mcmd));
>> + if (clen == strlen(T7XX_FB_CMD_RTS) &&
>> + (!strncmp(mcmd, T7XX_FB_CMD_RTS, strlen(T7XX_FB_CMD_RTS)))) {
>> + memset(mdata, 0, T7XX_FB_MDATA_SIZE);
>> + dlen = 0;
>
> Unnecessary assignment.
>
>> + memset(mcmd, 0, sizeof(mcmd));
>> + clen = snprintf(mcmd, sizeof(mcmd), "%s", T7XX_FB_CMD_CTS);
>> +
>> + if (t7xx_devlink_port_write(port, mcmd, clen) != clen) {
>> + dev_err(port->dev, "write for _CTS failed:%d\n", clen);
>> + goto get_core_free_mem;
>> + }
>> +
>> + dlen = t7xx_devlink_port_read(port, mdata, T7XX_FB_MDATA_SIZE);
>> + if (dlen <= 0) {
>> + dev_err(port->dev, "read data error(%d)\n", dlen);
>> + goto get_core_free_mem;
>> + }
>> +
>> + zipsize += (unsigned long long)(dlen);
>> + memcpy(mrdump_region->dump + offset_dlen, mdata, dlen);
>> + offset_dlen += dlen;
>
> Why both offset_dlen and zipsize???
>
>> + memset(mcmd, 0, sizeof(mcmd));
>> + clen = snprintf(mcmd, sizeof(mcmd), "%s", T7XX_FB_CMD_FIN);
>> + if (t7xx_devlink_port_write(port, mcmd, clen) != clen) {
>> + dev_err(port->dev, "%s: _FIN failed, (Read %05d:%05llu)\n",
>> + __func__, clen, zipsize);
>
> Printing __func__ probably isn't that helpful here.
>
>> + goto get_core_free_mem;
>> + }
>> + } else if ((clen == strlen(T7XX_FB_RESP_MRDUMP_DONE)) &&
>> + (!strncmp(mcmd, T7XX_FB_RESP_MRDUMP_DONE,
>> + strlen(T7XX_FB_RESP_MRDUMP_DONE)))) {
>
> strcmp()
>
>> + dev_dbg(port->dev, "%s! size:%zd\n", T7XX_FB_RESP_MRDUMP_DONE, offset_dlen);
>> + mrdump_region->actual_size = offset_dlen;
>> + snprintf(mrdump_complete_event, sizeof(mrdump_complete_event),
>> + "%s size=%zu", T7XX_UEVENT_MRDUMP_READY, offset_dlen);
>> + t7xx_uevent_send(dl->dev, mrdump_complete_event);
>> + kfree(mdata);
>> + result = 0;
>> + goto get_core_exit;
>> + } else {
>> + dev_err(port->dev, "getcore protocol error (read len %05d)\n", clen);
>> + goto get_core_free_mem;
>> + }
>> + }
>> +
>> + dev_err(port->dev, "mrdump exceeds %uMB size. Discarded!", mrd_mb);
>> + t7xx_uevent_send(port->dev, T7XX_UEVENT_MRD_DISCD);
>> +
>> +get_core_free_mem:
>> + kfree(mdata);
>> + vfree(mrdump_region->dump);
>> + clear_bit(T7XX_MRDUMP_STATUS, &dl->status);
>> + return -EPROTO;
>> +
>> +get_core_exit:
>> + clear_bit(T7XX_MRDUMP_STATUS, &dl->status);
>> + return result;
>
> Error handling/cleanup paths need to be cleaned up here.
>
>> +}
>> +
>> +static int t7xx_devlink_fb_dump_log(struct t7xx_port *port)
>> +{
>> + struct t7xx_devlink_region_info *lkdump_region;
>> + char lkdump_complete_event[T7XX_FB_EVENT_SIZE];
>> + struct t7xx_devlink *dl = port->dl;
>> + int dlen, datasize = 0, result;
>> + size_t offset_dlen = 0;
>> + u8 *data;
>> +
>> + set_bit(T7XX_LKDUMP_STATUS, &dl->status);
>> + result = t7xx_devlink_fb_raw_command(T7XX_FB_CMD_OEM_LKDUMP, port, &datasize);
>> + if (result) {
>> + dev_err(port->dev, "%s command returns failure\n", T7XX_FB_CMD_OEM_LKDUMP);
>> + goto lkdump_exit;
>> + }
>> +
>> + lkdump_region = dl->dl_region_info[T7XX_LKDUMP_INDEX];
>> + if (datasize > lkdump_region->default_size) {
>> + dev_err(port->dev, "lkdump size is more than %dKB. Discarded!",
>> + T7XX_LKDUMP_SIZE / 1024);
>> + t7xx_uevent_send(dl->dev, T7XX_UEVENT_LKD_DISCD);
>> + result = -EPROTO;
>> + goto lkdump_exit;
>> + }
>> +
>> + data = kzalloc(datasize, GFP_KERNEL);
>> + if (!data) {
>> + result = -ENOMEM;
>> + goto lkdump_exit;
>> + }
>> +
>> + lkdump_region->dump = vmalloc(lkdump_region->default_size);
>> + if (!lkdump_region->dump) {
>> + kfree(data);
>> + result = -ENOMEM;
>> + goto lkdump_exit;
>
> Ditto.
>
>> + }
>> +
>> + while (datasize > 0) {
>> + dlen = t7xx_devlink_port_read(port, data, datasize);
>> + if (dlen <= 0) {
>> + dev_err(port->dev, "lkdump read error ret = %d", dlen);
>> + kfree(data);
>> + result = -EPROTO;
>> + goto lkdump_exit;
>
> Ditto.
>
>> + }
>> +
>> + memcpy(lkdump_region->dump + offset_dlen, data, dlen);
>> + datasize -= dlen;
>> + offset_dlen += dlen;
>> + }
>> +
>> + dev_dbg(port->dev, "LKDUMP DONE! size:%zd\n", offset_dlen);
>> + lkdump_region->actual_size = offset_dlen;
>> + snprintf(lkdump_complete_event, sizeof(lkdump_complete_event), "%s size=%zu",
>> + T7XX_UEVENT_LKDUMP_READY, offset_dlen);
>> + t7xx_uevent_send(dl->dev, lkdump_complete_event);
>> + kfree(data);
>> + clear_bit(T7XX_LKDUMP_STATUS, &dl->status);
>> + return t7xx_devlink_fb_handle_response(port, NULL);
>> +
>> +lkdump_exit:
>> + clear_bit(T7XX_LKDUMP_STATUS, &dl->status);
>> + return result;
>> +}
>
>
>> +/* To create regions for dump files */
>> +static int t7xx_devlink_create_region(struct t7xx_devlink *dl)
>> +{
>> + struct devlink_region_ops *region_ops;
>> + int rc, i;
>> +
>> + region_ops = dl->dl_region_ops;
>> + for (i = 0; i < T7XX_TOTAL_REGIONS; i++) {
>
> Perhaps this is a matter of taste thing but I'd use
> ARRAY_SIZE(t7xx_devlink_region_list).
>
>> + region_ops[i].name = t7xx_devlink_region_list[i].region_name;
>> + region_ops[i].snapshot = t7xx_devlink_region_snapshot;
>> + region_ops[i].destructor = vfree;
>> + dl->dl_region[i] =
>> + devlink_region_create(dl->dl_ctx, ®ion_ops[i], T7XX_MAX_SNAPSHOTS,
>> + t7xx_devlink_region_list[i].default_size);
>
> Indentation.
>
>> + if (IS_ERR(dl->dl_region[i])) {
>> + rc = PTR_ERR(dl->dl_region[i]);
>> + dev_err(dl->dev, "devlink region fail,err %d", rc);
>
> Please fix message formatting.
>
>> + rc = t7xx_devlink_create_region(dl);
>> + if (rc) {
>> + dev_err(dl->dev, "devlink region creation failed, rc %d", rc);
>
> Since this is user visible error, "rc" should be replaced with something
> meaningful.
>
>
>> +static int t7xx_devlink_init(struct t7xx_port *port)
>> +{
>> + struct t7xx_devlink_work *dl_work;
>> + struct workqueue_struct *wq;
>> +
>> + dl_work = kmalloc(sizeof(*dl_work), GFP_KERNEL);
>> + if (!dl_work)
>> + return -ENOMEM;
>> +
>> + wq = create_workqueue("t7xx_devlink");
>> + if (!wq) {
>> + kfree(dl_work);
>> + dev_err(port->dev, "create_workqueue failed\n");
>> + return -ENODATA;
>> + }
>> +
>> + INIT_WORK(&dl_work->work, t7xx_devlink_work_handler);
>> + dl_work->port = port;
>> + port->rx_length_th = T7XX_MAX_QUEUE_LENGTH;
>> +
>> + if (!t7xx_devlink_region_init(port, dl_work, wq))
>> + return -ENOMEM;
>
> Leaks?
>
>> +static int t7xx_devlink_disable_chl(struct t7xx_port *port)
>> +{
>> + spin_lock(&port->port_update_lock);
>> + port->chan_enable = false;
>> + spin_unlock(&port->port_update_lock);
>> +
>> + return 0;
>> +}
>
> This is identical to t7xx_port_wwan_disable_chl().
>
>> +struct t7xx_devlink_region_info {
>> + char region_name[T7XX_MAX_REGION_NAME_LENGTH];
>> + u32 default_size;
>> + u32 actual_size;
>> + u32 entry;
>
> entry seems pretty useless, used only to show an index within a debug
> message.
>
>> diff --git a/drivers/net/wwan/t7xx/t7xx_reg.h b/drivers/net/wwan/t7xx/t7xx_reg.h
>> index 60e025e57baa..3a758bf79a4e 100644
>> --- a/drivers/net/wwan/t7xx/t7xx_reg.h
>> +++ b/drivers/net/wwan/t7xx/t7xx_reg.h
>> @@ -101,11 +101,17 @@ enum t7xx_pm_resume_state {
>> PM_RESUME_REG_STATE_L2_EXP,
>> };
>>
>> +enum host_event_e {
>> + HOST_EVENT_INIT = 0,
>> + FASTBOOT_DL_NOTY = 0x3,
>
> NOTIFY?
>
>
--
Chetan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 4/5] net: wwan: t7xx: Enable devlink based fw flashing and coredump collection
2022-09-06 10:15 ` Kumar, M Chetan
@ 2022-09-06 10:31 ` Ilpo Järvinen
0 siblings, 0 replies; 10+ messages in thread
From: Ilpo Järvinen @ 2022-09-06 10:31 UTC (permalink / raw)
To: Kumar, M Chetan
Cc: Netdev, kuba, davem, johannes, ryazanov.s.a, loic.poulain,
krishna.c.sudi, linuxwwan, Devegowda Chandrashekar,
Mishra Soumya Prakash
On Tue, 6 Sep 2022, Kumar, M Chetan wrote:
> Thank you Ilpo for reviewing patches.
> Will address patch4 & patch5 review comments in v2.
A small correction & one additional item below.
> > > + read_len = count > skb->len ? skb->len : count;
> >
> > max_t()
I should have said min_t(), obviously.
> > > + actual_count = count > txq_mtu ? txq_mtu : count;
> >
> > max_t()
Here too, min_t().
> > > + if (read_bytes < 0) {
> > > + dev_err(port->dev, "status read failed");
> >
> > Printing "read failed" for -ERESTARTSYS/-EINTR??
Missing \n. There are also other similar dev_err()s, please check them
through (and perhaps others too besides dev_err()).
--
i.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 4/5] net: wwan: t7xx: Enable devlink based fw flashing and coredump collection
2022-09-03 8:31 ` Kumar, M Chetan
@ 2022-09-06 23:24 ` Sergey Ryazanov
2022-09-07 15:23 ` Loic Poulain
0 siblings, 1 reply; 10+ messages in thread
From: Sergey Ryazanov @ 2022-09-06 23:24 UTC (permalink / raw)
To: Kumar, M Chetan
Cc: netdev, Jakub Kicinski, David Miller, Johannes Berg,
Loic Poulain, Sudi, Krishna C, Intel Corporation,
Devegowda Chandrashekar, Mishra Soumya Prakash
On Sat, Sep 3, 2022 at 11:32 AM Kumar, M Chetan
<m.chetan.kumar@linux.intel.com> wrote:
> On 8/30/2022 7:51 AM, Sergey Ryazanov wrote:
>> On Tue, Aug 16, 2022 at 7:12 AM <m.chetan.kumar@intel.com> wrote:
>>> From: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
>>>
>>> This patch brings-in support for t7xx wwan device firmware flashing &
>>> coredump collection using devlink.
>>>
>>> Driver Registers with Devlink framework.
>>> Implements devlink ops flash_update callback that programs modem firmware.
>>> Creates region & snapshot required for device coredump log collection.
>>> On early detection of wwan device in fastboot mode driver sets up CLDMA0 HW
>>> tx/rx queues for raw data transfer then registers with devlink framework.
>>> Upon receiving firmware image & partition details driver sends fastboot
>>> commands for flashing the firmware.
>>>
>>> In this flow the fastboot command & response gets exchanged between driver
>>> and device. Once firmware flashing is success completion status is reported
>>> to user space application.
>>>
>>> Below is the devlink command usage for firmware flashing
>>>
>>> $devlink dev flash pci/$BDF file ABC.img component ABC
>>>
>>> Note: ABC.img is the firmware to be programmed to "ABC" partition.
>>>
>>> In case of coredump collection when wwan device encounters an exception
>>> it reboots & stays in fastboot mode for coredump collection by host driver.
>>> On detecting exception state driver collects the core dump, creates the
>>> devlink region & reports an event to user space application for dump
>>> collection. The user space application invokes devlink region read command
>>> for dump collection.
>>>
>>> Below are the devlink commands used for coredump collection.
>>>
>>> devlink region new pci/$BDF/mr_dump
>>> devlink region read pci/$BDF/mr_dump snapshot $ID address $ADD length $LEN
>>> devlink region del pci/$BDF/mr_dump snapshot $ID
>>>
>>> Signed-off-by: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
>>> Signed-off-by: Devegowda Chandrashekar <chandrashekar.devegowda@intel.com>
>>> Signed-off-by: Mishra Soumya Prakash <soumya.prakash.mishra@intel.com>
>>
>> [skipped]
>>
>>> diff --git a/drivers/net/wwan/t7xx/t7xx_state_monitor.c b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>>> index 9c222809371b..00e143c8d568 100644
>>> --- a/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>>> +++ b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>>
>> [skipped]
>>
>>> @@ -239,8 +252,16 @@ static void t7xx_lk_stage_event_handling(struct t7xx_fsm_ctl *ctl, unsigned int
>>> return;
>>> }
>>>
>>> + if (lk_event == LK_EVENT_CREATE_PD_PORT)
>>> + port->dl->mode = T7XX_FB_DUMP_MODE;
>>> + else
>>> + port->dl->mode = T7XX_FB_DL_MODE;
>>> port->port_conf->ops->enable_chl(port);
>>> t7xx_cldma_start(md_ctrl);
>>> + if (lk_event == LK_EVENT_CREATE_PD_PORT)
>>> + t7xx_uevent_send(dev, T7XX_UEVENT_MODEM_FASTBOOT_DUMP_MODE);
>>> + else
>>> + t7xx_uevent_send(dev, T7XX_UEVENT_MODEM_FASTBOOT_DL_MODE);
>>> break;
>>>
>>> case LK_EVENT_RESET:
>>
>> [skipped]
>>
>>> @@ -318,6 +349,7 @@ static void fsm_routine_ready(struct t7xx_fsm_ctl *ctl)
>>>
>>> ctl->curr_state = FSM_STATE_READY;
>>> t7xx_fsm_broadcast_ready_state(ctl);
>>> + t7xx_uevent_send(&md->t7xx_dev->pdev->dev, T7XX_UEVENT_MODEM_READY);
>>> t7xx_md_event_notify(md, FSM_READY);
>>> }
>>
>> These UEVENT things look at least unrelated to the patch. If the
>> deriver is really need it, please factor out it into a separate patch
>> with a comment describing why userspace wants to see these events.
>>
>> On the other hand, this looks like a custom tracing implementation. It
>> might be better to use simple debug messages instead or even the
>> tracing API, which is much more powerful than any uevent.
>
> Driver is reporting modem status (up, down, exception, etc) via uevent.
> The wwan user space services use these states for taking some action.
> So we have choose uevent for reporting modem status to user space.
>
> Is it ok we retain this logic ? I will drop it from this patch and send
> it as a separate patch for review.
Usually some subsystem generates common events for served devices. And
it is quite unusual for drivers to generate custom uevents. I found
only a few examples of such drivers.
I am not against the uevent usage, I just doubt that some userspace
software could benefit from custom driver uevents. If this case is
special, then please send these uevent changes as a separate patch
with a comment describing why userspace wants to see them.
--
Sergey
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 4/5] net: wwan: t7xx: Enable devlink based fw flashing and coredump collection
2022-09-06 23:24 ` Sergey Ryazanov
@ 2022-09-07 15:23 ` Loic Poulain
2022-09-08 15:08 ` Kumar, M Chetan
0 siblings, 1 reply; 10+ messages in thread
From: Loic Poulain @ 2022-09-07 15:23 UTC (permalink / raw)
To: Sergey Ryazanov
Cc: Kumar, M Chetan, netdev, Jakub Kicinski, David Miller,
Johannes Berg, Sudi, Krishna C, Intel Corporation,
Devegowda Chandrashekar, Mishra Soumya Prakash
On Wed, 7 Sept 2022 at 01:25, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>
> On Sat, Sep 3, 2022 at 11:32 AM Kumar, M Chetan
> <m.chetan.kumar@linux.intel.com> wrote:
> > On 8/30/2022 7:51 AM, Sergey Ryazanov wrote:
> >> On Tue, Aug 16, 2022 at 7:12 AM <m.chetan.kumar@intel.com> wrote:
> >>> From: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
> >>>
> >>> This patch brings-in support for t7xx wwan device firmware flashing &
> >>> coredump collection using devlink.
> >>>
> >>> Driver Registers with Devlink framework.
> >>> Implements devlink ops flash_update callback that programs modem firmware.
> >>> Creates region & snapshot required for device coredump log collection.
> >>> On early detection of wwan device in fastboot mode driver sets up CLDMA0 HW
> >>> tx/rx queues for raw data transfer then registers with devlink framework.
> >>> Upon receiving firmware image & partition details driver sends fastboot
> >>> commands for flashing the firmware.
> >>>
> >>> In this flow the fastboot command & response gets exchanged between driver
> >>> and device. Once firmware flashing is success completion status is reported
> >>> to user space application.
> >>>
> >>> Below is the devlink command usage for firmware flashing
> >>>
> >>> $devlink dev flash pci/$BDF file ABC.img component ABC
> >>>
> >>> Note: ABC.img is the firmware to be programmed to "ABC" partition.
> >>>
> >>> In case of coredump collection when wwan device encounters an exception
> >>> it reboots & stays in fastboot mode for coredump collection by host driver.
> >>> On detecting exception state driver collects the core dump, creates the
> >>> devlink region & reports an event to user space application for dump
> >>> collection. The user space application invokes devlink region read command
> >>> for dump collection.
> >>>
> >>> Below are the devlink commands used for coredump collection.
> >>>
> >>> devlink region new pci/$BDF/mr_dump
> >>> devlink region read pci/$BDF/mr_dump snapshot $ID address $ADD length $LEN
> >>> devlink region del pci/$BDF/mr_dump snapshot $ID
> >>>
> >>> Signed-off-by: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
> >>> Signed-off-by: Devegowda Chandrashekar <chandrashekar.devegowda@intel.com>
> >>> Signed-off-by: Mishra Soumya Prakash <soumya.prakash.mishra@intel.com>
> >>
> >> [skipped]
> >>
> >>> diff --git a/drivers/net/wwan/t7xx/t7xx_state_monitor.c b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
> >>> index 9c222809371b..00e143c8d568 100644
> >>> --- a/drivers/net/wwan/t7xx/t7xx_state_monitor.c
> >>> +++ b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
> >>
> >> [skipped]
> >>
> >>> @@ -239,8 +252,16 @@ static void t7xx_lk_stage_event_handling(struct t7xx_fsm_ctl *ctl, unsigned int
> >>> return;
> >>> }
> >>>
> >>> + if (lk_event == LK_EVENT_CREATE_PD_PORT)
> >>> + port->dl->mode = T7XX_FB_DUMP_MODE;
> >>> + else
> >>> + port->dl->mode = T7XX_FB_DL_MODE;
> >>> port->port_conf->ops->enable_chl(port);
> >>> t7xx_cldma_start(md_ctrl);
> >>> + if (lk_event == LK_EVENT_CREATE_PD_PORT)
> >>> + t7xx_uevent_send(dev, T7XX_UEVENT_MODEM_FASTBOOT_DUMP_MODE);
> >>> + else
> >>> + t7xx_uevent_send(dev, T7XX_UEVENT_MODEM_FASTBOOT_DL_MODE);
> >>> break;
> >>>
> >>> case LK_EVENT_RESET:
> >>
> >> [skipped]
> >>
> >>> @@ -318,6 +349,7 @@ static void fsm_routine_ready(struct t7xx_fsm_ctl *ctl)
> >>>
> >>> ctl->curr_state = FSM_STATE_READY;
> >>> t7xx_fsm_broadcast_ready_state(ctl);
> >>> + t7xx_uevent_send(&md->t7xx_dev->pdev->dev, T7XX_UEVENT_MODEM_READY);
> >>> t7xx_md_event_notify(md, FSM_READY);
> >>> }
> >>
> >> These UEVENT things look at least unrelated to the patch. If the
> >> deriver is really need it, please factor out it into a separate patch
> >> with a comment describing why userspace wants to see these events.
> >>
> >> On the other hand, this looks like a custom tracing implementation. It
> >> might be better to use simple debug messages instead or even the
> >> tracing API, which is much more powerful than any uevent.
> >
> > Driver is reporting modem status (up, down, exception, etc) via uevent.
> > The wwan user space services use these states for taking some action.
> > So we have choose uevent for reporting modem status to user space.
> >
> > Is it ok we retain this logic ? I will drop it from this patch and send
> > it as a separate patch for review.
>
> Usually some subsystem generates common events for served devices. And
> it is quite unusual for drivers to generate custom uevents. I found
> only a few examples of such drivers.
>
> I am not against the uevent usage, I just doubt that some userspace
> software could benefit from custom driver uevents. If this case is
> special, then please send these uevent changes as a separate patch
> with a comment describing why userspace wants to see them.
Yes, would be good to avoid new custom uevents if possible. I'm not
familiar with devlink but in the case of firmware flashing I assume
the device state is fully managed internally by the driver, and the
command terminates with success (or not), so we don't really need to
report async events. For firmware state, maybe having a 'state' sysfs
prop would be a good start (as for remoteproc), with generic state
names like "running", "crashed"...
BTW, don't remember if we already mention/address this, but isn't the
device coredump framework more appropriate for such dump?
Regards,
Loic
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 4/5] net: wwan: t7xx: Enable devlink based fw flashing and coredump collection
2022-09-07 15:23 ` Loic Poulain
@ 2022-09-08 15:08 ` Kumar, M Chetan
2022-09-08 15:48 ` Kumar, M Chetan
0 siblings, 1 reply; 10+ messages in thread
From: Kumar, M Chetan @ 2022-09-08 15:08 UTC (permalink / raw)
To: Loic Poulain, Sergey Ryazanov
Cc: netdev, Jakub Kicinski, David Miller, Johannes Berg, Sudi,
Krishna C, Intel Corporation, Devegowda Chandrashekar,
Mishra Soumya Prakash
On 9/7/2022 8:53 PM, Loic Poulain wrote:
> On Wed, 7 Sept 2022 at 01:25, Sergey Ryazanov <ryazanov.s.a@gmail.com> wrote:
>>
>> On Sat, Sep 3, 2022 at 11:32 AM Kumar, M Chetan
>> <m.chetan.kumar@linux.intel.com> wrote:
>>> On 8/30/2022 7:51 AM, Sergey Ryazanov wrote:
>>>> On Tue, Aug 16, 2022 at 7:12 AM <m.chetan.kumar@intel.com> wrote:
>>>>> From: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
>>>>>
>>>>> This patch brings-in support for t7xx wwan device firmware flashing &
>>>>> coredump collection using devlink.
>>>>>
>>>>> Driver Registers with Devlink framework.
>>>>> Implements devlink ops flash_update callback that programs modem firmware.
>>>>> Creates region & snapshot required for device coredump log collection.
>>>>> On early detection of wwan device in fastboot mode driver sets up CLDMA0 HW
>>>>> tx/rx queues for raw data transfer then registers with devlink framework.
>>>>> Upon receiving firmware image & partition details driver sends fastboot
>>>>> commands for flashing the firmware.
>>>>>
>>>>> In this flow the fastboot command & response gets exchanged between driver
>>>>> and device. Once firmware flashing is success completion status is reported
>>>>> to user space application.
>>>>>
>>>>> Below is the devlink command usage for firmware flashing
>>>>>
>>>>> $devlink dev flash pci/$BDF file ABC.img component ABC
>>>>>
>>>>> Note: ABC.img is the firmware to be programmed to "ABC" partition.
>>>>>
>>>>> In case of coredump collection when wwan device encounters an exception
>>>>> it reboots & stays in fastboot mode for coredump collection by host driver.
>>>>> On detecting exception state driver collects the core dump, creates the
>>>>> devlink region & reports an event to user space application for dump
>>>>> collection. The user space application invokes devlink region read command
>>>>> for dump collection.
>>>>>
>>>>> Below are the devlink commands used for coredump collection.
>>>>>
>>>>> devlink region new pci/$BDF/mr_dump
>>>>> devlink region read pci/$BDF/mr_dump snapshot $ID address $ADD length $LEN
>>>>> devlink region del pci/$BDF/mr_dump snapshot $ID
>>>>>
>>>>> Signed-off-by: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
>>>>> Signed-off-by: Devegowda Chandrashekar <chandrashekar.devegowda@intel.com>
>>>>> Signed-off-by: Mishra Soumya Prakash <soumya.prakash.mishra@intel.com>
>>>>
>>>> [skipped]
>>>>
>>>>> diff --git a/drivers/net/wwan/t7xx/t7xx_state_monitor.c b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>>>>> index 9c222809371b..00e143c8d568 100644
>>>>> --- a/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>>>>> +++ b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>>>>
>>>> [skipped]
>>>>
>>>>> @@ -239,8 +252,16 @@ static void t7xx_lk_stage_event_handling(struct t7xx_fsm_ctl *ctl, unsigned int
>>>>> return;
>>>>> }
>>>>>
>>>>> + if (lk_event == LK_EVENT_CREATE_PD_PORT)
>>>>> + port->dl->mode = T7XX_FB_DUMP_MODE;
>>>>> + else
>>>>> + port->dl->mode = T7XX_FB_DL_MODE;
>>>>> port->port_conf->ops->enable_chl(port);
>>>>> t7xx_cldma_start(md_ctrl);
>>>>> + if (lk_event == LK_EVENT_CREATE_PD_PORT)
>>>>> + t7xx_uevent_send(dev, T7XX_UEVENT_MODEM_FASTBOOT_DUMP_MODE);
>>>>> + else
>>>>> + t7xx_uevent_send(dev, T7XX_UEVENT_MODEM_FASTBOOT_DL_MODE);
>>>>> break;
>>>>>
>>>>> case LK_EVENT_RESET:
>>>>
>>>> [skipped]
>>>>
>>>>> @@ -318,6 +349,7 @@ static void fsm_routine_ready(struct t7xx_fsm_ctl *ctl)
>>>>>
>>>>> ctl->curr_state = FSM_STATE_READY;
>>>>> t7xx_fsm_broadcast_ready_state(ctl);
>>>>> + t7xx_uevent_send(&md->t7xx_dev->pdev->dev, T7XX_UEVENT_MODEM_READY);
>>>>> t7xx_md_event_notify(md, FSM_READY);
>>>>> }
>>>>
>>>> These UEVENT things look at least unrelated to the patch. If the
>>>> deriver is really need it, please factor out it into a separate patch
>>>> with a comment describing why userspace wants to see these events.
>>>>
>>>> On the other hand, this looks like a custom tracing implementation. It
>>>> might be better to use simple debug messages instead or even the
>>>> tracing API, which is much more powerful than any uevent.
>>>
>>> Driver is reporting modem status (up, down, exception, etc) via uevent.
>>> The wwan user space services use these states for taking some action.
>>> So we have choose uevent for reporting modem status to user space.
>>>
>>> Is it ok we retain this logic ? I will drop it from this patch and send
>>> it as a separate patch for review.
>>
>> Usually some subsystem generates common events for served devices. And
>> it is quite unusual for drivers to generate custom uevents. I found
>> only a few examples of such drivers.
>>
>> I am not against the uevent usage, I just doubt that some userspace
>> software could benefit from custom driver uevents. If this case is
>> special, then please send these uevent changes as a separate patch
>> with a comment describing why userspace wants to see them.
>
> Yes, would be good to avoid new custom uevents if possible. I'm not
> familiar with devlink but in the case of firmware flashing I assume
> the device state is fully managed internally by the driver, and the
> command terminates with success (or not), so we don't really need to
> report async events. For firmware state, maybe having a 'state' sysfs
> prop would be a good start (as for remoteproc), with generic state
> names like "running", "crashed"...
Driver needs a way to report custom events to user space.
If sysfs is a way to report custom events will change the
implementation to it.
new 'state' entry will be created under dev and modem status
will be written to it. So user space could get the modem status.
>
> BTW, don't remember if we already mention/address this, but isn't the
> device coredump framework more appropriate for such dump?
Devlink region provides similar capability as dev coredump.
It allows the driver to collect the dump snapshot and can be
easily accessed via devlink region read or dump commands.
Since for firmware flashing we are using devlink interface we
also choose devlink region for dump collection.
I hope it is ok to continue with devlink region.
--
Chetan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 4/5] net: wwan: t7xx: Enable devlink based fw flashing and coredump collection
2022-09-08 15:08 ` Kumar, M Chetan
@ 2022-09-08 15:48 ` Kumar, M Chetan
0 siblings, 0 replies; 10+ messages in thread
From: Kumar, M Chetan @ 2022-09-08 15:48 UTC (permalink / raw)
To: Loic Poulain, Sergey Ryazanov
Cc: netdev, Jakub Kicinski, David Miller, Johannes Berg, Sudi,
Krishna C, Intel Corporation, Devegowda Chandrashekar,
Mishra Soumya Prakash
On 9/8/2022 8:38 PM, Kumar, M Chetan wrote:
> On 9/7/2022 8:53 PM, Loic Poulain wrote:
>> On Wed, 7 Sept 2022 at 01:25, Sergey Ryazanov <ryazanov.s.a@gmail.com>
>> wrote:
>>>
>>> On Sat, Sep 3, 2022 at 11:32 AM Kumar, M Chetan
>>> <m.chetan.kumar@linux.intel.com> wrote:
>>>> On 8/30/2022 7:51 AM, Sergey Ryazanov wrote:
>>>>> On Tue, Aug 16, 2022 at 7:12 AM <m.chetan.kumar@intel.com> wrote:
>>>>>> From: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
>>>>>>
>>>>>> This patch brings-in support for t7xx wwan device firmware flashing &
>>>>>> coredump collection using devlink.
>>>>>>
>>>>>> Driver Registers with Devlink framework.
>>>>>> Implements devlink ops flash_update callback that programs modem
>>>>>> firmware.
>>>>>> Creates region & snapshot required for device coredump log
>>>>>> collection.
>>>>>> On early detection of wwan device in fastboot mode driver sets up
>>>>>> CLDMA0 HW
>>>>>> tx/rx queues for raw data transfer then registers with devlink
>>>>>> framework.
>>>>>> Upon receiving firmware image & partition details driver sends
>>>>>> fastboot
>>>>>> commands for flashing the firmware.
>>>>>>
>>>>>> In this flow the fastboot command & response gets exchanged
>>>>>> between driver
>>>>>> and device. Once firmware flashing is success completion status is
>>>>>> reported
>>>>>> to user space application.
>>>>>>
>>>>>> Below is the devlink command usage for firmware flashing
>>>>>>
>>>>>> $devlink dev flash pci/$BDF file ABC.img component ABC
>>>>>>
>>>>>> Note: ABC.img is the firmware to be programmed to "ABC" partition.
>>>>>>
>>>>>> In case of coredump collection when wwan device encounters an
>>>>>> exception
>>>>>> it reboots & stays in fastboot mode for coredump collection by
>>>>>> host driver.
>>>>>> On detecting exception state driver collects the core dump,
>>>>>> creates the
>>>>>> devlink region & reports an event to user space application for dump
>>>>>> collection. The user space application invokes devlink region read
>>>>>> command
>>>>>> for dump collection.
>>>>>>
>>>>>> Below are the devlink commands used for coredump collection.
>>>>>>
>>>>>> devlink region new pci/$BDF/mr_dump
>>>>>> devlink region read pci/$BDF/mr_dump snapshot $ID address $ADD
>>>>>> length $LEN
>>>>>> devlink region del pci/$BDF/mr_dump snapshot $ID
>>>>>>
>>>>>> Signed-off-by: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
>>>>>> Signed-off-by: Devegowda Chandrashekar
>>>>>> <chandrashekar.devegowda@intel.com>
>>>>>> Signed-off-by: Mishra Soumya Prakash
>>>>>> <soumya.prakash.mishra@intel.com>
>>>>>
>>>>> [skipped]
>>>>>
>>>>>> diff --git a/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>>>>>> b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>>>>>> index 9c222809371b..00e143c8d568 100644
>>>>>> --- a/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>>>>>> +++ b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>>>>>
>>>>> [skipped]
>>>>>
>>>>>> @@ -239,8 +252,16 @@ static void
>>>>>> t7xx_lk_stage_event_handling(struct t7xx_fsm_ctl *ctl, unsigned int
>>>>>> return;
>>>>>> }
>>>>>>
>>>>>> + if (lk_event == LK_EVENT_CREATE_PD_PORT)
>>>>>> + port->dl->mode = T7XX_FB_DUMP_MODE;
>>>>>> + else
>>>>>> + port->dl->mode = T7XX_FB_DL_MODE;
>>>>>> port->port_conf->ops->enable_chl(port);
>>>>>> t7xx_cldma_start(md_ctrl);
>>>>>> + if (lk_event == LK_EVENT_CREATE_PD_PORT)
>>>>>> + t7xx_uevent_send(dev,
>>>>>> T7XX_UEVENT_MODEM_FASTBOOT_DUMP_MODE);
>>>>>> + else
>>>>>> + t7xx_uevent_send(dev,
>>>>>> T7XX_UEVENT_MODEM_FASTBOOT_DL_MODE);
>>>>>> break;
>>>>>>
>>>>>> case LK_EVENT_RESET:
>>>>>
>>>>> [skipped]
>>>>>
>>>>>> @@ -318,6 +349,7 @@ static void fsm_routine_ready(struct
>>>>>> t7xx_fsm_ctl *ctl)
>>>>>>
>>>>>> ctl->curr_state = FSM_STATE_READY;
>>>>>> t7xx_fsm_broadcast_ready_state(ctl);
>>>>>> + t7xx_uevent_send(&md->t7xx_dev->pdev->dev,
>>>>>> T7XX_UEVENT_MODEM_READY);
>>>>>> t7xx_md_event_notify(md, FSM_READY);
>>>>>> }
>>>>>
>>>>> These UEVENT things look at least unrelated to the patch. If the
>>>>> deriver is really need it, please factor out it into a separate patch
>>>>> with a comment describing why userspace wants to see these events.
>>>>>
>>>>> On the other hand, this looks like a custom tracing implementation. It
>>>>> might be better to use simple debug messages instead or even the
>>>>> tracing API, which is much more powerful than any uevent.
>>>>
>>>> Driver is reporting modem status (up, down, exception, etc) via uevent.
>>>> The wwan user space services use these states for taking some action.
>>>> So we have choose uevent for reporting modem status to user space.
>>>>
>>>> Is it ok we retain this logic ? I will drop it from this patch and send
>>>> it as a separate patch for review.
>>>
>>> Usually some subsystem generates common events for served devices. And
>>> it is quite unusual for drivers to generate custom uevents. I found
>>> only a few examples of such drivers.
>>>
>>> I am not against the uevent usage, I just doubt that some userspace
>>> software could benefit from custom driver uevents. If this case is
>>> special, then please send these uevent changes as a separate patch
>>> with a comment describing why userspace wants to see them.
>>
>> Yes, would be good to avoid new custom uevents if possible. I'm not
>> familiar with devlink but in the case of firmware flashing I assume
>> the device state is fully managed internally by the driver, and the
>> command terminates with success (or not), so we don't really need to
>> report async events. For firmware state, maybe having a 'state' sysfs
>> prop would be a good start (as for remoteproc), with generic state
>> names like "running", "crashed"...
>
> Driver needs a way to report custom events to user space.
> If sysfs is a way to report custom events will change the
> implementation to it.
>
> new 'state' entry will be created under dev and modem status
> will be written to it. So user space could get the modem status.
>
>>
>> BTW, don't remember if we already mention/address this, but isn't the
>> device coredump framework more appropriate for such dump?
>
> Devlink region provides similar capability as dev coredump.
> It allows the driver to collect the dump snapshot and can be
> easily accessed via devlink region read or dump commands.
>
> Since for firmware flashing we are using devlink interface we
> also choose devlink region for dump collection.
>
> I hope it is ok to continue with devlink region.
Also, during 4G driver (IOSM) submission reviewer[1] suggested us
to consider using devlink region and health for coredump collection.
This is also another reason why we choose devlink region interface for
dump collection.
The same is followed for 5G driver.
[1]
Andrew LunnJan. 7, 2021, 7:35 p.m. UTC | #1
On Thu, Jan 07, 2021 at 10:35:12PM +0530, M Chetan Kumar wrote:
> Implements a char device for flashing Modem FW image while Device
> is in boot rom phase and for collecting traces on modem crash.
Since this is a network device, you might want to take a look at
devlink support for flashing devices.
https://www.kernel.org/doc/html/latest/networking/devlink/devlink-flash.html
And for collecting crashes and other health information, consider
devlink region and devlink health.
It is much better to reuse existing infrastructure than do something
proprietary with a char dev.
Andrew
--
Chetan
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-09-08 15:49 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-16 4:24 [PATCH net-next 4/5] net: wwan: t7xx: Enable devlink based fw flashing and coredump collection m.chetan.kumar
2022-08-18 14:33 ` Ilpo Järvinen
2022-09-06 10:15 ` Kumar, M Chetan
2022-09-06 10:31 ` Ilpo Järvinen
2022-08-30 2:21 ` Sergey Ryazanov
2022-09-03 8:31 ` Kumar, M Chetan
2022-09-06 23:24 ` Sergey Ryazanov
2022-09-07 15:23 ` Loic Poulain
2022-09-08 15:08 ` Kumar, M Chetan
2022-09-08 15:48 ` Kumar, M Chetan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.