[v2] i2c: mediatek: Move suspend and resume handling to NOIRQ phase

[v2] i2c: mediatek: Move suspend and resume handling to NOIRQ phase

[qii.wang]

From: Qii Wang <qii.wang@mediatek.com>

Some i2c device driver indirectly uses I2C driver when it is now
being suspended. The i2c devices driver is suspended during the
NOIRQ phase and this cannot be changed due to other dependencies.
Therefore, we also need to move the suspend handling for the I2C
controller driver to the NOIRQ phase as well.

Signed-off-by: Qii Wang <qii.wang@mediatek.com>
---

Changes in v2:
	- fix the wrong spelling medaitek to mediatek 

 drivers/i2c/busses/i2c-mt65xx.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index 33de99b..6f61595 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -1258,7 +1258,8 @@ static int mtk_i2c_probe(struct platform_device *pdev)
 	mtk_i2c_clock_disable(i2c);
 
 	ret = devm_request_irq(&pdev->dev, irq, mtk_i2c_irq,
-			       IRQF_TRIGGER_NONE, I2C_DRV_NAME, i2c);
+			       IRQF_NO_SUSPEND | IRQF_TRIGGER_NONE,
+			       I2C_DRV_NAME, i2c);
 	if (ret < 0) {
 		dev_err(&pdev->dev,
 			"Request I2C IRQ %d fail\n", irq);
@@ -1285,7 +1286,16 @@ static int mtk_i2c_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM_SLEEP
-static int mtk_i2c_resume(struct device *dev)
+static int mtk_i2c_suspend_noirq(struct device *dev)
+{
+	struct mtk_i2c *i2c = dev_get_drvdata(dev);
+
+	i2c_mark_adapter_suspended(&i2c->adap);
+
+	return 0;
+}
+
+static int mtk_i2c_resume_noirq(struct device *dev)
 {
 	int ret;
 	struct mtk_i2c *i2c = dev_get_drvdata(dev);
@@ -1300,12 +1310,15 @@ static int mtk_i2c_resume(struct device *dev)
 
 	mtk_i2c_clock_disable(i2c);
 
+	i2c_mark_adapter_resumed(&i2c->adap);
+
 	return 0;
 }
 #endif
 
 static const struct dev_pm_ops mtk_i2c_pm = {
-	SET_SYSTEM_SLEEP_PM_OPS(NULL, mtk_i2c_resume)
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_i2c_suspend_noirq,
+				      mtk_i2c_resume_noirq)
 };
 
 static struct platform_driver mtk_i2c_driver = {
-- 
1.9.1

Re: [v2] i2c: mediatek: Move suspend and resume handling to NOIRQ phase

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 480 bytes --]

Hi,

> Some i2c device driver indirectly uses I2C driver when it is now
> being suspended. The i2c devices driver is suspended during the
> NOIRQ phase and this cannot be changed due to other dependencies.
> Therefore, we also need to move the suspend handling for the I2C
> controller driver to the NOIRQ phase as well.
> 
> Signed-off-by: Qii Wang <qii.wang@mediatek.com>

Is this a bugfix and should go into 5.10? Or can it wait for 5.11?

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [v2] i2c: mediatek: Move suspend and resume handling to NOIRQ phase

[Qii Wang]

On Wed, 2020-12-02 at 16:35 +0100, Wolfram Sang wrote:
> Hi,
> 
> > Some i2c device driver indirectly uses I2C driver when it is now
> > being suspended. The i2c devices driver is suspended during the
> > NOIRQ phase and this cannot be changed due to other dependencies.
> > Therefore, we also need to move the suspend handling for the I2C
> > controller driver to the NOIRQ phase as well.
> > 
> > Signed-off-by: Qii Wang <qii.wang@mediatek.com>
> 
> Is this a bugfix and should go into 5.10? Or can it wait for 5.11?
> 

Yes, Can you help to apply it into 5.10? Thanks

> Thanks,
> 
>    Wolfram
> 

Re: [v2] i2c: mediatek: Move suspend and resume handling to NOIRQ phase

[Grygorii Strashko]

