Re: [PATCH RFC 2/4] soc: qcom: Add modemsmem for Google phones

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Probe fails unconditionally if the SMEM item already exists, breaking module reload and pre-allocated items.
- [High] Newly allocated SMEM memory is not zeroed, exposing uninitialized data to the modem firmware.
--

commit 7a67ae0180630b09d3437d4fa514d468a7075572
Author: Richard Acayan <mailingradian@gmail.com>

soc: qcom: Add modemsmem for Google phones

Modems on Google Pixel devices require extra information about the
SoC to be passed through a vendor-controlled Shared Memory (SMEM)
channel. This driver provides this information.

> diff --git a/drivers/soc/qcom/modemsmem.c b/drivers/soc/qcom/modemsmem.c
> index 0000000000000..7a733422ff7c1
> --- /dev/null
> +++ b/drivers/soc/qcom/modemsmem.c
[ ... ]
> +static int write_socinfo(struct modem_smem_info *target)
> +{
> +	struct socinfo *socinfo;
> +	u32 plat_ver;
> +
> +	socinfo = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, NULL);
> +	if (IS_ERR(socinfo))
> +		return PTR_ERR(socinfo);
> +
> +	/* hw_plat_subtype was added in socinfo format 0.6 */
> +	if (le32_to_cpu(socinfo->fmt) < SOCINFO_VERSION(0, 6))
> +		return -EOPNOTSUPP;
> +
> +	plat_ver = le32_to_cpu(socinfo->plat_ver);
> +
> +	target->version = cpu_to_le32(MODEM_SMEM_VERSION);
> +	target->major_id = cpu_to_le32(PLAT_VER_TO_MAJOR_ID(plat_ver));
> +	target->minor_id = cpu_to_le32(PLAT_VER_TO_MINOR_ID(plat_ver));
> +	target->platform = socinfo->hw_plat;
> +	target->subtype = socinfo->hw_plat_subtype;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this code leave uninitialized fields in struct modem_smem_info exposed to
the modem firmware?

When the SMEM item is freshly allocated by qcom_smem_alloc(), the memory is
not zeroed. This function assigns 5 of the 8 fields in struct modem_smem_info,
leaving modem_flag, efs_magic, and ftm_magic uninitialized.

Could this SRAM garbage cause undefined modem behavior or misconfiguration?
Follow-up patches in the series do not fix this regression.

> +
> +	return 0;
> +}
> +
> +static int modemsmem_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct modem_smem_info *info;
> +	u32 smem_id;
> +	int ret;
> +
> +	ret = of_property_read_u32(dev->of_node, "qcom,smem-id", &smem_id);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Could not read smem id\n");
> +
> +	ret = qcom_smem_alloc(QCOM_SMEM_HOST_ANY, smem_id, sizeof(*info));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Could not allocate modem smem\n");
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Will this logic fail the probe process if the SMEM item already exists?

qcom_smem_alloc() returns -EEXIST when the item has already been allocated by
an earlier boot stage or during a previous module load.

Treating any non-zero return as a fatal error prevents the driver from updating
a pre-allocated SMEM region. Follow-up patches in the series do not fix this
regression.

> +
> +	info = qcom_smem_get(QCOM_SMEM_HOST_ANY, smem_id, NULL);
> +	if (IS_ERR(info))
> +		return dev_err_probe(dev, PTR_ERR(info), "Could not get modem smem\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906-modemsmem-v1-0-a3634b553841@ixit.cz?part=2