| 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
| ||
|
Message-ID: <20250409110840.GG31475@pendragon.ideasonboard.com>
Date: Wed, 9 Apr 2025 14:08:40 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Biju Das <biju.das.jz@...renesas.com>
Cc: "Lad, Prabhakar" <prabhakar.csengg@...il.com>,
Tommaso Merciai <tommaso.merciai.xr@...renesas.com>,
Tommaso Merciai <tomm.merciai@...il.com>,
"linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>,
"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...renesas.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Geert Uytterhoeven <geert+renesas@...der.be>,
Magnus Damm <magnus.damm@...il.com>,
Hans Verkuil <hverkuil@...all.nl>,
Uwe Kleine-König <u.kleine-koenig@...libre.com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 11/17] media: rzg2l-cru: Add register mapping support
Hi Biju,
On Wed, Apr 09, 2025 at 07:25:43AM +0000, Biju Das wrote:
> On 09 April 2025 02:29, Laurent Pinchart wrote:
> > On Mon, Apr 07, 2025 at 04:55:33PM +0000, Lad, Prabhakar wrote:
> > > On Wed, Apr 2, 2025 at 10:39 AM Biju Das wrote:
> > > > On 02 April 2025 10:26, Laurent Pinchart wrote:
> > > > > On Wed, Apr 02, 2025 at 08:25:06AM +0000, Lad, Prabhakar wrote:
> > > > > > On Wed, Apr 2, 2025 at 9:20 AM Biju Das wrote:
> > > > > > > On 02 April 2025 08:35, Lad, Prabhakar wrote:
> > > > > > > > On Wed, Apr 2, 2025 at 7:31 AM Biju Das wrote:
> > > > > > > > > > On 28 March 2025 17:30, Tommaso Merciai wrote:
> > > > > > > > > > From: Lad Prabhakar
> > > > > > > > > > <prabhakar.mahadev-lad.rj@...renesas.com>
> > > > > > > > > >
> > > > > > > > > > Prepare for adding support for RZ/G3E and RZ/V2HP SoCs,
> > > > > > > > > > which have a CRU-IP that is mostly identical to RZ/G2L
> > > > > > > > > > but with different register offsets and additional
> > > > > > > > > > registers. Introduce a flexible register mapping
> > > > > > > > > > mechanism to handle these variations.
> > > > > > > > > >
> > > > > > > > > > Define the `rzg2l_cru_info` structure to store register
> > > > > > > > > > mappings and pass it as part of the OF match data.
> > > > > > > > > > Update the read/write functions to check out-of-bound
> > > > > > > > > > accesses and use indexed register offsets from
> > > > > > > > > > `rzg2l_cru_info`, ensuring compatibility across different SoC variants.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Lad Prabhakar
> > > > > > > > > > <prabhakar.mahadev-lad.rj@...renesas.com>
> > > > > > > > > > Signed-off-by: Tommaso Merciai
> > > > > > > > > > <tommaso.merciai.xr@...renesas.com>
> > > > > > > > > > ---
> > > > > > > > > > Changes since v2:
> > > > > > > > > > - Implemented new rzg2l_cru_write/read() that now are checking out-of-bound
> > > > > > > > > > accesses as suggested by LPinchart.
> > > > > > > > > > - Fixed AMnMBxADDRL() and AMnMBxADDRH() as suggested by LPinchart.
> > > > > > > > > > - Update commit body
> > > > > > > > > >
> > > > > > > > > > Changes since v4:
> > > > > > > > > > - Mark __rzg2l_cru_write_constant/__rzg2l_cru_read_constant
> > > > > > > > > > as __always_inline
> > > > > > > > > >
> > > > > > > > > > .../platform/renesas/rzg2l-cru/rzg2l-core.c | 46 ++++++++++++-
> > > > > > > > > > .../renesas/rzg2l-cru/rzg2l-cru-regs.h | 66 ++++++++++---------
> > > > > > > > > > .../platform/renesas/rzg2l-cru/rzg2l-cru.h | 4 ++
> > > > > > > > > > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 58
> > > > > > > > > > ++++++++++++++--
> > > > > > > > > > 4 files changed, 139 insertions(+), 35 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git
> > > > > > > > > > a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > > > > > > > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > > > > > > > > index eed9d2bd08414..abc2a979833aa 100644
> > > > > > > > > > ---
> > > > > > > > > > a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > > > > > > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cor
> > > > > > > > > > +++ e.c
> > > > > > > > > > @@ -22,6 +22,7 @@
> > > > > > > > > > #include <media/v4l2-mc.h>
> > > > > > > > > >
> > > > > > > > > > #include "rzg2l-cru.h"
> > > > > > > > > > +#include "rzg2l-cru-regs.h"
> > > > > > > > > >
> > > > > > > > > > static inline struct rzg2l_cru_dev
> > > > > > > > > > *notifier_to_cru(struct v4l2_async_notifier *n) { @@
> > > > > > > > > > -269,6 +270,9 @@ static int rzg2l_cru_probe(struct
> > > > > > > > > > platform_device *pdev)
> > > > > > > > > >
> > > > > > > > > > cru->dev = dev;
> > > > > > > > > > cru->info = of_device_get_match_data(dev);
> > > > > > > > > > + if (!cru->info)
> > > > > > > > > > + return dev_err_probe(dev, -EINVAL,
> > > > > > > > > > + "Failed to get OF
> > > > > > > > > > + match data\n");
> > > > > > > > > >
> > > > > > > > > > irq = platform_get_irq(pdev, 0);
> > > > > > > > > > if (irq < 0)
> > > > > > > > > > @@ -317,8 +321,48 @@ static void rzg2l_cru_remove(struct platform_device *pdev)
> > > > > > > > > > rzg2l_cru_dma_unregister(cru); }
> > > > > > > > > >
> > > > > > > > > > +static const u16 rzg2l_cru_regs[] = {
> > > > > > > > > > + [CRUnCTRL] = 0x0,
> > > > > > > > > > + [CRUnIE] = 0x4,
> > > > > > > > > > + [CRUnINTS] = 0x8,
> > > > > > > > > > + [CRUnRST] = 0xc,
> > > > > > > > > > + [AMnMB1ADDRL] = 0x100,
> > > > > > > > > > + [AMnMB1ADDRH] = 0x104,
> > > > > > > > > > + [AMnMB2ADDRL] = 0x108,
> > > > > > > > > > + [AMnMB2ADDRH] = 0x10c,
> > > > > > > > > > + [AMnMB3ADDRL] = 0x110,
> > > > > > > > > > + [AMnMB3ADDRH] = 0x114,
> > > > > > > > > > + [AMnMB4ADDRL] = 0x118,
> > > > > > > > > > + [AMnMB4ADDRH] = 0x11c,
> > > > > > > > > > + [AMnMB5ADDRL] = 0x120,
> > > > > > > > > > + [AMnMB5ADDRH] = 0x124,
> > > > > > > > > > + [AMnMB6ADDRL] = 0x128,
> > > > > > > > > > + [AMnMB6ADDRH] = 0x12c,
> > > > > > > > > > + [AMnMB7ADDRL] = 0x130,
> > > > > > > > > > + [AMnMB7ADDRH] = 0x134,
> > > > > > > > > > + [AMnMB8ADDRL] = 0x138,
> > > > > > > > > > + [AMnMB8ADDRH] = 0x13c,
> > > > > > > > > > + [AMnMBVALID] = 0x148,
> > > > > > > > > > + [AMnMBS] = 0x14c,
> > > > > > > > > > + [AMnAXIATTR] = 0x158,
> > > > > > > > > > + [AMnFIFOPNTR] = 0x168,
> > > > > > > > > > + [AMnAXISTP] = 0x174,
> > > > > > > > > > + [AMnAXISTPACK] = 0x178,
> > > > > > > > > > + [ICnEN] = 0x200,
> > > > > > > > > > + [ICnMC] = 0x208,
> > > > > > > > > > + [ICnMS] = 0x254,
> > > > > > > > > > + [ICnDMR] = 0x26c,
> > > > > > > > > > +};
> > > > > > > > >
> > > > > > > > > Do we need enum, can't we use struct instead with all these entries instead?
> > > > > > > > >
> > > > > > > > What benefit do you foresee when using struct? With the
> > > > > > > > current approach being used a minimal diff is generated when
> > > > > > > > switched to struct there will be lots of changes.
> > > > > > >
> > > > > > > The mapping is convinient when you want to iterate throught it.
> > > > > > > Here, if you just want to access the offset value from its
> > > > > > > name, a structure looks more appropriate.
> > > > > >
> > > > > > Thanks, as this patch has been reviewed by Laurent a couple of
> > > > > > times we will change this to struct If he insists.
> > > > >
> > > > > How would a struct look like ? I'm not sure what is being proposed.
> > > >
> > > > It will be
> > > >
> > > > struct rzg2l_cru_regs {
> > > > u16 cru_n_ctrl;
> > > > u16 cru_n_ie;
> > > > u16 cru_n_ints;
> > > > u16 cru_n_rst;
> > > > };
> > > >
> > > > static const struct rzg2l_cru_regs rzg2l_cru_regs = {
> > > > .cru_n_ctrl = 0x0,
> > > > .cru_n_ie = 0x4,
> > > > .cru_n_ints = 0x8,
> > > > .cru_n_rst = 0xc,
> > > > };
> > > >
> > > > You can access it using info->regs->cru_n_ctrl instead of
> > > > info->regs[CRUnCTRL] This is proposal.
> > >
> > > Are you OK with the above proposal?
> >
> > I may be missing something, but I don't see why this would be a significantly better option. It seems
> > it would make the callers more complex, and decrease readability.
>
> Basically,
> I guess sruct will allow us to avoid (WARN_ON(offset >= RZG2L_CRU_MAX_REG) and
> BUILD_BUG_ON(offset >= RZG2L_CRU_MAX_REG); checks as there is no array, so there is no
> buffer overrun condition and also we can drop enum aswell.
That's right fpr the BUILD_BUG_ON(), but I don't think that would be an
improvement. The BUILD_BUG_ON() catches at compile time the issues that
are comparable to mistyping a struct field name, so incorrect code
patterns will result in compile-time errors in both cases. The WARN_ON()
is there for cases where the offset needs to be dynamically computed,
and if we wanted to have similar safeguards with a struct, we would also
need a runtime check.
Using a struct would also prevent (I think) implementing additional
safeguards. Not registers exist on all platforms (hence the need for
this mapping mechanism), and it would be nice to catch attempts to
access a register that does not exist. With the existing series, we
could quite easily add a check in the read and write functions to ensure
the regs array entry is not zero (except when the register is CRUnCTRL).
With a struct-based approach, the read/write functions would get the
offset but wouldn't know which register is being accessed, so the same
logic can't be implemented.
Last but not least, I think the current approach is more readable in the
callers: lines are shorter, and register names match the documentation,
including being uppercase.
> So, if using struct decreases readability and makes the code complex,
> then current patch is fine.
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists