Re: [PATCH v2 1/3] misc: fastrpc: Rename phys to dma_addr for clarity

["Arnd Bergmann" <arnd@arndb.de>]

On Wed, Oct 15, 2025, at 06:57, Kumari Pallavi wrote:
> Update all references of buf->phys and map->phys to buf->dma_addr and
> map->dma_addr to accurately represent that these fields store DMA
> addresses, not physical addresses. This change improves code clarity
> and aligns with kernel conventions for dma_addr_t usage.
>
> Signed-off-by: Kumari Pallavi <kumari.pallavi@oss.qualcomm.com>

Thanks for the update!

>  				&src_perms, &perm, 1);
>  			if (err) {
> -				dev_err(map->fl->sctx->dev, "Failed to assign memory phys 0x%llx 
> size 0x%llx err %d\n",
> -						map->phys, map->len, err);
> +				dev_err(map->fl->sctx->dev, "Failed to assign memory dma_addr 
> 0x%llx size 0x%llx err %d\n",
> +						map->dma_addr, map->len, err);
>  				return;

Note that using %llx is not a portable way to print a dma_addr_t,
you should use %pad instead even if your method works on all
arm64 configurations.

%pad requires passing the dma_addr_t variable by reference though.

> @@ -783,10 +783,10 @@ static int fastrpc_map_attach(struct fastrpc_user 
> *fl, int fd,
>  	map->table = table;
> 
>  	if (attr & FASTRPC_ATTR_SECUREMAP) {
> -		map->phys = sg_phys(map->table->sgl);
> +		map->dma_addr = sg_phys(map->table->sgl);
>  	} else {

This is technically still wrong, because sg_phys() returns
a phys_addr_t that is only the same as the dma_addr_t value
if there is no iommu or dma offset.

At the minimum, this requires a comment explaining what you
are doing here, and I would add a '(dma_addr_t)' cast as
well.

If possible, use sg_dma_address() instead of sg_phys() for
portability if they produce the same bit value.

> @@ -813,10 +813,10 @@ static int fastrpc_map_attach(struct fastrpc_user 
> *fl, int fd,
>  		dst_perms[1].vmid = fl->cctx->vmperms[0].vmid;
>  		dst_perms[1].perm = QCOM_SCM_PERM_RWX;
>  		map->attr = attr;
> -		err = qcom_scm_assign_mem(map->phys, (u64)map->len, &src_perms, 
> dst_perms, 2);
> +		err = qcom_scm_assign_mem(map->dma_addr, (u64)map->len, &src_perms, 

This one has the reverse problem, as qcom_scm_assign_mem() takes
a phys_addr_t instead of a dma_addr_t, again relying on the
absence of an iommu.

> dst_perms, 2);
>  		if (err) {
> -			dev_err(sess->dev, "Failed to assign memory with phys 0x%llx size 
> 0x%llx err %d\n",
> -					map->phys, map->len, err);
> +			dev_err(sess->dev, "Failed to assign memory with dma_addr 0x%llx 
> size 0x%llx err %d\n",
> +					map->dma_addr, map->len, err);
>  			goto map_err;

%pad

>  		}
>  	}
> @@ -1007,7 +1007,7 @@ static int fastrpc_get_args(u32 kernel, struct 
> fastrpc_invoke_ctx *ctx)
>  			struct vm_area_struct *vma = NULL;
> 
>  			rpra[i].buf.pv = (u64) ctx->args[i].ptr;
> -			pages[i].addr = ctx->maps[i]->phys;
> +			pages[i].addr = ctx->maps[i]->dma_addr;
> 

pages[i].addr is declared as

      "u64 addr;               /* physical address */"

I guess the other side of this is the same CPU in a different
exception level instead of an external device, right? This
could also use a clarification.

        Arnd