| Message ID | 20200407181116.61066-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | i2c: designware: platdrv: Remove DPM_FLAG_SMART_SUSPEND flag on BYT and CHT | expand |
On Tue, Apr 07, 2020 at 08:11:16PM +0200, Hans de Goede wrote: > We already set DPM_FLAG_SMART_PREPARE, so we completely skip all > callbacks (other then prepare) where possible, quoting from > dw_i2c_plat_prepare(): > > /* > * If the ACPI companion device object is present for this device, it > * may be accessed during suspend and resume of other devices via I2C > * operation regions, so tell the PM core and middle layers to avoid > * skipping system suspend/resume callbacks for it in that case. > */ > return !has_acpi_companion(dev); > > Also setting the DPM_FLAG_SMART_SUSPEND will cause acpi_subsys_suspend() > to leave the controller runtime-suspended even if dw_i2c_plat_prepare() > returned 0. > > Leaving the controller runtime-suspended normally, when the I2C controller > is suspended during the suspend_late phase, is not an issue because > the pm_runtime_get_sync() done by i2c_dw_xfer() will (runtime-)resume it. > > But for dw I2C controllers on Bay- and Cherry-Trail devices acpi_lpss.c > leaves the controller alive until the suspend_noirq phase, because it may > be used by the _PS3 ACPI methods of PCI devices and PCI devices are left > powered on until the suspend_noirq phase. > > Between the suspend_late and resume_early phases runtime-pm is disabled. > So for any ACPI I2C OPRegion accesses done after the suspend_late phase, > the pm_runtime_get_sync() done by i2c_dw_xfer() is a no-op and the > controller is left runtime-suspended. > > i2c_dw_xfer() has a check to catch this condition (rather then waiting > for the I2C transfer to timeout because the controller is suspended). > acpi_subsys_suspend() leaving the controller runtime-suspended in > combination with an ACPI I2C OPRegion access done after the suspend_late > phase triggers this check, leading to the following error being logged > on a Bay Trail based Lenovo Thinkpad 8 tablet: > > [ 93.275882] i2c_designware 80860F41:00: Transfer while suspended > [ 93.275993] WARNING: CPU: 0 PID: 412 at drivers/i2c/busses/i2c-designware-master.c:429 i2c_dw_xfer+0x239/0x280 > ... > [ 93.276252] Workqueue: kacpi_notify acpi_os_execute_deferred > [ 93.276267] RIP: 0010:i2c_dw_xfer+0x239/0x280 > ... > [ 93.276340] Call Trace: > [ 93.276366] __i2c_transfer+0x121/0x520 > [ 93.276379] i2c_transfer+0x4c/0x100 > [ 93.276392] i2c_acpi_space_handler+0x219/0x510 > [ 93.276408] ? up+0x40/0x60 > [ 93.276419] ? i2c_acpi_notify+0x130/0x130 > [ 93.276433] acpi_ev_address_space_dispatch+0x1e1/0x252 > ... > > So since on BYT and CHT platforms we want ACPI I2c OPRegion accesses > to work until the suspend_noirq phase, we need the controller to be > runtime-resumed during the suspend phase if it is runtime-suspended > suspended at that time. This means that we must not set the > DPM_FLAG_SMART_SUSPEND on these platforms. > > On BYT and CHT we already have a special ACCESS_NO_IRQ_SUSPEND flag > to make sure the controller stays functional until the suspend_noirq > phase. This commit makes the driver not set the DPM_FLAG_SMART_SUSPEND > flag when that flag is set. FWIW, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: stable@vger.kernel.org > Fixes: b30f2f65568f ("i2c: designware: Set IRQF_NO_SUSPEND flag for all BYT and CHT controllers") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/i2c/busses/i2c-designware-platdrv.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c > index 3b7d58c2fe85..15b4b965b443 100644 > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c > @@ -371,10 +371,16 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) > adap->dev.of_node = pdev->dev.of_node; > adap->nr = -1; > > - dev_pm_set_driver_flags(&pdev->dev, > - DPM_FLAG_SMART_PREPARE | > - DPM_FLAG_SMART_SUSPEND | > - DPM_FLAG_LEAVE_SUSPENDED); > + if (dev->flags & ACCESS_NO_IRQ_SUSPEND) { > + dev_pm_set_driver_flags(&pdev->dev, > + DPM_FLAG_SMART_PREPARE | > + DPM_FLAG_LEAVE_SUSPENDED); > + } else { > + dev_pm_set_driver_flags(&pdev->dev, > + DPM_FLAG_SMART_PREPARE | > + DPM_FLAG_SMART_SUSPEND | > + DPM_FLAG_LEAVE_SUSPENDED); > + } > > /* The code below assumes runtime PM to be disabled. */ > WARN_ON(pm_runtime_enabled(&pdev->dev)); > -- > 2.26.0 >
On Tue, Apr 7, 2020 at 8:11 PM Hans de Goede <hdegoede@redhat.com> wrote: > > We already set DPM_FLAG_SMART_PREPARE, so we completely skip all > callbacks (other then prepare) where possible, quoting from > dw_i2c_plat_prepare(): > > /* > * If the ACPI companion device object is present for this device, it > * may be accessed during suspend and resume of other devices via I2C > * operation regions, so tell the PM core and middle layers to avoid > * skipping system suspend/resume callbacks for it in that case. > */ > return !has_acpi_companion(dev); > > Also setting the DPM_FLAG_SMART_SUSPEND will cause acpi_subsys_suspend() > to leave the controller runtime-suspended even if dw_i2c_plat_prepare() > returned 0. > > Leaving the controller runtime-suspended normally, when the I2C controller > is suspended during the suspend_late phase, is not an issue because > the pm_runtime_get_sync() done by i2c_dw_xfer() will (runtime-)resume it. > > But for dw I2C controllers on Bay- and Cherry-Trail devices acpi_lpss.c > leaves the controller alive until the suspend_noirq phase, because it may > be used by the _PS3 ACPI methods of PCI devices and PCI devices are left > powered on until the suspend_noirq phase. > > Between the suspend_late and resume_early phases runtime-pm is disabled. > So for any ACPI I2C OPRegion accesses done after the suspend_late phase, > the pm_runtime_get_sync() done by i2c_dw_xfer() is a no-op and the > controller is left runtime-suspended. > > i2c_dw_xfer() has a check to catch this condition (rather then waiting > for the I2C transfer to timeout because the controller is suspended). > acpi_subsys_suspend() leaving the controller runtime-suspended in > combination with an ACPI I2C OPRegion access done after the suspend_late > phase triggers this check, leading to the following error being logged > on a Bay Trail based Lenovo Thinkpad 8 tablet: > > [ 93.275882] i2c_designware 80860F41:00: Transfer while suspended > [ 93.275993] WARNING: CPU: 0 PID: 412 at drivers/i2c/busses/i2c-designware-master.c:429 i2c_dw_xfer+0x239/0x280 > ... > [ 93.276252] Workqueue: kacpi_notify acpi_os_execute_deferred > [ 93.276267] RIP: 0010:i2c_dw_xfer+0x239/0x280 > ... > [ 93.276340] Call Trace: > [ 93.276366] __i2c_transfer+0x121/0x520 > [ 93.276379] i2c_transfer+0x4c/0x100 > [ 93.276392] i2c_acpi_space_handler+0x219/0x510 > [ 93.276408] ? up+0x40/0x60 > [ 93.276419] ? i2c_acpi_notify+0x130/0x130 > [ 93.276433] acpi_ev_address_space_dispatch+0x1e1/0x252 > ... > > So since on BYT and CHT platforms we want ACPI I2c OPRegion accesses > to work until the suspend_noirq phase, we need the controller to be > runtime-resumed during the suspend phase if it is runtime-suspended > suspended at that time. This means that we must not set the > DPM_FLAG_SMART_SUSPEND on these platforms. > > On BYT and CHT we already have a special ACCESS_NO_IRQ_SUSPEND flag > to make sure the controller stays functional until the suspend_noirq > phase. This commit makes the driver not set the DPM_FLAG_SMART_SUSPEND > flag when that flag is set. OK Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Cc: stable@vger.kernel.org > Fixes: b30f2f65568f ("i2c: designware: Set IRQF_NO_SUSPEND flag for all BYT and CHT controllers") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/i2c/busses/i2c-designware-platdrv.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c > index 3b7d58c2fe85..15b4b965b443 100644 > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c > @@ -371,10 +371,16 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) > adap->dev.of_node = pdev->dev.of_node; > adap->nr = -1; > > - dev_pm_set_driver_flags(&pdev->dev, > - DPM_FLAG_SMART_PREPARE | > - DPM_FLAG_SMART_SUSPEND | > - DPM_FLAG_LEAVE_SUSPENDED); > + if (dev->flags & ACCESS_NO_IRQ_SUSPEND) { > + dev_pm_set_driver_flags(&pdev->dev, > + DPM_FLAG_SMART_PREPARE | > + DPM_FLAG_LEAVE_SUSPENDED); > + } else { > + dev_pm_set_driver_flags(&pdev->dev, > + DPM_FLAG_SMART_PREPARE | > + DPM_FLAG_SMART_SUSPEND | > + DPM_FLAG_LEAVE_SUSPENDED); > + } > > /* The code below assumes runtime PM to be disabled. */ > WARN_ON(pm_runtime_enabled(&pdev->dev)); > -- > 2.26.0 >
On 4/7/20 10:30 PM, Rafael J. Wysocki wrote: > On Tue, Apr 7, 2020 at 8:11 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> We already set DPM_FLAG_SMART_PREPARE, so we completely skip all >> callbacks (other then prepare) where possible, quoting from >> dw_i2c_plat_prepare(): >> >> /* >> * If the ACPI companion device object is present for this device, it >> * may be accessed during suspend and resume of other devices via I2C >> * operation regions, so tell the PM core and middle layers to avoid >> * skipping system suspend/resume callbacks for it in that case. >> */ >> return !has_acpi_companion(dev); >> >> Also setting the DPM_FLAG_SMART_SUSPEND will cause acpi_subsys_suspend() >> to leave the controller runtime-suspended even if dw_i2c_plat_prepare() >> returned 0. >> >> Leaving the controller runtime-suspended normally, when the I2C controller >> is suspended during the suspend_late phase, is not an issue because >> the pm_runtime_get_sync() done by i2c_dw_xfer() will (runtime-)resume it. >> >> But for dw I2C controllers on Bay- and Cherry-Trail devices acpi_lpss.c >> leaves the controller alive until the suspend_noirq phase, because it may >> be used by the _PS3 ACPI methods of PCI devices and PCI devices are left >> powered on until the suspend_noirq phase. >> >> Between the suspend_late and resume_early phases runtime-pm is disabled. >> So for any ACPI I2C OPRegion accesses done after the suspend_late phase, >> the pm_runtime_get_sync() done by i2c_dw_xfer() is a no-op and the >> controller is left runtime-suspended. >> >> i2c_dw_xfer() has a check to catch this condition (rather then waiting >> for the I2C transfer to timeout because the controller is suspended). >> acpi_subsys_suspend() leaving the controller runtime-suspended in >> combination with an ACPI I2C OPRegion access done after the suspend_late >> phase triggers this check, leading to the following error being logged >> on a Bay Trail based Lenovo Thinkpad 8 tablet: >> >> [ 93.275882] i2c_designware 80860F41:00: Transfer while suspended >> [ 93.275993] WARNING: CPU: 0 PID: 412 at drivers/i2c/busses/i2c-designware-master.c:429 i2c_dw_xfer+0x239/0x280 >> ... >> [ 93.276252] Workqueue: kacpi_notify acpi_os_execute_deferred >> [ 93.276267] RIP: 0010:i2c_dw_xfer+0x239/0x280 >> ... >> [ 93.276340] Call Trace: >> [ 93.276366] __i2c_transfer+0x121/0x520 >> [ 93.276379] i2c_transfer+0x4c/0x100 >> [ 93.276392] i2c_acpi_space_handler+0x219/0x510 >> [ 93.276408] ? up+0x40/0x60 >> [ 93.276419] ? i2c_acpi_notify+0x130/0x130 >> [ 93.276433] acpi_ev_address_space_dispatch+0x1e1/0x252 >> ... >> >> So since on BYT and CHT platforms we want ACPI I2c OPRegion accesses >> to work until the suspend_noirq phase, we need the controller to be >> runtime-resumed during the suspend phase if it is runtime-suspended >> suspended at that time. This means that we must not set the >> DPM_FLAG_SMART_SUSPEND on these platforms. >> >> On BYT and CHT we already have a special ACCESS_NO_IRQ_SUSPEND flag >> to make sure the controller stays functional until the suspend_noirq >> phase. This commit makes the driver not set the DPM_FLAG_SMART_SUSPEND >> flag when that flag is set. > > OK > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
On Tue, Apr 07, 2020 at 08:11:16PM +0200, Hans de Goede wrote: > We already set DPM_FLAG_SMART_PREPARE, so we completely skip all > callbacks (other then prepare) where possible, quoting from > dw_i2c_plat_prepare(): > > /* > * If the ACPI companion device object is present for this device, it > * may be accessed during suspend and resume of other devices via I2C > * operation regions, so tell the PM core and middle layers to avoid > * skipping system suspend/resume callbacks for it in that case. > */ > return !has_acpi_companion(dev); > > Also setting the DPM_FLAG_SMART_SUSPEND will cause acpi_subsys_suspend() > to leave the controller runtime-suspended even if dw_i2c_plat_prepare() > returned 0. > > Leaving the controller runtime-suspended normally, when the I2C controller > is suspended during the suspend_late phase, is not an issue because > the pm_runtime_get_sync() done by i2c_dw_xfer() will (runtime-)resume it. > > But for dw I2C controllers on Bay- and Cherry-Trail devices acpi_lpss.c > leaves the controller alive until the suspend_noirq phase, because it may > be used by the _PS3 ACPI methods of PCI devices and PCI devices are left > powered on until the suspend_noirq phase. > > Between the suspend_late and resume_early phases runtime-pm is disabled. > So for any ACPI I2C OPRegion accesses done after the suspend_late phase, > the pm_runtime_get_sync() done by i2c_dw_xfer() is a no-op and the > controller is left runtime-suspended. > > i2c_dw_xfer() has a check to catch this condition (rather then waiting > for the I2C transfer to timeout because the controller is suspended). > acpi_subsys_suspend() leaving the controller runtime-suspended in > combination with an ACPI I2C OPRegion access done after the suspend_late > phase triggers this check, leading to the following error being logged > on a Bay Trail based Lenovo Thinkpad 8 tablet: > > [ 93.275882] i2c_designware 80860F41:00: Transfer while suspended > [ 93.275993] WARNING: CPU: 0 PID: 412 at drivers/i2c/busses/i2c-designware-master.c:429 i2c_dw_xfer+0x239/0x280 > ... > [ 93.276252] Workqueue: kacpi_notify acpi_os_execute_deferred > [ 93.276267] RIP: 0010:i2c_dw_xfer+0x239/0x280 > ... > [ 93.276340] Call Trace: > [ 93.276366] __i2c_transfer+0x121/0x520 > [ 93.276379] i2c_transfer+0x4c/0x100 > [ 93.276392] i2c_acpi_space_handler+0x219/0x510 > [ 93.276408] ? up+0x40/0x60 > [ 93.276419] ? i2c_acpi_notify+0x130/0x130 > [ 93.276433] acpi_ev_address_space_dispatch+0x1e1/0x252 > ... > > So since on BYT and CHT platforms we want ACPI I2c OPRegion accesses > to work until the suspend_noirq phase, we need the controller to be > runtime-resumed during the suspend phase if it is runtime-suspended > suspended at that time. This means that we must not set the > DPM_FLAG_SMART_SUSPEND on these platforms. > > On BYT and CHT we already have a special ACCESS_NO_IRQ_SUSPEND flag > to make sure the controller stays functional until the suspend_noirq > phase. This commit makes the driver not set the DPM_FLAG_SMART_SUSPEND > flag when that flag is set. > > Cc: stable@vger.kernel.org > Fixes: b30f2f65568f ("i2c: designware: Set IRQF_NO_SUSPEND flag for all BYT and CHT controllers") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Applied to for-current, thanks!
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 3b7d58c2fe85..15b4b965b443 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -371,10 +371,16 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) adap->dev.of_node = pdev->dev.of_node; adap->nr = -1; - dev_pm_set_driver_flags(&pdev->dev, - DPM_FLAG_SMART_PREPARE | - DPM_FLAG_SMART_SUSPEND | - DPM_FLAG_LEAVE_SUSPENDED); + if (dev->flags & ACCESS_NO_IRQ_SUSPEND) { + dev_pm_set_driver_flags(&pdev->dev, + DPM_FLAG_SMART_PREPARE | + DPM_FLAG_LEAVE_SUSPENDED); + } else { + dev_pm_set_driver_flags(&pdev->dev, + DPM_FLAG_SMART_PREPARE | + DPM_FLAG_SMART_SUSPEND | + DPM_FLAG_LEAVE_SUSPENDED); + } /* The code below assumes runtime PM to be disabled. */ WARN_ON(pm_runtime_enabled(&pdev->dev));
We already set DPM_FLAG_SMART_PREPARE, so we completely skip all callbacks (other then prepare) where possible, quoting from dw_i2c_plat_prepare(): /* * If the ACPI companion device object is present for this device, it * may be accessed during suspend and resume of other devices via I2C * operation regions, so tell the PM core and middle layers to avoid * skipping system suspend/resume callbacks for it in that case. */ return !has_acpi_companion(dev); Also setting the DPM_FLAG_SMART_SUSPEND will cause acpi_subsys_suspend() to leave the controller runtime-suspended even if dw_i2c_plat_prepare() returned 0. Leaving the controller runtime-suspended normally, when the I2C controller is suspended during the suspend_late phase, is not an issue because the pm_runtime_get_sync() done by i2c_dw_xfer() will (runtime-)resume it. But for dw I2C controllers on Bay- and Cherry-Trail devices acpi_lpss.c leaves the controller alive until the suspend_noirq phase, because it may be used by the _PS3 ACPI methods of PCI devices and PCI devices are left powered on until the suspend_noirq phase. Between the suspend_late and resume_early phases runtime-pm is disabled. So for any ACPI I2C OPRegion accesses done after the suspend_late phase, the pm_runtime_get_sync() done by i2c_dw_xfer() is a no-op and the controller is left runtime-suspended. i2c_dw_xfer() has a check to catch this condition (rather then waiting for the I2C transfer to timeout because the controller is suspended). acpi_subsys_suspend() leaving the controller runtime-suspended in combination with an ACPI I2C OPRegion access done after the suspend_late phase triggers this check, leading to the following error being logged on a Bay Trail based Lenovo Thinkpad 8 tablet: [ 93.275882] i2c_designware 80860F41:00: Transfer while suspended [ 93.275993] WARNING: CPU: 0 PID: 412 at drivers/i2c/busses/i2c-designware-master.c:429 i2c_dw_xfer+0x239/0x280 ... [ 93.276252] Workqueue: kacpi_notify acpi_os_execute_deferred [ 93.276267] RIP: 0010:i2c_dw_xfer+0x239/0x280 ... [ 93.276340] Call Trace: [ 93.276366] __i2c_transfer+0x121/0x520 [ 93.276379] i2c_transfer+0x4c/0x100 [ 93.276392] i2c_acpi_space_handler+0x219/0x510 [ 93.276408] ? up+0x40/0x60 [ 93.276419] ? i2c_acpi_notify+0x130/0x130 [ 93.276433] acpi_ev_address_space_dispatch+0x1e1/0x252 ... So since on BYT and CHT platforms we want ACPI I2c OPRegion accesses to work until the suspend_noirq phase, we need the controller to be runtime-resumed during the suspend phase if it is runtime-suspended suspended at that time. This means that we must not set the DPM_FLAG_SMART_SUSPEND on these platforms. On BYT and CHT we already have a special ACCESS_NO_IRQ_SUSPEND flag to make sure the controller stays functional until the suspend_noirq phase. This commit makes the driver not set the DPM_FLAG_SMART_SUSPEND flag when that flag is set. Cc: stable@vger.kernel.org Fixes: b30f2f65568f ("i2c: designware: Set IRQF_NO_SUSPEND flag for all BYT and CHT controllers") Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/i2c/busses/i2c-designware-platdrv.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)