Re: [PATCH v7 2/6] i2c: qcom-geni: Isolate serial engine setup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Asynchronous pm_runtime_put() followed immediately by pm_runtime_disable() leaves hardware permanently powered on during probe failures.
- [Medium] DMA channels allocated during hardware initialization are permanently leaked if I2C adapter registration fails.
--

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

i2c: qcom-geni: Isolate serial engine setup
    
Moving the serial engine setup to geni_i2c_init() API for a cleaner
probe function and utilizes the PM runtime API to control resources
instead of direct clock-related APIs for better resource management.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index d2f5055b0b108..3ee0be228d7c1 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -990,10 +990,77 @@ static int setup_gpi_dma(struct geni_i2c_dev *gi2c)
>  	return ret;
>  }
>  
> +static int geni_i2c_init(struct geni_i2c_dev *gi2c)
> +{
> +	const struct geni_i2c_desc *desc = NULL;
> +	u32 proto, tx_depth;
> +	bool fifo_disable;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(gi2c->se.dev);

[ ... ]

> +	if (fifo_disable) {
> +		/* FIFO is disabled, so we can only use GPI DMA */
> +		gi2c->gpi_mode = true;
> +		ret = setup_gpi_dma(gi2c);
> +		if (ret)
> +			goto err;

[ ... ]

> +err:
> +	pm_runtime_put(gi2c->se.dev);
> +	return ret;
> +}
> +
>  static int geni_i2c_probe(struct platform_device *pdev)
>  {

[ ... ]

>  	pm_runtime_set_suspended(gi2c->se.dev);
>  	pm_runtime_set_autosuspend_delay(gi2c->se.dev, I2C_AUTO_SUSPEND_DELAY);
>  	pm_runtime_use_autosuspend(gi2c->se.dev);
>  	pm_runtime_enable(gi2c->se.dev);
>  
> +	ret = geni_i2c_init(gi2c);
> +	if (ret < 0) {
> +		pm_runtime_disable(gi2c->se.dev);

[Severity: Medium]
If geni_i2c_init() fails, it drops its runtime PM reference using the
asynchronous pm_runtime_put(). Will calling pm_runtime_disable() right
afterwards cancel the pending suspend work, leaving the device in the
RPM_ACTIVE state and leaking power resources?

> +		return ret;
> +	}
> +
>  	ret = i2c_add_adapter(&gi2c->adap);
>  	if (ret) {
>  		dev_err_probe(dev, ret, "Error adding i2c adapter\n");
>  		pm_runtime_disable(gi2c->se.dev);
> -		goto err_dma;
> +		return ret;

[Severity: Medium]
Does this code leak the DMA channels if i2c_add_adapter() fails?

Before this patch, a failure here would jump to err_dma which called
release_gpi_dma(gi2c). Now it returns directly, but geni_i2c_init()
may have successfully allocated the DMA channels via setup_gpi_dma().

Additionally, similar to the check above, will pm_runtime_disable() cancel
the async suspend from the successful geni_i2c_init() path, leaving the
hardware powered on?

>  	}
>  
>  	dev_dbg(dev, "Geni-I2C adaptor successfully added\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-enable-i2c-on-sa8255p-v7-0-ad736dbeab57@oss.qualcomm.com?part=2