Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | From | CK Hu (胡俊光) <> | Subject | Re: [PATCH] drm/mediatek: Init `ddp_comp` with devm_kcalloc() | Date | Mon, 1 Apr 2024 03:33:57 +0000 |
| |
Hi, Douglas:
On Thu, 2024-03-28 at 09:22 -0700, Douglas Anderson wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > In the case where `conn_routes` is true we allocate an extra slot in > the `ddp_comp` array but mtk_drm_crtc_create() never seemed to > initialize it in the test case I ran. For me, this caused a later > crash when we looped through the array in mtk_drm_crtc_mode_valid(). > This showed up for me when I booted with `slub_debug=FZPUA` which > poisons the memory initially. Without `slub_debug` I couldn't > reproduce, presumably because the later code handles the value being > NULL and in most cases (not guaranteed in all cases) the memory the > allocator returned started out as 0. > > It really doesn't hurt to initialize the array with devm_kcalloc() > since the array is small and the overhead of initting a handful of > elements to 0 is small. In general initting memory to zero is a safer > practice and usually it's suggested to only use the non-initting > alloc > functions if you really need to. > > Let's switch the function to use an allocation function that zeros > the > memory. For me, this avoids the crash.
Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > Fixes: 01389b324c97 ("drm/mediatek: Add connector dynamic selection > capability") > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > I don't have a ton of experience with this driver to know if the fact > that the array item was still uninitialized when > mtk_drm_crtc_mode_valid() ran is the sign of a bug that should be > fixed. However, even if it is a bug and that bug is fixed then > zeroing > memory when we allocate is still safer. If it's a bug that this > memory > wasn't initialized then please consider this patch a bug report. ;-) > > I'll also note that I reproduced this on a downstream 6.1-based > kernel. It appears that only mt8188 uses `conn_routes` and, as far as > I can tell, mt8188 isn't supported upstream yet. > > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > index a04499c4f9ca..29207b2756c1 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > @@ -1009,10 +1009,10 @@ int mtk_drm_crtc_create(struct drm_device > *drm_dev, > > mtk_crtc->mmsys_dev = priv->mmsys_dev; > mtk_crtc->ddp_comp_nr = path_len; > - mtk_crtc->ddp_comp = devm_kmalloc_array(dev, > - mtk_crtc->ddp_comp_nr + > (conn_routes ? 1 : 0), > - sizeof(*mtk_crtc- > >ddp_comp), > - GFP_KERNEL); > + mtk_crtc->ddp_comp = devm_kcalloc(dev, > + mtk_crtc->ddp_comp_nr + > (conn_routes ? 1 : 0), > + sizeof(*mtk_crtc->ddp_comp), > + GFP_KERNEL); > if (!mtk_crtc->ddp_comp) > return -ENOMEM; > > -- > 2.44.0.396.g6e790dbe36-goog
| ![\](/images/icornerr.gif) |