This patch series adds support for TEE based BNXT firmware management module and the driver changes to invoke OP-TEE APIs to fastboot firmware and to collect crash dump.
Changes from v1: - address review comemnt from scott, - update error msg and introduce HANDLE_ERROR Macro - address review comment from Greg, - use KBUILD_MODNAME, indent comment, build with COMPILE_TEST
Vasundhara Volam (2): bnxt_en: Add support to invoke OP-TEE API to reset firmware bnxt_en: Add support to collect crash dump via ethtool
Vikas Gupta (1): firmware: broadcom: add OP-TEE based BNXT f/w manager
drivers/firmware/broadcom/Kconfig | 8 + drivers/firmware/broadcom/Makefile | 1 + drivers/firmware/broadcom/tee_bnxt_fw.c | 283 ++++++++++++++++++++++ drivers/net/ethernet/broadcom/bnxt/bnxt.c | 13 +- drivers/net/ethernet/broadcom/bnxt/bnxt.h | 6 + drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 36 ++- drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h | 2 + include/linux/firmware/broadcom/tee_bnxt_fw.h | 14 ++ 8 files changed, 359 insertions(+), 4 deletions(-) create mode 100644 drivers/firmware/broadcom/tee_bnxt_fw.c create mode 100644 include/linux/firmware/broadcom/tee_bnxt_fw.h
From: Vikas Gupta vikas.gupta@broadcom.com
This driver registers on TEE bus to interact with OP-TEE based BNXT firmware management modules
Signed-off-by: Vikas Gupta vikas.gupta@broadcom.com Signed-off-by: Sheetal Tigadoli sheetal.tigadoli@broadcom.com --- drivers/firmware/broadcom/Kconfig | 8 + drivers/firmware/broadcom/Makefile | 1 + drivers/firmware/broadcom/tee_bnxt_fw.c | 283 ++++++++++++++++++++++++++ include/linux/firmware/broadcom/tee_bnxt_fw.h | 14 ++ 4 files changed, 306 insertions(+) create mode 100644 drivers/firmware/broadcom/tee_bnxt_fw.c create mode 100644 include/linux/firmware/broadcom/tee_bnxt_fw.h
diff --git a/drivers/firmware/broadcom/Kconfig b/drivers/firmware/broadcom/Kconfig index d03ed8e..79505ad 100644 --- a/drivers/firmware/broadcom/Kconfig +++ b/drivers/firmware/broadcom/Kconfig @@ -22,3 +22,11 @@ config BCM47XX_SPROM In case of SoC devices SPROM content is stored on a flash used by bootloader firmware CFE. This driver provides method to ssb and bcma drivers to read SPROM on SoC. + +config TEE_BNXT_FW + bool "Broadcom BNXT firmware manager" + depends on (ARCH_BCM_IPROC && OPTEE) || COMPILE_TEST + default ARCH_BCM_IPROC + help + This module help to manage firmware on Broadcom BNXT device. The module + registers on tee bus and invoke calls to manage firmware on BNXT device. diff --git a/drivers/firmware/broadcom/Makefile b/drivers/firmware/broadcom/Makefile index 72c7fdc..17c5061 100644 --- a/drivers/firmware/broadcom/Makefile +++ b/drivers/firmware/broadcom/Makefile @@ -1,3 +1,4 @@ # SPDX-License-Identifier: GPL-2.0-only obj-$(CONFIG_BCM47XX_NVRAM) += bcm47xx_nvram.o obj-$(CONFIG_BCM47XX_SPROM) += bcm47xx_sprom.o +obj-$(CONFIG_TEE_BNXT_FW) += tee_bnxt_fw.o diff --git a/drivers/firmware/broadcom/tee_bnxt_fw.c b/drivers/firmware/broadcom/tee_bnxt_fw.c new file mode 100644 index 0000000..1e1e54c --- /dev/null +++ b/drivers/firmware/broadcom/tee_bnxt_fw.c @@ -0,0 +1,283 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2019 Broadcom. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/tee_drv.h> +#include <linux/uuid.h> + +#include <linux/firmware/broadcom/tee_bnxt_fw.h> + +#define MAX_SHM_MEM_SZ SZ_4M + +#define MAX_TEE_PARAM_ARRY_MEMB 4 + +#define HANDLE_ERROR(ret, tee_ret, cmd) \ +do { \ + if ((ret < 0) || (tee_ret != 0)) { \ + dev_err(pvt_data.dev, \ + #cmd " invoke failed TEE err: %x, ret:%x\n", \ + tee_ret, ret); \ + return -EINVAL; \ + } \ +} while (0) + +enum ta_cmd { + /* + * TA_CMD_BNXT_FASTBOOT - boot bnxt device by copying f/w into sram + * + * param[0] unused + * param[1] unused + * param[2] unused + * param[3] unused + * + * Result: + * TEE_SUCCESS - Invoke command success + * TEE_ERROR_ITEM_NOT_FOUND - Corrupt f/w image found on memory + */ + TA_CMD_BNXT_FASTBOOT = 0, + + /* + * TA_CMD_BNXT_COPY_COREDUMP - copy the core dump into shm + * + * param[0] (in value) - value.a: offset, data to be copied from + * value.b: size of the data + * param[1] unused + * param[2] unused + * param[3] unused + * + * Result: + * TEE_SUCCESS - Invoke command success + * TEE_ERROR_BAD_PARAMETERS - Incorrect input param + * TEE_ERROR_ITEM_NOT_FOUND - Corrupt core dump + */ + TA_CMD_BNXT_COPY_COREDUMP = 3, +}; + +/** + * struct tee_bnxt_fw_private - OP-TEE bnxt private data + * @dev: OP-TEE based bnxt device. + * @ctx: OP-TEE context handler. + * @session_id: TA session identifier. + */ +struct tee_bnxt_fw_private { + struct device *dev; + struct tee_context *ctx; + u32 session_id; + struct tee_shm *fw_shm_pool; +}; + +static struct tee_bnxt_fw_private pvt_data; + +static inline void prepare_args(int cmd, + struct tee_ioctl_invoke_arg *inv_arg, + struct tee_param *param) +{ + memset(inv_arg, 0, sizeof(*inv_arg)); + memset(param, 0, (MAX_TEE_PARAM_ARRY_MEMB * sizeof(*param))); + + inv_arg->func = cmd; + inv_arg->session = pvt_data.session_id; + inv_arg->num_params = MAX_TEE_PARAM_ARRY_MEMB; + + /* Fill invoke cmd params */ + switch (cmd) { + case TA_CMD_BNXT_COPY_COREDUMP: + param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT; + param[0].u.memref.shm = pvt_data.fw_shm_pool; + param[0].u.memref.size = MAX_SHM_MEM_SZ; + param[0].u.memref.shm_offs = 0; + param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; + break; + case TA_CMD_BNXT_FASTBOOT: + default: + /* Nothing to do */ + break; + } +} + +/** + * tee_bnxt_fw_load() - Load the bnxt firmware + * Uses an OP-TEE call to start a secure + * boot process. + * Returns 0 on success, negative errno otherwise. + */ +int tee_bnxt_fw_load(void) +{ + int ret = 0; + struct tee_ioctl_invoke_arg inv_arg; + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMB]; + + if (!pvt_data.ctx) + return -ENODEV; + + prepare_args(TA_CMD_BNXT_FASTBOOT, &inv_arg, param); + + ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param); + HANDLE_ERROR(ret, inv_arg.ret, TA_CMD_BNXT_FASTBOOT); + + return 0; +} +EXPORT_SYMBOL(tee_bnxt_fw_load); + +/** + * tee_bnxt_copy_coredump() - Copy coredump from the allocated memory + * Uses an OP-TEE call to copy coredump + * @buf: desintation buffer where core dump is copied into + * @offset: offset from the base address of core dump area + * @size: size of the dump + * + * Returns 0 on success, negative errno otherwise. + */ +int tee_bnxt_copy_coredump(void *buf, u32 offset, u32 size) +{ + struct tee_ioctl_invoke_arg inv_arg; + struct tee_param param[MAX_TEE_PARAM_ARRY_MEMB]; + void *core_data; + u32 rbytes = size; + u32 nbytes = 0; + int ret = 0; + + if (!pvt_data.ctx) + return -ENODEV; + + if (!buf) + return -EINVAL; + + prepare_args(TA_CMD_BNXT_COPY_COREDUMP, &inv_arg, param); + + while (rbytes) { + nbytes = rbytes; + + nbytes = min_t(u32, rbytes, param[0].u.memref.size); + + /* Fill additional invoke cmd params */ + param[1].u.value.a = offset; + param[1].u.value.b = nbytes; + + ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param); + HANDLE_ERROR(ret, inv_arg.ret, TA_CMD_BNXT_COPY_COREDUMP); + + core_data = tee_shm_get_va(pvt_data.fw_shm_pool, 0); + if (IS_ERR(core_data)) { + dev_err(pvt_data.dev, "tee_shm_get_va failed\n"); + return PTR_ERR(core_data); + } + + memcpy(buf, core_data, nbytes); + + rbytes -= nbytes; + buf += nbytes; + offset += nbytes; + } + + return 0; +} +EXPORT_SYMBOL(tee_bnxt_copy_coredump); + +static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data) +{ + if (ver->impl_id == TEE_IMPL_ID_OPTEE) + return 1; + else + return 0; +} + +static int tee_bnxt_fw_probe(struct device *dev) +{ + struct tee_client_device *bnxt_device = to_tee_client_device(dev); + int ret, err = -ENODEV; + struct tee_ioctl_open_session_arg sess_arg; + struct tee_shm *fw_shm_pool; + + memset(&sess_arg, 0, sizeof(sess_arg)); + + /* Open context with TEE driver */ + pvt_data.ctx = tee_client_open_context(NULL, optee_ctx_match, NULL, + NULL); + if (IS_ERR(pvt_data.ctx)) + return -ENODEV; + + /* Open session with Bnxt load Trusted App */ + memcpy(sess_arg.uuid, bnxt_device->id.uuid.b, TEE_IOCTL_UUID_LEN); + sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC; + sess_arg.num_params = 0; + + ret = tee_client_open_session(pvt_data.ctx, &sess_arg, NULL); + if ((ret < 0) || (sess_arg.ret != 0)) { + dev_err(dev, "tee_client_open_session failed, err: %x\n", + sess_arg.ret); + err = -EINVAL; + goto out_ctx; + } + pvt_data.session_id = sess_arg.session; + + pvt_data.dev = dev; + + fw_shm_pool = tee_shm_alloc(pvt_data.ctx, MAX_SHM_MEM_SZ, + TEE_SHM_MAPPED | TEE_SHM_DMA_BUF); + if (IS_ERR(fw_shm_pool)) { + tee_client_close_context(pvt_data.ctx); + dev_err(pvt_data.dev, "tee_shm_alloc failed\n"); + err = PTR_ERR(fw_shm_pool); + goto out_sess; + } + + pvt_data.fw_shm_pool = fw_shm_pool; + + return 0; + +out_sess: + tee_client_close_session(pvt_data.ctx, pvt_data.session_id); +out_ctx: + tee_client_close_context(pvt_data.ctx); + + return err; +} + +static int tee_bnxt_fw_remove(struct device *dev) +{ + tee_client_close_session(pvt_data.ctx, pvt_data.session_id); + tee_client_close_context(pvt_data.ctx); + pvt_data.ctx = NULL; + + return 0; +} + +static const struct tee_client_device_id tee_bnxt_fw_id_table[] = { + {UUID_INIT(0x6272636D, 0x2019, 0x0716, + 0x42, 0x43, 0x4D, 0x5F, 0x53, 0x43, 0x48, 0x49)}, + {} +}; + +MODULE_DEVICE_TABLE(tee, tee_bnxt_fw_id_table); + +static struct tee_client_driver tee_bnxt_fw_driver = { + .id_table = tee_bnxt_fw_id_table, + .driver = { + .name = KBUILD_MODNAME, + .bus = &tee_bus_type, + .probe = tee_bnxt_fw_probe, + .remove = tee_bnxt_fw_remove, + }, +}; + +static int __init tee_bnxt_fw_mod_init(void) +{ + return driver_register(&tee_bnxt_fw_driver.driver); +} + +static void __exit tee_bnxt_fw_mod_exit(void) +{ + driver_unregister(&tee_bnxt_fw_driver.driver); +} + +module_init(tee_bnxt_fw_mod_init); +module_exit(tee_bnxt_fw_mod_exit); + +MODULE_AUTHOR("Broadcom"); +MODULE_DESCRIPTION("Broadcom bnxt firmware manager"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/firmware/broadcom/tee_bnxt_fw.h b/include/linux/firmware/broadcom/tee_bnxt_fw.h new file mode 100644 index 0000000..f24c82d --- /dev/null +++ b/include/linux/firmware/broadcom/tee_bnxt_fw.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: BSD-2-Clause */ +/* + * Copyright 2019 Broadcom. + */ + +#ifndef _BROADCOM_TEE_BNXT_FW_H +#define _BROADCOM_TEE_BNXT_FW_H + +#include <linux/types.h> + +int tee_bnxt_fw_load(void); +int tee_bnxt_copy_coredump(void *buf, u32 offset, u32 size); + +#endif /* _BROADCOM_TEE_BNXT_FW_H */
On Thu, 17 Oct 2019 17:31:20 +0530, Sheetal Tigadoli wrote:
From: Vikas Gupta vikas.gupta@broadcom.com
This driver registers on TEE bus to interact with OP-TEE based BNXT firmware management modules
Signed-off-by: Vikas Gupta vikas.gupta@broadcom.com Signed-off-by: Sheetal Tigadoli sheetal.tigadoli@broadcom.com
drivers/firmware/broadcom/Kconfig | 8 + drivers/firmware/broadcom/Makefile | 1 + drivers/firmware/broadcom/tee_bnxt_fw.c | 283 ++++++++++++++++++++++++++ include/linux/firmware/broadcom/tee_bnxt_fw.h | 14 ++ 4 files changed, 306 insertions(+) create mode 100644 drivers/firmware/broadcom/tee_bnxt_fw.c create mode 100644 include/linux/firmware/broadcom/tee_bnxt_fw.h
diff --git a/drivers/firmware/broadcom/Kconfig b/drivers/firmware/broadcom/Kconfig index d03ed8e..79505ad 100644 --- a/drivers/firmware/broadcom/Kconfig +++ b/drivers/firmware/broadcom/Kconfig @@ -22,3 +22,11 @@ config BCM47XX_SPROM In case of SoC devices SPROM content is stored on a flash used by bootloader firmware CFE. This driver provides method to ssb and bcma drivers to read SPROM on SoC.
+config TEE_BNXT_FW
- bool "Broadcom BNXT firmware manager"
- depends on (ARCH_BCM_IPROC && OPTEE) || COMPILE_TEST
- default ARCH_BCM_IPROC
- help
This module help to manage firmware on Broadcom BNXT device. The moduleregisters on tee bus and invoke calls to manage firmware on BNXT device.diff --git a/drivers/firmware/broadcom/Makefile b/drivers/firmware/broadcom/Makefile index 72c7fdc..17c5061 100644 --- a/drivers/firmware/broadcom/Makefile +++ b/drivers/firmware/broadcom/Makefile @@ -1,3 +1,4 @@ # SPDX-License-Identifier: GPL-2.0-only obj-$(CONFIG_BCM47XX_NVRAM) += bcm47xx_nvram.o obj-$(CONFIG_BCM47XX_SPROM) += bcm47xx_sprom.o +obj-$(CONFIG_TEE_BNXT_FW) += tee_bnxt_fw.o diff --git a/drivers/firmware/broadcom/tee_bnxt_fw.c b/drivers/firmware/broadcom/tee_bnxt_fw.c new file mode 100644 index 0000000..1e1e54c --- /dev/null +++ b/drivers/firmware/broadcom/tee_bnxt_fw.c @@ -0,0 +1,283 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright 2019 Broadcom.
- */
+#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/tee_drv.h> +#include <linux/uuid.h>
+#include <linux/firmware/broadcom/tee_bnxt_fw.h>
+#define MAX_SHM_MEM_SZ SZ_4M
+#define MAX_TEE_PARAM_ARRY_MEMB 4
+#define HANDLE_ERROR(ret, tee_ret, cmd) \ +do { \
- if ((ret < 0) || (tee_ret != 0)) { \
The parenthesis should be around macro parameters.
if ((ret) < 0 || (tee_ret) != 0) {
|| has low priority
dev_err(pvt_data.dev, \#cmd " invoke failed TEE err: %x, ret:%x\n", \tee_ret, ret); \return -EINVAL; \
macros shouldn't change control flow, return statements in macros are bad
- } \
+} while (0)
+enum ta_cmd {
- /*
* TA_CMD_BNXT_FASTBOOT - boot bnxt device by copying f/w into sram** param[0] unused* param[1] unused* param[2] unused* param[3] unused** Result:* TEE_SUCCESS - Invoke command success* TEE_ERROR_ITEM_NOT_FOUND - Corrupt f/w image found on memory*/- TA_CMD_BNXT_FASTBOOT = 0,
- /*
* TA_CMD_BNXT_COPY_COREDUMP - copy the core dump into shm** param[0] (in value) - value.a: offset, data to be copied from* value.b: size of the data* param[1] unused* param[2] unused* param[3] unused** Result:* TEE_SUCCESS - Invoke command success* TEE_ERROR_BAD_PARAMETERS - Incorrect input param* TEE_ERROR_ITEM_NOT_FOUND - Corrupt core dump*/- TA_CMD_BNXT_COPY_COREDUMP = 3,
+};
+/**
- struct tee_bnxt_fw_private - OP-TEE bnxt private data
- @dev: OP-TEE based bnxt device.
- @ctx: OP-TEE context handler.
- @session_id: TA session identifier.
- */
+struct tee_bnxt_fw_private {
- struct device *dev;
- struct tee_context *ctx;
- u32 session_id;
- struct tee_shm *fw_shm_pool;
+};
+static struct tee_bnxt_fw_private pvt_data;
+static inline void prepare_args(int cmd,
No static inlines in .c files, please, the compiler will know what to inline.
struct tee_ioctl_invoke_arg *inv_arg,
inv usually means invalid, please rename to arg
struct tee_param *param)+{
- memset(inv_arg, 0, sizeof(*inv_arg));
- memset(param, 0, (MAX_TEE_PARAM_ARRY_MEMB * sizeof(*param)));
array_size(), outer parens unnecessary
- inv_arg->func = cmd;
- inv_arg->session = pvt_data.session_id;
- inv_arg->num_params = MAX_TEE_PARAM_ARRY_MEMB;
- /* Fill invoke cmd params */
- switch (cmd) {
- case TA_CMD_BNXT_COPY_COREDUMP:
param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT;param[0].u.memref.shm = pvt_data.fw_shm_pool;param[0].u.memref.size = MAX_SHM_MEM_SZ;param[0].u.memref.shm_offs = 0;param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;break;- case TA_CMD_BNXT_FASTBOOT:
- default:
/* Nothing to do */break;- }
+}
+/**
- tee_bnxt_fw_load() - Load the bnxt firmware
Uses an OP-TEE call to start a secure
boot process.
- Returns 0 on success, negative errno otherwise.
- */
+int tee_bnxt_fw_load(void)
This is a little strange. This function takes no parameters, so how does this thing know where to load the firmware? Is it not possible to have two devices that'd both require FW?
+{
- int ret = 0;
- struct tee_ioctl_invoke_arg inv_arg;
- struct tee_param param[MAX_TEE_PARAM_ARRY_MEMB];
- if (!pvt_data.ctx)
return -ENODEV;- prepare_args(TA_CMD_BNXT_FASTBOOT, &inv_arg, param);
- ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
- HANDLE_ERROR(ret, inv_arg.ret, TA_CMD_BNXT_FASTBOOT);
- return 0;
+} +EXPORT_SYMBOL(tee_bnxt_fw_load);
+/**
- tee_bnxt_copy_coredump() - Copy coredump from the allocated memory
Uses an OP-TEE call to copy coredump
- @buf: desintation buffer where core dump is copied into
- @offset: offset from the base address of core dump area
- @size: size of the dump
- Returns 0 on success, negative errno otherwise.
- */
+int tee_bnxt_copy_coredump(void *buf, u32 offset, u32 size) +{
- struct tee_ioctl_invoke_arg inv_arg;
- struct tee_param param[MAX_TEE_PARAM_ARRY_MEMB];
- void *core_data;
- u32 rbytes = size;
- u32 nbytes = 0;
- int ret = 0;
- if (!pvt_data.ctx)
return -ENODEV;- if (!buf)
return -EINVAL;
Generally we don't check args like that in the kernel, callers should not be broken.
- prepare_args(TA_CMD_BNXT_COPY_COREDUMP, &inv_arg, param);
- while (rbytes) {
nbytes = rbytes;nbytes = min_t(u32, rbytes, param[0].u.memref.size);/* Fill additional invoke cmd params */param[1].u.value.a = offset;param[1].u.value.b = nbytes;
But the doc above says param 1 is unused
ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);HANDLE_ERROR(ret, inv_arg.ret, TA_CMD_BNXT_COPY_COREDUMP);core_data = tee_shm_get_va(pvt_data.fw_shm_pool, 0);if (IS_ERR(core_data)) {dev_err(pvt_data.dev, "tee_shm_get_va failed\n");return PTR_ERR(core_data);}memcpy(buf, core_data, nbytes);rbytes -= nbytes;buf += nbytes;offset += nbytes;- }
- return 0;
+} +EXPORT_SYMBOL(tee_bnxt_copy_coredump);
+static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data) +{
- if (ver->impl_id == TEE_IMPL_ID_OPTEE)
return 1;- else
return 0;
Also known as return ver->impl_id == TEE_IMPL_ID_OPTEE; ...
+}
+static int tee_bnxt_fw_probe(struct device *dev) +{
- struct tee_client_device *bnxt_device = to_tee_client_device(dev);
- int ret, err = -ENODEV;
- struct tee_ioctl_open_session_arg sess_arg;
- struct tee_shm *fw_shm_pool;
- memset(&sess_arg, 0, sizeof(sess_arg));
- /* Open context with TEE driver */
- pvt_data.ctx = tee_client_open_context(NULL, optee_ctx_match, NULL,
NULL);- if (IS_ERR(pvt_data.ctx))
return -ENODEV;- /* Open session with Bnxt load Trusted App */
- memcpy(sess_arg.uuid, bnxt_device->id.uuid.b, TEE_IOCTL_UUID_LEN);
- sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
- sess_arg.num_params = 0;
- ret = tee_client_open_session(pvt_data.ctx, &sess_arg, NULL);
- if ((ret < 0) || (sess_arg.ret != 0)) {
parens unnecessary
dev_err(dev, "tee_client_open_session failed, err: %x\n",sess_arg.ret);err = -EINVAL;goto out_ctx;- }
- pvt_data.session_id = sess_arg.session;
- pvt_data.dev = dev;
- fw_shm_pool = tee_shm_alloc(pvt_data.ctx, MAX_SHM_MEM_SZ,
TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);- if (IS_ERR(fw_shm_pool)) {
tee_client_close_context(pvt_data.ctx);dev_err(pvt_data.dev, "tee_shm_alloc failed\n");err = PTR_ERR(fw_shm_pool);goto out_sess;- }
- pvt_data.fw_shm_pool = fw_shm_pool;
- return 0;
+out_sess:
- tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
+out_ctx:
- tee_client_close_context(pvt_data.ctx);
- return err;
+}
+static int tee_bnxt_fw_remove(struct device *dev) +{
- tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
- tee_client_close_context(pvt_data.ctx);
- pvt_data.ctx = NULL;
- return 0;
+}
+static const struct tee_client_device_id tee_bnxt_fw_id_table[] = {
- {UUID_INIT(0x6272636D, 0x2019, 0x0716,
0x42, 0x43, 0x4D, 0x5F, 0x53, 0x43, 0x48, 0x49)},- {}
+};
+MODULE_DEVICE_TABLE(tee, tee_bnxt_fw_id_table);
+static struct tee_client_driver tee_bnxt_fw_driver = {
- .id_table = tee_bnxt_fw_id_table,
- .driver = {
.name = KBUILD_MODNAME,.bus = &tee_bus_type,.probe = tee_bnxt_fw_probe,.remove = tee_bnxt_fw_remove,- },
+};
+static int __init tee_bnxt_fw_mod_init(void) +{
- return driver_register(&tee_bnxt_fw_driver.driver);
+}
+static void __exit tee_bnxt_fw_mod_exit(void) +{
- driver_unregister(&tee_bnxt_fw_driver.driver);
+}
+module_init(tee_bnxt_fw_mod_init); +module_exit(tee_bnxt_fw_mod_exit);
Someone should add a module_tee_driver macro, perhaps?
+MODULE_AUTHOR("Broadcom");
I think you can forego the author macro is it's a company.
+MODULE_DESCRIPTION("Broadcom bnxt firmware manager"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/firmware/broadcom/tee_bnxt_fw.h b/include/linux/firmware/broadcom/tee_bnxt_fw.h new file mode 100644 index 0000000..f24c82d --- /dev/null +++ b/include/linux/firmware/broadcom/tee_bnxt_fw.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: BSD-2-Clause */ +/*
- Copyright 2019 Broadcom.
- */
+#ifndef _BROADCOM_TEE_BNXT_FW_H +#define _BROADCOM_TEE_BNXT_FW_H
+#include <linux/types.h>
+int tee_bnxt_fw_load(void); +int tee_bnxt_copy_coredump(void *buf, u32 offset, u32 size);
+#endif /* _BROADCOM_TEE_BNXT_FW_H */
Hi Jakub,
Thanks for your comments.
On Fri, Oct 18, 2019 at 12:46 AM Jakub Kicinski jakub.kicinski@netronome.com wrote:
On Thu, 17 Oct 2019 17:31:20 +0530, Sheetal Tigadoli wrote:
From: Vikas Gupta vikas.gupta@broadcom.com
This driver registers on TEE bus to interact with OP-TEE based BNXT firmware management modules
Signed-off-by: Vikas Gupta vikas.gupta@broadcom.com Signed-off-by: Sheetal Tigadoli sheetal.tigadoli@broadcom.com
drivers/firmware/broadcom/Kconfig | 8 + drivers/firmware/broadcom/Makefile | 1 + drivers/firmware/broadcom/tee_bnxt_fw.c | 283 ++++++++++++++++++++++++++ include/linux/firmware/broadcom/tee_bnxt_fw.h | 14 ++ 4 files changed, 306 insertions(+) create mode 100644 drivers/firmware/broadcom/tee_bnxt_fw.c create mode 100644 include/linux/firmware/broadcom/tee_bnxt_fw.h
diff --git a/drivers/firmware/broadcom/Kconfig b/drivers/firmware/broadcom/Kconfig index d03ed8e..79505ad 100644 --- a/drivers/firmware/broadcom/Kconfig +++ b/drivers/firmware/broadcom/Kconfig @@ -22,3 +22,11 @@ config BCM47XX_SPROM In case of SoC devices SPROM content is stored on a flash used by bootloader firmware CFE. This driver provides method to ssb and bcma drivers to read SPROM on SoC.
+config TEE_BNXT_FW
bool "Broadcom BNXT firmware manager"depends on (ARCH_BCM_IPROC && OPTEE) || COMPILE_TESTdefault ARCH_BCM_IPROChelpThis module help to manage firmware on Broadcom BNXT device. The moduleregisters on tee bus and invoke calls to manage firmware on BNXT device.diff --git a/drivers/firmware/broadcom/Makefile b/drivers/firmware/broadcom/Makefile index 72c7fdc..17c5061 100644 --- a/drivers/firmware/broadcom/Makefile +++ b/drivers/firmware/broadcom/Makefile @@ -1,3 +1,4 @@ # SPDX-License-Identifier: GPL-2.0-only obj-$(CONFIG_BCM47XX_NVRAM) += bcm47xx_nvram.o obj-$(CONFIG_BCM47XX_SPROM) += bcm47xx_sprom.o +obj-$(CONFIG_TEE_BNXT_FW) += tee_bnxt_fw.o diff --git a/drivers/firmware/broadcom/tee_bnxt_fw.c b/drivers/firmware/broadcom/tee_bnxt_fw.c new file mode 100644 index 0000000..1e1e54c --- /dev/null +++ b/drivers/firmware/broadcom/tee_bnxt_fw.c @@ -0,0 +1,283 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright 2019 Broadcom.
- */
+#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/tee_drv.h> +#include <linux/uuid.h>
+#include <linux/firmware/broadcom/tee_bnxt_fw.h>
+#define MAX_SHM_MEM_SZ SZ_4M
+#define MAX_TEE_PARAM_ARRY_MEMB 4
+#define HANDLE_ERROR(ret, tee_ret, cmd) \ +do { \
if ((ret < 0) || (tee_ret != 0)) { \The parenthesis should be around macro parameters.
if ((ret) < 0 || (tee_ret) != 0) {|| has low priority
dev_err(pvt_data.dev, \#cmd " invoke failed TEE err: %x, ret:%x\n", \tee_ret, ret); \return -EINVAL; \macros shouldn't change control flow, return statements in macros are bad
Will remove the macro HANDLE_ERROR
} \+} while (0)
+enum ta_cmd {
/** TA_CMD_BNXT_FASTBOOT - boot bnxt device by copying f/w into sram** param[0] unused* param[1] unused* param[2] unused* param[3] unused** Result:* TEE_SUCCESS - Invoke command success* TEE_ERROR_ITEM_NOT_FOUND - Corrupt f/w image found on memory*/TA_CMD_BNXT_FASTBOOT = 0,/** TA_CMD_BNXT_COPY_COREDUMP - copy the core dump into shm** param[0] (in value) - value.a: offset, data to be copied from* value.b: size of the data* param[1] unused* param[2] unused* param[3] unused** Result:* TEE_SUCCESS - Invoke command success* TEE_ERROR_BAD_PARAMETERS - Incorrect input param* TEE_ERROR_ITEM_NOT_FOUND - Corrupt core dump*/TA_CMD_BNXT_COPY_COREDUMP = 3,+};
+/**
- struct tee_bnxt_fw_private - OP-TEE bnxt private data
- @dev: OP-TEE based bnxt device.
- @ctx: OP-TEE context handler.
- @session_id: TA session identifier.
- */
+struct tee_bnxt_fw_private {
struct device *dev;struct tee_context *ctx;u32 session_id;struct tee_shm *fw_shm_pool;+};
+static struct tee_bnxt_fw_private pvt_data;
+static inline void prepare_args(int cmd,
No static inlines in .c files, please, the compiler will know what to inline.
Will change this to static function.
struct tee_ioctl_invoke_arg *inv_arg,inv usually means invalid, please rename to arg
Ok.
struct tee_param *param)+{
memset(inv_arg, 0, sizeof(*inv_arg));memset(param, 0, (MAX_TEE_PARAM_ARRY_MEMB * sizeof(*param)));array_size(), outer parens unnecessary
Ok.
inv_arg->func = cmd;inv_arg->session = pvt_data.session_id;inv_arg->num_params = MAX_TEE_PARAM_ARRY_MEMB;/* Fill invoke cmd params */switch (cmd) {case TA_CMD_BNXT_COPY_COREDUMP:param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT;param[0].u.memref.shm = pvt_data.fw_shm_pool;param[0].u.memref.size = MAX_SHM_MEM_SZ;param[0].u.memref.shm_offs = 0;param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;break;case TA_CMD_BNXT_FASTBOOT:default:/* Nothing to do */break;}+}
+/**
- tee_bnxt_fw_load() - Load the bnxt firmware
Uses an OP-TEE call to start a secure
boot process.
- Returns 0 on success, negative errno otherwise.
- */
+int tee_bnxt_fw_load(void)
This is a little strange. This function takes no parameters, so how does this thing know where to load the firmware? Is it not possible to have two devices that'd both require FW?
This function invokes command to optee pta, which is aware of where to load the firmware and we don't have multiple devices
+{
int ret = 0;struct tee_ioctl_invoke_arg inv_arg;struct tee_param param[MAX_TEE_PARAM_ARRY_MEMB];if (!pvt_data.ctx)return -ENODEV;prepare_args(TA_CMD_BNXT_FASTBOOT, &inv_arg, param);ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);HANDLE_ERROR(ret, inv_arg.ret, TA_CMD_BNXT_FASTBOOT);return 0;+} +EXPORT_SYMBOL(tee_bnxt_fw_load);
+/**
- tee_bnxt_copy_coredump() - Copy coredump from the allocated memory
Uses an OP-TEE call to copy coredump
- @buf: desintation buffer where core dump is copied into
- @offset: offset from the base address of core dump area
- @size: size of the dump
- Returns 0 on success, negative errno otherwise.
- */
+int tee_bnxt_copy_coredump(void *buf, u32 offset, u32 size) +{
struct tee_ioctl_invoke_arg inv_arg;struct tee_param param[MAX_TEE_PARAM_ARRY_MEMB];void *core_data;u32 rbytes = size;u32 nbytes = 0;int ret = 0;if (!pvt_data.ctx)return -ENODEV;if (!buf)return -EINVAL;Generally we don't check args like that in the kernel, callers should not be broken.
Ok. will remove
prepare_args(TA_CMD_BNXT_COPY_COREDUMP, &inv_arg, param);while (rbytes) {nbytes = rbytes;nbytes = min_t(u32, rbytes, param[0].u.memref.size);/* Fill additional invoke cmd params */param[1].u.value.a = offset;param[1].u.value.b = nbytes;But the doc above says param 1 is unused
will update the doc/enum description
ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);HANDLE_ERROR(ret, inv_arg.ret, TA_CMD_BNXT_COPY_COREDUMP);core_data = tee_shm_get_va(pvt_data.fw_shm_pool, 0);if (IS_ERR(core_data)) {dev_err(pvt_data.dev, "tee_shm_get_va failed\n");return PTR_ERR(core_data);}memcpy(buf, core_data, nbytes);rbytes -= nbytes;buf += nbytes;offset += nbytes;}return 0;+} +EXPORT_SYMBOL(tee_bnxt_copy_coredump);
+static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data) +{
if (ver->impl_id == TEE_IMPL_ID_OPTEE)return 1;elsereturn 0;Also known as return ver->impl_id == TEE_IMPL_ID_OPTEE; ...
Ok.
+}
+static int tee_bnxt_fw_probe(struct device *dev) +{
struct tee_client_device *bnxt_device = to_tee_client_device(dev);int ret, err = -ENODEV;struct tee_ioctl_open_session_arg sess_arg;struct tee_shm *fw_shm_pool;memset(&sess_arg, 0, sizeof(sess_arg));/* Open context with TEE driver */pvt_data.ctx = tee_client_open_context(NULL, optee_ctx_match, NULL,NULL);if (IS_ERR(pvt_data.ctx))return -ENODEV;/* Open session with Bnxt load Trusted App */memcpy(sess_arg.uuid, bnxt_device->id.uuid.b, TEE_IOCTL_UUID_LEN);sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;sess_arg.num_params = 0;ret = tee_client_open_session(pvt_data.ctx, &sess_arg, NULL);if ((ret < 0) || (sess_arg.ret != 0)) {parens unnecessary
Will remove parens
dev_err(dev, "tee_client_open_session failed, err: %x\n",sess_arg.ret);err = -EINVAL;goto out_ctx;}pvt_data.session_id = sess_arg.session;pvt_data.dev = dev;fw_shm_pool = tee_shm_alloc(pvt_data.ctx, MAX_SHM_MEM_SZ,TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);if (IS_ERR(fw_shm_pool)) {tee_client_close_context(pvt_data.ctx);dev_err(pvt_data.dev, "tee_shm_alloc failed\n");err = PTR_ERR(fw_shm_pool);goto out_sess;}pvt_data.fw_shm_pool = fw_shm_pool;return 0;+out_sess:
tee_client_close_session(pvt_data.ctx, pvt_data.session_id);+out_ctx:
tee_client_close_context(pvt_data.ctx);return err;+}
+static int tee_bnxt_fw_remove(struct device *dev) +{
tee_client_close_session(pvt_data.ctx, pvt_data.session_id);tee_client_close_context(pvt_data.ctx);pvt_data.ctx = NULL;return 0;+}
+static const struct tee_client_device_id tee_bnxt_fw_id_table[] = {
{UUID_INIT(0x6272636D, 0x2019, 0x0716,0x42, 0x43, 0x4D, 0x5F, 0x53, 0x43, 0x48, 0x49)},{}+};
+MODULE_DEVICE_TABLE(tee, tee_bnxt_fw_id_table);
+static struct tee_client_driver tee_bnxt_fw_driver = {
.id_table = tee_bnxt_fw_id_table,.driver = {.name = KBUILD_MODNAME,.bus = &tee_bus_type,.probe = tee_bnxt_fw_probe,.remove = tee_bnxt_fw_remove,},+};
+static int __init tee_bnxt_fw_mod_init(void) +{
return driver_register(&tee_bnxt_fw_driver.driver);+}
+static void __exit tee_bnxt_fw_mod_exit(void) +{
driver_unregister(&tee_bnxt_fw_driver.driver);+}
+module_init(tee_bnxt_fw_mod_init); +module_exit(tee_bnxt_fw_mod_exit);
Someone should add a module_tee_driver macro, perhaps?
Not clear on this comment
+MODULE_AUTHOR("Broadcom");
I think you can forego the author macro is it's a company.
will update it to individual
+MODULE_DESCRIPTION("Broadcom bnxt firmware manager"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/firmware/broadcom/tee_bnxt_fw.h b/include/linux/firmware/broadcom/tee_bnxt_fw.h new file mode 100644 index 0000000..f24c82d --- /dev/null +++ b/include/linux/firmware/broadcom/tee_bnxt_fw.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: BSD-2-Clause */ +/*
- Copyright 2019 Broadcom.
- */
+#ifndef _BROADCOM_TEE_BNXT_FW_H +#define _BROADCOM_TEE_BNXT_FW_H
+#include <linux/types.h>
+int tee_bnxt_fw_load(void); +int tee_bnxt_copy_coredump(void *buf, u32 offset, u32 size);
+#endif /* _BROADCOM_TEE_BNXT_FW_H */
From: Vasundhara Volam vasundhara-v.volam@broadcom.com
In error recovery process when firmware indicates that it is completely down, initiate a firmware reset by calling OP-TEE API.
Cc: Michael Chan michael.chan@broadcom.com Signed-off-by: Vasundhara Volam vasundhara-v.volam@broadcom.com Signed-off-by: Sheetal Tigadoli sheetal.tigadoli@broadcom.com --- drivers/net/ethernet/broadcom/bnxt/bnxt.c | 13 +++++++++++-- drivers/net/ethernet/broadcom/bnxt/bnxt.h | 3 +++ 2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index b4a8cf6..b60b24e 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -10581,14 +10581,23 @@ static void bnxt_fw_reset_writel(struct bnxt *bp, int reg_idx) static void bnxt_reset_all(struct bnxt *bp) { struct bnxt_fw_health *fw_health = bp->fw_health; - int i; + int i, rc; + + if (bp->fw_cap & BNXT_FW_CAP_ERR_RECOVER_RELOAD) { +#ifdef CONFIG_TEE_BNXT_FW + rc = tee_bnxt_fw_load(); + if (rc) + netdev_err(bp->dev, "Unable to reset FW rc=%d\n", rc); + bp->fw_reset_timestamp = jiffies; +#endif + return; + }
if (fw_health->flags & ERROR_RECOVERY_QCFG_RESP_FLAGS_HOST) { for (i = 0; i < fw_health->fw_reset_seq_cnt; i++) bnxt_fw_reset_writel(bp, i); } else if (fw_health->flags & ERROR_RECOVERY_QCFG_RESP_FLAGS_CO_CPU) { struct hwrm_fw_reset_input req = {0}; - int rc;
bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_FW_RESET, -1, -1); req.resp_addr = cpu_to_le64(bp->hwrm_cmd_kong_resp_dma_addr); diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h index d333589..0943715 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h @@ -25,6 +25,9 @@ #include <net/dst_metadata.h> #include <net/xdp.h> #include <linux/dim.h> +#ifdef CONFIG_TEE_BNXT_FW +#include <linux/firmware/broadcom/tee_bnxt_fw.h> +#endif
struct page_pool;
From: Vasundhara Volam vasundhara-v.volam@broadcom.com
Driver supports 2 types of core dumps.
1. Live dump - Firmware dump when system is up and running. 2. Crash dump - Dump which is collected during firmware crash that can be retrieved after recovery. Crash dump is currently supported only on specific 58800 chips which can be retrieved using OP-TEE API only, as firmware cannot access this region directly.
User needs to set the dump flag using following command before initiating the dump collection:
$ ethtool -W|--set-dump eth0 N
Where N is "0" for live dump and "1" for crash dump
Command to collect the dump after setting the flag:
$ ethtool -w eth0 data Filename
Cc: Michael Chan michael.chan@broadcom.com Signed-off-by: Vasundhara Volam vasundhara-v.volam@broadcom.com Signed-off-by: Sheetal Tigadoli sheetal.tigadoli@broadcom.com --- drivers/net/ethernet/broadcom/bnxt/bnxt.h | 3 ++ drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 36 +++++++++++++++++++++-- drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h | 2 ++ 3 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h index 0943715..3e7d1fb 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h @@ -1807,6 +1807,9 @@ struct bnxt {
u8 num_leds; struct bnxt_led_info leds[BNXT_MAX_LED]; + u16 dump_flag; +#define BNXT_DUMP_LIVE 0 +#define BNXT_DUMP_CRASH 1
struct bpf_prog *xdp_prog;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c index 51c1404..1596221 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c @@ -3311,6 +3311,23 @@ static int bnxt_get_coredump(struct bnxt *bp, void *buf, u32 *dump_len) return rc; }
+static int bnxt_set_dump(struct net_device *dev, struct ethtool_dump *dump) +{ + struct bnxt *bp = netdev_priv(dev); + +#ifndef CONFIG_TEE_BNXT_FW + return -EOPNOTSUPP; +#endif + + if (dump->flag > BNXT_DUMP_CRASH) { + netdev_err(dev, "Supports only Live(0) and Crash(1) dumps.\n"); + return -EINVAL; + } + + bp->dump_flag = dump->flag; + return 0; +} + static int bnxt_get_dump_flag(struct net_device *dev, struct ethtool_dump *dump) { struct bnxt *bp = netdev_priv(dev); @@ -3323,7 +3340,12 @@ static int bnxt_get_dump_flag(struct net_device *dev, struct ethtool_dump *dump) bp->ver_resp.hwrm_fw_bld_8b << 8 | bp->ver_resp.hwrm_fw_rsvd_8b;
- return bnxt_get_coredump(bp, NULL, &dump->len); + dump->flag = bp->dump_flag; + if (bp->dump_flag == BNXT_DUMP_CRASH) + dump->len = BNXT_CRASH_DUMP_LEN; + else + bnxt_get_coredump(bp, NULL, &dump->len); + return 0; }
static int bnxt_get_dump_data(struct net_device *dev, struct ethtool_dump *dump, @@ -3336,7 +3358,16 @@ static int bnxt_get_dump_data(struct net_device *dev, struct ethtool_dump *dump,
memset(buf, 0, dump->len);
- return bnxt_get_coredump(bp, buf, &dump->len); + dump->flag = bp->dump_flag; + if (dump->flag == BNXT_DUMP_CRASH) { +#ifdef CONFIG_TEE_BNXT_FW + return tee_bnxt_copy_coredump(buf, 0, dump->len); +#endif + } else { + return bnxt_get_coredump(bp, buf, &dump->len); + } + + return 0; }
void bnxt_ethtool_init(struct bnxt *bp) @@ -3446,6 +3477,7 @@ void bnxt_ethtool_free(struct bnxt *bp) .set_phys_id = bnxt_set_phys_id, .self_test = bnxt_self_test, .reset = bnxt_reset, + .set_dump = bnxt_set_dump, .get_dump_flag = bnxt_get_dump_flag, .get_dump_data = bnxt_get_dump_data, }; diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h index b5b65b3..01de7e7 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h @@ -59,6 +59,8 @@ struct hwrm_dbg_cmn_output { #define HWRM_DBG_CMN_FLAGS_MORE 1 };
+#define BNXT_CRASH_DUMP_LEN (8 << 20) + #define BNXT_LED_DFLT_ENA \ (PORT_LED_CFG_REQ_ENABLES_LED0_ID | \ PORT_LED_CFG_REQ_ENABLES_LED0_STATE | \
On Thu, 17 Oct 2019 17:31:22 +0530, Sheetal Tigadoli wrote:
From: Vasundhara Volam vasundhara-v.volam@broadcom.com
Driver supports 2 types of core dumps.
- Live dump - Firmware dump when system is up and running.
- Crash dump - Dump which is collected during firmware crash that can be retrieved after recovery.
Crash dump is currently supported only on specific 58800 chips which can be retrieved using OP-TEE API only, as firmware cannot access this region directly.
User needs to set the dump flag using following command before initiating the dump collection:
$ ethtool -W|--set-dump eth0 NWhere N is "0" for live dump and "1" for crash dump
Command to collect the dump after setting the flag:
$ ethtool -w eth0 data FilenameCc: Michael Chan michael.chan@broadcom.com Signed-off-by: Vasundhara Volam vasundhara-v.volam@broadcom.com Signed-off-by: Sheetal Tigadoli sheetal.tigadoli@broadcom.com
drivers/net/ethernet/broadcom/bnxt/bnxt.h | 3 ++ drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 36 +++++++++++++++++++++-- drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h | 2 ++ 3 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h index 0943715..3e7d1fb 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h @@ -1807,6 +1807,9 @@ struct bnxt { u8 num_leds; struct bnxt_led_info leds[BNXT_MAX_LED];
- u16 dump_flag;
+#define BNXT_DUMP_LIVE 0 +#define BNXT_DUMP_CRASH 1 struct bpf_prog *xdp_prog; diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c index 51c1404..1596221 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c @@ -3311,6 +3311,23 @@ static int bnxt_get_coredump(struct bnxt *bp, void *buf, u32 *dump_len) return rc; } +static int bnxt_set_dump(struct net_device *dev, struct ethtool_dump *dump) +{
- struct bnxt *bp = netdev_priv(dev);
+#ifndef CONFIG_TEE_BNXT_FW
- return -EOPNOTSUPP;
+#endif
if (!IS_ENABLED(...)) return x;
reads better IMHO
But also you seem to be breaking live dump for systems with CONFIG_TEE_BNXT_FW=n
- if (dump->flag > BNXT_DUMP_CRASH) {
netdev_err(dev, "Supports only Live(0) and Crash(1) dumps.\n");
more of an _info than _err, if at all
return -EINVAL;- }
- bp->dump_flag = dump->flag;
- return 0;
+}
static int bnxt_get_dump_flag(struct net_device *dev, struct ethtool_dump *dump) { struct bnxt *bp = netdev_priv(dev); @@ -3323,7 +3340,12 @@ static int bnxt_get_dump_flag(struct net_device *dev, struct ethtool_dump *dump) bp->ver_resp.hwrm_fw_bld_8b << 8 | bp->ver_resp.hwrm_fw_rsvd_8b;
- return bnxt_get_coredump(bp, NULL, &dump->len);
- dump->flag = bp->dump_flag;
- if (bp->dump_flag == BNXT_DUMP_CRASH)
dump->len = BNXT_CRASH_DUMP_LEN;- else
bnxt_get_coredump(bp, NULL, &dump->len);- return 0;
} static int bnxt_get_dump_data(struct net_device *dev, struct ethtool_dump *dump, @@ -3336,7 +3358,16 @@ static int bnxt_get_dump_data(struct net_device *dev, struct ethtool_dump *dump, memset(buf, 0, dump->len);
- return bnxt_get_coredump(bp, buf, &dump->len);
- dump->flag = bp->dump_flag;
- if (dump->flag == BNXT_DUMP_CRASH) {
+#ifdef CONFIG_TEE_BNXT_FW
return tee_bnxt_copy_coredump(buf, 0, dump->len);+#endif
- } else {
return bnxt_get_coredump(bp, buf, &dump->len);- }
- return 0;
} void bnxt_ethtool_init(struct bnxt *bp) @@ -3446,6 +3477,7 @@ void bnxt_ethtool_free(struct bnxt *bp) .set_phys_id = bnxt_set_phys_id, .self_test = bnxt_self_test, .reset = bnxt_reset,
- .set_dump = bnxt_set_dump, .get_dump_flag = bnxt_get_dump_flag, .get_dump_data = bnxt_get_dump_data,
}; diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h index b5b65b3..01de7e7 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h @@ -59,6 +59,8 @@ struct hwrm_dbg_cmn_output { #define HWRM_DBG_CMN_FLAGS_MORE 1 }; +#define BNXT_CRASH_DUMP_LEN (8 << 20)
#define BNXT_LED_DFLT_ENA \ (PORT_LED_CFG_REQ_ENABLES_LED0_ID | \ PORT_LED_CFG_REQ_ENABLES_LED0_STATE | \
On Fri, Oct 18, 2019 at 12:52 AM Jakub Kicinski jakub.kicinski@netronome.com wrote:
On Thu, 17 Oct 2019 17:31:22 +0530, Sheetal Tigadoli wrote:
From: Vasundhara Volam vasundhara-v.volam@broadcom.com
Driver supports 2 types of core dumps.
- Live dump - Firmware dump when system is up and running.
- Crash dump - Dump which is collected during firmware crash that can be retrieved after recovery.
Crash dump is currently supported only on specific 58800 chips which can be retrieved using OP-TEE API only, as firmware cannot access this region directly.
User needs to set the dump flag using following command before initiating the dump collection:
$ ethtool -W|--set-dump eth0 NWhere N is "0" for live dump and "1" for crash dump
Command to collect the dump after setting the flag:
$ ethtool -w eth0 data FilenameCc: Michael Chan michael.chan@broadcom.com Signed-off-by: Vasundhara Volam vasundhara-v.volam@broadcom.com Signed-off-by: Sheetal Tigadoli sheetal.tigadoli@broadcom.com
drivers/net/ethernet/broadcom/bnxt/bnxt.h | 3 ++ drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 36 +++++++++++++++++++++-- drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h | 2 ++ 3 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h index 0943715..3e7d1fb 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h @@ -1807,6 +1807,9 @@ struct bnxt {
u8 num_leds; struct bnxt_led_info leds[BNXT_MAX_LED];
u16 dump_flag;+#define BNXT_DUMP_LIVE 0 +#define BNXT_DUMP_CRASH 1
struct bpf_prog *xdp_prog;diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c index 51c1404..1596221 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c @@ -3311,6 +3311,23 @@ static int bnxt_get_coredump(struct bnxt *bp, void *buf, u32 *dump_len) return rc; }
+static int bnxt_set_dump(struct net_device *dev, struct ethtool_dump *dump) +{
struct bnxt *bp = netdev_priv(dev);+#ifndef CONFIG_TEE_BNXT_FW
return -EOPNOTSUPP;+#endif
if (!IS_ENABLED(...)) return x;reads better IMHO
Okay.
But also you seem to be breaking live dump for systems with CONFIG_TEE_BNXT_FW=n
Yes, we are supporting set_dump only if crash dump is supported.
if (dump->flag > BNXT_DUMP_CRASH) {netdev_err(dev, "Supports only Live(0) and Crash(1) dumps.\n");more of an _info than _err, if at all
I made this err, as we are returning error on invalid flag value. I can modify the log to something like "Invalid dump flag. Supports only Live(0) and Crash(1) dumps.\n" to make it more like error log.
return -EINVAL;}bp->dump_flag = dump->flag;return 0;+}
static int bnxt_get_dump_flag(struct net_device *dev, struct ethtool_dump *dump) { struct bnxt *bp = netdev_priv(dev); @@ -3323,7 +3340,12 @@ static int bnxt_get_dump_flag(struct net_device *dev, struct ethtool_dump *dump) bp->ver_resp.hwrm_fw_bld_8b << 8 | bp->ver_resp.hwrm_fw_rsvd_8b;
return bnxt_get_coredump(bp, NULL, &dump->len);
dump->flag = bp->dump_flag;if (bp->dump_flag == BNXT_DUMP_CRASH)dump->len = BNXT_CRASH_DUMP_LEN;elsebnxt_get_coredump(bp, NULL, &dump->len);return 0;}
static int bnxt_get_dump_data(struct net_device *dev, struct ethtool_dump *dump, @@ -3336,7 +3358,16 @@ static int bnxt_get_dump_data(struct net_device *dev, struct ethtool_dump *dump,
memset(buf, 0, dump->len);
return bnxt_get_coredump(bp, buf, &dump->len);
dump->flag = bp->dump_flag;if (dump->flag == BNXT_DUMP_CRASH) {+#ifdef CONFIG_TEE_BNXT_FW
return tee_bnxt_copy_coredump(buf, 0, dump->len);+#endif
} else {return bnxt_get_coredump(bp, buf, &dump->len);}return 0;}
void bnxt_ethtool_init(struct bnxt *bp) @@ -3446,6 +3477,7 @@ void bnxt_ethtool_free(struct bnxt *bp) .set_phys_id = bnxt_set_phys_id, .self_test = bnxt_self_test, .reset = bnxt_reset,
.set_dump = bnxt_set_dump, .get_dump_flag = bnxt_get_dump_flag, .get_dump_data = bnxt_get_dump_data,}; diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h index b5b65b3..01de7e7 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h @@ -59,6 +59,8 @@ struct hwrm_dbg_cmn_output { #define HWRM_DBG_CMN_FLAGS_MORE 1 };
+#define BNXT_CRASH_DUMP_LEN (8 << 20)
#define BNXT_LED_DFLT_ENA \ (PORT_LED_CFG_REQ_ENABLES_LED0_ID | \ PORT_LED_CFG_REQ_ENABLES_LED0_STATE | \
Thanks, Vasundhara
On Fri, 18 Oct 2019 12:04:35 +0530, Vasundhara Volam wrote:
On Fri, Oct 18, 2019 at 12:52 AM Jakub Kicinski wrote:
On Thu, 17 Oct 2019 17:31:22 +0530, Sheetal Tigadoli wrote:
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c index 51c1404..1596221 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c @@ -3311,6 +3311,23 @@ static int bnxt_get_coredump(struct bnxt *bp, void *buf, u32 *dump_len) return rc; }
+static int bnxt_set_dump(struct net_device *dev, struct ethtool_dump *dump) +{
struct bnxt *bp = netdev_priv(dev);+#ifndef CONFIG_TEE_BNXT_FW
return -EOPNOTSUPP;+#endif
if (!IS_ENABLED(...)) return x;reads better IMHO
Okay.
But also you seem to be breaking live dump for systems with CONFIG_TEE_BNXT_FW=n
Yes, we are supporting set_dump only if crash dump is supported.
It's wrong.
if (dump->flag > BNXT_DUMP_CRASH) {netdev_err(dev, "Supports only Live(0) and Crash(1) dumps.\n");more of an _info than _err, if at all
I made this err, as we are returning error on invalid flag value. I can modify the log to something like "Invalid dump flag. Supports only Live(0) and Crash(1) dumps.\n" to make it more like error log.
Not an error.
On Fri, Oct 18, 2019 at 10:31 PM Jakub Kicinski jakub.kicinski@netronome.com wrote:
On Fri, 18 Oct 2019 12:04:35 +0530, Vasundhara Volam wrote:
On Fri, Oct 18, 2019 at 12:52 AM Jakub Kicinski wrote:
On Thu, 17 Oct 2019 17:31:22 +0530, Sheetal Tigadoli wrote:
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c index 51c1404..1596221 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c @@ -3311,6 +3311,23 @@ static int bnxt_get_coredump(struct bnxt *bp, void *buf, u32 *dump_len) return rc; }
+static int bnxt_set_dump(struct net_device *dev, struct ethtool_dump *dump) +{
struct bnxt *bp = netdev_priv(dev);+#ifndef CONFIG_TEE_BNXT_FW
return -EOPNOTSUPP;+#endif
if (!IS_ENABLED(...)) return x;reads better IMHO
Okay.
But also you seem to be breaking live dump for systems with CONFIG_TEE_BNXT_FW=n
Yes, we are supporting set_dump only if crash dump is supported.
It's wrong.
Sorry not very clear. You are saying that support set_dump all the time and return error, if the config option is not enabled? If yes, I will modify the same way as it makes sense.
if (dump->flag > BNXT_DUMP_CRASH) {netdev_err(dev, "Supports only Live(0) and Crash(1) dumps.\n");more of an _info than _err, if at all
I made this err, as we are returning error on invalid flag value. I can modify the log to something like "Invalid dump flag. Supports only Live(0) and Crash(1) dumps.\n" to make it more like error log.
Not an error.
Okay.
On Mon, 21 Oct 2019 10:05:18 +0530, Vasundhara Volam wrote:
On Fri, Oct 18, 2019 at 10:31 PM Jakub Kicinski wrote:
On Fri, 18 Oct 2019 12:04:35 +0530, Vasundhara Volam wrote:
On Fri, Oct 18, 2019 at 12:52 AM Jakub Kicinski wrote:
On Thu, 17 Oct 2019 17:31:22 +0530, Sheetal Tigadoli wrote:
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c index 51c1404..1596221 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c @@ -3311,6 +3311,23 @@ static int bnxt_get_coredump(struct bnxt *bp, void *buf, u32 *dump_len) return rc; }
+static int bnxt_set_dump(struct net_device *dev, struct ethtool_dump *dump) +{
struct bnxt *bp = netdev_priv(dev);+#ifndef CONFIG_TEE_BNXT_FW
return -EOPNOTSUPP;+#endif
if (!IS_ENABLED(...)) return x;reads better IMHO
Okay.
But also you seem to be breaking live dump for systems with CONFIG_TEE_BNXT_FW=n
Yes, we are supporting set_dump only if crash dump is supported.
It's wrong.
Sorry not very clear. You are saying that support set_dump all the time and return error, if the config option is not enabled? If yes, I will modify the same way as it makes sense.
Yes, I think users can expect that set will work even if there is only one dump type/flag supported. Technically implementing the set is not required from functional perspective, however, we could imagine software that always does a set before a get, and the set should not fail.
-
Jakub Kicinski -
Sheetal Tigadoli -
Vasundhara Volam