linux-kernel - Re: [PATCH v11 2/2] media: V3s: Add support for Allwinner CSI.

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

Date:   Mon, 8 Oct 2018 18:58:29 +0300
From:   Sakari Ailus <sakari.ailus@...ux.intel.com>
To:     Yong <yong.deng@...ewell.com>
Cc:     Maxime Ripard <maxime.ripard@...tlin.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Chen-Yu Tsai <wens@...e.org>,
        "David S. Miller" <davem@...emloft.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Arnd Bergmann <arnd@...db.de>,
        Hans Verkuil <hans.verkuil@...co.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Jacob Chen <jacob-chen@...wrt.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Thierry Reding <treding@...dia.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Todor Tomov <todor.tomov@...aro.org>,
        linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-sunxi@...glegroups.com
Subject: Re: [PATCH v11 2/2] media: V3s: Add support for Allwinner CSI.

Hi Yong,

On Sat, Sep 29, 2018 at 06:18:44PM +0800, Yong wrote:
> Hi Sakari,
> 
> On Fri, 28 Sep 2018 13:19:37 +0300
> Sakari Ailus <sakari.ailus@...ux.intel.com> wrote:
> 
> > Hi Yong,
> > 
> > Thanks for the update! This looks pretty good; some small bits to fix
> > still, please see my comments below.
> > 
> > On Wed, Sep 26, 2018 at 04:43:40PM +0800, Yong Deng wrote:
> > > Allwinner V3s SoC features two CSI module. CSI0 is used for MIPI CSI-2
> > > interface and CSI1 is used for parallel interface. This is not
> > > documented in datasheet but by test and guess.
> > > 
> > > This patch implement a v4l2 framework driver for it.
> > > 
> > > Currently, the driver only support the parallel interface. MIPI-CSI2,
> > > ISP's support are not included in this patch.
> > > 
> 
> ...
> 
> > > +
> > > +static void sun6i_csi_v4l2_cleanup(struct sun6i_csi *csi)
> > > +{
> > > +	v4l2_async_notifier_unregister(&csi->notifier);
> > > +	v4l2_async_notifier_cleanup(&csi->notifier);
> > > +	sun6i_video_cleanup(&csi->video);
> > > +	v4l2_device_unregister(&csi->v4l2_dev);
> > > +	media_device_unregister(&csi->media_dev);
> > 
> > Please unregister the media device as first.
> 
> I notice that the release order of intel ipu3 is:
>     cio2_notifier_exit(cio2);
>     cio2_fbpt_exit_dummy(cio2);
>     for (i = 0; i < CIO2_QUEUES; i++) 
>         cio2_queue_exit(cio2, &cio2->queue[i]);
>     v4l2_device_unregister(&cio2->v4l2_dev);
>     media_device_unregister(&cio2->media_dev);
>     media_device_cleanup(&cio2->media_dev);
>     mutex_destroy(&cio2->lock);

Well, yeah... this doesn't make a big difference after all. Unregistering
the media device first is still preferred.

> 
> > 
> > You're missing freeing the control handler.
> > 
> > > +	media_device_cleanup(&csi->media_dev);
> > > +}
> > > +
> > > +static int sun6i_csi_v4l2_init(struct sun6i_csi *csi)
> > > +{
> > > +	int ret;
> > > +
> > > +	csi->media_dev.dev = csi->dev;
> > > +	strlcpy(csi->media_dev.model, "Allwinner Video Capture Device",
> > > +		sizeof(csi->media_dev.model));
> > 
> > strscpy, please.
> > 
> > > +	csi->media_dev.hw_revision = 0;
> > > +
> > > +	media_device_init(&csi->media_dev);
> > > +
> > > +	ret = v4l2_ctrl_handler_init(&csi->ctrl_handler, 0);
> > 
> > Do you need controls? The driver doesn't appear to register any.
> 
> Some one suggest this. They want the drver can pass control queries to 
> the subdev.

If you need that, it should be done in the user space instead. The imx
staging driver does so but it is not supported by the control framework:
you may only call v4l2_ctrl_handler_free() for the purpose of freeing the
resources of the control handler --- not while it is still accessible from
the user space.

> 
> > 
> > > +	if (ret) {
> > > +		dev_err(csi->dev, "V4L2 controls handler init failed (%d)\n",
> > > +			ret);
> > > +		goto clean_media;
> > > +	}
> > > +
> > > +	csi->v4l2_dev.mdev = &csi->media_dev;
> > > +	csi->v4l2_dev.ctrl_handler = &csi->ctrl_handler;
> > > +	ret = v4l2_device_register(csi->dev, &csi->v4l2_dev);
> > > +	if (ret) {
> > > +		dev_err(csi->dev, "V4L2 device registration failed (%d)\n",
> 
> ...
> 
> > > +
> > > +/*
> > > + * PHYS_OFFSET isn't available on all architectures. In order to
> > > + * accomodate for COMPILE_TEST, let's define it to something dumb.
> > > + */
> > > +#ifndef PHYS_OFFSET
> > 
> > How about:
> > 
> > #if !defined(CONFIG_COMPILE_TEST) && !defined(PHYS_OFFSET)
> 
> Do you mean:
> #if defined(CONFIG_COMPILE_TEST) && !defined(PHYS_OFFSET)

Oh, yes. Indeed.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@...ux.intel.com