On 03/12/2020 03:25, Qii Wang wrote:
> On Wed, 2020-12-02 at 16:35 +0100, Wolfram Sang wrote:
>> Hi,
>>
>>> Some i2c device driver indirectly uses I2C driver when it is now
>>> being suspended. The i2c devices driver is suspended during the
>>> NOIRQ phase and this cannot be changed due to other dependencies.
>>> Therefore, we also need to move the suspend handling for the I2C
>>> controller driver to the NOIRQ phase as well.
>>>
>>> Signed-off-by: Qii Wang <qii.wang@mediatek.com>
>>
>> Is this a bugfix and should go into 5.10? Or can it wait for 5.11?
>>
> 
> Yes, Can you help to apply it into 5.10? Thanks

To be honest if you still do have any i2c device which accessing i2c buss after _noirq
stage and your driver does not implement .master_xfer_atomic() - you definitely have a bigger problem.
So adding IRQF_NO_SUSPEND sound like a hack and probably works just by luck.


-- 
Best regards,
grygorii

Re: [v2] i2c: mediatek: Move suspend and resume handling to NOIRQ phase

[Qii Wang]

Hi:
Thank you very much for your patience review.
There are two main purposes of this patch:
1.i2c_mark_adapter_suspended&i2c_mark_adapter_resumed
Avoid accessing the adapter while it is suspended by marking it
suspended during suspend.  This allows the I2C core to catch this, and
print a warning.
https://patchwork.kernel.org/project/linux-arm-kernel/patch/20181219164827.20985-2-wsa+renesas@sang-engineering.com/

2. IRQF_NO_SUSPEND.
Having interrupts disabled means not only that an interrupt will not
occur at an awkward time, but also that using any functionality that
requires interrupts will not work. So if the driver uses an I2C bus or
similar to tell the device to turn off, and if the I2C bus uses
interrupts to indicate completion (which is normal), then either the
device must be powered-off in suspend_late, so the I2C interrupt must be
marked IRQF_NO_SUSPEND.
https://patchwork.kernel.org/project/linux-acpi/patch/20180923135812.29574-8-hdegoede@redhat.com/

On Thu, 2020-12-03 at 10:01 +0200, Grygorii Strashko wrote:
> 
> On 03/12/2020 03:25, Qii Wang wrote:
> > On Wed, 2020-12-02 at 16:35 +0100, Wolfram Sang wrote:
> >> Hi,
> >>
> >>> Some i2c device driver indirectly uses I2C driver when it is now
> >>> being suspended. The i2c devices driver is suspended during the
> >>> NOIRQ phase and this cannot be changed due to other dependencies.
> >>> Therefore, we also need to move the suspend handling for the I2C
> >>> controller driver to the NOIRQ phase as well.
> >>>
> >>> Signed-off-by: Qii Wang <qii.wang@mediatek.com>
> >>
> >> Is this a bugfix and should go into 5.10? Or can it wait for 5.11?
> >>
> > 
> > Yes, Can you help to apply it into 5.10? Thanks
> 
> To be honest if you still do have any i2c device which accessing i2c buss after _noirq
> stage and your driver does not implement .master_xfer_atomic() - you definitely have a bigger problem.
> So adding IRQF_NO_SUSPEND sound like a hack and probably works just by luck.
> 

At present, it is only a problem caused by missing interrupts,
and .master_xfer_atomic() just a implement in polling mode. Why not set
the interrupt to a state that can always be triggered?

Re: [v2] i2c: mediatek: Move suspend and resume handling to NOIRQ phase

[Grygorii Strashko]

On 07/12/2020 09:33, Qii Wang wrote:
> Hi:
> Thank you very much for your patience review.
> There are two main purposes of this patch:
> 1.i2c_mark_adapter_suspended&i2c_mark_adapter_resumed
> Avoid accessing the adapter while it is suspended by marking it
> suspended during suspend.  This allows the I2C core to catch this, and
> print a warning.
> https://patchwork.kernel.org/project/linux-arm-kernel/patch/20181219164827.20985-2-wsa+renesas@sang-engineering.com/
> 
> 2. IRQF_NO_SUSPEND.
> Having interrupts disabled means not only that an interrupt will not
> occur at an awkward time, but also that using any functionality that
> requires interrupts will not work. So if the driver uses an I2C bus or
> similar to tell the device to turn off, and if the I2C bus uses
> interrupts to indicate completion (which is normal), then either the
> device must be powered-off in suspend_late, so the I2C interrupt must be
> marked IRQF_NO_SUSPEND.
> https://patchwork.kernel.org/project/linux-acpi/patch/20180923135812.29574-8-hdegoede@redhat.com/
> 

