Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Secure dmabuf support #101

Open
wants to merge 18 commits into
base: optee
Choose a base branch
from

Conversation

omasse-linaro
Copy link

No description provided.

jbech-linaro and others added 12 commits April 4, 2022 09:34
From the commit below, the mt8173-evb failed to boot to console due to
changes in the mt8173 device tree files.

  commit c0d6fe2
  Merge: b44a3d2 3e4dda7
  Author: Linus Torvalds <[email protected]>
  Date:   Tue Nov 10 15:06:26 2015 -0800

      Merge tag 'armsoc-dt' of
      git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc

Until properly solved, let's just remove the section in the device tree
blob that causes this.

Signed-off-by: Joakim Bech <[email protected]>
Reviewed-by: Pascal Brand <[email protected]>
Configures foundation-v8 with OP-TEE.

Signed-off-by: Jens Wiklander <[email protected]>
[jf: rebase onto v5.9-rc7]
Signed-off-by: Jerome Forissier <[email protected]>
Configures Juno with OP-TEE.

Reviewed-by: Pascal Brand <[email protected]>
Signed-off-by: Jens Wiklander <[email protected]>
[jf: rebase onto v5.9-rc7]
Signed-off-by: Jerome Forissier <[email protected]>
…dation-v8 **not for mainline**

