linux-kernel - Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 8 Aug 2023 15:04:11 +0000
From:   Jason-JH Lin (林睿祥) 
        <Jason-JH.Lin@...iatek.com>
To:     "amergnat@...libre.com" <amergnat@...libre.com>,
        "chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>,
        "angelogioacchino.delregno@...labora.com" 
        <angelogioacchino.delregno@...labora.com>,
        "eugen.hristev@...labora.com" <eugen.hristev@...labora.com>
CC:     "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        Singo Chang (張興國) 
        <Singo.Chang@...iatek.com>,
        Johnson Wang (王聖鑫) 
        <Johnson.Wang@...iatek.com>,
        Jason-ch Chen (陳建豪) 
        <Jason-ch.Chen@...iatek.com>,
        Shawn Sung (宋孝謙) 
        <Shawn.Sung@...iatek.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Nancy Lin (林欣螢) <Nancy.Lin@...iatek.com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        Project_Global_Chrome_Upstream_Group 
        <Project_Global_Chrome_Upstream_Group@...iatek.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc
 enabling

Hi Eugen,

Thanks for the reviews.

On Tue, 2023-08-08 at 14:55 +0300, Eugen Hristev wrote:
> Hi Jason,
> 
> On 8/7/23 04:51, Jason-JH.Lin wrote:
> > The plane_state of drm_atomic_state is not sync to the
> > mtk_plane_state
> > stored in mtk_crtc during crtc enabling.
> > 
> > So we need to update the mtk_plane_state stored in mtk_crtc by the
> > drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> > 
> > While updating mtk_plane_state, OVL layer should be disabled when
> > the fb
> > in plane_state of drm_atomic_state is NULL.
> > 
> > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> > MT8173.")
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@...iatek.com>
> > ---
> > Change in RESEND v4:
> > Remove redundant plane_state assigning.
> > ---
> >   drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
> >   drivers/gpu/drm/mediatek/mtk_drm_plane.c | 11 ++++++++---
> >   drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
> >   3 files changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index d40142842f85..7db4d6551da7 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -328,7 +328,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl,
> > void *mssg)
> >   }
> >   #endif
> >   
> > -static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> > +static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc,
> > struct drm_atomic_state *state)
> >   {
> >   	struct drm_crtc *crtc = &mtk_crtc->base;
> >   	struct drm_connector *connector;
> > @@ -405,11 +405,17 @@ static int mtk_crtc_ddp_hw_init(struct
> > mtk_drm_crtc *mtk_crtc)
> >   	/* Initially configure all planes */
> >   	for (i = 0; i < mtk_crtc->layer_nr; i++) {
> >   		struct drm_plane *plane = &mtk_crtc->planes[i];
> > -		struct mtk_plane_state *plane_state;
> > +		struct drm_plane_state *new_state;
> > +		struct mtk_plane_state *plane_state =
> > to_mtk_plane_state(plane->state);
> >   		struct mtk_ddp_comp *comp;
> >   		unsigned int local_layer;
> >   
> > -		plane_state = to_mtk_plane_state(plane->state);
> 
> any reason why you moved the initialization of plane_state at the 
> declaration phase ?
> 
I just like to assign it at the declaration phase.
But it's not related to this fix patch, so I'll move it back.
Thanks.

> > +		/* sync the new plane state from drm_atomic_state */
> > +		if (state->planes[i].ptr) {
> > +			new_state =
> > drm_atomic_get_new_plane_state(state, state->planes[i].ptr);
> 
> Can drm_atomic_get_new_plane_state fail ? and new_state becomes null
> ?

drm_atomic_get_new_plane_state() won't fail and new_state won't be
NULL.

But if state->planes[i].ptr is NULL, it'll crash in drm_plane_index()
inside drm_atomic_get_new_plane_state().

> 
> I see mtk_plane_update_new_state assumes new_state being a correct 
> state/pointer.

The usage is the same as mtk_plane_atomic_update().

Regards,
Jason-JH.Lin

> 
> Regards,
>