Il 07/12/23 12:17, Fei Shao ha scritto:
Hi Angelo,
On Wed, Dec 6, 2023 at 6:19 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
Il 13/11/23 13:26, Fei Shao ha scritto:
mtk_vcodec_mem_free() shouldn't print error if the target DMA buffer has
never been allocated or was freed properly in the previous call. That
makes log confusing.
Update the error path to print log only when the caller attempts to free
nonzero-size buffer with VA being NULL, which indicates something indeed
went wrong.
This brings another benefit that the callers no more need to check
mem->va explicitly to avoid the error, which can make the code more
compact and neat.
Signed-off-by: Fei Shao <fshao@xxxxxxxxxxxx>
I think that this error is supposed to catch two issues in one:
- We're called to free no memory (something that does make no sense),
this may happen for example when calling xxx_free() twice, and it
is a mistake that *must* be fixed;
When I made the change, I was thinking of kfree() that doesn't warn
against a NULL pointer.
I imagine mtk_vcodec_mem_free() calls with NULL VA and mem size 0
probably have the similar nuance (if the buffer exists, free it; never
mind otherwise), but I could have missed some important differences
specific to the MTK vcodec driver.
Looking at the mtk_vcodec_mem_free() usages, almost every one of those
checks VA beforehand, but nothing else - they don't warn or do
anything special when they encounter a NULL VA, and they should if
that's a concern.
Some even don't check at all (and I think that's why I ended up seeing
the errors mentioned in the cover letter). As for that, I think
there's nothing else we can fix except prepending "if (mem->va)".
So from all this, I feel perhaps we don't need to worry much about
those NULL VA, and we can further remove the checks (or at least move
it into mtk_vcodec_mem_free()) to trim the lines in the driver. That's
the reason for patch [4/4].
Not sure if that makes sense to you.
What you say does make sense - and a lot - but still, I think that freeing
a NULL VA (= freeing nothing) is something that shouldn't happen...
- We're failing to free memory for real (which you covered)
....that said, I think that if you want to clarify the error messages
in this function, it should look something like this:
if (!mem->va) {
mtk_v4l2_err(plat_dev, "%s: Tried to free a NULL VA", __func__);
if (mem->size)
mtk_v4l2_err(plat_dev, "Failed to free %lu bytes", mem->size);
return;
}
Sure, I can revise the patch with this, but I also want to make sure
if the NULL VA print needs to be an error.
If you still think it should, I guess I'll drop the current patch
[4/4] and instead add the check before every mtk_vcodec_mem_free()
calls. This should also work for the issue I want to address in the
first place.
... because if you notice, some of the calls to mtk_vcodec_mem_free() are not
checked with `if (something->va)` beforehand, so I think that those are cases
in which freeing with a NULL VA would actually be an indication of something
going wrong and/or not as expected anyway (checking beforehand = error won't
get printed from mtk_vcodec_mem_free(), not checking = print error if va==NULL)
It's an easy check:
cd drivers/media/platform/mediatek/vcodec
grep -rb1 mtk_vcodec_mem_free
P.S.: h264_if, av1_req_lat :-)
That's why I think that you should drop your [4/4] - unless MediaTek comes in
stating that the missed checks are something unintended, and that every instance
of VA==NULL should print an error.
I honestly wouldn't be surprised if they did so, because anyway this occurs only
in two decoders...
Regards,
Angelo
