Re: [PATCH 2/2] phy: qcom-snps-femto-v2: Add support for SA8255P

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect power domain enablement and teardown ordering in d3 <--> d0 transitions.
- [Medium] The `sa8255p_snps_hs_phy` match data is defined as a scalar struct but iterated over as an array, risking out-of-bounds memory access.
--

--- Patch [2]: [PATCH 2/2] phy: qcom-snps-femto-v2: Add support for SA8255P ---
commit 337a4b6fa46b42e255f6aa6b19e2b52e65e59f7d
Author: Sriram Dash <quic_sriramd@quicinc.com>

phy: qcom-snps-femto-v2: Add support for SA8255P

The Qualcomm automotive SA8255p SoC relies on firmware to configure
platform resources, including clocks, interconnects and TLMM.
The driver requests resources operations over SCMI using power protocols.

> diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> index eb0b0f61d98e0..4a9bd3ae0e535 100644
> --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c

[ ... ]

> @@ -136,8 +139,81 @@ struct qcom_snps_hsphy {
> +/* d3_to_d0 transition by turning on all the suppliers */
> +static int qcom_snps_d3_to_d0(struct qcom_snps_hsphy *hsphy)
> +{
> +	int ret;
> +
> +	if (!hsphy->pd_list)
> +		return 0;
> +
> +	ret = pm_runtime_resume_and_get(hsphy->pd_list->pd_devs[0]);
> +	if (ret)
> +		return ret;
> +
> +	ret = pm_runtime_resume_and_get(hsphy->pd_list->pd_devs[1]);
> +	if (ret)
> +		pm_runtime_put_sync(hsphy->pd_list->pd_devs[0]);
> +
> +	return ret;
> +}

[Severity: High]
Does this resume the power domains in the correct order?

The commit message states that the power domain 'core' (pd_devs[1]) controls
the GDSC, and 'transfer' (pd_devs[0]) controls clocks and interconnects.
Resuming 'transfer' before 'core' means clocks are enabled without their
powering GDSC, which could cause bus hangs on Qualcomm SoCs.

Additionally, since state d1 has 'transfer' OFF and 'core' ON, shouldn't 'core'
always be enabled first and disabled last?

> +
> +/* d0_to_d3 transition by turning off all the suppliers */
> +static void qcom_snps_d0_to_d3(struct qcom_snps_hsphy *hsphy)
> +{
> +	if (!hsphy->pd_list)
> +		return;
> +
> +	pm_runtime_put_sync(hsphy->pd_list->pd_devs[1]);
> +	pm_runtime_put_sync(hsphy->pd_list->pd_devs[0]);
> +}

[Severity: High]
Should the 'transfer' domain (pd_devs[0]) be put before the 'core'
domain (pd_devs[1])?

Putting 'core' before 'transfer' disables the GDSC while the clocks are
still active.

[ ... ]

> @@ -316,6 +394,10 @@ static const struct override_param ls_fs_output_impedance_sc7280[] = {
>  	{ 1310, 0 },
>  };
>  
> +static const struct override_param_map sa8255p_snps_hs_phy = {
> +	.fw_managed = true,
> +};
> +

[Severity: Medium]
Will this scalar struct cause an out-of-bounds read in
qcom_snps_hsphy_read_override_param_seq()?

The match data is defined here as a single scalar struct, but
qcom_snps_hsphy_read_override_param_seq() casts the match data to an array and
iterates over it until cfg[i].prop_name != NULL:

drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c:qcom_snps_hsphy_read_override_param_seq() {
    ...
	cfg = of_device_get_match_data(dev);
	if (!cfg)
		return;

	for (i = 0; cfg[i].prop_name != NULL; i++) {
    ...
}

Currently this terminates on the first iteration because the uninitialized
.prop_name field is implicitly NULL, but if a property is ever added to this
struct in the future, it will read past the end of the struct into adjacent
memory.

Should this be defined as an array with an empty sentinel element like the
other param maps?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-phy-qcom-snps-femto-v2-8255-v1-0-8b8bf5105a68@kernel.org?part=2

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy