From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.5 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C5B31C49ED9 for ; Tue, 10 Sep 2019 17:16:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 9493B2168B for ; Tue, 10 Sep 2019 17:16:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1568135766; bh=hUYdhn7tFl6XOIHOWQRJF5fSSz97EzLxhwquR6JM8RQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:List-ID:From; b=g9vfEMaK5uMjKGuKD8oLhqUg2f7aV9x+p3acX0jFGWnLqfBMu782fYpAJqvEo6Bd0 0R0aBo1r9wOzoiOp/VtfS/XtxRRMZEJpFHBTK01yoiZgvt6iop2DnRFV90T4xOs1N6 MuGEm2qxNe6Zq9EBc8GF8EiYH+FClEkNPJKaACtI= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2437100AbfIJRQG (ORCPT ); Tue, 10 Sep 2019 13:16:06 -0400 Received: from mail.kernel.org ([198.145.29.99]:38658 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2437088AbfIJRQF (ORCPT ); Tue, 10 Sep 2019 13:16:05 -0400 Received: from localhost (110.8.30.213.rev.vodafone.pt [213.30.8.110]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 6EADF2081B; Tue, 10 Sep 2019 17:16:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1568135764; bh=hUYdhn7tFl6XOIHOWQRJF5fSSz97EzLxhwquR6JM8RQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=sXDmL5mSBT5euAGAZPqxXUqzw0UFP0uFkP/pG7aKaZmh1fF9xAXMlJ2OpEWpOo7of TnQ4oaGoaAZljjOztF78qwJSuXeGtowodS81bvPIhet6Gtu564hp73M0stF3Q1r/lX +hNkdh4MuZPDYjJvtt3mVdoSElPBVlcsplcLQ0Jk= Date: Tue, 10 Sep 2019 18:16:01 +0100 From: Greg Kroah-Hartman To: Sheetal Tigadoli Cc: =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= , Ard Biesheuvel , Ingo Molnar , Thomas Gleixner , Michal Simek , Rajan Vaja , Scott Branden , Ray Jui , Vikram Prakash , tee-dev@lists.linaro.org, bcm-kernel-feedback-list@broadcom.com, linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org, Vikas Gupta Subject: Re: [PATCH] firmware: broadcom: add OP-TEE based BNXT f/w manager Message-ID: <20190910171601.GA12665@kroah.com> References: <1568128624-2902-1-git-send-email-sheetal.tigadoli@broadcom.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1568128624-2902-1-git-send-email-sheetal.tigadoli@broadcom.com> User-Agent: Mutt/1.12.1 (2019-06-15) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 10, 2019 at 08:47:04PM +0530, Sheetal Tigadoli wrote: > From: Vikas Gupta > > This driver registers on TEE bus to interact with OP-TEE based > BNXT firmware management modules > > Signed-off-by: Vikas Gupta > Signed-off-by: Sheetal Tigadoli > --- > 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 && OPTEE No ability to build with compile testing? > + 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 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 > +#include > +#include > +#include > +#include > + > +#include > + > +#define DRIVER_NAME "tee-bnxt-fw" KBUILD_MODNAME? > +#define MAX_SHM_MEM_SZ SZ_4M Why? > + > +#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. > +/* > + * 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? > + > +/* > + * 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? > + 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? This feels really wrong, are you sure this all is correct? I stopped reading here :) thanks, greg k-h