linux-kernel - Re: [PATCH v5 3/9] vcodec: mediatek: Add Mediatek V4L2 Video Decoder Driver

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: <1477306152.2916.1.camel@mtksdaap41>
Date:   Mon, 24 Oct 2016 18:49:12 +0800
From:   Tiffany Lin <tiffany.lin@...iatek.com>
To:     Mauro Carvalho Chehab <mchehab@....samsung.com>
CC:     Hans Verkuil <hans.verkuil@...co.com>,
        <daniel.thompson@...aro.org>, "Rob Herring" <robh+dt@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Daniel Kurtz <djkurtz@...omium.org>,
        Pawel Osciak <posciak@...omium.org>,
        Eddie Huang <eddie.huang@...iatek.com>,
        Yingjoe Chen <yingjoe.chen@...iatek.com>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-media@...r.kernel.org>,
        <linux-mediatek@...ts.infradead.org>, <PoChun.Lin@...iatek.com>
Subject: Re: [PATCH v5 3/9] vcodec: mediatek: Add Mediatek V4L2 Video
 Decoder Driver

Hi Mauro,

On Mon, 2016-10-24 at 07:05 -0200, Mauro Carvalho Chehab wrote:
> Em Mon, 24 Oct 2016 11:22:08 +0800
> Tiffany Lin <tiffany.lin@...iatek.com> escreveu:
> 
> > Hi Mauro,
> > 
> > On Fri, 2016-10-21 at 11:01 -0200, Mauro Carvalho Chehab wrote:
> > > Em Fri, 2 Sep 2016 20:19:54 +0800
> > > Tiffany Lin <tiffany.lin@...iatek.com> escreveu:
> > >   
> > > > Add v4l2 layer decoder driver for MT8173
> > > > 
> > > > Signed-off-by: Tiffany Lin <tiffany.lin@...iatek.com>  
> > >   
> > > > +int vdec_if_init(struct mtk_vcodec_ctx *ctx, unsigned int fourcc)
> > > > +{
> > > > +	int ret = 0;
> > > > +
> > > > +	switch (fourcc) {
> > > > +	case V4L2_PIX_FMT_H264:
> > > > +	case V4L2_PIX_FMT_VP8:
> > > > +	default:
> > > > +		return -EINVAL;
> > > > +	}  
> > > 
> > > Did you ever test this driver? The above code will *always* return
> > > -EINVAL, with will cause vidioc_vdec_s_fmt() to always fail!
> > > 
> > > I suspect that what you wanted to do, instead, is:
> > > 
> > > 	switch (fourcc) {
> > > 	case V4L2_PIX_FMT_H264:
> > > 	case V4L2_PIX_FMT_VP8:
> > > 		break;
> > > 	default:
> > > 		return -EINVAL;
> > >   
> > 
> > The original idea here is that vp8 and h264 are added in later patches.
> > If get this patch without later patches, it should return -EINVAL.
> 
> I noticed your idea, but next time, don't add dead code like that.
> Reviewers check patch by patch at the order they're present at the
> patch series.
> 
> So, don't add something broken by purpose, assuming that it would
> be fixed later.
> 
Got it.
> > 
> > 
> > > Btw, this patch series has also several issues that were pointed by
> > > checkpatch. Please *always* run checkpatch when submitting your work.
> > > 
> > > You should take a look at the Kernel documentation about how to
> > > submit patches, at:
> > > 	https://mchehab.fedorapeople.org/kernel_docs/process/index.html
> > > 
> > > PS.: this time, I fixed the checkpatch issues for you. So, let me know
> > > if the patch below is OK, and I'll merge it at media upstream,
> > > assuming that the other patches in this series are ok.
> > >   
> > 
> > I did run checkpatch, but I don't know why these issues missed.
> > probably I run checkpatch for all files not for patches.
> > I will take a look at the documentation and keep this in mind for future
> > upstream.
> > Appreciated for your help.
> 
> Checkpatch should be run patch by patch, as we expect that all patches
> will follow the coding style and will compile fine, without introducing
> warnings.
> 
> I do compile the Kernel for every single patch I merge.
> 
Got it. I will follow this.

best regards,
Tiffany

> Regards,
> Mauro