Hi,

Thank you for this new version.

On 22-12-18 21:26, Wolfram Sang wrote:

Using the 'is_suspended' flag from the PM core, we now reject new
transfers if the parent of the adapter is already marked suspended.

I've been running some tests and I'm afraid that those have
exposed multiple issues:

1) It seems that adap->dev.parent can be NULL in some cases, specifically
this patch causes the i915 driver to crash during probe on an Apollo Lake
laptop with an eDP panel. I've attached a fixup patch which fixes this.

2) There are multiple suspend stages: prepare suspend, suspend_late,
suspend_no_irq and several devices which need access to i2c-transfers
suspend from the suspend_late or even the suspend_no_irq handler.

The way this works is that first all devices are moved to the prepared
state, then a second run is done moving all devices over to suspended,
then a third run moving all devices over to suspend_late, etc.

To make this work, e.g. the i2c-designware driver has a nop suspend
callback and does the actual suspend from its suspend_late or
suspend_no_irq callback. But the is_suspended flag we are testing for
now gets set during the suspend phase. So when we are then asked to
do an i2c-transfer during the suspend_late phase we get a false-positive
triggering of the:

	if (WARN(device_is_suspended(adap->dev.parent),
		 "Accessing already suspended I2C/SMBus adapter"))
		return -ESHUTDOWN;

WARN and a return of -ESHUTDOWN, because the adapter is in the
suspended state, but it has not actually been suspended / moved
to the D3 low-power state as that happens later when we reach
e.g. the suspend_no_irq phase of the suspend.

This is not only a problem with the somewhat complex PMIC
situation on some Cherry Trail devices, but it also breaks the
i915 driver since as a PCI device the i915 device also only
really gets turned off from the suspend_no_irq phase of the suspend.

Sorry, this is something which I should have realized before, but
well I didn't.

TL;DR: really only the adapter driver knows when the device is
put in such a state that it can no longer do transfers, as it
actually turns of clks / moves it D3 / etc. Which may happen at
any of the 3 suspend phases so any "core" based solution is going
to get this wrong.

I share your desire to have the check for this shared in core code,
but I'm afraid that just is not going to work.

Regards,

Hans