From: Qii Wang <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
Is this a bugfix and should go into 5.10? Or can it wait for 5.11?
Thanks,
Wolfram
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 <[email protected]>
>
> 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
>
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 <[email protected]>
>>
>> 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
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/[email protected]/
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/[email protected]/
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 <[email protected]>
> >>
> >> 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?
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/[email protected]/
>
> 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/[email protected]/
>
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 <[email protected]>
>>>>
>>>> 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
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 <[email protected]>
> >>>>
> >>>> 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?
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 <[email protected]>
>>>>>>
>>>>>> 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
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 <[email protected]>
> >>>>>>
> >>>>>> 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?
> >
>
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 <[email protected]>
>>>>>>>>
>>>>>>>> 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
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 <[email protected]>
> >>>>>>>>
> >>>>>>>> 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?
> >>>
> >>
> >
>
Hi sirs:
If there is no new comment, I will resent it in 5.11.