Re: [PATCH v2 2/4] spi: qcom-geni: Use geni_se_resources_init() for resource initialization

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Unchecked ERR_PTR for se->clk on ACPI platforms causes kernel panic in clk_round_rate().

Pre-existing issues:
- [High] Missing return value check for pm_runtime_get_sync() in spi_geni_init() leads to unclocked hardware access.
--

commit c69b8f64e97b3440b056b6b346311b9f4e470629
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

spi: qcom-geni: Use geni_se_resources_init() for resource initialization

Replace resources initialization such as clocks, ICC path and OPP with the
common geni_se_resources_init() function to avoid code duplication across
all drivers.

The geni_se_resources_init() function handles all these resources
internally, reducing code duplication and ensuring consistent resource
management across GENI SE drivers.

> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index a04cdc1e5ad4d..333216df922e8 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -1049,10 +1048,6 @@ static int spi_geni_probe(struct platform_device *pdev)
>  	if (IS_ERR(base))
>  		return PTR_ERR(base);
>  
> -	clk = devm_clk_get(dev, "se");
> -	if (IS_ERR(clk))
> -		return PTR_ERR(clk);
> -
>  	if (device_property_read_bool(dev, "spi-slave"))
>  		spi = devm_spi_alloc_target(dev, sizeof(*mas));
>  	else
> @@ -1068,17 +1063,10 @@ static int spi_geni_probe(struct platform_device *pdev)
>  	mas->se.dev = dev;
>  	mas->se.wrapper = dev_get_drvdata(dev->parent);
>  	mas->se.base = base;
> -	mas->se.clk = clk;
>  
> -	ret = devm_pm_opp_set_clkname(&pdev->dev, "se");
> +	ret = geni_se_resources_init(&mas->se);

[Severity: High]
Does this change introduce a potential kernel panic on ACPI platforms?

By switching to geni_se_resources_init(), the driver now tolerates a missing
se->clk on ACPI platforms (leaving se->clk as an ERR_PTR):

drivers/soc/qcom/qcom-geni-se.c:geni_se_resources_init() {
    ...
    se->clk = devm_clk_get(se->dev, "se");
    if (IS_ERR(se->clk) && !has_acpi_companion(se->dev))
        return dev_err_probe(...);
    ...
}

However, the driver unconditionally triggers frequency matching later, which
evaluates the ERR_PTR in clk_round_rate():

drivers/spi/spi-geni-qcom.c:get_spi_clk_cfg() {
    ...
    ret = geni_se_clk_freq_match(&mas->se, speed_hz * mas->oversampling,
                                 clk_idx, &sclk_freq, false);
    ...
}

Because clk_round_rate() only checks for NULL and not IS_ERR(), it will
dereference the error pointer and cause a panic. Prior to this patch, the
driver correctly aborted probing if the clock was missing.

>  	if (ret)
>  		return ret;
> -	/* OPP table is optional */
> -	ret = devm_pm_opp_of_add_table(&pdev->dev);
> -	if (ret && ret != -ENODEV) {
> -		dev_err(&pdev->dev, "invalid OPP table in device tree\n");
> -		return ret;
> -	}

[ ... ]

> @@ -1104,24 +1092,12 @@ static int spi_geni_probe(struct platform_device *pdev)
>  	if (spi->target)
>  		spi->target_abort = spi_geni_target_abort;
>  
> -	ret = geni_icc_get(&mas->se, NULL);
> -	if (ret)
> -		return ret;
> -
>  	pm_runtime_use_autosuspend(&pdev->dev);
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, 250);
>  	ret = devm_pm_runtime_enable(dev);
>  	if (ret)
>  		return ret;
>  
> -	/* Set the bus quota to a reasonable value for register access */
> -	mas->se.icc_paths[GENI_TO_CORE].avg_bw = Bps_to_icc(CORE_2X_50_MHZ);
> -	mas->se.icc_paths[CPU_TO_GENI].avg_bw = GENI_DEFAULT_BW;
> -
> -	ret = geni_icc_set_bw(&mas->se);
> -	if (ret)
> -		return ret;
> -
>  	ret = spi_geni_init(mas);

[Severity: High]
This is a pre-existing issue, but does spi_geni_init() need to check the
return value of pm_runtime_get_sync()?

Looking at spi_geni_init(), it accesses hardware registers immediately after
calling pm_runtime_get_sync() without checking for failure:

drivers/spi/spi-geni-qcom.c:spi_geni_init() {
    ...
    pm_runtime_get_sync(mas->dev);

    proto = geni_se_read_proto(se);
    ...
}

If the runtime resume fails (for example, due to power domain or clock
issues), the device hardware remains unpowered. The geni_se_read_proto() call
performs a memory-mapped I/O read. Reading from an unclocked or unpowered
register on ARM systems can trigger a synchronous external abort (SError),
crashing the entire system.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530-enable-spi-on-sa8255p-v2-0-17574601bd63@oss.qualcomm.com?part=2