| Message ID | 20231113123049.4117280-4-fshao@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Improvement around mtk_vcodec_mem_free() logging and usage | expand |
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
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 >
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
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 --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; }
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(-)