All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2 net-next 1/6] net: wwan: iosm: devlink registration
@ 2021-09-19 17:26 M Chetan Kumar
  2021-09-20  6:53 ` Dan Carpenter
  0 siblings, 1 reply; 2+ messages in thread
From: M Chetan Kumar @ 2021-09-19 17:26 UTC (permalink / raw)
  To: netdev
  Cc: kuba, davem, johannes, ryazanov.s.a, loic.poulain,
	krishna.c.sudi, m.chetan.kumar, linuxwwan, dan.carpenter

Register with devlink framework and implment callbacks required
for fw flashing and coredump collection.

Signed-off-by: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
---
v2:
* Fixes memory leak in ipc_devlink_create_region().
* Fixes NULL parameter deference in ipc_devlink_flash_update() reported by
  smatch static checker.
---
 drivers/net/wwan/iosm/iosm_ipc_devlink.c | 363 +++++++++++++++++++++++
 drivers/net/wwan/iosm/iosm_ipc_devlink.h | 207 +++++++++++++
 2 files changed, 570 insertions(+)
 create mode 100644 drivers/net/wwan/iosm/iosm_ipc_devlink.c
 create mode 100644 drivers/net/wwan/iosm/iosm_ipc_devlink.h

diff --git a/drivers/net/wwan/iosm/iosm_ipc_devlink.c b/drivers/net/wwan/iosm/iosm_ipc_devlink.c
new file mode 100644
index 000000000000..7fd7956cc61e
--- /dev/null
+++ b/drivers/net/wwan/iosm/iosm_ipc_devlink.c
@@ -0,0 +1,363 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2020-2021 Intel Corporation.
+ */
+
+#include "iosm_ipc_chnl_cfg.h"
+#include "iosm_ipc_coredump.h"
+#include "iosm_ipc_devlink.h"
+#include "iosm_ipc_flash.h"
+
+/* Coredump list */
+static struct iosm_coredump_file_info list[IOSM_NOF_CD_REGION] = {
+	{"report.json", REPORT_JSON_SIZE,},
+	{"coredump.fcd", COREDUMP_FCD_SIZE,},
+	{"cdd.log", CDD_LOG_SIZE,},
+	{"eeprom.bin", EEPROM_BIN_SIZE,},
+	{"bootcore_trace.bin", BOOTCORE_TRC_BIN_SIZE,},
+	{"bootcore_prev_trace.bin", BOOTCORE_PREV_TRC_BIN_SIZE,},
+};
+
+/* Get the param values for the specific param ID's */
+static int ipc_devlink_get_param(struct devlink *dl, u32 id,
+				 struct devlink_param_gset_ctx *ctx)
+{
+	struct iosm_devlink *ipc_devlink = devlink_priv(dl);
+	int rc = 0;
+
+	switch (id) {
+	case IOSM_DEVLINK_PARAM_ID_ERASE_FULL_FLASH:
+		ctx->val.vu8 = ipc_devlink->param.erase_full_flash;
+		break;
+
+	case IOSM_DEVLINK_PARAM_ID_DOWNLOAD_REGION:
+		ctx->val.vu8 = ipc_devlink->param.download_region;
+		break;
+
+	case IOSM_DEVLINK_PARAM_ID_ADDRESS:
+		ctx->val.vu32 = ipc_devlink->param.address;
+		break;
+
+	case IOSM_DEVLINK_PARAM_ID_REGION_COUNT:
+		ctx->val.vu8 = ipc_devlink->param.region_count;
+		break;
+
+	default:
+		rc = -EOPNOTSUPP;
+		break;
+	}
+
+	return rc;
+}
+
+/* Set the param values for the specific param ID's */
+static int ipc_devlink_set_param(struct devlink *dl, u32 id,
+				 struct devlink_param_gset_ctx *ctx)
+{
+	struct iosm_devlink *ipc_devlink = devlink_priv(dl);
+	int rc = 0;
+
+	switch (id) {
+	case IOSM_DEVLINK_PARAM_ID_ERASE_FULL_FLASH:
+		ipc_devlink->param.erase_full_flash = ctx->val.vu8;
+		break;
+
+	case IOSM_DEVLINK_PARAM_ID_DOWNLOAD_REGION:
+		ipc_devlink->param.download_region = ctx->val.vu8;
+		break;
+
+	case IOSM_DEVLINK_PARAM_ID_ADDRESS:
+		ipc_devlink->param.address = ctx->val.vu32;
+		break;
+
+	case IOSM_DEVLINK_PARAM_ID_REGION_COUNT:
+		ipc_devlink->param.region_count = ctx->val.vu8;
+		break;
+
+	default:
+		rc = -EOPNOTSUPP;
+		break;
+	}
+
+	return rc;
+}
+
+/* Devlink param structure array */
+static const struct devlink_param iosm_devlink_params[] = {
+	DEVLINK_PARAM_DRIVER(IOSM_DEVLINK_PARAM_ID_ERASE_FULL_FLASH,
+			     "erase_full_flash", DEVLINK_PARAM_TYPE_BOOL,
+			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
+			     ipc_devlink_get_param, ipc_devlink_set_param,
+			     NULL),
+	DEVLINK_PARAM_DRIVER(IOSM_DEVLINK_PARAM_ID_DOWNLOAD_REGION,
+			     "download_region", DEVLINK_PARAM_TYPE_BOOL,
+			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
+			     ipc_devlink_get_param, ipc_devlink_set_param,
+			     NULL),
+	DEVLINK_PARAM_DRIVER(IOSM_DEVLINK_PARAM_ID_ADDRESS,
+			     "address", DEVLINK_PARAM_TYPE_U32,
+			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
+			     ipc_devlink_get_param, ipc_devlink_set_param,
+			     NULL),
+	DEVLINK_PARAM_DRIVER(IOSM_DEVLINK_PARAM_ID_REGION_COUNT,
+			     "region_count", DEVLINK_PARAM_TYPE_U8,
+			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
+			     ipc_devlink_get_param, ipc_devlink_set_param,
+			     NULL),
+};
+
+/* Get devlink flash component type */
+static enum iosm_flash_comp_type
+ipc_devlink_get_flash_comp_type(const char comp_str[], u32 len)
+{
+	enum iosm_flash_comp_type fls_type;
+
+	if (!strncmp("PSI", comp_str, len))
+		fls_type = FLASH_COMP_TYPE_PSI;
+	else if (!strncmp("EBL", comp_str, len))
+		fls_type = FLASH_COMP_TYPE_EBL;
+	else if (!strncmp("FLS", comp_str, len))
+		fls_type = FLASH_COMP_TYPE_FLS;
+	else
+		fls_type = FLASH_COMP_TYPE_INVAL;
+
+	return fls_type;
+}
+
+/* Function triggered on devlink flash command
+ * Flash update function which calls multiple functions based on
+ * component type specified in the flash command
+ */
+static int ipc_devlink_flash_update(struct devlink *devlink,
+				    struct devlink_flash_update_params *params,
+				    struct netlink_ext_ack *extack)
+{
+	struct iosm_devlink *ipc_devlink = devlink_priv(devlink);
+	enum iosm_flash_comp_type fls_type;
+	u32 rc = -EINVAL;
+	u8 *mdm_rsp;
+
+	if (!params->component)
+		return rc;
+
+	mdm_rsp = kzalloc(IOSM_EBL_DW_PACK_SIZE, GFP_KERNEL);
+	if (!mdm_rsp)
+		return -ENOMEM;
+
+	fls_type = ipc_devlink_get_flash_comp_type(params->component,
+						   strlen(params->component));
+
+	switch (fls_type) {
+	case FLASH_COMP_TYPE_PSI:
+		rc = ipc_flash_boot_psi(ipc_devlink, params->fw);
+		break;
+	case FLASH_COMP_TYPE_EBL:
+		rc = ipc_flash_boot_ebl(ipc_devlink, params->fw);
+		if (!rc)
+			rc = ipc_flash_boot_set_capabilities(ipc_devlink,
+							     mdm_rsp);
+		if (!rc)
+			rc = ipc_flash_read_swid(ipc_devlink, mdm_rsp);
+		break;
+	case FLASH_COMP_TYPE_FLS:
+		rc = ipc_flash_send_fls(ipc_devlink, params->fw, mdm_rsp);
+		break;
+	default:
+		devlink_flash_update_status_notify(devlink, "Invalid component",
+						   params->component, 0, 0);
+		break;
+	}
+
+	if (!rc)
+		devlink_flash_update_status_notify(devlink, "Flashing success",
+						   params->component, 0, 0);
+	else
+		devlink_flash_update_status_notify(devlink, "Flashing failed",
+						   params->component, 0, 0);
+
+	kfree(mdm_rsp);
+	return rc;
+}
+
+/* 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 = ipc_devlink_flash_update,
+};
+
+/* Send command to modem to collect data */
+int ipc_devlink_send_cmd(struct iosm_devlink *ipc_devlink, u16 cmd, u32 entry)
+{
+	struct iosm_rpsi_cmd rpsi_cmd;
+
+	rpsi_cmd.param.dword = cpu_to_le32(entry);
+	rpsi_cmd.cmd = cpu_to_le16(cmd);
+	rpsi_cmd.crc = rpsi_cmd.param.word[0] ^ rpsi_cmd.param.word[1] ^
+		       rpsi_cmd.cmd;
+
+	return ipc_imem_sys_devlink_write(ipc_devlink, (u8 *)&rpsi_cmd,
+					  sizeof(rpsi_cmd));
+}
+
+static int ipc_devlink_coredump_snapshot(struct devlink *dl,
+					 const struct devlink_region_ops *ops,
+					 struct netlink_ext_ack *extack,
+					 u8 **data)
+{
+	struct iosm_devlink *ipc_devlink = devlink_priv(dl);
+	struct iosm_coredump_file_info *cd_list = ops->priv;
+	u32 region_size;
+	int rc;
+
+	dev_dbg(ipc_devlink->dev, "Region:%s, ID:%d", ops->name,
+		cd_list->entry);
+	region_size = cd_list->default_size;
+	rc = ipc_coredump_collect(ipc_devlink, data, cd_list->entry,
+				  region_size);
+	if (rc) {
+		dev_err(ipc_devlink->dev, "Fail to create snapshot,err %d", rc);
+		goto coredump_collect_err;
+	}
+
+	/* Send coredump end cmd indicating end of coredump collection */
+	if (cd_list->entry == (IOSM_NOF_CD_REGION - 1))
+		ipc_coredump_get_list(ipc_devlink, rpsi_cmd_coredump_end);
+
+	return rc;
+coredump_collect_err:
+	ipc_coredump_get_list(ipc_devlink, rpsi_cmd_coredump_end);
+	return rc;
+}
+
+/* To create regions for coredump files */
+static int ipc_devlink_create_region(struct iosm_devlink *devlink)
+{
+	struct devlink_region_ops *mdm_coredump;
+	int rc = 0;
+	int i;
+
+	mdm_coredump = devlink->iosm_devlink_mdm_coredump;
+	for (i = 0; i < IOSM_NOF_CD_REGION; i++) {
+		mdm_coredump[i].name = list[i].filename;
+		mdm_coredump[i].snapshot = ipc_devlink_coredump_snapshot;
+		mdm_coredump[i].destructor = vfree;
+		devlink->cd_regions[i] =
+			devlink_region_create(devlink->devlink_ctx,
+					      &mdm_coredump[i], MAX_SNAPSHOTS,
+					      list[i].default_size);
+
+		if (IS_ERR(devlink->cd_regions[i])) {
+			rc = PTR_ERR(devlink->cd_regions[i]);
+			dev_err(devlink->dev, "Devlink region fail,err %d", rc);
+			/* Delete previously created regions */
+			for ( ; i >= 0; i--)
+				devlink_region_destroy(devlink->cd_regions[i]);
+			goto region_create_fail;
+		}
+		list[i].entry = i;
+		mdm_coredump[i].priv = list + i;
+	}
+region_create_fail:
+	return rc;
+}
+
+/* To Destroy devlink regions */
+static void ipc_devlink_destroy_region(struct iosm_devlink *ipc_devlink)
+{
+	u8 i;
+
+	for (i = 0; i < IOSM_NOF_CD_REGION; i++)
+		devlink_region_destroy(ipc_devlink->cd_regions[i]);
+}
+
+/* Handle registration to devlink framework */
+struct iosm_devlink *ipc_devlink_init(struct iosm_imem *ipc_imem)
+{
+	struct ipc_chnl_cfg chnl_cfg_flash = { 0 };
+	struct iosm_devlink *ipc_devlink;
+	struct devlink *devlink_ctx;
+	int rc;
+
+	devlink_ctx = devlink_alloc(&devlink_flash_ops,
+				    sizeof(struct iosm_devlink),
+				    ipc_imem->dev);
+	if (!devlink_ctx) {
+		dev_err(ipc_imem->dev, "devlink_alloc failed");
+		goto devlink_alloc_fail;
+	}
+
+	ipc_devlink = devlink_priv(devlink_ctx);
+	ipc_devlink->devlink_ctx = devlink_ctx;
+	ipc_devlink->pcie = ipc_imem->pcie;
+	ipc_devlink->dev = ipc_imem->dev;
+	rc = devlink_register(devlink_ctx);
+	if (rc) {
+		dev_err(ipc_devlink->dev, "devlink_register failed rc %d", rc);
+		goto free_dl;
+	}
+
+	rc = devlink_params_register(devlink_ctx, iosm_devlink_params,
+				     ARRAY_SIZE(iosm_devlink_params));
+	if (rc) {
+		dev_err(ipc_devlink->dev,
+			"devlink_params_register failed. rc %d", rc);
+		goto param_reg_fail;
+	}
+
+	devlink_params_publish(devlink_ctx);
+	ipc_devlink->cd_file_info = list;
+
+	rc = ipc_devlink_create_region(ipc_devlink);
+	if (rc) {
+		dev_err(ipc_devlink->dev, "Devlink Region create failed, rc %d",
+			rc);
+		goto region_create_fail;
+	}
+
+	if (ipc_chnl_cfg_get(&chnl_cfg_flash, IPC_MEM_CTRL_CHL_ID_7) < 0)
+		goto chnl_get_fail;
+
+	ipc_imem_channel_init(ipc_imem, IPC_CTYPE_CTRL,
+			      chnl_cfg_flash, IRQ_MOD_OFF);
+
+	init_completion(&ipc_devlink->devlink_sio.read_sem);
+	skb_queue_head_init(&ipc_devlink->devlink_sio.rx_list);
+
+	dev_dbg(ipc_devlink->dev, "iosm devlink register success");
+
+	return ipc_devlink;
+
+chnl_get_fail:
+	ipc_devlink_destroy_region(ipc_devlink);
+region_create_fail:
+	devlink_params_unpublish(devlink_ctx);
+	devlink_params_unregister(devlink_ctx, iosm_devlink_params,
+				  ARRAY_SIZE(iosm_devlink_params));
+param_reg_fail:
+	devlink_unregister(devlink_ctx);
+free_dl:
+	devlink_free(devlink_ctx);
+devlink_alloc_fail:
+	return NULL;
+}
+
+/* Handle unregistration of devlink */
+void ipc_devlink_deinit(struct iosm_devlink *ipc_devlink)
+{
+	struct devlink *devlink_ctx = ipc_devlink->devlink_ctx;
+
+	ipc_devlink_destroy_region(ipc_devlink);
+	devlink_params_unpublish(devlink_ctx);
+	devlink_params_unregister(devlink_ctx, iosm_devlink_params,
+				  ARRAY_SIZE(iosm_devlink_params));
+	if (ipc_devlink->devlink_sio.devlink_read_pend) {
+		complete(&ipc_devlink->devlink_sio.read_sem);
+		complete(&ipc_devlink->devlink_sio.channel->ul_sem);
+	}
+	if (!ipc_devlink->devlink_sio.devlink_read_pend)
+		skb_queue_purge(&ipc_devlink->devlink_sio.rx_list);
+
+	ipc_imem_sys_devlink_close(ipc_devlink);
+	devlink_unregister(devlink_ctx);
+	devlink_free(devlink_ctx);
+}
diff --git a/drivers/net/wwan/iosm/iosm_ipc_devlink.h b/drivers/net/wwan/iosm/iosm_ipc_devlink.h
new file mode 100644
index 000000000000..392735080cb3
--- /dev/null
+++ b/drivers/net/wwan/iosm/iosm_ipc_devlink.h
@@ -0,0 +1,207 @@
+/* SPDX-License-Identifier: GPL-2.0-only
+ *
+ * Copyright (C) 2020-2021 Intel Corporation.
+ */
+
+#ifndef _IOSM_IPC_DEVLINK_H_
+#define _IOSM_IPC_DEVLINK_H_
+
+#include <net/devlink.h>
+
+#include "iosm_ipc_imem.h"
+#include "iosm_ipc_imem_ops.h"
+#include "iosm_ipc_pcie.h"
+
+/* MAX file name length */
+#define IOSM_MAX_FILENAME_LEN 32
+/* EBL response size */
+#define IOSM_EBL_RSP_SIZE 76
+/* MAX number of regions supported */
+#define IOSM_NOF_CD_REGION 6
+/* MAX number of SNAPSHOTS supported */
+#define MAX_SNAPSHOTS 1
+/* Default Coredump file size */
+#define REPORT_JSON_SIZE 0x800
+#define COREDUMP_FCD_SIZE 0x10E00000
+#define CDD_LOG_SIZE 0x30000
+#define EEPROM_BIN_SIZE 0x10000
+#define BOOTCORE_TRC_BIN_SIZE 0x8000
+#define BOOTCORE_PREV_TRC_BIN_SIZE 0x20000
+
+/**
+ * enum iosm_devlink_param_id - Enum type to different devlink params
+ * @IOSM_DEVLINK_PARAM_ID_BASE:			Devlink param base ID
+ * @IOSM_DEVLINK_PARAM_ID_ERASE_FULL_FLASH:     Set if full erase required
+ * @IOSM_DEVLINK_PARAM_ID_DOWNLOAD_REGION:	Set if fls file to be
+ *						flashed is Loadmap/region file
+ * @IOSM_DEVLINK_PARAM_ID_ADDRESS:		Address of the region to be
+ *						flashed
+ * @IOSM_DEVLINK_PARAM_ID_REGION_COUNT:		Max region count
+ */
+
+enum iosm_devlink_param_id {
+	IOSM_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
+	IOSM_DEVLINK_PARAM_ID_ERASE_FULL_FLASH,
+	IOSM_DEVLINK_PARAM_ID_DOWNLOAD_REGION,
+	IOSM_DEVLINK_PARAM_ID_ADDRESS,
+	IOSM_DEVLINK_PARAM_ID_REGION_COUNT,
+};
+
+/**
+ * enum iosm_rpsi_cmd_code - Enum type for RPSI command list
+ * @rpsi_cmd_code_ebl:		Command to load ebl
+ * @rpsi_cmd_coredump_start:    Command to get list of files and
+ *				file size info from PSI
+ * @rpsi_cmd_coredump_get:      Command to get the coredump data
+ * @rpsi_cmd_coredump_end:      Command to stop receiving the coredump
+ */
+enum iosm_rpsi_cmd_code {
+	rpsi_cmd_code_ebl = 0x02,
+	rpsi_cmd_coredump_start = 0x10,
+	rpsi_cmd_coredump_get   = 0x11,
+	rpsi_cmd_coredump_end   = 0x12,
+};
+
+/**
+ * enum iosm_flash_comp_type - Enum for different flash component types
+ * @FLASH_COMP_TYPE_PSI:	PSI flash comp type
+ * @FLASH_COMP_TYPE_EBL:	EBL flash comp type
+ * @FLASH_COMP_TYPE_FLS:	FLS flash comp type
+ * @FLASH_COMP_TYPE_INVAL:	Invalid flash comp type
+ */
+enum iosm_flash_comp_type {
+	FLASH_COMP_TYPE_PSI,
+	FLASH_COMP_TYPE_EBL,
+	FLASH_COMP_TYPE_FLS,
+	FLASH_COMP_TYPE_INVAL,
+};
+
+/**
+ * struct iosm_devlink_sio - SIO instance
+ * @rx_list:	Downlink skbuf list received from CP
+ * @read_sem:	Needed for the blocking read or downlink transfer
+ * @channel_id: Reserved channel id for flashing/CD collection to RAM
+ * @channel:	Channel instance for flashing and coredump
+ * @devlink_read_pend: Check if read is pending
+ */
+struct iosm_devlink_sio {
+	struct sk_buff_head rx_list;
+	struct completion read_sem;
+	int channel_id;
+	struct ipc_mem_channel *channel;
+	u32 devlink_read_pend;
+};
+
+/**
+ * struct iosm_flash_params - List of flash params required for flashing
+ * @address:		Address of the region file to be flashed
+ * @region_count:	Maximum no of regions for each fls file
+ * @download_region:	To be set if region is being flashed
+ * @erase_full_flash:   To set the flashing mode
+ *                      erase_full_flash = 1; full erase
+ *                      erase_full_flash = 0; no erase
+ * @erase_full_flash_done: Flag to check if it is a full erase
+ */
+struct iosm_flash_params {
+	u32 address;
+	u8 region_count;
+	u8 download_region;
+	u8 erase_full_flash;
+	u8 erase_full_flash_done;
+};
+
+/**
+ * struct iosm_ebl_ctx_data -  EBL ctx data used during flashing
+ * @ebl_sw_info_version: SWID version info obtained from EBL
+ * @m_ebl_resp:         Buffer used to read and write the ebl data
+ */
+struct iosm_ebl_ctx_data {
+	u8 ebl_sw_info_version;
+	u8 m_ebl_resp[IOSM_EBL_RSP_SIZE];
+};
+
+/**
+ * struct iosm_coredump_file_info -  Coredump file info
+ * @filename:		Name of coredump file
+ * @default_size:	Default size of coredump file
+ * @actual_size:	Actual size of coredump file
+ * @entry:		Index of the coredump file
+ */
+struct iosm_coredump_file_info {
+	char filename[IOSM_MAX_FILENAME_LEN];
+	u32 default_size;
+	u32 actual_size;
+	u32 entry;
+};
+
+/**
+ * struct iosm_devlink - IOSM Devlink structure
+ * @devlink_sio:        SIO instance for read/write functionality
+ * @pcie:               Pointer to PCIe component
+ * @dev:                Pointer to device struct
+ * @devlink_ctx:	Pointer to devlink context
+ * @param:		Params required for flashing
+ * @ebl_ctx:		Data to be read and written to Modem
+ * @cd_file_info:	coredump file info
+ * @iosm_devlink_mdm_coredump:	region ops for coredump collection
+ * @cd_regions:		coredump regions
+ */
+struct iosm_devlink {
+	struct iosm_devlink_sio devlink_sio;
+	struct iosm_pcie *pcie;
+	struct device *dev;
+	struct devlink *devlink_ctx;
+	struct iosm_flash_params param;
+	struct iosm_ebl_ctx_data ebl_ctx;
+	struct iosm_coredump_file_info *cd_file_info;
+	struct devlink_region_ops iosm_devlink_mdm_coredump[IOSM_NOF_CD_REGION];
+	struct devlink_region *cd_regions[IOSM_NOF_CD_REGION];
+};
+
+/**
+ * union iosm_rpsi_param_u - RPSI cmd param for CRC calculation
+ * @word:	Words member used in CRC calculation
+ * @dword:	Actual data
+ */
+union iosm_rpsi_param_u {
+	__le16 word[2];
+	__le32 dword;
+};
+
+/**
+ * struct iosm_rpsi_cmd - Structure for RPSI Command
+ * @param:      Used to calculate CRC
+ * @cmd:        Stores the RPSI command
+ * @crc:        Stores the CRC value
+ */
+struct iosm_rpsi_cmd {
+	union iosm_rpsi_param_u param;
+	__le16	cmd;
+	__le16	crc;
+};
+
+/**
+ * ipc_devlink_init - To initialize the devlink to IOSM driver
+ * @ipc_imem:	Pointer to struct iosm_imem
+ *
+ * Returns:	Pointer to iosm_devlink on success and NULL on failure
+ */
+struct iosm_devlink *ipc_devlink_init(struct iosm_imem *ipc_imem);
+
+/**
+ * ipc_devlink_deinit - To unintialize the devlink from IOSM driver.
+ * @ipc_devlink:	Devlink instance
+ */
+void ipc_devlink_deinit(struct iosm_devlink *ipc_devlink);
+
+/**
+ * ipc_devlink_send_cmd - Send command to Modem
+ * @ipc_devlink: Pointer to struct iosm_devlink
+ * @cmd:	 Command to be sent to modem
+ * @entry:	 Command entry number
+ *
+ * Returns:	 0 on success and failure value on error
+ */
+int ipc_devlink_send_cmd(struct iosm_devlink *ipc_devlink, u16 cmd, u32 entry);
+
+#endif /* _IOSM_IPC_DEVLINK_H */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH V2 net-next 1/6] net: wwan: iosm: devlink registration
  2021-09-19 17:26 [PATCH V2 net-next 1/6] net: wwan: iosm: devlink registration M Chetan Kumar
@ 2021-09-20  6:53 ` Dan Carpenter
  0 siblings, 0 replies; 2+ messages in thread
