Re: [PATCHv2 2/2] dmaengine: idma64: use sg_nents_for_dma to respect hardware descriptor length limit

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Sun, Jul 12, 2026 at 03:00:39PM -0700, Rosen Penev wrote:
> The iDMA 64-bit hardware has a 17-bit block transfer size field in the
> CTL_HI register (IDMA64C_CTLH_BLOCK_TS_MASK = 0x1ffff). When a
> scatterlist entry exceeds this limit, the driver would silently
> truncate the length, transferring fewer bytes than intended.
> 
> Use sg_nents_for_dma() to compute the number of hardware descriptors
> needed after splitting large SG entries into chunks that fit within
> the hardware limit. Split the loop to iterate over each chunk.

I appreciate the intention, but... I have issues with the implementation.

> Assisted-by: opencode:big-pickle
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/dma/idma64.c | 44 ++++++++++++++++++++++++++++++--------------
>  drivers/dma/idma64.h |  3 ++-
>  2 files changed, 32 insertions(+), 15 deletions(-)

First of all, the statistics and code readability. Next is the requirement
for drivers to do that. This should be done on the DMAengine core level
for all, this is software resplit and we just need a driver agreement of
getting a such to be done before handing over to the driver's callback.

OTOH, there is an API to get DMA maximum segment size (note, that your split
is incorrect since the BLOCK_TS is in "bus width" units, it may be up to 4
or 8 bytes and it depends on the alignment: so, in this form this patch is
no go). The consumer drivers should actually call it before preparing SG
list to make sure that resplit is not needed and the SG list is compatible
with what HW capable of.

See dma_set_max_seg_size() and dma_get_max_seg_size().

...

>  #ifndef __DMA_IDMA64_H__
>  #define __DMA_IDMA64_H__
>  
> +#include <linux/bits.h>
>  #include <linux/device.h>
>  #include <linux/io.h>
>  #include <linux/spinlock.h>

Yeah, the inclusions should be revisit as many changes happened after the
driver introduction in the area of how we split headers.

...

>  /* Bitfields in CTL_HI */
> -#define IDMA64C_CTLH_BLOCK_TS_MASK	((1 << 17) - 1)
> +#define IDMA64C_CTLH_BLOCK_TS_MASK	GENMASK_U32(16, 0)

Why? I think this becomes inconsistent. If you want to switch to bits.h, make
it in a separate patch for all eligible definitions.

-- 
With Best Regards,
Andy Shevchenko