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
| ||
|
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
Powered by blists - more mailing lists