Pls, do not top post.

> 
> On Thu, 2020-12-03 at 10:01 +0200, Grygorii Strashko wrote:
>>
>> On 03/12/2020 03:25, Qii Wang wrote:
>>> On Wed, 2020-12-02 at 16:35 +0100, Wolfram Sang wrote:
>>>> Hi,
>>>>
>>>>> Some i2c device driver indirectly uses I2C driver when it is now
>>>>> being suspended. The i2c devices driver is suspended during the
>>>>> NOIRQ phase and this cannot be changed due to other dependencies.
>>>>> Therefore, we also need to move the suspend handling for the I2C
>>>>> controller driver to the NOIRQ phase as well.
>>>>>
>>>>> Signed-off-by: Qii Wang <qii.wang@mediatek.com>
>>>>
>>>> Is this a bugfix and should go into 5.10? Or can it wait for 5.11?
>>>>
>>>
>>> Yes, Can you help to apply it into 5.10? Thanks
>>
>> To be honest if you still do have any i2c device which accessing i2c buss after _noirq
>> stage and your driver does not implement .master_xfer_atomic() - you definitely have a bigger problem.
>> So adding IRQF_NO_SUSPEND sound like a hack and probably works just by luck.
>>
> 
> At present, it is only a problem caused by missing interrupts,
> and .master_xfer_atomic() just a implement in polling mode. Why not set
> the interrupt to a state that can always be triggered?
> 
> 

Because you must not use any IRQ driven operations after _noirq suspend state as it might (and most probably will)
cause unpredictable behavior later  in suspend_enter():

	arch_suspend_disable_irqs();
	BUG_ON(!irqs_disabled());
^after this point any IRQ driven I2C transfer will cause IRQ to be re-enabled

if you need  turn off device from platform callbacks -  .master_xfer_atomic() has to be implemented and used.
  

-- 
Best regards,
grygorii

Re: [v2] i2c: mediatek: Move suspend and resume handling to NOIRQ phase

[Qii Wang]

On Mon, 2020-12-07 at 18:35 +0200, Grygorii Strashko wrote:
> 
> > 
> > On Thu, 2020-12-03 at 10:01 +0200, Grygorii Strashko wrote:
> >>
> >> On 03/12/2020 03:25, Qii Wang wrote:
> >>> On Wed, 2020-12-02 at 16:35 +0100, Wolfram Sang wrote:
> >>>> Hi,
> >>>>
> >>>>> Some i2c device driver indirectly uses I2C driver when it is now
> >>>>> being suspended. The i2c devices driver is suspended during the
> >>>>> NOIRQ phase and this cannot be changed due to other dependencies.
> >>>>> Therefore, we also need to move the suspend handling for the I2C
> >>>>> controller driver to the NOIRQ phase as well.
> >>>>>
> >>>>> Signed-off-by: Qii Wang <qii.wang@mediatek.com>
> >>>>
> >>>> Is this a bugfix and should go into 5.10? Or can it wait for 5.11?
> >>>>
> >>>
> >>> Yes, Can you help to apply it into 5.10? Thanks
> >>
> >> To be honest if you still do have any i2c device which accessing i2c buss after _noirq
> >> stage and your driver does not implement .master_xfer_atomic() - you definitely have a bigger problem.
> >> So adding IRQF_NO_SUSPEND sound like a hack and probably works just by luck.
> >>
> > 
> > At present, it is only a problem caused by missing interrupts,
> > and .master_xfer_atomic() just a implement in polling mode. Why not set
> > the interrupt to a state that can always be triggered?
> > 
> > 
> 
> Because you must not use any IRQ driven operations after _noirq suspend state as it might (and most probably will)
> cause unpredictable behavior later  in suspend_enter():
> 
> 	arch_suspend_disable_irqs();
> 	BUG_ON(!irqs_disabled());
> ^after this point any IRQ driven I2C transfer will cause IRQ to be re-enabled
> 
> if you need  turn off device from platform callbacks -  .master_xfer_atomic() has to be implemented and used.
>   
Maybe my comment is a bit disturbing.Our purpose is not to call i2c and
use interrupts after _noirq pauses.So We use
i2c_mark_adapter_suspended&i2c_mark_adapter_resumed to block these i2c
transfers， There will not have any IRQ driven I2C transfer after this
point:
        arch_suspend_disable_irqs();
        BUG_ON(!irqs_disabled());
