linux-kernel - Re: [PATCH v2 1/3] misc: fastrpc: Rename phys to dma

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <2e571b41-0006-4a37-9e3b-d333bf5eb7ed@app.fastmail.com>
Date: Wed, 15 Oct 2025 09:20:05 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Kumari Pallavi" <kumari.pallavi@....qualcomm.com>,
 kpallavi@....qualcomm.com, "Srinivas Kandagatla" <srini@...nel.org>,
 "Amol Maheshwari" <amahesh@....qualcomm.com>,
 "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>
Cc: quic_bkumar@...cinc.com, ekansh.gupta@....qualcomm.com,
 linux-kernel@...r.kernel.org, quic_chennak@...cinc.com,
 dri-devel@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org,
 "Jingyi Wang" <jingyi.wang@....qualcomm.com>, aiqun.yu@....qualcomm.com,
 ktadakam@....qualcomm.com
Subject: Re: [PATCH v2 1/3] misc: fastrpc: Rename phys to dma_addr for clarity

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