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 for Android: free password hash cracker in your pocket

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

Message-ID: <20121128092213.4bd0870f@redhat.com>
Date:	Wed, 28 Nov 2012 09:22:13 -0200
From:	Mauro Carvalho Chehab <mchehab@...hat.com>
To:	Prabhakar Lad <prabhakar.csengg@...il.com>
Cc:	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>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Sakari Ailus <sakari.ailus@....fi>,
	Hans Verkuil <hans.verkuil@...co.com>
Subject: Re: [PATCH v3 9/9] davinci: vpfe: Add documentation and TODO

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.

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.

Regards,
Mauro
--
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/