From: Dan Carpenter @ 2021-09-20  6:53 UTC (permalink / raw)
  To: M Chetan Kumar
  Cc: netdev, kuba, davem, johannes, ryazanov.s.a, loic.poulain,
	krishna.c.sudi, m.chetan.kumar, linuxwwan

Small style nits.

On Sun, Sep 19, 2021 at 10:56:18PM +0530, M Chetan Kumar wrote:
> +static int ipc_devlink_flash_update(struct devlink *devlink,
> +				    struct devlink_flash_update_params *params,
> +				    struct netlink_ext_ack *extack)
> +{
> +	struct iosm_devlink *ipc_devlink = devlink_priv(devlink);
> +	enum iosm_flash_comp_type fls_type;
> +	u32 rc = -EINVAL;

This should be an int.

> +	u8 *mdm_rsp;
> +
> +	if (!params->component)
> +		return rc;

It's more readable to return literals.  "return -EINVAL;"

> +
> +	mdm_rsp = kzalloc(IOSM_EBL_DW_PACK_SIZE, GFP_KERNEL);
> +	if (!mdm_rsp)
> +		return -ENOMEM;
> +
> +	fls_type = ipc_devlink_get_flash_comp_type(params->component,
> +						   strlen(params->component));
> +
> +	switch (fls_type) {
> +	case FLASH_COMP_TYPE_PSI:
> +		rc = ipc_flash_boot_psi(ipc_devlink, params->fw);
> +		break;
> +	case FLASH_COMP_TYPE_EBL:
> +		rc = ipc_flash_boot_ebl(ipc_devlink, params->fw);
> +		if (!rc)

Always do error handling.  Never do success handling.  If you do:

	if (rc)
		break;

> +			rc = ipc_flash_boot_set_capabilities(ipc_devlink,
> +							     mdm_rsp);

Then this fits on one line.

		rc = ipc_flash_boot_set_capabilities(ipc_devlink, mdm_rsp);


> +		if (!rc)
> +			rc = ipc_flash_read_swid(ipc_devlink, mdm_rsp);
> +		break;
> +	case FLASH_COMP_TYPE_FLS:
> +		rc = ipc_flash_send_fls(ipc_devlink, params->fw, mdm_rsp);
> +		break;
> +	default:
> +		devlink_flash_update_status_notify(devlink, "Invalid component",
> +						   params->component, 0, 0);
> +		break;
> +	}
> +
> +	if (!rc)
> +		devlink_flash_update_status_notify(devlink, "Flashing success",
> +						   params->component, 0, 0);
> +	else
> +		devlink_flash_update_status_notify(devlink, "Flashing failed",
> +						   params->component, 0, 0);
> +
> +	kfree(mdm_rsp);
> +	return rc;
> +}