But some device driver will do i2c transfer after
dpm_noirq_resume_devices in dpm_resume_noirq(PMSG_RESUME) when our
driver irq hasn't resume.
	void dpm_resume_noirq(pm_message_t state)
	{
        	dpm_noirq_resume_devices(state);
        	resume_device_irqs();
        	device_wakeup_disarm_wake_irqs();
        	cpuidle_resume();
	}
.master_xfer_atomic() seems to be invalid for this question at this
time?

Re: [v2] i2c: mediatek: Move suspend and resume handling to NOIRQ phase

[Grygorii Strashko]

On 10/12/2020 03:56, Qii Wang wrote:
> On Mon, 2020-12-07 at 18:35 +0200, Grygorii Strashko wrote:
>>
>>>
>>> On Thu, 2020-12-03 at 10:01 +0200, Grygorii Strashko wrote:
>>>>
>>>> On 03/12/2020 03:25, Qii Wang wrote:
>>>>> On Wed, 2020-12-02 at 16:35 +0100, Wolfram Sang wrote:
>>>>>> Hi,
>>>>>>
>>>>>>> Some i2c device driver indirectly uses I2C driver when it is now
>>>>>>> being suspended. The i2c devices driver is suspended during the
>>>>>>> NOIRQ phase and this cannot be changed due to other dependencies.
>>>>>>> Therefore, we also need to move the suspend handling for the I2C
>>>>>>> controller driver to the NOIRQ phase as well.
>>>>>>>
>>>>>>> Signed-off-by: Qii Wang <qii.wang@mediatek.com>
>>>>>>
>>>>>> Is this a bugfix and should go into 5.10? Or can it wait for 5.11?
>>>>>>
>>>>>
>>>>> Yes, Can you help to apply it into 5.10? Thanks
>>>>
>>>> To be honest if you still do have any i2c device which accessing i2c buss after _noirq
>>>> stage and your driver does not implement .master_xfer_atomic() - you definitely have a bigger problem.
>>>> So adding IRQF_NO_SUSPEND sound like a hack and probably works just by luck.
>>>>
>>>
>>> At present, it is only a problem caused by missing interrupts,
>>> and .master_xfer_atomic() just a implement in polling mode. Why not set
>>> the interrupt to a state that can always be triggered?
>>>
>>>
>>
>> Because you must not use any IRQ driven operations after _noirq suspend state as it might (and most probably will)
>> cause unpredictable behavior later  in suspend_enter():
>>
>> 	arch_suspend_disable_irqs();
>> 	BUG_ON(!irqs_disabled());
>> ^after this point any IRQ driven I2C transfer will cause IRQ to be re-enabled
>>
>> if you need  turn off device from platform callbacks -  .master_xfer_atomic() has to be implemented and used.
>>    
> Maybe my comment is a bit disturbing.Our purpose is not to call i2c and
> use interrupts after _noirq pauses.So We use
> i2c_mark_adapter_suspended&i2c_mark_adapter_resumed to block these i2c
> transfers， There will not have any IRQ driven I2C transfer after this
> point:
>          arch_suspend_disable_irqs();
>          BUG_ON(!irqs_disabled());
> But some device driver will do i2c transfer after
> dpm_noirq_resume_devices in dpm_resume_noirq(PMSG_RESUME) when our
> driver irq hasn't resume.
> 	void dpm_resume_noirq(pm_message_t state)
> 	{
>          	dpm_noirq_resume_devices(state);

Just to clarify. You have resume sequence in dpm_noirq_resume_devices
  dpm_noirq_resume_devices -> resume I2C -> resume some device -> do i2c transfer after?

Is "some device" in Kernel mainline?

>          	resume_device_irqs();
>          	device_wakeup_disarm_wake_irqs();
>          	cpuidle_resume();
> 	}
> .master_xfer_atomic() seems to be invalid for this question at this
> time?
> 

-- 
Best regards,
grygorii

Re: [v2] i2c: mediatek: Move suspend and resume handling to NOIRQ phase

[Qii Wang]

