Re: [PATCH V3 0/3] Add OP-TEE based bnxt f/w manager

From: Florian Fainelli <f.fainelli@gmail.com>
To: "Sheetal Tigadoli" <sheetal.tigadoli@broadcom.com>,
	"Rafał Miłecki" <zajec5@gmail.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Michal Simek" <michal.simek@xilinx.com>,
	"Rajan Vaja" <rajan.vaja@xilinx.com>,
	"Scott Branden" <scott.branden@broadcom.com>,
	"Ray Jui" <ray.jui@broadcom.com>,
	"Vikram Prakash" <vikram.prakash@broadcom.com>,
	"Jens Wiklander" <jens.wiklander@linaro.org>,
	"Michael Chan" <michael.chan@broadcom.com>,
	"David S. Miller" <davem@davemloft.net>,
	"Vikas Gupta" <vikas.gupta@broadcom.com>,
	"Vasundhara Volam" <vasundhara-v.volam@broadcom.com>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	tee-dev@lists.linaro.org, bcm-kernel-feedback-list@broadcom.com,
	netdev@vger.kernel.org
Subject: Re: [PATCH V3 0/3] Add OP-TEE based bnxt f/w manager
Date: Thu, 24 Oct 2019 11:39:42 -0700	[thread overview]
Message-ID: <183f68bc-8145-ef98-07ca-8d3f85d66a17@gmail.com> (raw)
In-Reply-To: <1571895161-26487-1-git-send-email-sheetal.tigadoli@broadcom.com>

On 10/23/19 10:32 PM, Sheetal Tigadoli wrote:
> 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.

Sorry for chiming on this so late, the more I look into this and the
more it seems like you have built a custom TEE firmware loading solution
rather than thinking about extending the firmware API to load a firmware
opaque handle from somewhere other than the filesystem.

The TEE integration appears okay to me in that you leverage the TEE bus
to advertise your driver. What seems to violating layers is that you
have bnxt directly tap into your TEE driver's services and that looks
not ideal to say the least. That approach does not scale well over
multiple drivers (bnxt or otherwise), but also does not really scale
over trusted components providers. TEE is one of them, but conceptually
the same thing could exist with ACPI/UEFI or any platform that has
services that offer some sort of secure/non-secured world differentiation.

The way I would imagine you to integrate this is to basically register a
TEE firmware provider through the firmware API, continue using the
firmware API from within bnxt, possibly with using a specific file
handle/flag that designates whether you want to favor loading from
disk/file system or TEE. It should not matter to bnxt how the firmware
is obtained basically.

> 
> changes from v2:
>  - address review comments from Jakub
> 
> 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           | 277 ++++++++++++++++++++++
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c         |  13 +-
>  drivers/net/ethernet/broadcom/bnxt/bnxt.h         |   6 +
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  37 ++-
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h |   2 +
>  include/linux/firmware/broadcom/tee_bnxt_fw.h     |  14 ++
>  8 files changed, 354 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/firmware/broadcom/tee_bnxt_fw.c
>  create mode 100644 include/linux/firmware/broadcom/tee_bnxt_fw.h
> 

-- 
Florian