On Tue, Jan 27, 2026 at 10:18:38AM +0800, Xingjing Deng wrote: > I identified this issue through static program analysis. All other > callers of this function validate its return value, so I believe a > validation check should also be added here. >
I agree with your findings. Please drop the dev_err() and mention that you found this through static analysis in the commit message. Thank you, Bjorn > Bjorn Andersson <[email protected]> 于2026年1月27日周二 04:53写道： > > > > On Mon, Jan 26, 2026 at 04:24:55PM +0100, Greg KH wrote: > > > On Sat, Jan 17, 2026 at 10:03:51PM +0800, Xingjing Deng wrote: > > > > In the SDSP probe path, qcom_scm_assign_mem() is used to assign the > > > > reserved memory to the configured VMIDs, but its return value was not > > > > checked. > > > > > > > > Fail the probe if the SCM call fails to avoid continuing with an > > > > unexpected/incorrect memory permission configuration. > > > > > > > > The file has passed the check of checkpatch. > > > > > > > > Fixes: c3c0363bc72d4 ("misc: fastrpc: support complete DMA pool access > > > > to the DSP") > > > > Cc: [email protected] # 6.11-rc1 > > > > Signed-off-by: Xingjing Deng <[email protected]> > > > > --- > > > > v5: > > > > - Squash the functional change and indentation fix into a single patch. > > > > - Link to v4: > > > > https://lore.kernel.org/linux-arm-msm/2026011637-statute-showy-2c3f@gregkh/T/#t > > > > > > > > v4: > > > > - Format the indentation > > > > - Link to v3: > > > > https://lore.kernel.org/linux-arm-msm/[email protected]/T/#t > > > > > > > > v3: > > > > - Add missing [email protected] to cc list. > > > > - Standarlize changelog placement/format. > > > > - Link to v2: > > > > https://lore.kernel.org/linux-arm-msm/[email protected]/T/#t > > > > > > > > v2: > > > > - Add Fixes: and Cc: stable tags. > > > > - Link to v1: > > > > https://lore.kernel.org/linux-arm-msm/[email protected]/T/#u > > > > --- > > > > drivers/misc/fastrpc.c | 9 +++++++-- > > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > > > > index fb3b54e05928..d9650efa443f 100644 > > > > --- a/drivers/misc/fastrpc.c > > > > +++ b/drivers/misc/fastrpc.c > > > > @@ -2338,8 +2338,13 @@ static int fastrpc_rpmsg_probe(struct > > > > rpmsg_device *rpdev) > > > > if (!err) { > > > > src_perms = BIT(QCOM_SCM_VMID_HLOS); > > > > > > > > - qcom_scm_assign_mem(res.start, resource_size(&res), > > > > &src_perms, > > > > - data->vmperms, data->vmcount); > > > > + err = qcom_scm_assign_mem(res.start, > > > > resource_size(&res), &src_perms, > > > > + data->vmperms, data->vmcount); > > > > + if (err) { > > > > + dev_err(rdev, "Failed to assign memory phys > > > > 0x%llx size 0x%llx err %d", > > > > + res.start, resource_size(&res), err); > > > > > > Shouldn't the caller function report the error? > > > > > > > That is correct, all codepaths through qcom_scm_assign_mem() will either > > be -ENOMEM or print an error message, so we shouldn't print yet another > > message in the log here. > > > > (The usefulness of the error message in qcom_scm_assign_mem() could > > certainly be improved, but that's a separate matter/patch). > > > > > How as this found and tested? > > > > > > > Looking forward to Xingjing's answer here. > > > > But failing to handle errors here means that we're ignoring the failure > > to map memory to the DSP, which will fail us later. So, that part is > > correct. Exiting through err_free_data looks good as well. > > > > Regards, > > Bjorn > > > > > thanks, > > > > > > greg k-h > > >
