diff mbox series

[v5] i2c: i2c-qcom-geni: Serve transfer during early resume stage

Message ID 20241227130236.755794-1-quic_msavaliy@quicinc.com
State New
Headers show
Series [v5] i2c: i2c-qcom-geni: Serve transfer during early resume stage | expand

Commit Message

Mukesh Kumar Savaliya Dec. 27, 2024, 1:02 p.m. UTC 
Fix the issue where pm_runtime_get_sync() fails during the PM early resume
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
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.

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@quicinc.com>
Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.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(-)

Comments

Mukesh Kumar Savaliya Jan. 3, 2025, 12:27 p.m. UTC | #1 
On 12/27/2024 11:51 PM, Bjorn Andersson wrote:
> 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".
> 
Sure Bjorn. I should consider problem statement to start with.
>> 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?
> 
It's like this : when touch interrupt comes, it goes to Touch driver and 
it directly starts reading via I2C. Hence, system is still resuming post 
receiving wakeup interrupt. Just wanted to explain, if you are fine with 
PCIe usecase, we can move ahead.
>> 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.
> 
Sure, started with problem statement.
> Read and follow https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> 
Done.
>>
>> 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@quicinc.com>
>> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.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))
Sure, Changed it.
> 
>> +		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?
> 
The debug log tells the usage_count when powered on during forced 
resume. since it's debug print, purpose is to catch anything wrong 
during issue time. It helps if usage_count is imbalanced because of any 
wrong.
>> +		#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.
> 
yes
> 
> 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()".
> 
Do you think we should use flag is_force_suspended from inside the 
function and accordingly act ? OR you mean to say this condition applied 
itself is wrong if force_suspend()/force_resume() not in pair ?
>> +		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
yes
> 2) !suspended => we invoked pm_runtime_force_resume() above, which
> invoked geni_i2c_runtime_resume(), which cleared suspended.
> 
yes
> Such spaghetti should be avoided, but if necessary need to be clearly
> documented, and #2 is completely ignored in the comment.
Sure, i thought by looking at code it would be clear to understand by 
looking at the conditions. The conditions are really required to serve 
early resume time transfer. So i think i should be adding comment 
clarifying it.
> 
> "else autosuspend the driver", that part you can obviously skip as it's
> that's exactly what the code below says.
sure.
> 
>> +		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().
> 
yes, i saw that but we don't get always things into pair that way. For 
that i see most of the drivers do use SET_NOIRQ_SYSTEM_SLEEP_PM_OPS().

> 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.
> 
I think there is also autosuspend timer being used inside 
force_resume(), so will not be back to back force_resume. You may 
correct me.
> 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().
> 
The way we intend to use is - direct i2c_xfer() function making the 
force_resume(), do transfer(), then keeping back to force_suspend().
Rest of the things should work by doing get_sync() and put_sync() as 
earlier.
> 
> Perhaps I'm misunderstanding something here?
> 
You may be right. I have reviewed other drivers also, most of the 
drivers are implementing SET_NOIRQ_SYSTEM_SLEEP_PM_OPS() and which in 
turn calls force_suspend() and force_resume().

Please let me know if that can be followed in this case too.
> 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
>>
>>
diff mbox series

Patch

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))
+		dev_dbg(dev, "Forced power ON, pm_usage_count: %d\n",
+			atomic_read(&dev->power.usage_count));
+		#endif
+		ret = pm_runtime_force_resume(dev);
+		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) {
+		ret = pm_runtime_force_suspend(dev);
+		#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",