linux-kernel - Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <X+pzKDNWpiQWenHy@pendragon.ideasonboard.com>
Date:   Tue, 29 Dec 2020 02:07:04 +0200
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Daniel Scally <djrscally@...il.com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        devel@...ica.org, "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Yong Zhi <yong.zhi@...el.com>,
        Bingbu Cao <bingbu.cao@...el.com>,
        Tian Shu Qiu <tian.shu.qiu@...el.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Robert Moore <robert.moore@...el.com>,
        Erik Kaneda <erik.kaneda@...el.com>,
        Petr Mladek <pmladek@...e.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
        Jacopo Mondi <jacopo+renesas@...ndi.org>,
        kieran.bingham+renesas@...asonboard.com,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Marco Felsch <m.felsch@...gutronix.de>,
        niklas.soderlund+renesas@...natech.se,
        Steve Longerbeam <slongerbeam@...il.com>,
        "Krogerus, Heikki" <heikki.krogerus@...ux.intel.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Jordan Hand <jorhand@...ux.microsoft.com>
Subject: Re: [PATCH v3 14/14] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver

Hi Andy,

On Tue, Dec 29, 2020 at 01:54:59AM +0200, Andy Shevchenko wrote:
> On Tue, Dec 29, 2020 at 1:08 AM Laurent Pinchart wrote:
> >
> > On Tue, Dec 29, 2020 at 12:55:44AM +0200, Andy Shevchenko wrote:
> > > On Mon, Dec 28, 2020 at 10:37:38PM +0000, Daniel Scally wrote:
> > > > On 28/12/2020 17:05, Sakari Ailus wrote:
> 
> ...
> 
> > > > Which do you prefer?
> > >
> > > Actually ipu3-cio2.h misses a lot of inclusions (like mutex.h which I
> > > immediately noticed when scrolled over data types).
> >
> > Then ipu3-cio2.h should be fixed :-)
> 
> Below is a draft patch (it is possible mangled, due to Gmail). Can you
> look at it and tell me what you think?

Thank you for the patch.

> I believe some headers can be removed, but I have no idea about header
> inclusion guarantees that v4l2 provides.
> 
> From 10fa6c7ff66ded35a246677ffe20c677e8453f5b3 Mon Sep 17 00:00:00 2001
> From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> Date: Tue, 29 Dec 2020 01:42:03 +0200
> Subject: [PATCH 1/1] media: ipu3-cio2: Add headers that ipu3-cio2.h is direct
>  user of
> 
> Add headers that ipu3-cio2.h is direct user of.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
>  drivers/media/pci/intel/ipu3/ipu3-cio2.h | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> index ccf0b85ae36f..9ea154c50ba1 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h
> @@ -4,8 +4,25 @@
>  #ifndef __IPU3_CIO2_H
>  #define __IPU3_CIO2_H
> 
> +#include <linux/bits.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/kernel.h>
> +#include <linux/mutex.h>
>  #include <linux/types.h>
> 
> +#include <asm/page.h>
> +
> +#include <linux/videodev2.h>

I think this can be dropped.

> +
> +#include <media/media-device.h>
> +#include <media/media-entity.h>
> +#include <media/v4l2-async.h>
> +#include <media/v4l2-dev.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/videobuf2-core.h>
> +#include <media/videobuf2-v4l2.h>
> +
>  #define CIO2_NAME "ipu3-cio2"
>  #define CIO2_DEVICE_NAME "Intel IPU3 CIO2"
>  #define CIO2_ENTITY_NAME "ipu3-csi2"
> @@ -325,6 +342,8 @@ struct csi2_bus_info {
>   u32 lanes;
>  };
> 
> +struct cio2_fbpt_entry;
> +
>  struct cio2_queue {
>   /* mutex to be used by vb2_queue */
>   struct mutex lock;
> @@ -355,6 +374,8 @@ struct cio2_queue {
>   atomic_t bufs_queued;
>  };
> 
> +struct pci_dev;
> +

How about grouping all forward declarations at the top ?

Otherwise this looks good,

Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>

If I was maintaining this driver I would likely move the structure
definitions to ipu3-cio2.c though.

>  struct cio2_device {
>   struct pci_dev *pci_dev;
>   void __iomem *base;

-- 
Regards,

Laurent Pinchart