[ snip ]

> +static int ipc_devlink_coredump_snapshot(struct devlink *dl,
> +					 const struct devlink_region_ops *ops,
> +					 struct netlink_ext_ack *extack,
> +					 u8 **data)
> +{
> +	struct iosm_devlink *ipc_devlink = devlink_priv(dl);
> +	struct iosm_coredump_file_info *cd_list = ops->priv;
> +	u32 region_size;
> +	int rc;
> +
> +	dev_dbg(ipc_devlink->dev, "Region:%s, ID:%d", ops->name,
> +		cd_list->entry);
> +	region_size = cd_list->default_size;
> +	rc = ipc_coredump_collect(ipc_devlink, data, cd_list->entry,
> +				  region_size);
> +	if (rc) {
> +		dev_err(ipc_devlink->dev, "Fail to create snapshot,err %d", rc);
> +		goto coredump_collect_err;
> +	}
> +
> +	/* Send coredump end cmd indicating end of coredump collection */
> +	if (cd_list->entry == (IOSM_NOF_CD_REGION - 1))
> +		ipc_coredump_get_list(ipc_devlink, rpsi_cmd_coredump_end);
> +
> +	return rc;

Return literals if possible.

	return 0;

Put a blank line after the "return 0;"

> +coredump_collect_err:
> +	ipc_coredump_get_list(ipc_devlink, rpsi_cmd_coredump_end);
> +	return rc;
> +}


