On 9/11/19 10:53 AM, Sheetal Tigadoli wrote:
Thanks for the review and comments.
On Tue, Sep 10, 2019 at 10:46 PM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Tue, Sep 10, 2019 at 08:47:04PM +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 | 447 ++++++++++++++++++++++++++ include/linux/firmware/broadcom/tee_bnxt_fw.h | 17 + 4 files changed, 473 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 6468082..a846a21 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 && OPTEENo ability to build with compile testing?
Will add "|| COMPILE_TEST"
default 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 00000000..89a48fd --- /dev/null +++ b/drivers/firmware/broadcom/tee_bnxt_fw.c @@ -0,0 +1,447 @@ +// 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 DRIVER_NAME "tee-bnxt-fw"
KBUILD_MODNAME?
Will remove DRIVER_NAME macro and use KBUILD_MODNAME instead.
+#define MAX_SHM_MEM_SZ SZ_4M
Why?
Limiting max data buffer size per request to optee to 4MB.
+#define MAX_TEE_PARAM_ARRY_MEMB 4
+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,Please indent the comments too. As-is this is hard to read.
Will do
+/*
- TA_CMD_BNXT_HEALTH_STATUS - to check health of bnxt device
- param[0] (out value) - value.a: health status
- param[1] unused
- param[2] unused
- param[3] unused
- Result:
- TEE_SUCCESS - Invoke command success
- TEE_ERROR_BAD_PARAMETERS - Incorrect input param
- */
TA_CMD_BNXT_HEALTH_STATUS,Should all of these have explicit values?
Will initialize each cmd (TA_CMD_BNXT_*) explicitly
+/*
- TA_CMD_BNXT_HANDSHAKE - to check bnxt device is booted
- param[0] (in value) - value.a: max timeout value
- param[0] (out value) - value.a: boot status
- param[1] unused
- param[2] unused
- param[3] unused
- Result:
- TEE_SUCCESS - Invoke command success
- TEE_ERROR_BAD_PARAMETERS - Incorrect input param
- */
TA_CMD_BNXT_HANDSHAKE,+/*
- TA_CMD_BNXT_COPY_COREDUMP - copy the core dump into shm
- param[0] (in value) - value.a: offset at which 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,+/*
- TA_CMD_BNXT_FW_UPGRADE - upgrade the bnxt firmware
- param[0] (in value) - value.a: size of the f/w image
- param[1] unused
- param[2] unused
- param[3] unused
- Result:
- TEE_SUCCESS - Invoke command success
- TEE_ERROR_BAD_PARAMETERS - Incorrect input param
- */
TA_CMD_BNXT_FW_UPGRADE,+};
+/**
- 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;Why is the pointer back needed?
the dev pointer is used in "dev_err()"
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_HEALTH_STATUS:param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;break;case TA_CMD_BNXT_HANDSHAKE:param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT;break;case TA_CMD_BNXT_COPY_COREDUMP:case TA_CMD_BNXT_FW_UPGRADE: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);if ((ret < 0) || (inv_arg.ret != 0)) {dev_err(pvt_data.dev, "TA_CMD_BNXT_LOAD invoke err: %x\n",(ret < 0) ? ret : inv_arg.ret);return -EINVAL;}return 0;+} +EXPORT_SYMBOL(tee_bnxt_fw_load);
Why are you exporting symbols for a single file? What uses these?
These apis will be used by bnxt driver, still working on open sourcing changes
Hi Greg,
The above API is used by the bnxt_en Ethernet driver to load firmware that is stored in the secure region of DDR that can only be accessed through secure calls via TEE.
EXPORT_SYMBOL is needed given that bnxt_en driver (the client that uses this API) is usually loaded as a module.
This feels really wrong, are you sure this all is correct?
Is there anything specific that looks wrong? Please help to point out and we'll fix it.
Thanks,
Ray
I stopped reading here :)
thanks,
greg k-h