On Thu, 2020-12-10 at 15:03 +0200, Grygorii Strashko wrote:
> 
> On 10/12/2020 03:56, Qii Wang wrote:
> > On Mon, 2020-12-07 at 18:35 +0200, Grygorii Strashko wrote:
> >>
> >>>
> >>> On Thu, 2020-12-03 at 10:01 +0200, Grygorii Strashko wrote:
> >>>>
> >>>> On 03/12/2020 03:25, Qii Wang wrote:
> >>>>> On Wed, 2020-12-02 at 16:35 +0100, Wolfram Sang wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>>> Some i2c device driver indirectly uses I2C driver when it is now
> >>>>>>> being suspended. The i2c devices driver is suspended during the
> >>>>>>> NOIRQ phase and this cannot be changed due to other dependencies.
> >>>>>>> Therefore, we also need to move the suspend handling for the I2C
> >>>>>>> controller driver to the NOIRQ phase as well.
> >>>>>>>
> >>>>>>> Signed-off-by: Qii Wang <qii.wang@mediatek.com>
> >>>>>>
> >>>>>> Is this a bugfix and should go into 5.10? Or can it wait for 5.11?
> >>>>>>
> >>>>>
> >>>>> Yes, Can you help to apply it into 5.10? Thanks
> >>>>
> >>>> To be honest if you still do have any i2c device which accessing i2c buss after _noirq
> >>>> stage and your driver does not implement .master_xfer_atomic() - you definitely have a bigger problem.
> >>>> So adding IRQF_NO_SUSPEND sound like a hack and probably works just by luck.
> >>>>
> >>>
> >>> At present, it is only a problem caused by missing interrupts,
> >>> and .master_xfer_atomic() just a implement in polling mode. Why not set
> >>> the interrupt to a state that can always be triggered?
> >>>
> >>>
> >>
> >> Because you must not use any IRQ driven operations after _noirq suspend state as it might (and most probably will)
> >> cause unpredictable behavior later  in suspend_enter():
> >>
> >> 	arch_suspend_disable_irqs();
> >> 	BUG_ON(!irqs_disabled());
> >> ^after this point any IRQ driven I2C transfer will cause IRQ to be re-enabled
> >>
> >> if you need  turn off device from platform callbacks -  .master_xfer_atomic() has to be implemented and used.
> >>    
> > Maybe my comment is a bit disturbing.Our purpose is not to call i2c and
> > use interrupts after _noirq pauses.So We use
> > i2c_mark_adapter_suspended&i2c_mark_adapter_resumed to block these i2c
> > transfers， There will not have any IRQ driven I2C transfer after this
> > point:
> >          arch_suspend_disable_irqs();
> >          BUG_ON(!irqs_disabled());
> > But some device driver will do i2c transfer after
> > dpm_noirq_resume_devices in dpm_resume_noirq(PMSG_RESUME) when our
> > driver irq hasn't resume.
> > 	void dpm_resume_noirq(pm_message_t state)
> > 	{
> >          	dpm_noirq_resume_devices(state);
> 
> Just to clarify. You have resume sequence in dpm_noirq_resume_devices
>   dpm_noirq_resume_devices -> resume I2C -> resume some device -> do i2c transfer after?
> 

Yes.

> Is "some device" in Kernel mainline?
> 

The problematic device driver is drivers/regulator/da9211-regulator.c in
Kernel mainline.

> >          	resume_device_irqs();
> >          	device_wakeup_disarm_wake_irqs();
> >          	cpuidle_resume();
> > 	}
> > .master_xfer_atomic() seems to be invalid for this question at this
> > time?
> > 
> 

Re: [v2] i2c: mediatek: Move suspend and resume handling to NOIRQ phase

[Grygorii Strashko]

