linux-kernel - Re: [PATCH v3 9/9] davinci: vpfe: Add documentation and TODO

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1555450.K4uAzFNhY7@avalon>
Date:	Wed, 28 Nov 2012 14:00:14 +0100
From:	Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:	Mauro Carvalho Chehab <mchehab@...hat.com>
Cc:	Prabhakar Lad <prabhakar.csengg@...il.com>,
	LMML <linux-media@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	DLOS <davinci-linux-open-source@...ux.davincidsp.com>,
	Manjunath Hadli <manjunath.hadli@...com>,
	Prabhakar Lad <prabhakar.lad@...com>,
	devel@...verdev.osuosl.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Sakari Ailus <sakari.ailus@....fi>,
	Hans Verkuil <hans.verkuil@...co.com>
Subject: Re: [PATCH v3 9/9] davinci: vpfe: Add documentation and TODO

Hi Mauro,

Please see below.

On Wednesday 28 November 2012 09:22:13 Mauro Carvalho Chehab wrote:
> Hi Prabhakar,
> 
> Em Wed, 28 Nov 2012 16:12:09 +0530
> 
> Prabhakar Lad <prabhakar.csengg@...il.com> escreveu:
> > +Introduction
> > +============
> > +
> > + This file documents the Texas Instruments Davinci Video processing Front
> > + End (VPFE) driver located under drivers/media/platform/davinci. The
> > + original driver exists for Davinci VPFE, which is now being changed to
> > + Media Controller Framework.
> 
> Hmm... please correct me if I'm wrong, but are you wanting to replace an
> existing driver at drivers/media/platform/davinci, by another one at
> staging that has lots of known issues, as pointed at your TODO????
> 
> If so, please don't do that. Replacing a driver by some other one is
> generally a very bad idea, especially in this case, where the new driver
> has clearly several issues, the main one being to define its own proprietary
> and undocumented API:
>
> > +As of now since the interface will undergo few changes all the include
> > +files are present in staging itself, to build for dm365 follow below
> > +steps,
> > +
> > +- copy vpfe.h from drivers/staging/media/davinci_vpfe/ to
> > +  include/media/davinci/ folder for building the uImage.
> > +- copy davinci_vpfe_user.h from drivers/staging/media/davinci_vpfe/ to
> > +  include/uapi/linux/davinci_vpfe.h, and add a entry in Kbuild (required
> > +  for building application).
> > +- copy dm365_ipipeif_user.h from drivers/staging/media/davinci_vpfe/ to
> > +  include/uapi/linux/dm365_ipipeif.h and a entry in Kbuild (required
> > +  for building application).
> 
> Among other things, with those ugly and very likely mandatory API calls:
>
> >+/*
> >+ * Private IOCTL
> >+ * VIDIOC_VPFE_IPIPEIF_S_CONFIG: Set IPIEIF configuration
> >+ * VIDIOC_VPFE_IPIPEIF_G_CONFIG: Get IPIEIF configuration
> >+ */
> >+#define VIDIOC_VPFE_IPIPEIF_S_CONFIG \
> >+	_IOWR('I', BASE_VIDIOC_PRIVATE + 1, struct ipipeif_params)
> >+#define VIDIOC_VPFE_IPIPEIF_G_CONFIG \
> >+	_IOWR('I', BASE_VIDIOC_PRIVATE + 2, struct ipipeif_params)
> >+
> >+#endif
> 
> I remember we rejected already drivers like that with obscure "S_CONFIG"
> private ioctl that were suspect to send a big initialization undocumented
> blob to the driver, as only the vendor's application would be able to use
> such driver.

That's correct, and that's why the driver is going to staging. From there it 
will be incrementally fixed and then moved to drivers/media/, or dropped if 
not maintained.

> So, instead, of submitting it to staging, you should be sending incremental
> patches for the existing driver, adding newer functionality there, and
> using the proper V4L2 API, with makes life easier for reviewers and
> application developers.

I agree that it would be the best thing to do, but I don't think it's going to 
happen. We need to decide between two options.

- Push back now and insist in incremental patches for the existing driver, and 
get nothing back as TI will very likely give up completely.
- Accept the driver in staging, get it fixed incrementally, and finally move 
it to drivers/media/

There's a political side to this issue, we need to decide whether we want to 
insist vendors getting everything right before any code reaches mainline, in 
which case I believe we will lose some of them in the process, including major 
vendors such as TI, or if we can make the mainline learning curve and 
experience a bit more smooth by accepting such code in staging.

I would vote for the second option, with a very clear rule that getting the 
driver in staging is only one step in the journey: if the development effort 
stops there, the driver *will* be removed.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/