diff mbox series

[3/4] media: mediatek: vcodec: Fix mtk_vcodec_mem_free() error log criteria

Message ID 20231113123049.4117280-4-fshao@chromium.org (mailing list archive)
State New, archived
Headers show
Series Improvement around mtk_vcodec_mem_free() logging and usage | expand

Commit Message

Fei Shao Nov. 13, 2023, 12:26 p.m. UTC
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@chromium.org>
---

 .../media/platform/mediatek/vcodec/common/mtk_vcodec_util.c  | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

AngeloGioacchino Del Regno Dec. 6, 2023, 10:19 a.m. UTC | #1
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@chromium.org>

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

Cheers,
Angelo
Fei Shao Dec. 7, 2023, 11:17 a.m. UTC | #2
Hi Angelo,

On Wed, Dec 6, 2023 at 6:19 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> 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@chromium.org>
>
> 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.

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

And thanks for the review.  :)

Regards,
Fei

> Cheers,
> Angelo
>
AngeloGioacchino Del Regno Dec. 7, 2023, 12:54 p.m. UTC | #3
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@collabora.com> 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@chromium.org>
>>
>> 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
Fei Shao Dec. 8, 2023, 4:22 a.m. UTC | #4
On Thu, Dec 7, 2023 at 8:55 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> 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@collabora.com> 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@chromium.org>
> >>
> >> 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 :-)
Yes, these are exactly what I wanted to imply in ">> Some even don't
check at all", and I should have pointed them out to avoid
ambiguity...
And I understand your concern. Presuming the NULL VA case is and will
always be safe to ignore can be too assertive, and getting explicit
error logs is always better than lurking bugs.

>
> 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...
Agree, it would be nice if Yunfei can share some thoughts here, or I
can reach out to him through other channels for alignment first, and
adjust this series based on the response.
The h264_if part was written a while ago (2016) and the av1_req_lat
part is relatively new (mid 2023), I hope it won't be too hard to
reach a conclusion for those.

Regards,
Fei

>
> Regards,
> Angelo
diff mbox series

Patch

diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
index 23bea2702c9a..5eb267decfb6 100644
--- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
+++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_util.c
@@ -96,8 +96,9 @@  void mtk_vcodec_mem_free(void *priv, struct mtk_vcodec_mem *mem)
 	}
 
 	if (!mem->va) {
-		mtk_v4l2_err(plat_dev, "%s dma_free size=0x%zx failed!",
-			     __func__, mem->size);
+		if (mem->size)
+			mtk_v4l2_err(plat_dev, "%s VA is NULL but size = 0x%zx",
+				     __func__, mem->size);
 		return;
 	}