On 14/12/2020 10:48, Qii Wang wrote:
> On Thu, 2020-12-10 at 15:03 +0200, Grygorii Strashko wrote:
>>
>> On 10/12/2020 03:56, Qii Wang wrote:
>>> On Mon, 2020-12-07 at 18:35 +0200, Grygorii Strashko wrote:
>>>>
>>>>>
>>>>> On Thu, 2020-12-03 at 10:01 +0200, Grygorii Strashko wrote:
>>>>>>
>>>>>> On 03/12/2020 03:25, Qii Wang wrote:
>>>>>>> On Wed, 2020-12-02 at 16:35 +0100, Wolfram Sang wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>>> Some i2c device driver indirectly uses I2C driver when it is now
>>>>>>>>> being suspended. The i2c devices driver is suspended during the
>>>>>>>>> NOIRQ phase and this cannot be changed due to other dependencies.
>>>>>>>>> Therefore, we also need to move the suspend handling for the I2C
>>>>>>>>> controller driver to the NOIRQ phase as well.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Qii Wang <qii.wang@mediatek.com>
>>>>>>>>
>>>>>>>> Is this a bugfix and should go into 5.10? Or can it wait for 5.11?
>>>>>>>>
>>>>>>>
>>>>>>> Yes, Can you help to apply it into 5.10? Thanks
>>>>>>
>>>>>> To be honest if you still do have any i2c device which accessing i2c buss after _noirq
>>>>>> stage and your driver does not implement .master_xfer_atomic() - you definitely have a bigger problem.
>>>>>> So adding IRQF_NO_SUSPEND sound like a hack and probably works just by luck.
>>>>>>
>>>>>
>>>>> At present, it is only a problem caused by missing interrupts,
>>>>> and .master_xfer_atomic() just a implement in polling mode. Why not set
>>>>> the interrupt to a state that can always be triggered?
>>>>>
>>>>>
>>>>
>>>> Because you must not use any IRQ driven operations after _noirq suspend state as it might (and most probably will)
>>>> cause unpredictable behavior later  in suspend_enter():
>>>>
>>>> 	arch_suspend_disable_irqs();
>>>> 	BUG_ON(!irqs_disabled());
>>>> ^after this point any IRQ driven I2C transfer will cause IRQ to be re-enabled
>>>>
>>>> if you need  turn off device from platform callbacks -  .master_xfer_atomic() has to be implemented and used.
>>>>     
>>> Maybe my comment is a bit disturbing.Our purpose is not to call i2c and
>>> use interrupts after _noirq pauses.So We use
>>> i2c_mark_adapter_suspended&i2c_mark_adapter_resumed to block these i2c
>>> transfers， There will not have any IRQ driven I2C transfer after this
>>> point:
>>>           arch_suspend_disable_irqs();
>>>           BUG_ON(!irqs_disabled());
>>> But some device driver will do i2c transfer after
>>> dpm_noirq_resume_devices in dpm_resume_noirq(PMSG_RESUME) when our
>>> driver irq hasn't resume.
>>> 	void dpm_resume_noirq(pm_message_t state)
>>> 	{
>>>           	dpm_noirq_resume_devices(state);
>>
>> Just to clarify. You have resume sequence in dpm_noirq_resume_devices
>>    dpm_noirq_resume_devices -> resume I2C -> resume some device -> do i2c transfer after?
>>
> 
> Yes.

huh. First consider IRQF_EARLY_RESUME - it's better, but still will be a hack

> 
>> Is "some device" in Kernel mainline?
>>
> 
> The problematic device driver is drivers/regulator/da9211-regulator.c in
> Kernel mainline.

regulator is passive device, somebody should call it !?

And da9211-regulator IRQ handler should remain disabled till resume_device_irqs() call.

note. regulator_class implements only

static const struct dev_pm_ops __maybe_unused regulator_pm_ops = {
	.suspend	= regulator_suspend,
	.resume		= regulator_resume,
};


> 
>>>           	resume_device_irqs();
>>>           	device_wakeup_disarm_wake_irqs();
>>>           	cpuidle_resume();
>>> 	}
>>> .master_xfer_atomic() seems to be invalid for this question at this
>>> time?
>>>
>>
> 

-- 
Best regards,
grygorii

Re: [v2] i2c: mediatek: Move suspend and resume handling to NOIRQ phase

[Qii Wang]

