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

[Kumari Pallavi <kumari.pallavi@oss.qualcomm.com>]

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

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