linux-kernel - Re: [PATCH v6 1/5] media: platform: Fix jpeg dec driver bug and improve code quality

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]

Message-ID: <1582537569.859.14.camel@mhfsdcap03>
Date:   Mon, 24 Feb 2020 17:46:09 +0800
From:   Xia Jiang <xia.jiang@...iatek.com>
To:     Tomasz Figa <tfiga@...omium.org>
CC:     Hans Verkuil <hverkuil-cisco@...all.nl>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        "Matthias Brugger" <matthias.bgg@...il.com>,
        Rick Chang <rick.chang@...iatek.com>,
        <linux-media@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-mediatek@...ts.infradead.org>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        <srv_heupstream@...iatek.com>
Subject: Re: [PATCH v6 1/5] media: platform: Fix jpeg dec driver bug and
 improve code quality

On Fri, 2020-02-14 at 18:35 +0900, Tomasz Figa wrote:
> Hi Xia,
> 
> On Tue, Jan 21, 2020 at 05:53:17PM +0800, Xia Jiang wrote:
> > Fix v4l2-compliance test bug and improve code quality of jpeg decode
> > driver, because the jpeg encode driver will base on it.
> > 
> > Signed-off-by: Xia Jiang <xia.jiang@...iatek.com>
> > ---
> > v6: alignment 'MTK_JPEG_DCTSIZE' match open parenthesis.
> >                                            
> > v5: Use clamp()to replace mtk_jpeg_bound_align_image() and round_up()
> >     to replace mtk_jpeg_align().
> >     Get correct compose value in mtk_jpeg_selection().
> >     Cancel spin lock and unlock operation in device run function.
> >     Change register offset hex numberals from upercase to lowercase.
> > 
> > v4: new add patch for v4l2-compliance test bug fix.
> 
> Thanks for the patch. The changes look good to me, but each of the
> unrelated changes should be split into its own patch, with proper
> explanation in its commit message. Especially the ones that introduce
> behavior changes, such as the S_SELECTION or locking change.
> 
> Also please see one comment inline.
> 
> [snip]
> 
> > @@ -801,7 +778,6 @@ static void mtk_jpeg_device_run(void *priv)
> >  	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> >  	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> >  	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> > -	unsigned long flags;
> >  	struct mtk_jpeg_src_buf *jpeg_src_buf;
> >  	struct mtk_jpeg_bs bs;
> >  	struct mtk_jpeg_fb fb;
> > @@ -829,13 +805,11 @@ static void mtk_jpeg_device_run(void *priv)
> >  	if (mtk_jpeg_set_dec_dst(ctx, &jpeg_src_buf->dec_param, &dst_buf->vb2_buf, &fb))
> >  		goto dec_end;
> >  
> > -	spin_lock_irqsave(&jpeg->hw_lock, flags);
> 
> Why is it safe to remove the locking here?
Dear Tomasz,

I will split unrelated changes into different patches.

My opinion about remove locking is following(after deep thinking):

The device_run function can be only called once for one instance.
For multi-instance,is there any posssibility of hw override if removing locking?I think so.
What about your further opinion?

Best Regards,
Xia Jiang
> >  	mtk_jpeg_dec_reset(jpeg->dec_reg_base);
> >  	mtk_jpeg_dec_set_config(jpeg->dec_reg_base,
> >  				&jpeg_src_buf->dec_param, &bs, &fb);
> >  
> >  	mtk_jpeg_dec_start(jpeg->dec_reg_base);
> > -	spin_unlock_irqrestore(&jpeg->hw_lock, flags);
> >  	return;
> >  
> >  dec_end:
> 
> Best regards,
> Tomasz
>