On Mon, 2020-12-14 at 22:08 +0200, Grygorii Strashko wrote:
> 
> On 14/12/2020 10:48, Qii Wang wrote:
> > On Thu, 2020-12-10 at 15:03 +0200, Grygorii Strashko wrote:
> >>
> >> On 10/12/2020 03:56, Qii Wang wrote:
> >>> On Mon, 2020-12-07 at 18:35 +0200, Grygorii Strashko wrote:
> >>>>
> >>>>>
> >>>>> On Thu, 2020-12-03 at 10:01 +0200, Grygorii Strashko wrote:
> >>>>>>
> >>>>>> On 03/12/2020 03:25, Qii Wang wrote:
> >>>>>>> On Wed, 2020-12-02 at 16:35 +0100, Wolfram Sang wrote:
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>>> Some i2c device driver indirectly uses I2C driver when it is now
> >>>>>>>>> being suspended. The i2c devices driver is suspended during the
> >>>>>>>>> NOIRQ phase and this cannot be changed due to other dependencies.
> >>>>>>>>> Therefore, we also need to move the suspend handling for the I2C
> >>>>>>>>> controller driver to the NOIRQ phase as well.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Qii Wang <qii.wang@mediatek.com>
> >>>>>>>>
> >>>>>>>> Is this a bugfix and should go into 5.10? Or can it wait for 5.11?
> >>>>>>>>
> >>>>>>>
> >>>>>>> Yes, Can you help to apply it into 5.10? Thanks
> >>>>>>
> >>>>>> To be honest if you still do have any i2c device which accessing i2c buss after _noirq
> >>>>>> stage and your driver does not implement .master_xfer_atomic() - you definitely have a bigger problem.
> >>>>>> So adding IRQF_NO_SUSPEND sound like a hack and probably works just by luck.
> >>>>>>
> >>>>>
> >>>>> At present, it is only a problem caused by missing interrupts,
> >>>>> and .master_xfer_atomic() just a implement in polling mode. Why not set
> >>>>> the interrupt to a state that can always be triggered?
> >>>>>
> >>>>>
> >>>>
> >>>> Because you must not use any IRQ driven operations after _noirq suspend state as it might (and most probably will)
> >>>> cause unpredictable behavior later  in suspend_enter():
> >>>>
> >>>> 	arch_suspend_disable_irqs();
> >>>> 	BUG_ON(!irqs_disabled());
> >>>> ^after this point any IRQ driven I2C transfer will cause IRQ to be re-enabled
> >>>>
> >>>> if you need  turn off device from platform callbacks -  .master_xfer_atomic() has to be implemented and used.
> >>>>     
> >>> Maybe my comment is a bit disturbing.Our purpose is not to call i2c and
> >>> use interrupts after _noirq pauses.So We use
> >>> i2c_mark_adapter_suspended&i2c_mark_adapter_resumed to block these i2c
> >>> transfers， There will not have any IRQ driven I2C transfer after this
> >>> point:
> >>>           arch_suspend_disable_irqs();
> >>>           BUG_ON(!irqs_disabled());
> >>> But some device driver will do i2c transfer after
> >>> dpm_noirq_resume_devices in dpm_resume_noirq(PMSG_RESUME) when our
> >>> driver irq hasn't resume.
> >>> 	void dpm_resume_noirq(pm_message_t state)
> >>> 	{
> >>>           	dpm_noirq_resume_devices(state);
> >>
> >> Just to clarify. You have resume sequence in dpm_noirq_resume_devices
> >>    dpm_noirq_resume_devices -> resume I2C -> resume some device -> do i2c transfer after?
> >>
> > 
> > Yes.
> 
> huh. First consider IRQF_EARLY_RESUME - it's better, but still will be a hack
> 
There should be the same problem during the suspend process, So
IRQF_EARLY_RESUME should not be able to solve the problem.

> > 
> >> Is "some device" in Kernel mainline?
> >>
> > 
> > The problematic device driver is drivers/regulator/da9211-regulator.c in
> > Kernel mainline.
> 
> regulator is passive device, somebody should call it !?
> 
> And da9211-regulator IRQ handler should remain disabled till resume_device_irqs() call.
> 

Not only will i2c transfer be called in da9211-regulator IRQ handler,
but also other drivers will call da9211_buck_ops which containing i2c
transfers.

> note. regulator_class implements only
> 
> static const struct dev_pm_ops __maybe_unused regulator_pm_ops = {
> 	.suspend	= regulator_suspend,
> 	.resume		= regulator_resume,
> };
> 
> 
> > 
> >>>           	resume_device_irqs();
> >>>           	device_wakeup_disarm_wake_irqs();
> >>>           	cpuidle_resume();
> >>> 	}
> >>> .master_xfer_atomic() seems to be invalid for this question at this
> >>> time?
> >>>
> >>
> > 
> 

Re: [v2] i2c: mediatek: Move suspend and resume handling to NOIRQ phase

[Qii Wang]

Hi sirs:
	If there is no new comment, I will resent it in 5.11.