[ snip ]

> +/**
> + * struct iosm_rpsi_cmd - Structure for RPSI Command
> + * @param:      Used to calculate CRC
> + * @cmd:        Stores the RPSI command
> + * @crc:        Stores the CRC value
> + */
> +struct iosm_rpsi_cmd {
> +	union iosm_rpsi_param_u param;
> +	__le16	cmd;
> +	__le16	crc;
> +};
> +
> +/**
> + * ipc_devlink_init - To initialize the devlink to IOSM driver
> + * @ipc_imem:	Pointer to struct iosm_imem
> + *
> + * Returns:	Pointer to iosm_devlink on success and NULL on failure
> + */

These function comments should go in the .c file instead of the .h file.

> +struct iosm_devlink *ipc_devlink_init(struct iosm_imem *ipc_imem);
> +
> +/**
> + * ipc_devlink_deinit - To unintialize the devlink from IOSM driver.
> + * @ipc_devlink:	Devlink instance
> + */

Same.

> +void ipc_devlink_deinit(struct iosm_devlink *ipc_devlink);
> +
> +/**
> + * ipc_devlink_send_cmd - Send command to Modem
> + * @ipc_devlink: Pointer to struct iosm_devlink
> + * @cmd:	 Command to be sent to modem
> + * @entry:	 Command entry number
> + *
> + * Returns:	 0 on success and failure value on error
> + */

Same.

> +int ipc_devlink_send_cmd(struct iosm_devlink *ipc_devlink, u16 cmd, u32 entry);
> +
> +#endif /* _IOSM_IPC_DEVLINK_H */

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-09-20  6:53 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-19 17:26 [PATCH V2 net-next 1/6] net: wwan: iosm: devlink registration M Chetan Kumar
2021-09-20  6:53 ` Dan Carpenter

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.