From: sashiko-bot@kernel.org
Subject: Re: [PATCHv2] dmaengine: fsl_raid: switch __b32 to u32
Reply-To: sashiko-reviews@lists.linux.dev
To: "Rosen Penev" <rosenp@gmail.com>
Cc: dmaengine@vger.kernel.org, Frank.Li@kernel.org, vkoul@kernel.org
In-Reply-To: <20260910194616.34760-1-rosenp@gmail.com>
References: <20260910194616.34760-1-rosenp@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Thu, 10 Sep 2026 20:00:45 +0000
Message-Id: <20260910200045.6C2781F000FF@smtp.kernel.org>
Precedence: bulk

Thank you for your contribution! Sashiko AI review found 4 potential issue(=
s) to consider:

Pre-existing issues:
- [High] The commit message claims that the CDB structures keep their `__be=
32` annotations because they are correctly written via `cpu_to_be32()`. How=
ever, the driver directly assigns native `u32` values to the `cdb32` fields=
 of the XOR, PQ, and MOVE structures without using `cpu_to_be32()`, violati=
ng the claim and leaving an endianness bug on little-endian architectures.
- [High] Lock held while invoking client DMA callback, causing a self-deadl=
ock if the client submits a new transaction.
- [High] Missing `free_irq()` leads to IRQ leak and potential use-after-fre=
e.
- [High] Erroneous bitwise logic for splitting ring base addresses causes s=
ilent truncation.
--

commit 7b9c559b3810d3491f22a6353857e63e9743b1e1
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: fsl_raid: switch __b32 to u32

The register maps in fsl_raid.h annotate every field as __be32, but
they are accessed only through ioread32be()/iowrite32be(), which
already perform the byte swap and take plain u32 __iomem pointers.
The __be32 annotation therefore makes sparse reject each access:

  drivers/dma/fsl_raid.c:116:17: sparse: incorrect type in argument 1
      (different base types) ... got restricted __be32 [noderef] __iomem *

Drop __be32 from struct fsl_re_ctrl and struct fsl_re_chan_cfg; the
descriptor/CDB structures written via cpu_to_be32() keep their
annotations.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202608200637.ro1CqMxi-lkp@int=
el.com/
Fixes: 68b7fbc23529 ("dma: fsl_raid: keep MMIO bases as void __iomem and ca=
st at access")
Assisted-by: opencode:deepseek-v4-flash-free
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>

> Drop __be32 from struct fsl_re_ctrl and struct fsl_re_chan_cfg; the
> descriptor/CDB structures written via cpu_to_be32() keep their
> annotations.

[Severity: High]
This isn't a bug introduced by this patch, but the commit message claims
that the CDB structures keep their __be32 annotations because they are
correctly written via cpu_to_be32().

Is this description accurate? Looking at fsl_re_prep_dma_genq(), the driver
directly assigns native u32 values to the cdb32 field of the XOR structure
without using cpu_to_be32():

drivers/dma/fsl_raid.c:fsl_re_prep_dma_genq() {
    ...
    xor->cdb32 =3D cdb;
    ...
}

Similar direct assignments occur in fsl_re_prep_dma_pq() for pq->cdb32
and fsl_re_prep_dma_memcpy() for move->cdb32.

Could this lead to the DMA hardware receiving a byteswapped command
descriptor block on little-endian architectures, causing it to misinterpret
opcodes and potentially corrupt memory?


[Severity: High]
This is a pre-existing issue, but can a self-deadlock occur if the client
callback submits a new transaction?

drivers/dma/fsl_raid.c:fsl_re_tx_submit() {
    ...
    spin_lock_irqsave(&re_chan->desc_lock, flags);
    ...
}

drivers/dma/fsl_raid.c:fsl_re_dequeue() {
    ...
    fsl_re_desc_done(desc);
    ...
}

If a client submits a new DMA transaction from the callback triggered by
fsl_re_desc_done() while re_chan->desc_lock is still held, fsl_re_tx_submit=
()
will unconditionally attempt to acquire the exact same lock again. Does this
result in a complete system hang?


[Severity: High]
This is a pre-existing issue, but does this code leak the IRQ and potential=
ly
cause a use-after-free?

drivers/dma/fsl_raid.c:fsl_re_chan_probe() {
    ...
    ret =3D request_irq(chan->irq, fsl_re_isr, 0, chan->name, chandev);
    ...
}

drivers/dma/fsl_raid.c:fsl_re_remove_chan() {
    ...
    tasklet_kill(&chan->irqtask);
    ...
}

The teardown function kills the tasklet but appears to miss the required
free_irq() call. If the module is unloaded or the device is unbound, and an
interrupt fires after the re_chan structure is freed, will the ISR
fsl_re_isr() dereference the freed pointer?


[Severity: High]
This is a pre-existing issue, but is the bitwise logic for splitting ring
base addresses correct here?

drivers/dma/fsl_raid.c:fsl_re_chan_probe() {
    ...
    out_be32(&chan->jrregs->inbring_base_h,
             chan->inb_phys_addr & FSL_RE_ADDR_BIT_MASK);
    out_be32(&chan->jrregs->oubring_base_h,
             chan->oub_phys_addr & FSL_RE_ADDR_BIT_MASK);
    out_be32(&chan->jrregs->inbring_base_l,
             chan->inb_phys_addr >> FSL_RE_ADDR_BIT_SHIFT);
    ...
}

This logic appears to use an incorrect mask and shift, truncating the high
bits and completely shifting out the low bits. Could this result in the DMA
rings being placed at the wrong physical memory locations, leading to
immediate DMA failure or memory corruption?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260910194616.3476=
0-1-rosenp@gmail.com?part=3D1