Date:   Fri, 8 Dec 2023 12:22:24 +0800
From:   Fei Shao <fshao@...omium.org>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
Cc:     Yunfei Dong <yunfei.dong@...iatek.com>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Andrew-CT Chen <andrew-ct.chen@...iatek.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Nicolas Dufresne <nicolas.dufresne@...labora.com>,
        Nícolas F. R. A. Prado 
        <nfraprado@...labora.com>, Tiffany Lin <tiffany.lin@...iatek.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-media@...r.kernel.org, linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH 3/4] media: mediatek: vcodec: Fix mtk_vcodec_mem_free()
 error log criteria

On Thu, Dec 7, 2023 at 8:55 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.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@...labora.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@...omium.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

Powered by blists - more mailing lists