Message-ID: <aihmuvxxop7ipbsfzblbmywthxnklx34x2xlbvlrd4rio6rg77@gfqnzyjjewz3>
Date: Fri, 27 Dec 2024 12:21:35 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
Cc: andi.shyti@...nel.org, linux-arm-msm@...r.kernel.org, 
	linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org, quic_bjorande@...cinc.com, 
	konrad.dybcio@...aro.org, quic_vdadhani@...cinc.com, vkoul@...nel.org
Subject: Re: [PATCH v5] i2c: i2c-qcom-geni: Serve transfer during early
 resume stage

On Fri, Dec 27, 2024 at 06:32:36PM +0530, Mukesh Kumar Savaliya wrote:
> Fix the issue where pm_runtime_get_sync() fails during the PM early resume

No.

As we discussed, start your commit messages with a description of your
problem. Don't say "Fix bla bla because of problem description", say
"The QPS615 PCIe switch needs to be configured over I2C before the PCIe
link is restored, but the I2C geni driver does not allow transfers
during the early resume phase".

> phase, returning -EACCES because runtime PM for the device is disabled at
> this stage. This failure causes I2C transfers to fail. To resolve this,
> serve transfers with a forced resume.
> 
> Allow certain I2C clients, such as PCI or Touch devices, to request I2C

I still don't understand what kind of touch device you have attached
that requires I2C communication before the system is resumed. Who are
you going to deliver the input events to at this point in time?

> transfers during the early resume stage. Enable any I2C client to initiate
> a transfer request very early in the resume stage, such as during the
> noirq phase of PM. Register an interrupt with the IRQF_EARLY_RESUME and
> IRQF_NO_SUSPEND flags to handle these transfers and avoid timeouts when
> IRQs are not enabled during the early stage.
> 
> A Potential usecase: PCIe client -> PCIe Driver -> I2C driver. It involves
> a PCIe client driver communicating with the PCIe driver, which in turn
> interfaces with the I2C driver. Upon powering on the PCIe device, send
> certain configurations over I2C. During the suspend phase, use the
> suspend_noirq() routine to turn off the PCIe device, as some PCIe clients
> continue transfers until this phase. During the resume_noirq() phase,
> restore power to the PCIe device and reconfigure it via I2C. This ensures
> that the PCIe device is properly configured and operational after resuming
> from a suspended state.

This paragraph looks like a problem description, it should be at the
top.

Read and follow https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

