linux-kernel - Re: [PATCH v4 4/7] irqchip/renesas-rzv2h: Add rzv2h_icu_register_dma_req

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: <CAMuHMdUpNJiaGf3OTaSjjh4SA+nXODQj38P1-M0gG2mnxC_zLw@mail.gmail.com>
Date: Mon, 24 Feb 2025 13:59:19 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Fabrizio Castro <fabrizio.castro.jz@...esas.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org, 
	Biju Das <biju.das.jz@...renesas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>, linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH v4 4/7] irqchip/renesas-rzv2h: Add rzv2h_icu_register_dma_req_ack()

Hi Fabrizio,

On Thu, 20 Feb 2025 at 16:01, Fabrizio Castro
<fabrizio.castro.jz@...esas.com> wrote:
> On the Renesas RZ/V2H(P) family of SoCs, DMAC IPs are connected
> to the Interrupt Control Unit (ICU).
> For DMA transfers, a request number and an ack number must be
> registered with the ICU, which means that the DMAC driver has
> to be able to instruct the ICU driver with the registration of
> such ids.
>
> Export rzv2h_icu_register_dma_req_ack() so that the DMA driver
> can register both ids in one go.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@...esas.com>
> Reviewed-by: Thomas Gleixner <tglx@...utronix.de>
> ---
> v3->v4:
> * No change.
> v2->v3:
> * Replaced rzv2h_icu_register_dma_req_ack with
>   rzv2h_icu_register_dma_req_ack() in changelog.
> * Added dummy for rzv2h_icu_register_dma_req_ack().
> * Added Rb Thomas.

Thanks for the update!

> --- a/drivers/irqchip/irq-renesas-rzv2h.c
> +++ b/drivers/irqchip/irq-renesas-rzv2h.c

> @@ -94,6 +110,45 @@ struct rzv2h_icu_priv {
>         raw_spinlock_t                  lock;
>  };
>
> +void rzv2h_icu_register_dma_req_ack(struct platform_device *icu_dev, u8 dmac_index, u8 dmac_channel,
> +                                   u16 req_no, u8 ack_no)
> +{
> +       struct rzv2h_icu_priv *priv = platform_get_drvdata(icu_dev);
> +       u32 icu_dmackselk, dmaack, dmaack_mask;
> +       u32 icu_dmksely, dmareq, dmareq_mask;
> +       u8 k, field_no;
> +       u8 y, upper;
> +
> +       if (req_no >= RZV2H_ICU_DMAC_REQ_NO_MIN_FIX_OUTPUT)
> +               req_no = RZV2H_ICU_DMAC_REQ_NO_DEFAULT;

What is the purpose of this check?
The hardware register field size is 10 bits, so I think it is better
to just limit it to 0x3ff.
Checking for RZV2H_ICU_DMAC_REQ_NO_MIN_FIX_OUTPUT means you will have to
update this check when a new SoC supports higher values than 0x1b5.

> +
> +       if (ack_no >= RZV2H_ICU_DMAC_ACK_NO_MIN_FIX_OUTPUT)
> +               ack_no = RZV2H_ICU_DMAC_ACK_NO_DEFAULT;

What is the purpose of this check?
There are only 23 DMACKSELk registers, so using
RZV2H_ICU_DMAC_ACK_NO_DEFAULT = 0x7f will write beyond the last
register below. And drivers/dma/sh/rz-dmac.c does call this
function with req_no = RZV2H_ICU_DMAC_REQ_NO_DEFAULT and ack_no =
RZV2H_ICU_DMAC_ACK_NO_DEFAULT...

> +
> +       y = dmac_channel / 2;
> +       upper = dmac_channel % 2;
> +
> +       dmareq = ICU_DMAC_PREP_DMAREQ(req_no, upper);
> +       dmareq_mask = ICU_DMAC_DMAREQ_MASK(upper);
> +
> +       k  = ack_no / 4;
> +       field_no = ack_no % 4;
> +
> +       dmaack_mask = ICU_DMAC_DACK_SEL_MASK(field_no);
> +       dmaack = ICU_DMAC_PREP_DACK_SEL(ack_no, field_no);
> +
> +       guard(raw_spinlock_irqsave)(&priv->lock);
> +
> +       icu_dmksely = readl(priv->base + ICU_DMkSELy(dmac_index, y));
> +       icu_dmksely = (icu_dmksely & ~dmareq_mask) | dmareq;
> +       writel(icu_dmksely, priv->base + ICU_DMkSELy(dmac_index, y));
> +
> +       icu_dmackselk = readl(priv->base + ICU_DMACKSELk(k));
> +       icu_dmackselk = (icu_dmackselk & ~dmaack_mask) | dmaack;
> +       writel(icu_dmackselk, priv->base + ICU_DMACKSELk(k));
> +}
> +EXPORT_SYMBOL_GPL(rzv2h_icu_register_dma_req_ack);
> +
>  static inline struct rzv2h_icu_priv *irq_data_to_priv(struct irq_data *data)
>  {
>         return data->domain->host_data;

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds