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@xxxxxxxxxxxxxxxxxxxx/
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@xxxxxxxxxx/
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@xxxxxxxxxxxx>
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
