Secure dmabuf support

[omasse-nxp]

No description provided.

Tested on Hikey960

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

Please find the description here
d04ae89

sorry for the flood of minor comments.

move before line 13

static attribute for the 2 above?

indeed

move after line 13

why this empty line?

removed

PTR_ERR(freelist_task)?

kthread_run returns the kthread or ERR_PTR(-ENOMEM).

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

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

remove 0x prefix

fix indentation (up to 100char/line allowed)

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

s/dev/teedev/

s/file/File/

yes. yet, not a strong opinion.

fit in a single line

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.

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));

already tested/handled from secure_heap_add().

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.

indeed.

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

done

indentation

not needed IMHO, list_nr_pages is already zero initialized.

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

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

can remove this line

extra space char to remove

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

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

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

Can remove this declaration, function is not implemented.

done here d15d2b5

s/TEE_IOC_SHM_ALLOC/TEE_IOC_SHM_REGISTER_FD/

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

s/IS_ERR_OR_NULL/IS_ERR/

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

s/IS_ERR_OR_NULL/IS_ERR/
Ditto at line 196.

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.

agree

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))
		return ERR_PTR(-EINVAL);

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

copyright notice missing

done

remove this useless line?

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.

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).

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 ?

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

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);

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