> 
> Use pm_runtime_force_resume() to address the failure of
> pm_runtime_get_sync() returning a negative value when runtime PM is
> disabled.
> 
> Co-developed-by: Viken Dadhaniya <quic_vdadhani@...cinc.com>
> Signed-off-by: Viken Dadhaniya <quic_vdadhani@...cinc.com>
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
> ---
> Link to V4: https://lore.kernel.org/lkml/bd699719-4958-445a-a685-4b5f6a8ad81f@quicinc.com/
> 
> v4->v5:
> - Commit log enhanced considering Bjorn's comments by explaining PCIe usecase.
> - Enhanced comment with reason when using pm_runtime_force_resume().
> - Corrected IS_ENABLED(CONFIG_PM) condition inside geni_i2c_xfer().
> - Improved debug log as per Bjorn's suggestions during suspend, resume.
> - Reverted back comment before devm_request_irq().
> 
> ---
> Link to V3: https://lore.kernel.org/all/20241119143031.3331753-1-quic_msavaliy@quicinc.com/T/
> 
> v3->v4 :
>  - Enhanced commit log by explaining client usecase scenario during early resume.
>  - Covered 'usage_count' of 'struct dev_pm_info' under CONFIG_PM to compile non PM CONFIG.
> 
> ---
> Link to V2: https://lore.kernel.org/lkml/202410132233.P25W2vKq-lkp@intel.com/T/
> 
>  v2 -> v3:
>  - Updated exact usecase and scenario in the commit log description.
>  - Removed bulleted points from technical description, added details in free flow.
>  - Used pm_runtime_force_resume/suspend() instead customized local implementation.
>  - Added debug log after pm_runtime_force_suspend().
> 
> ---
> 
>  v1 -> v2:
>  - Changed gi2c->se.dev to dev during dev_dbg() calls.
>  - Addressed review comments from Andi and Bjorn.
>  - Returned 0 instead garbage inside geni_i2c_force_resume().
>  - Added comments explaining forced resume transfer when runtime PM
>    remains disabled.
> ---
> 
>     V1 link: https://patches.linaro.org/project/linux-i2c/patch/20240328123743.1713696-1-quic_msavaliy@quicinc.com/
> ---
> ---
>  drivers/i2c/busses/i2c-qcom-geni.c | 47 +++++++++++++++++++++++-------
>  1 file changed, 37 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 7a22e1f46e60..1885e1ceb11c 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -695,17 +695,31 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
>  			 int num)
>  {
>  	struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap);
> +	struct device *dev = gi2c->se.dev;
>  	int ret;
>  
>  	gi2c->err = 0;
>  	reinit_completion(&gi2c->done);
> -	ret = pm_runtime_get_sync(gi2c->se.dev);
> -	if (ret < 0) {
> -		dev_err(gi2c->se.dev, "error turning SE resources:%d\n", ret);
> -		pm_runtime_put_noidle(gi2c->se.dev);
> -		/* Set device in suspended since resume failed */
> -		pm_runtime_set_suspended(gi2c->se.dev);
> -		return ret;
> +	/* During early resume stage, runtime PM is disabled and pm_runtime_get_sync()
> +	 * returns error Hence use force_resume() and serve transfer.
> +	 */
> +	if (!pm_runtime_enabled(dev) && gi2c->suspended) {
> +		#if (!IS_ENABLED(CONFIG_PM))

Per Documentation/process/coding-style.rst section 21, this should be:
	if (!IS_ENABLED(CONFIG_PM))

> +		dev_dbg(dev, "Forced power ON, pm_usage_count: %d\n",
> +			atomic_read(&dev->power.usage_count));

What does this debug print tell you? Shouldn't usage_count be zero in
all cases where suspended = true?

> +		#endif
> +		ret = pm_runtime_force_resume(dev);

If I understand the implementation of this function, in the case that
you did not invoke pm_runtime_force_suspend() this will solely invoke
pm_runtime_enable(), not actually invoking any callbacks and hence
wouldn't turn any resources on.

Which makes sense given that this isn't the documented intended usage of
the function pair.


In fact, the kernel-doc even says "Prior invoking this function we
expect the user to have brought the device into low power state by a
call to pm_runtime_force_suspend()".

> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = pm_runtime_get_sync(gi2c->se.dev);
> +		if (ret < 0) {
> +			dev_err(gi2c->se.dev, "Error turning resources: %d\n", ret);
> +			pm_runtime_put_noidle(gi2c->se.dev);
> +			/* Set device in suspended since resume failed */
> +			pm_runtime_set_suspended(gi2c->se.dev);
> +			return ret;
> +		}
>  	}
>  
>  	qcom_geni_i2c_conf(gi2c);
> @@ -715,8 +729,20 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
>  	else
>  		ret = geni_i2c_fifo_xfer(gi2c, msgs, num);
>  
> -	pm_runtime_mark_last_busy(gi2c->se.dev);
> -	pm_runtime_put_autosuspend(gi2c->se.dev);
> +	/* if Runtime PM is disabled, do force_suspend() else autosuspend the driver */
> +	if (!pm_runtime_enabled(dev) && !gi2c->suspended) {

I find the asymmetry here suspicious, but I presume what you're checking is:
1) !pm_runtime_enabled() => we're probably in late-suspend or early-resume
2) !suspended => we invoked pm_runtime_force_resume() above, which
invoked geni_i2c_runtime_resume(), which cleared suspended.

Such spaghetti should be avoided, but if necessary need to be clearly
documented, and #2 is completely ignored in the comment.

"else autosuspend the driver", that part you can obviously skip as it's
that's exactly what the code below says.

> +		ret = pm_runtime_force_suspend(dev);

The expectation expressed in the kernel-doc of these functions is that
pm_runtime_force_suspend() should be followed by a
pm_runtime_force_resume().

As you don't follow that, most of pm_runtime_force_resume() will be
skipped, but here the full pm_runtime_force_suspend() will happen.

In the event that this is the last i2c_xfer before the system is
entering the resume-phase, you will leave the device in force-suspend
and then resume it "normally" on the next i2c_xfer. 

If you instead make another i2c_xfer within the early resume, the above
pm_runtime_force_resume() will suddenly behave completely differently -
it will actually invoke geni_i2c_runtime_resume().


Perhaps I'm misunderstanding something here?

Regards,
Bjorn

> +		#if (!IS_ENABLED(CONFIG_PM))
> +		dev_dbg(dev, "Forced power OFF, pm_usage_count: %d\n",
> +			atomic_read(&dev->power.usage_count));
> +		#endif
> +		if (ret)
> +			return ret;
> +	} else {
> +		pm_runtime_mark_last_busy(gi2c->se.dev);
> +		pm_runtime_put_autosuspend(gi2c->se.dev);
> +	}
> +
>  	gi2c->cur = NULL;
>  	gi2c->err = 0;
>  	return ret;
> @@ -835,7 +861,8 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, gi2c);
>  
>  	/* Keep interrupts disabled initially to allow for low-power modes */
> -	ret = devm_request_irq(dev, gi2c->irq, geni_i2c_irq, IRQF_NO_AUTOEN,
> +	ret = devm_request_irq(dev, gi2c->irq, geni_i2c_irq,
> +			       IRQF_NO_AUTOEN | IRQF_EARLY_RESUME | IRQF_NO_SUSPEND,
>  			       dev_name(dev), gi2c);
>  	if (ret) {
>  		dev_err(dev, "Request_irq failed:%d: err:%d\n",
> -- 
> 2.25.1
> 
> 

Powered by blists - more mailing lists