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

[Kumari Pallavi]

On 10/15/2025 12:50 PM, Arnd Bergmann wrote:
> 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@xxxxxxxxxxxxxxxx>
>
> 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.

Ack.

> > @@ -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.

Based on historical behavior, when the FASTRPC_ATTR_SECUREMAP flag is 
set, S2 mapping expects a physical address to be passed to the 
qcom_scm_assign_mem() API.
reference- 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/misc/fastrpc.c?id=e90d911906196bf987492c94e38f10ca611dfd7b

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

To ensure clarity, i will add the comment. Adding '(dma_addr_t)' cast
result in incorrect behavior due to potential offsets.

> 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

Ack

> >   		}
> >   	}
> > @@ -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.

Ack

>          Arnd