linux-kernel - RE: [PATCH v5 2/6] media: platform: visconti: Add Toshiba Visconti Video Input Interface 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: <TYAPR01MB6201A43FFF4572CA8295B1E192D19@TYAPR01MB6201.jpnprd01.prod.outlook.com>
Date:   Wed, 1 Feb 2023 11:22:14 +0000
From:   <yuji2.ishikawa@...hiba.co.jp>
To:     <laurent.pinchart@...asonboard.com>
CC:     <sakari.ailus@....fi>, <hverkuil@...all.nl>, <mchehab@...nel.org>,
        <nobuhiro1.iwamatsu@...hiba.co.jp>, <robh+dt@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <rafael.j.wysocki@...el.com>,
        <broonie@...nel.org>, <linux-media@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>
Subject: RE: [PATCH v5 2/6] media: platform: visconti: Add Toshiba Visconti
 Video Input Interface driver

Hello Laurent,

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> Sent: Wednesday, February 1, 2023 6:42 PM
> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <yuji2.ishikawa@...hiba.co.jp>
> Cc: sakari.ailus@....fi; hverkuil@...all.nl; mchehab@...nel.org; iwamatsu
> nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ) <nobuhiro1.iwamatsu@...hiba.co.jp>;
> robh+dt@...nel.org; krzysztof.kozlowski+dt@...aro.org;
> rafael.j.wysocki@...el.com; broonie@...nel.org; linux-media@...r.kernel.org;
> linux-arm-kernel@...ts.infradead.org; linux-kernel@...r.kernel.org;
> devicetree@...r.kernel.org
> Subject: Re: [PATCH v5 2/6] media: platform: visconti: Add Toshiba Visconti
> Video Input Interface driver
> 
> Hello Ishikawa-san,
> 
> On Wed, Feb 01, 2023 at 02:02:43AM +0000, yuji2.ishikawa@...hiba.co.jp wrote:
> > Hello Sakari,
> >
> > Sorry for sending the reply again.
> > My mail agent posted the previous one with HTML format.
> >
> > Thank you for reviewing and your comments.
> >
> > > -----Original Message-----
> > > From: Sakari Ailus sakari.ailus@....fi
> > > Sent: Wednesday, January 18, 2023 7:40 AM
> > > To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> > > yuji2.ishikawa@...hiba.co.jp
> > > Cc: Hans Verkuil hverkuil@...all.nl; Laurent Pinchart
> > > laurent.pinchart@...asonboard.com; Mauro Carvalho Chehab
> > > mchehab@...nel.org; iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ)
> > > nobuhiro1.iwamatsu@...hiba.co.jp; Rob Herring robh+dt@...nel.org;
> > > Krzysztof Kozlowski krzysztof.kozlowski+dt@...aro.org; Rafael J .
> > > Wysocki rafael.j.wysocki@...el.com; Mark Brown broonie@...nel.org;
> > > linux-media@...r.kernel.org; linux-arm-kernel@...ts.infradead.org;
> > > linux-kernel@...r.kernel.org; devicetree@...r.kernel.org
> > > Subject: Re: [PATCH v5 2/6] media: platform: visconti: Add Toshiba
> > > Visconti Video Input Interface driver
> 
> [snip]
> 
> > > > diff --git a/drivers/media/platform/visconti/hwd_viif_reg.h
> > > > b/drivers/media/platform/visconti/hwd_viif_reg.h
> > > > new file mode 100644
> > > > index 00000000000..b7f43c5fe95
> > > > --- /dev/null
> > > > +++ b/drivers/media/platform/visconti/hwd_viif_reg.h
> > > > @@ -0,0 +1,2802 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */
> > > > +/* Toshiba Visconti Video Capture Support
> > > > + *
> > > > + * (C) Copyright 2022 TOSHIBA CORPORATION
> > > > + * (C) Copyright 2022 Toshiba Electronic Devices & Storage
> > > > +Corporation  */
> > > > +
> > > > +#ifndef HWD_VIIF_REG_H
> > > > +#define HWD_VIIF_REG_H
> > > > +
> > > > +/**
> > > > + * struct hwd_viif_csi2host_reg - Registers for VIIF CSI2HOST
> > > > +control  */ struct hwd_viif_csi2host_reg {
> > > > +    u32 RESERVED_A_1;
> > > > +    u32 CSI2RX_NLANES;
> > > > +    u32 CSI2RX_RESETN;
> > > > +    u32 CSI2RX_INT_ST_MAIN;
> > > > +    u32 CSI2RX_DATA_IDS_1;
> > > > +    u32 CSI2RX_DATA_IDS_2;
> > > > +    u32 RESERVED_B_1[10];
> > > > +    u32 CSI2RX_PHY_SHUTDOWNZ;
> > > > +    u32 CSI2RX_PHY_RSTZ;
> > > > +    u32 CSI2RX_PHY_RX;
> > > > +    u32 CSI2RX_PHY_STOPSTATE;
> > > > +    u32 CSI2RX_PHY_TESTCTRL0;
> > > > +    u32 CSI2RX_PHY_TESTCTRL1;
> > > > +    u32 RESERVED_B_2[34];
> > > > +    u32 CSI2RX_INT_ST_PHY_FATAL;
> > > > +    u32 CSI2RX_INT_MSK_PHY_FATAL;
> > > > +    u32 CSI2RX_INT_FORCE_PHY_FATAL;
> > > > +    u32 RESERVED_B_3[1];
> > > > +    u32 CSI2RX_INT_ST_PKT_FATAL;
> > > > +    u32 CSI2RX_INT_MSK_PKT_FATAL;
> > > > +    u32 CSI2RX_INT_FORCE_PKT_FATAL;
> > > > +    u32 RESERVED_B_4[1];
> > > > +    u32 CSI2RX_INT_ST_FRAME_FATAL;
> > > > +    u32 CSI2RX_INT_MSK_FRAME_FATAL;
> > > > +    u32 CSI2RX_INT_FORCE_FRAME_FATAL;
> > > > +    u32 RESERVED_B_5[1];
> > > > +    u32 CSI2RX_INT_ST_PHY;
> > > > +    u32 CSI2RX_INT_MSK_PHY;
> > > > +    u32 CSI2RX_INT_FORCE_PHY;
> > > > +    u32 RESERVED_B_6[1];
> > > > +    u32 CSI2RX_INT_ST_PKT;
> > > > +    u32 CSI2RX_INT_MSK_PKT;
> > > > +    u32 CSI2RX_INT_FORCE_PKT;
> > > > +    u32 RESERVED_B_7[1];
> > > > +    u32 CSI2RX_INT_ST_LINE;
> > > > +    u32 CSI2RX_INT_MSK_LINE;
> > > > +    u32 CSI2RX_INT_FORCE_LINE;
> > > > +    u32 RESERVED_B_8[113];
> > > > +    u32 RESERVED_A_2;
> > > > +    u32 RESERVED_A_3;
> > > > +    u32 RESERVED_A_4;
> > > > +    u32 RESERVED_A_5;
> > > > +    u32 RESERVED_A_6;
> > > > +    u32 RESERVED_B_9[58];
> > > > +    u32 RESERVED_A_7;
> > >
> > > These should be lower case, they're struct members.
> > >
> > > This way of defining a hardware register interface is highly
> > > unconventional. I'm not saying no to it, not now at least, but
> > > something should be done to make this more robust against accidental
> > > changes: adding a field in the middle changes the address of
> > > anything that comes after it, and it's really difficult to say from
> > > the code alone that the address of a given register is what it's intended to be.
> Maybe pahole would still help?
> > > But some documentation would be needed in that case.
> > >
> > > I wonder what others think.
> >
> > I understand the risk.
> > I'll remove these struct-style definition and introduce macro style definition.
> > I've hesitated this migration simply because it seemed difficult to
> > complete without any defects especially on calculating the offset of each
> member.
> > I try find a series of operations that will complete the migration safely.
> 
> I agree with you about the migration risk. Maybe a script that parses the header
> file and generates macros would take less time to implement than doing it
> manually, and would be safer ?

Thank you for the advice.
I'm also thinking about generating macro definitions from headers.
The pahole tool might help me checking if the generated macros are correct.

> > > > +};
> > > > +
> 
> --
> Regards,
> 
> Laurent Pinchart

Regards,
Yuji Ishikawa