From: Allen Pais apais@linux.microsoft.com
The following out of memory errors are seen on kexec reboot from the optee core.
[ 0.368428] tee_bnxt_fw optee-clnt0: tee_shm_alloc failed [ 0.368461] tee_bnxt_fw: probe of optee-clnt0 failed with error -22
tee_shm_release() is not invoked on dma shm buffer.
Implement .shutdown() in optee core as well as bnxt firmware driver to handle the release of the buffers correctly.
More info: https://github.com/OP-TEE/optee_os/issues/3637
Allen Pais (2): optee: fix tee out of memory failure seen during kexec reboot firmware: tee_bnxt: implement shutdown method to handle kexec reboots
drivers/firmware/broadcom/tee_bnxt_fw.c | 9 ++++ drivers/tee/optee/core.c | 69 ++++++++++++++++++------- 2 files changed, 58 insertions(+), 20 deletions(-)
From: Allen Pais apais@linux.microsoft.com
The following out of memory errors are seen on kexec reboot from the optee core.
[ 0.368428] tee_bnxt_fw optee-clnt0: tee_shm_alloc failed [ 0.368461] tee_bnxt_fw: probe of optee-clnt0 failed with error -22
tee_shm_release() is not invoked on dma shm buffer.
Implement .shutdown() method to handle the release of the buffers correctly.
More info: https://github.com/OP-TEE/optee_os/issues/3637
Signed-off-by: Allen Pais apais@linux.microsoft.com --- drivers/tee/optee/core.c | 69 ++++++++++++++++++++++++++++------------ 1 file changed, 49 insertions(+), 20 deletions(-)
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index cf4718c6d35d..b402e5eace7b 100644 --- a/drivers/tee/optee/core.c +++ b/drivers/tee/optee/core.c @@ -582,36 +582,64 @@ static optee_invoke_fn *get_invoke_func(struct device *dev) return ERR_PTR(-EINVAL); }
-static int optee_remove(struct platform_device *pdev) +static int __optee_shutoff(struct platform_device *pdev, bool shutdown) { struct optee *optee = platform_get_drvdata(pdev);
- /* - * Ask OP-TEE to free all cached shared memory objects to decrease - * reference counters and also avoid wild pointers in secure world - * into the old shared memory range. - */ - optee_disable_shm_cache(optee); + if (shutdown) { + optee_disable_shm_cache(optee); + } else { + /* + * Ask OP-TEE to free all cached shared memory + * objects to decrease reference counters and + * also avoid wild pointers in secure world + * into the old shared memory range. + */ + optee_disable_shm_cache(optee);
- /* - * The two devices have to be unregistered before we can free the - * other resources. - */ - tee_device_unregister(optee->supp_teedev); - tee_device_unregister(optee->teedev); + /* + * The two devices have to be unregistered before + * we can free the other resources. + */ + tee_device_unregister(optee->supp_teedev); + tee_device_unregister(optee->teedev);
- tee_shm_pool_free(optee->pool); - if (optee->memremaped_shm) - memunmap(optee->memremaped_shm); - optee_wait_queue_exit(&optee->wait_queue); - optee_supp_uninit(&optee->supp); - mutex_destroy(&optee->call_queue.mutex); + tee_shm_pool_free(optee->pool); + if (optee->memremaped_shm) + memunmap(optee->memremaped_shm); + optee_wait_queue_exit(&optee->wait_queue); + optee_supp_uninit(&optee->supp); + mutex_destroy(&optee->call_queue.mutex);
- kfree(optee); + kfree(optee); + }
return 0; }
+/* optee_remove - Device Removal Routine + * @pdev: platform device information struct + * + * optee_remove is called by platform subsystem to alter the driver + * that it should release the device + */ +static int optee_remove(struct platform_device *pdev) +{ + return __optee_shutoff(pdev, false); +} + +/* optee_shutdown - Device Removal Routine + * @pdev: platform device information struct + * + * platform_shutdown is called by the platform subsystem to alter + * the driver that a shutdown/reboot(or kexec) is happening and + * device must be disabled. + */ +static void optee_shutdown(struct platform_device *pdev) +{ + __optee_shutoff(pdev, true); +} + static int optee_probe(struct platform_device *pdev) { optee_invoke_fn *invoke_fn; @@ -738,6 +766,7 @@ MODULE_DEVICE_TABLE(of, optee_dt_match); static struct platform_driver optee_driver = { .probe = optee_probe, .remove = optee_remove, + .shutdown = optee_shutdown, .driver = { .name = "optee", .of_match_table = optee_dt_match,
On Wed, 17 Feb 2021 14:57:12 +0530, Allen Pais wrote:
- /*
* Ask OP-TEE to free all cached shared memory objects to decrease* reference counters and also avoid wild pointers in secure world* into the old shared memory range.*/- optee_disable_shm_cache(optee);
- if (shutdown) {
optee_disable_shm_cache(optee);- } else {
/** Ask OP-TEE to free all cached shared memory* objects to decrease reference counters and* also avoid wild pointers in secure world* into the old shared memory range.*/optee_disable_shm_cache(optee);Calling optee_disable_shm_cache() in both if and else. It could be put in front of if().
Ideally, I could just use optee_remove for shutdown() too. But it would not look good. Hence this approach.
- Allen
On Mon, Feb 22, 2021 at 06:15:08PM +0530, Allen Pais wrote:
On Wed, 17 Feb 2021 14:57:12 +0530, Allen Pais wrote:
- /*
* Ask OP-TEE to free all cached shared memory objects to decrease* reference counters and also avoid wild pointers in secure world* into the old shared memory range.*/- optee_disable_shm_cache(optee);
- if (shutdown) {
optee_disable_shm_cache(optee);- } else {
/** Ask OP-TEE to free all cached shared memory* objects to decrease reference counters and* also avoid wild pointers in secure world* into the old shared memory range.*/optee_disable_shm_cache(optee);Calling optee_disable_shm_cache() in both if and else. It could be put in front of if().
Ideally, I could just use optee_remove for shutdown() too. But it would not look good. Hence this approach.
What is the problem with using optee_remove() for shutdown()?
Cheers, Jens
- /*
* Ask OP-TEE to free all cached shared memory objects to decrease* reference counters and also avoid wild pointers in secure world* into the old shared memory range.*/- optee_disable_shm_cache(optee);
- if (shutdown) {
optee_disable_shm_cache(optee);- } else {
/** Ask OP-TEE to free all cached shared memory* objects to decrease reference counters and* also avoid wild pointers in secure world* into the old shared memory range.*/optee_disable_shm_cache(optee);Calling optee_disable_shm_cache() in both if and else. It could be put in front of if().
Ideally, I could just use optee_remove for shutdown() too. But it would not look good. Hence this approach.
What is the problem with using optee_remove() for shutdown()?
There is no problem, I just thought it would be more cleaner/readable with this approach. If you'd like to keep it simple by just calling optee_remove() for shutdown() too, I could quickly send out V2.
Thanks for the review.
- Allen
On Tue, Feb 23, 2021 at 09:56:13PM +0530, Allen Pais wrote:
- /*
* Ask OP-TEE to free all cached shared memory objects to decrease* reference counters and also avoid wild pointers in secure world* into the old shared memory range.*/- optee_disable_shm_cache(optee);
- if (shutdown) {
optee_disable_shm_cache(optee);- } else {
/** Ask OP-TEE to free all cached shared memory* objects to decrease reference counters and* also avoid wild pointers in secure world* into the old shared memory range.*/optee_disable_shm_cache(optee);Calling optee_disable_shm_cache() in both if and else. It could be put in front of if().
Ideally, I could just use optee_remove for shutdown() too. But it would not look good. Hence this approach.
What is the problem with using optee_remove() for shutdown()?
There is no problem, I just thought it would be more cleaner/readable with this approach. If you'd like to keep it simple by just calling optee_remove() for shutdown() too, I could quickly send out V2.
In the patch you posted it looks like you'd like to call only optee_disable_shm_cache() in the case of shutdown. Like:
static void optee_shutdown(struct platform_device *pdev) { optee_disable_shm_cache(platform_get_drvdata(pdev)); }
and optee_remove() kept as it was before this patch.
Cheers, Jens
- /*
* Ask OP-TEE to free all cached shared memory objects to decrease* reference counters and also avoid wild pointers in secure world* into the old shared memory range.*/- optee_disable_shm_cache(optee);
- if (shutdown) {
optee_disable_shm_cache(optee);- } else {
/** Ask OP-TEE to free all cached shared memory* objects to decrease reference counters and* also avoid wild pointers in secure world* into the old shared memory range.*/optee_disable_shm_cache(optee);Calling optee_disable_shm_cache() in both if and else. It could be put in front of if().
Ideally, I could just use optee_remove for shutdown() too.But it would not look good. Hence this approach.
What is the problem with using optee_remove() for shutdown()?
There is no problem, I just thought it would be more cleaner/readable with this approach. If you'd like to keep it simple by just calling optee_remove() for shutdown() too, I could quickly send out V2.
In the patch you posted it looks like you'd like to call only optee_disable_shm_cache() in the case of shutdown. Like:
static void optee_shutdown(struct platform_device *pdev) { optee_disable_shm_cache(platform_get_drvdata(pdev)); }
and optee_remove() kept as it was before this patch.
Sure, Will have it fixed and send out V2.
Thanks.
From: Allen Pais apais@linux.microsoft.com
On kexec reboot the firmware driver fails to deallocate shm memory leading to a memory leak. Implement .shutdown() method to handle kexec reboots and to release shm buffers correctly.
Signed-off-by: Allen Pais apais@linux.microsoft.com --- drivers/firmware/broadcom/tee_bnxt_fw.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/drivers/firmware/broadcom/tee_bnxt_fw.c b/drivers/firmware/broadcom/tee_bnxt_fw.c index ed10da5313e8..4c62e044a99f 100644 --- a/drivers/firmware/broadcom/tee_bnxt_fw.c +++ b/drivers/firmware/broadcom/tee_bnxt_fw.c @@ -242,6 +242,14 @@ static int tee_bnxt_fw_remove(struct device *dev) return 0; }
+static void tee_bnxt_fw_shutdown(struct device *dev) +{ + tee_shm_free(pvt_data.fw_shm_pool); + tee_client_close_session(pvt_data.ctx, pvt_data.session_id); + tee_client_close_context(pvt_data.ctx); + pvt_data.ctx = NULL; +} + 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)}, @@ -257,6 +265,7 @@ static struct tee_client_driver tee_bnxt_fw_driver = { .bus = &tee_bus_type, .probe = tee_bnxt_fw_probe, .remove = tee_bnxt_fw_remove, + .shutdown = tee_bnxt_fw_shutdown, }, };
From: Allen Pais apais@linux.microsoft.com
On Wed, 17 Feb 2021 14:57:12 +0530, Allen Pais wrote:
The following out of memory errors are seen on kexec reboot from the optee core. [ 0.368428] tee_bnxt_fw optee-clnt0: tee_shm_alloc failed [ 0.368461] tee_bnxt_fw: probe of optee-clnt0 failed with error -22 tee_shm_release() is not invoked on dma shm buffer. Implement .shutdown() in optee core as well as bnxt firmware driver to handle the release of the buffers correctly. More info: https://github.com/OP-TEE/optee_os/issues/3637
CC: linux-kernel@vger.kernel.org instead of linux-mips? TEE / TrustZone is ARM.
Also, for Broadcom specific - CC: bcm-kernel-feedback-list@broadcom.com
The following out of memory errors are seen on kexec reboot from the optee core. [ 0.368428] tee_bnxt_fw optee-clnt0: tee_shm_alloc failed [ 0.368461] tee_bnxt_fw: probe of optee-clnt0 failed with error -22 tee_shm_release() is not invoked on dma shm buffer. Implement .shutdown() in optee core as well as bnxt firmware driver to handle the release of the buffers correctly. More info: https://github.com/OP-TEE/optee_os/issues/3637
Jens,
Could you please take sometime out and review the series.
Thanks.
Allen Pais (2): optee: fix tee out of memory failure seen during kexec reboot firmware: tee_bnxt: implement shutdown method to handle kexec reboots
drivers/firmware/broadcom/tee_bnxt_fw.c | 9 ++++ drivers/tee/optee/core.c | 69 ++++++++++++++++++------- 2 files changed, 58 insertions(+), 20 deletions(-)
op-tee@lists.trustedfirmware.org
-
Allen Pais -
Allen Pais -
Dhananjay Phadke -
Jens Wiklander