All the platforms that reserve memory for OP-TEE statically via the
DT (i.e., not those that reserve it via UEFI or that patch the DT
dynamically thanks to OP-TEE's CFG_DT option) have to mark it 'no-map'
so that only the TEE driver may map it.

Signed-off-by: Jens Wiklander <[email protected]>
… **not for mainline**

All the platforms that reserve memory for OP-TEE statically via the
DT (i.e., not those that reserve it via UEFI or that patch the DT
dynamically thanks to OP-TEE's CFG_DT option) have to mark it 'no-map'
so that only the TEE driver may map it.

Signed-off-by: Jens Wiklander <[email protected]>
Signed-off-by: Joakim Bech <[email protected]>
Reviewed-by: Pascal Brand <[email protected]>
Reviewed-by: Jerome Forissier <[email protected]>
Reserve memory for bootloader purposes.

Acked-by: Jerome Forissier <[email protected]>
Signed-off-by: Igor Opaniuk <[email protected]>
Add optee node, so OP-TEE driver is probed properly.

Acked-by: Jerome Forissier <[email protected]>
Signed-off-by: Igor Opaniuk <[email protected]>
Define OP-TEE firmware node for stm32mp15 based platforms. The node
if disable by default.

Enable the OP-TEE node and define OP-TEE reserved memory for
stm32mp157c-dk2.

Signed-off-by: Etienne Carriere <[email protected]>
[jf: rebase onto v5.9]
Signed-off-by: Jerome Forissier <[email protected]>
Signed-off-by: Javier Almansa Sobrino <[email protected]>
Acked-by: Joakim Bech <[email protected]>
Link: linaro-swg#85
[jf: not currently intended for upstream; add link to PR]
Signed-off-by: Jerome Forissier <[email protected]>
The optee device status is disabled by default, change its status to 'okay'
in the dts of the EV1 board

Signed-off-by: Timothée Cercueil <[email protected]>
[jf: rebase onto v5.17]
Signed-off-by: Jerome Forissier <[email protected]>
This change fixes EV1 configuration which lacked OP-TEE
reserved memory. This change also makes ED1 board ready
the host OP-TEE by enabling OP-TEE node and defining the
OP-TEE reserved memory for that board. This change defines
these resources in ED1 DTS file which is included in EV1 DTS
file.

Signed-off-by: Etienne Carriere <[email protected]>
Acked-by: Jerome Forissier <[email protected]>
[jf: rebase onto v5.17]
Signed-off-by: Jerome Forissier <[email protected]>
@omasse-linaro
Copy link
Author

Tested on Hikey960

@omasse-linaro omasse-linaro force-pushed the secure_dmabuf_support branch 3 times, most recently from fb42f9e to b68bcbc Compare June 1, 2022 09:13

description:
Linaro OP-TEE firmware need a reserved memory for the
Secure Data Path feature.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to explain a bit more about SDP here (its purpose and how it achieves it in a few words).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please find the description here
d04ae89

Copy link

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the flood of minor comments.

#include <linux/list.h>
#include <linux/slab.h>
#include <linux/swap.h>
#include <linux/sched/signal.h>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move before line 13

static LIST_HEAD(free_list);
static size_t list_nr_pages;
wait_queue_head_t freelist_waitqueue;
struct task_struct *freelist_task;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static attribute for the 2 above?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed

*
*/

#include <linux/genalloc.h>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move after line 13

struct list_head list;
bool no_map;
bool mapped;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this empty line?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

"%s", "dmabuf-deferred-free-worker");
if (IS_ERR(freelist_task)) {
pr_err("Creating thread for deferred free failed\n");
return -1;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTR_ERR(freelist_task)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kthread_run returns the kthread or ERR_PTR(-ENOMEM).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then return -ENOMEM? (I still think return PTR_ERR(freelist_task); is more appropriate).
-1 stands for -EPERM which is not relevant here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I misunderstood the comment.
you mean return PTR_ERR(freelist_task) when I understood to modify the if (IS_ERR(freelist_task))

#address-cells = <2>;
#size-cells = <2>;

sdp@0x3e800000 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove 0x prefix

@@ -356,6 +356,42 @@ tee_ioctl_shm_register(struct tee_context *ctx,
return ret;
}

static int tee_ioctl_shm_register_fd(struct tee_context *ctx,
struct tee_ioctl_shm_register_fd_data __user *udata)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix indentation (up to 100char/line allowed)

* tee_shm_register_fd() - Register shared memory from file descriptor
*
* @ctx: Context that allocates the shared memory
* @fd: shared memory file descriptor reference.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpciking (for consistency): s/shared/Shared/ and remove trailing period.


/**
* tee_shm_priv_alloc() - Allocate shared memory privately
* @dev: Device that allocates the shared memory

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/dev/teedev/

@@ -121,6 +121,35 @@ struct tee_ioctl_shm_alloc_data {
#define TEE_IOC_SHM_ALLOC _IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + 1, \
struct tee_ioctl_shm_alloc_data)

/**
* struct tee_ioctl_shm_register_fd_data - Shared memory registering argument
* @fd: [in] file descriptor identifying the shared memory

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/file/File/

* is under memory pressure. Usually
* from a shrinker. Avoid allocating
* memory in the free call, as it may
* fail.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. yet, not a strong opinion.

static DEFINE_MUTEX(pool_list_lock);

static
struct page *dmabuf_page_pool_alloc_pages(struct dmabuf_page_pool *pool)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fit in a single line

drivers/dma-buf/heaps/page_pool.c Show resolved Hide resolved
depends on DMABUF_HEAPS && DMABUF_HEAPS_DEFERRED_FREE
help
Choose this option to enable the secure dmabuf heap. The secure heap
is allocated by gen allocater. it's allocated according the dts.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The secure heap is allocated by gen allocater. it's allocated according the dts.

If i understood is correctly, it should rather be phrased as:

The secure heap pools are defined according to the DT. Heaps are allocated
in the pools using gen allocater.

drivers/dma-buf/heaps/secure_heap.c Show resolved Hide resolved
gen_pool_free(info->pool,
sg_dma_address(sg),
sg_dma_len(sg));
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coding style (move braces and line breaks):

	for_each_sg(table->sgl, sg, table->nents, i)
		gen_pool_free(info->pool, sg_dma_address(sg), sg_dma_len(sg));


for (i = 0; i < secure_data_count; i++) {
if (secure_data[i].base == 0 || secure_data[i].size == 0)
return -EINVAL;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

already tested/handled from secure_heap_add().

s++;
}

strncpy(secure_data[secure_data_count].name, rmem->name, name_len);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if name_len == MAX_HEAP_NAME_LEN, then secure_data[secure_data_count].name will no be null terminated, which is likely an issue, at least at line 584.

Copy link
Author

@omasse-linaro omasse-linaro Jun 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed.

secure_data_count++;
return 0;
} else {
return -EINVAL;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion:
WARN_ONCE("Cannot handle more than %u secure heaps\n", MAX_SECURE_HEAP);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

rmem->ops = &rmem_dma_ops;
pr_info("Reserved memory: DMA buf secure pool %s at %pa, size %ld MiB\n",
secure_data[secure_data_count].name,
&rmem->base, (unsigned long)rmem->size / SZ_1M);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation

@omasse-linaro omasse-linaro force-pushed the secure_dmabuf_support branch 2 times, most recently from 6768628 to f9e9b79 Compare June 20, 2022 14:43

static int deferred_freelist_init(void)
{
list_nr_pages = 0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed IMHO, list_nr_pages is already zero initialized.

"%s", "dmabuf-deferred-free-worker");
if (IS_ERR(freelist_task)) {
pr_err("Creating thread for deferred free failed\n");
return -1;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then return -ENOMEM? (I still think return PTR_ERR(freelist_task); is more appropriate).
-1 stands for -EPERM which is not relevant here.

};

/**
* deferred_free - call to add item to the deferred free list

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove call to, simply * deferred_free() - Add item to the deferred free list

mutex_lock(&buffer->lock);
if (buffer->vmap_cnt) {
buffer->vmap_cnt++;
vaddr = buffer->vaddr;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can remove this line

struct secure_heap_buffer *buffer;
struct secure_heap_info *info = dma_heap_get_drvdata(heap);
DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
unsigned long size = roundup(len, PAGE_SIZE);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space char to remove

goto free_pool;

sg_set_page(table->sgl,
phys_to_page(phy_addr),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

	sg_set_page(table->sgl, phys_to_page(phy_addr), size, 0);

* @ctx: Context that allocates the shared memory
* @fd: Shared memory file descriptor reference
*
* @returns a pointer to 'struct tee_shm'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* @returns a pointer to 'struct tee_shm' on success, or a errno on failure

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From other inline description, could be
* @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure

*
* @returns a pointer to 'struct tee_shm'
*/
struct tee_shm *tee_shm_priv_alloc(struct tee_device *teedev, size_t size);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove this declaration, function is not implemented.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done here d15d2b5

*
* The flags field should currently be zero as input. Updated by the call
* with actual flags as defined by TEE_IOCTL_SHM_* above.
* This structure is used as argument for TEE_IOC_SHM_ALLOC below.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/TEE_IOC_SHM_ALLOC/TEE_IOC_SHM_REGISTER_FD/

Copy link

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed to post few more comments on the last commit.

return -EINVAL;

shm = tee_shm_register_fd(ctx, data.fd);
if (IS_ERR_OR_NULL(shm))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/IS_ERR_OR_NULL/IS_ERR/

* @ctx: Context that allocates the shared memory
* @fd: Shared memory file descriptor reference
*
* @returns a pointer to 'struct tee_shm'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From other inline description, could be
* @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure

}

ref->attach = dma_buf_attach(ref->dmabuf, &ref->shm.ctx->teedev->dev);
if (IS_ERR_OR_NULL(ref->attach)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/IS_ERR_OR_NULL/IS_ERR/
Ditto at line 196.


ref->attach = dma_buf_attach(ref->dmabuf, &ref->shm.ctx->teedev->dev);
if (IS_ERR_OR_NULL(ref->attach)) {
rc = ERR_PTR(-EINVAL);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of return -EINVAL, i think this should better return the error reported by dma_buf_attach().

Actually I think it would be better for maintenance to change rc from void * type int and use return ERR_PTR(rc); in the error path, at line 244.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree

Copy link

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you squash commit "tee: remove unused tee_shm_priv_alloc function definition" into commit "tee: new ioctl to a register tee_shm from a dmabuf file descriptor" into commit". Feel free to use Co-developed-by: Etienne ... tag and add your S-o-b tag on the resulting commit.

Same for the 1st commits: prefer squash commits "ANDROID: dma-buf: heaps: fix dma-buf heap pool pages stat" and "ANDROID: dma-buf: heaps: fix a warning in dmabuf page pools" into commit "ANDROID: dma-buf: heaps: Add a shrinker controlled page pool" and add "Co-dev" + your S-o-b tag on the resulting commit. This is how, I think, these patch should be eventually posted to the LKML, to prevent introducing bugged commits in the kernel commit history.

if (!tee_device_get(ctx->teedev))
{
rc = -EINVAL;
goto out;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

	if (!tee_device_get(ctx->teedev))
		return ERR_PTR(-EINVAL);

goto error;
}

pool = gen_pool_create(PAGE_SHIFT+4, -1);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think granule should be 4kByte, the MMU mapping granule. It is the client responsibility to allocate large enough buffers for its needs.

@@ -0,0 +1,55 @@
/* SPDX-License-Identifier: GPL-2.0 */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copyright notice missing

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

* DMABUF secure heap exporter
*
* Copyright 2021 NXP.
*

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this useless line?

ref->shm.id = -1;

ref->dmabuf = dma_buf_get(fd);
if (!ref->dmabuf) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked dma_buf_get(). It returns a ERR_PTR value hence must use IS_ERR() here.

Line below, should be rc = PTR_ERR(ref->dmabuf);
Ditto for the other below error cases.

teedev_ctx_get(ctx);
mutex_lock(&ref->shm.ctx->teedev->mutex);
mutex_unlock(&ref->shm.ctx->teedev->mutex);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this 5 lines. No need to get tee context, already done at function entry and balanced in fucntion error path and in tee_shm_release() (line 70 added by this P-R).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to replace this block by teedev_ctx_get(ctx); instead ?
I guess we still need to increment tee ctx refcount , isn't it ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, indeed you're right. Sorry, i mess-up with tee_device_get/put().

DMA_BIDIRECTIONAL);
if (ref->attach)
dma_buf_detach(ref->dmabuf, ref->attach);
if (ref->dmabuf)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these will not work since those references are not NULL if the related resource failed to be retrieved: they all get a non-0 ERR_PTR value. Here you must use the standard Linux kernel error sequence scheme:

err_idr_remove:
	mutex_lock(&ctx->teedev->mutex);
	idr_remove(&ctx->teedev->idr, ref->shm.id);
	mutex_unlock(&ctx->teedev->mutex);
err_unmap_attachement:
	dma_buf_unmap_attachment(ref->attach, ref->sgt, DMA_BIDIRECTIONAL);
err_detach:
	dma_buf_detach(ref->dmabuf, ref->attach);
err_put_dmabuf:
	dma_buf_put(ref->dmabuf);
err_free_ref:
	kfree(ref);
err_put_tee:
	tee_device_put(ctx->teedev);

	return ERR_PTR(rc);

This patch provides infrastructure for deferring buffer frees.

This is a feature ION provided which when used with some form
of a page pool, provides a nice performance boost in an
allocation microbenchmark. The reason it helps is it allows the
page-zeroing to be done out of the normal allocation/free path,
and pushed off to a kthread.

As not all heaps will find this useful, its implemented as
a optional helper library that heaps can utilize.

Cc: Daniel Vetter <[email protected]>
Cc: Sumit Semwal <[email protected]>
Cc: Liam Mark <[email protected]>
Cc: Chris Goldsworthy <[email protected]>
Cc: Laura Abbott <[email protected]>
Cc: Brian Starkey <[email protected]>
Cc: Hridya Valsaraju <[email protected]>
Cc: Suren Baghdasaryan <[email protected]>
Cc: Sandeep Patil <[email protected]>
Cc: Daniel Mentz <[email protected]>
Cc: Ørjan Eide <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Ezequiel Garcia <[email protected]>
Cc: Simon Ser <[email protected]>
Cc: James Jones <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: John Stultz <[email protected]>
Bug: 168742043
@omasse-linaro omasse-linaro force-pushed the secure_dmabuf_support branch 2 times, most recently from 2da9c70 to 74420c1 Compare June 22, 2022 13:40
johnstultz-work and others added 5 commits June 22, 2022 15:44
This patch adds a simple shrinker controlled page pool to the
dmabuf heaps subsystem.

This replaces the use of the networking page_pool, over concerns
that the lack of a shrinker for that implementation may cause
additional low-memory kills

TODO: Take another pass at trying to unify this w/ the ttm pool

Thoughts and feedback would be greatly appreciated!

Cc: Sumit Semwal <[email protected]>
Cc: Liam Mark <[email protected]>
Cc: Laura Abbott <[email protected]>
Cc: Brian Starkey <[email protected]>
Cc: Hridya Valsaraju <[email protected]>
Cc: Suren Baghdasaryan <[email protected]>
Cc: Sandeep Patil <[email protected]>
Cc: Daniel Mentz <[email protected]>
Cc: Chris Goldsworthy <[email protected]>
Cc: Ørjan Eide <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Ezequiel Garcia <[email protected]>
Cc: Simon Ser <[email protected]>
Cc: James Jones <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Olivier Masse <[email protected]>
Bug: 168742043
add Linaro secure heap bindings: linaro,secure-heap
use genalloc to allocate/free buffer from buffer pool.
buffer pool info is from dts.
use sg_table instore the allocated memory info, the length of sg_table is 1.
implement secure_heap_buf_ops to implement buffer share in difference device:
1. Userspace passes this fd to all drivers it wants this buffer
to share with: First the filedescriptor is converted to a &dma_buf using
dma_buf_get(). Then the buffer is attached to the device using dma_buf_attach().
2. Once the buffer is attached to all devices userspace can initiate DMA
access to the shared buffer. In the kernel this is done by calling dma_buf_map_attachment()
3. get sg_table with dma_buf_map_attachment in difference device.

Signed-off-by: Olivier Masse <[email protected]>
DMABUF Reserved memory definition for OP-TEE SDP feaure.

Signed-off-by: Olivier Masse <[email protected]>
Add DMABUF_HEAPS_SECURE in defconfig

Signed-off-by: Olivier Masse <[email protected]>
This change allows userland to create a tee_shm object that refers
to a dmabuf reference.

Userland provides a dmabuf file descriptor as buffer reference.
The created tee_shm object exported as a brand new dmabuf reference
used to provide a clean fd to userland. Userland shall closed this new
fd to release the tee_shm object resources. The initial dmabuf resources
are tracked independently through original dmabuf file descriptor.

Once the buffer is registered and until it is released, TEE driver
keeps a refcount on the registered dmabuf structure.

This change only support dmabuf references that relates to physically
contiguous memory buffers.

New tee_shm flag to identify tee_shm objects built from a registered
dmabuf: TEE_SHM_EXT_DMA_BUF. Such tee_shm structures are flagged both
TEE_SHM_DMA_BUF and TEE_SHM_EXT_DMA_BUF.

Co-Developed-by: Etienne Carriere <[email protected]>
Signed-off-by: Olivier Masse <[email protected]>
From: https://github.com/linaro-swg/linux.git
(cherry picked from commit 41e21e5)
@omasse-linaro
Copy link
Author

omasse-linaro commented Jun 22, 2022

Could you squash commit "tee: remove unused tee_shm_priv_alloc function definition" into commit "tee: new ioctl to a register tee_shm from a dmabuf file descriptor" into commit". Feel free to use Co-developed-by: Etienne ... tag and add your S-o-b tag on the resulting commit.

Same for the 1st commits: prefer squash commits "ANDROID: dma-buf: heaps: fix dma-buf heap pool pages stat" and "ANDROID: dma-buf: heaps: fix a warning in dmabuf page pools" into commit "ANDROID: dma-buf: heaps: Add a shrinker controlled page pool" and add "Co-dev" + your S-o-b tag on the resulting commit. This is how, I think, these patch should be eventually posted to the LKML, to prevent introducing bugged commits in the kernel commit history.

ok, squash done and commit message updated.
but not sure I can add "Co-dev" to the dma-buf heap pool commit as I did not create it but only involved in patch delivery.
Anyway tell me if I do not follow correctly the submit rule, cause I tried to follow :
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

jbech-linaro added a commit to jbech-linaro/manifest that referenced this pull request Nov 15, 2023
This manifest will create a QEMU v8 based setup just as the other OP-TEE
QEMU v8 setup, but this also pulls the SDP Linux kernel patches from NXP
that can be found in GitHub PR 101:
  linaro-swg/linux#101

For this we also had to use a more up-to-date version of the toolchain,
hence we download a 12.3 gcc from Linaro.

The recommended commandline when building is:
  make -j12 CFG_SECURE_DATA_PATH=y CFG_CORE_ASLR=n CFG_TA_ASLR=n run

Signed-off-by: Joakim Bech <[email protected]>
jbech-linaro added a commit to jbech-linaro/manifest that referenced this pull request Nov 15, 2023
This manifest will create a QEMU v8 based setup just as the other OP-TEE
QEMU v8 setup, but this also pulls the SDP Linux kernel patches from NXP
that can be found in GitHub PR 101:
  linaro-swg/linux#101

For this we also had to use a more up-to-date version of the toolchain,
hence we download a 12.3 gcc from Linaro.

The recommended commandline when building is:
  make -j12 CFG_SECURE_DATA_PATH=y CFG_CORE_ASLR=n CFG_TA_ASLR=n run

Signed-off-by: Joakim Bech <[email protected]>
jbech-linaro added a commit to jbech-linaro/manifest that referenced this pull request Nov 15, 2023
This manifest will create a QEMU v8 based setup just as the other OP-TEE
QEMU v8 setup, but this also pulls the SDP Linux kernel patches from NXP
that can be found in GitHub PR 101:
  linaro-swg/linux#101

For this we also had to use a more up-to-date version of the toolchain,
hence we download a 12.3 gcc from Linaro.

The recommended commandline when building is:
  make -j12 CFG_SECURE_DATA_PATH=y CFG_CORE_ASLR=n CFG_TA_ASLR=n run

Signed-off-by: Joakim Bech <[email protected]>
jbech-linaro added a commit to jbech-linaro/manifest that referenced this pull request Nov 15, 2023
This manifest will create a QEMU v8 based setup just as the other OP-TEE
QEMU v8 setup, but this also pulls the SDP Linux kernel patches from NXP
that can be found in GitHub PR 101:
  linaro-swg/linux#101

For this we also had to use a more up-to-date version of the toolchain,
hence we download a 12.3 gcc from Linaro.

The recommended commandline when building is:
  make -j12 CFG_SECURE_DATA_PATH=y CFG_CORE_ASLR=n CFG_TA_ASLR=n run

Signed-off-by: Joakim Bech <[email protected]>
jbech-linaro added a commit to jbech-linaro/manifest that referenced this pull request Nov 15, 2023
This manifest will create a QEMU v8 based setup just as the other OP-TEE
QEMU v8 setup, but this also pulls the SDP Linux kernel patches from NXP
that can be found in GitHub PR 101:
  linaro-swg/linux#101

For this we also had to use a more up-to-date version of the toolchain,
hence we download a 12.3 gcc from Linaro.

The recommended commandline when building is:
  make -j12 CFG_SECURE_DATA_PATH=y CFG_CORE_ASLR=n CFG_TA_ASLR=n run

Signed-off-by: Joakim Bech <[email protected]>
jbech-linaro added a commit to jbech-linaro/manifest that referenced this pull request Nov 15, 2023
This manifest will create a QEMU v8 based setup just as the other OP-TEE
QEMU v8 setup, but this also pulls the SDP Linux kernel patches from NXP
that can be found in GitHub PR 101:
  linaro-swg/linux#101

For this we also had to use a more up-to-date version of the toolchain,
hence we download a 12.3 gcc from Linaro.

The recommended commandline when building is:
  make -j12 CFG_SECURE_DATA_PATH=y CFG_CORE_ASLR=n CFG_TA_ASLR=n run

Signed-off-by: Joakim Bech <[email protected]>
jbech-linaro added a commit to jbech-linaro/manifest that referenced this pull request Nov 15, 2023
This manifest will create a QEMU v8 based setup just as the other OP-TEE
QEMU v8 setup, but this also pulls the SDP Linux kernel patches from NXP
that can be found in GitHub PR 101:
  linaro-swg/linux#101

For this we also had to use a more up-to-date version of the toolchain,
hence we download a 12.3 gcc from Linaro.

The recommended commandline when building is:
  make -j12 CFG_SECURE_DATA_PATH=y CFG_CORE_ASLR=n CFG_TA_ASLR=n run

Signed-off-by: Joakim Bech <[email protected]>
jenswi-linaro pushed a commit that referenced this pull request Jan 30, 2024
On RZ/Five SMARC EVK, where probing of SDHI is deferred due to probe
deferral of the vqmmc-supply regulator:

    ------------[ cut here ]------------
    WARNING: CPU: 0 PID: 0 at kernel/time/timer.c:1738 __run_timers.part.0+0x1d0/0x1e8
    Modules linked in:
    CPU: 0 PID: 0 Comm: swapper Not tainted 6.7.0-rc4 #101
    Hardware name: Renesas SMARC EVK based on r9a07g043f01 (DT)
    epc : __run_timers.part.0+0x1d0/0x1e8
     ra : __run_timers.part.0+0x134/0x1e8
    epc : ffffffff800771a4 ra : ffffffff80077108 sp : ffffffc800003e60
     gp : ffffffff814f5028 tp : ffffffff8140c5c0 t0 : ffffffc800000000
     t1 : 0000000000000001 t2 : ffffffff81201300 s0 : ffffffc800003f20
     s1 : ffffffd8023bc4a0 a0 : 00000000fffee6b0 a1 : 0004010000400000
     a2 : ffffffffc0000016 a3 : ffffffff81488640 a4 : ffffffc800003e60
     a5 : 0000000000000000 a6 : 0000000004000000 a7 : ffffffc800003e68
     s2 : 0000000000000122 s3 : 0000000000200000 s4 : 0000000000000000
     s5 : ffffffffffffffff s6 : ffffffff81488678 s7 : ffffffff814886c0
     s8 : ffffffff814f49c0 s9 : ffffffff81488640 s10: 0000000000000000
     s11: ffffffc800003e60 t3 : 0000000000000240 t4 : 0000000000000a52
     t5 : ffffffd8024ae018 t6 : ffffffd8024ae038
    status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003
    [<ffffffff800771a4>] __run_timers.part.0+0x1d0/0x1e8
    [<ffffffff800771e0>] run_timer_softirq+0x24/0x4a
    [<ffffffff80809092>] __do_softirq+0xc6/0x1fa
    [<ffffffff80028e4c>] irq_exit_rcu+0x66/0x84
    [<ffffffff80800f7a>] handle_riscv_irq+0x40/0x4e
    [<ffffffff80808f48>] call_on_irq_stack+0x1c/0x28
    ---[ end trace 0000000000000000 ]---

What happens?

    renesas_sdhi_probe()
    {
    	tmio_mmc_host_alloc()
	    mmc_alloc_host()
		INIT_DELAYED_WORK(&host->detect, mmc_rescan);

	devm_request_irq(tmio_mmc_irq);

	/*
	 * After this, the interrupt handler may be invoked at any time
	 *
	 *  tmio_mmc_irq()
	 *  {
	 *	__tmio_mmc_card_detect_irq()
	 *	    mmc_detect_change()
	 *		_mmc_detect_change()
	 *		    mmc_schedule_delayed_work(&host->detect, delay);
	 *  }
	 */

	tmio_mmc_host_probe()
	    tmio_mmc_init_ocr()
		-EPROBE_DEFER

	tmio_mmc_host_free()
	    mmc_free_host()
    }

When expire_timers() runs later, it warns because the MMC host structure
containing the delayed work was freed, and now contains an invalid work
function pointer.

Fix this by cancelling any pending delayed work before releasing the
MMC host structure.

Signed-off-by: Geert Uytterhoeven <[email protected]>
Tested-by: Lad Prabhakar <[email protected]>
Cc: [email protected]
Link: https://lore.kernel.org/r/205dc4c91b47e31b64392fe2498c7a449e717b4b.1701689330.git.geert+renesas@glider.be
Signed-off-by: Ulf Hansson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants