Here is a next generation (previous one is [1]) of the long standing power
issue fix regarding to LPSS on Intel Baytrail and Braswell SoCs, in
particularly ASuS T100TA. There are few bugs already opened on kernel.org's and
RedHat's bugzilla sites.
The series depends on the patch submitted earlier [2].
The patch 1 brings a new notification to handle the case when ->probe() of the
driver fails. It allows to avoid a potential resource leak. I've noticed couple
of drivers that are using that in assumption that ->probe() never fails.
The patch 2 is needed to fix an I2C issue which Jarkko is currently
investigating.
It seems the best way to push it through linux-pm tree. Thus, it would be good
to get ACKs from the rest of maintainers.
Rafael, it would be nice to have an immutable branch or tag for this sice I
have more patches coming for dw_dmac driver which are based on top of this
series.
The patches have been tested on ASuS T100TA, Intel Cherrytrail, and Intel
Braswell SoCs.
[1] http://www.spinics.net/lists/linux-acpi/msg53963.html
[2] http://www.spinics.net/lists/kernel/msg2119229.html
Andy Shevchenko (7):
device core: add BUS_NOTIFY_BIND_DRIVER_ERROR notification
ACPI / LPSS: allow to use specific PM domain during ->probe()
ACPI / LPSS: do delay for all LPSS devices when D3->D0
ACPI / LPSS: override power state for LPSS DMA device
dmaengine: dw: platform: power on device on shutdown
dmaengine: dw: return immediately from IRQ when DMA isn't in use
Revert "dmaengine: dw: platform: provide platform data for Intel"
arch/x86/Kconfig | 3 +-
arch/x86/include/asm/iosf_mbi.h | 2 +
drivers/acpi/acpi_lpss.c | 184 ++++++++++++++++++++++++++++++++++++----
drivers/base/dd.c | 8 +-
drivers/dma/dw/core.c | 9 +-
drivers/dma/dw/platform.c | 29 +++----
include/linux/device.h | 1 +
7 files changed, 198 insertions(+), 38 deletions(-)
--
2.6.2
In case ->probe() fails the notifier does not inform a subscriber about this.
In the result it might happend that some resources that had been allocated will
stay allocated and therefore lead to resource leak.
Introduce a new notification to inform the subscriber that ->probe() failed.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/base/dd.c | 8 ++++++--
include/linux/device.h | 1 +
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index a641cf3..ac071a5 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -290,7 +290,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
/* If using pinctrl, bind pins now before probing */
ret = pinctrl_bind_pins(dev);
if (ret)
- goto probe_failed;
+ goto pinctrl_bind_failed;
if (driver_sysfs_add(dev)) {
printk(KERN_ERR "%s: driver_sysfs_add(%s) failed\n",
@@ -334,6 +334,11 @@ static int really_probe(struct device *dev, struct device_driver *drv)
goto done;
probe_failed:
+ if (dev->bus)
+ blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
+ BUS_NOTIFY_BIND_DRIVER_ERROR,
+ dev);
+pinctrl_bind_failed:
devres_release_all(dev);
driver_sysfs_remove(dev);
dev->driver = NULL;
@@ -701,7 +706,6 @@ static void __device_release_driver(struct device *dev)
blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
BUS_NOTIFY_UNBOUND_DRIVER,
dev);
-
}
}
diff --git a/include/linux/device.h b/include/linux/device.h
index b8f411b..87cf423 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -191,6 +191,7 @@ extern int bus_unregister_notifier(struct bus_type *bus,
unbound */
#define BUS_NOTIFY_UNBOUND_DRIVER 0x00000007 /* driver is unbound
from the device */
+#define BUS_NOTIFY_BIND_DRIVER_ERROR 0x80000004 /* driver fails to be bound */
extern struct kset *bus_get_kset(struct bus_type *bus);
extern struct klist *bus_get_device_klist(struct bus_type *bus);
--
2.6.2
This is an amendment to previously pushed commit 01ac170ba29a (ACPI / LPSS:
allow to use specific PM domain during ->probe()). We can't assign anything to
the platform device on ADD_DEVICE stage since it might be changed during
unbound / bind cycle.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/acpi/acpi_lpss.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index f9e0d09..e840229 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -705,8 +705,14 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
}
switch (action) {
- case BUS_NOTIFY_ADD_DEVICE:
+ case BUS_NOTIFY_BIND_DRIVER:
pdev->dev.pm_domain = &acpi_lpss_pm_domain;
+ break;
+ case BUS_NOTIFY_BIND_DRIVER_ERROR:
+ case BUS_NOTIFY_UNBOUND_DRIVER:
+ pdev->dev.pm_domain = NULL;
+ break;
+ case BUS_NOTIFY_ADD_DEVICE:
if (pdata->dev_desc->flags & LPSS_LTR)
return sysfs_create_group(&pdev->dev.kobj,
&lpss_attr_group);
@@ -714,7 +720,6 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
case BUS_NOTIFY_DEL_DEVICE:
if (pdata->dev_desc->flags & LPSS_LTR)
sysfs_remove_group(&pdev->dev.kobj, &lpss_attr_group);
- pdev->dev.pm_domain = NULL;
break;
default:
break;
--
2.6.2
The LPSS DMA device has no context to save, though it requires the same delay
like the rest of LPSS devices when power state is changed from D3 to D0.
Do delay for the DMA device as well.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/acpi/acpi_lpss.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index e840229..a155dec 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -574,6 +574,17 @@ static void acpi_lpss_restore_ctx(struct device *dev,
{
unsigned int i;
+ for (i = 0; i < LPSS_PRV_REG_COUNT; i++) {
+ unsigned long offset = i * sizeof(u32);
+
+ __lpss_reg_write(pdata->prv_reg_ctx[i], pdata, offset);
+ dev_dbg(dev, "restoring 0x%08x to LPSS reg at offset 0x%02lx\n",
+ pdata->prv_reg_ctx[i], offset);
+ }
+}
+
+static void acpi_lpss_d3_to_d0_delay(struct lpss_private_data *pdata)
+{
/*
* The following delay is needed or the subsequent write operations may
* fail. The LPSS devices are actually PCI devices and the PCI spec
@@ -586,14 +597,6 @@ static void acpi_lpss_restore_ctx(struct device *dev,
delay = 0;
msleep(delay);
-
- for (i = 0; i < LPSS_PRV_REG_COUNT; i++) {
- unsigned long offset = i * sizeof(u32);
-
- __lpss_reg_write(pdata->prv_reg_ctx[i], pdata, offset);
- dev_dbg(dev, "restoring 0x%08x to LPSS reg at offset 0x%02lx\n",
- pdata->prv_reg_ctx[i], offset);
- }
}
#ifdef CONFIG_PM_SLEEP
@@ -621,6 +624,8 @@ static int acpi_lpss_resume_early(struct device *dev)
if (ret)
return ret;
+ acpi_lpss_d3_to_d0_delay(pdata);
+
if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
acpi_lpss_restore_ctx(dev, pdata);
@@ -652,6 +657,8 @@ static int acpi_lpss_runtime_resume(struct device *dev)
if (ret)
return ret;
+ acpi_lpss_d3_to_d0_delay(pdata);
+
if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
acpi_lpss_restore_ctx(dev, pdata);
--
2.6.2
This is a third approach to workaround long standing issue with LPSS on
BayTrail. First one [1] was reverted since it didn't resolve the issue
comprehensively. Second one [2] was rejected by internal review.
The LPSS DMA controller does not have neither _PS0 nor _PS3 method. Moreover it
can be powered off automatically whenever the last LPSS device goes down. In
case of no power any access to the DMA controller will hang the system. The
behaviour is reproduced on some HP laptops based on Intel BayTrail [3,4] as
well as on ASuS T100TA transformer.
Power on the LPSS island through the registers accessible in a specific way.
[1] http://www.spinics.net/lists/linux-acpi/msg53963.html
[2] https://bugzilla.redhat.com/attachment.cgi?id=1066779&action=diff
[3] https://bugzilla.redhat.com/show_bug.cgi?id=1184273
[4] http://www.spinics.net/lists/dmaengine/msg01514.html
Signed-off-by: Andy Shevchenko <[email protected]>
---
arch/x86/Kconfig | 3 +-
arch/x86/include/asm/iosf_mbi.h | 2 +
drivers/acpi/acpi_lpss.c | 152 ++++++++++++++++++++++++++++++++++++++--
3 files changed, 149 insertions(+), 8 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 324ac56..7fab0b9 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -535,9 +535,10 @@ config X86_INTEL_QUARK
config X86_INTEL_LPSS
bool "Intel Low Power Subsystem Support"
- depends on ACPI
+ depends on X86 && ACPI
select COMMON_CLK
select PINCTRL
+ select IOSF_MBI
---help---
Select to build support for Intel Low Power Subsystem such as
found on Intel Lynxpoint PCH. Selecting this option enables
diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
index cdc5f63..b41ee16 100644
--- a/arch/x86/include/asm/iosf_mbi.h
+++ b/arch/x86/include/asm/iosf_mbi.h
@@ -19,6 +19,8 @@
/* IOSF SB read/write opcodes */
#define MBI_MMIO_READ 0x00
#define MBI_MMIO_WRITE 0x01
+#define MBI_CFG_READ 0x04
+#define MBI_CFG_WRITE 0x05
#define MBI_CR_READ 0x06
#define MBI_CR_WRITE 0x07
#define MBI_REG_READ 0x10
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index a155dec..49d954b 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -15,11 +15,16 @@
#include <linux/clk-provider.h>
#include <linux/err.h>
#include <linux/io.h>
+#include <linux/mutex.h>
#include <linux/platform_device.h>
#include <linux/platform_data/clk-lpss.h>
#include <linux/pm_runtime.h>
#include <linux/delay.h>
+#include <asm/cpu_device_id.h>
+#include <asm/iosf_mbi.h>
+#include <asm/pmc_atom.h>
+
#include "internal.h"
ACPI_MODULE_NAME("acpi_lpss");
@@ -71,7 +76,7 @@ struct lpss_device_desc {
void (*setup)(struct lpss_private_data *pdata);
};
-static struct lpss_device_desc lpss_dma_desc = {
+static const struct lpss_device_desc lpss_dma_desc = {
.flags = LPSS_CLK,
};
@@ -84,6 +89,105 @@ struct lpss_private_data {
u32 prv_reg_ctx[LPSS_PRV_REG_COUNT];
};
+/* LPSS run time quirks */
+static unsigned int lpss_quirks;
+static DEFINE_MUTEX(lpss_iosf_mutex);
+
+/*
+ * LPSS_QUIRK_ALWAYS_POWER_ON: override power state for LPSS DMA device.
+ *
+ * The LPSS DMA controller does not have neither _PS0 nor _PS3 method. Moreover
+ * it can be powered off automatically whenever the last LPSS device goes down.
+ * In case of no power any access to the DMA controller will hang the system.
+ * The behaviour is reproduced on some HP laptops based on Intel BayTrail as
+ * well as on ASuS T100TA transformer.
+ *
+ * This quirk overrides power state of entire LPSS island to keep DMA powered
+ * on whenever we have at least one other device in use.
+ */
+#define LPSS_QUIRK_ALWAYS_POWER_ON BIT(0)
+
+/* IOSF SB for LPSS island */
+#define LPSS_IOSF_UNIT_LPIOEP 0xA0
+#define LPSS_IOSF_UNIT_LPIO1 0xAB
+#define LPSS_IOSF_UNIT_LPIO2 0xAC
+
+#define LPSS_IOSF_PMCSR 0x84
+#define LPSS_PMCSR_D0 0
+#define LPSS_PMCSR_D3hot 3
+#define LPSS_PMCSR_Dx_MASK GENMASK(1, 0)
+
+#define LPSS_IOSF_GPIODEF0 0x154
+#define LPSS_GPIODEF0_DMA1_D3 BIT(2)
+#define LPSS_GPIODEF0_DMA2_D3 BIT(3)
+#define LPSS_GPIODEF0_DMA_D3_MASK GENMASK(3, 2)
+
+static void lpss_iosf_enter_d3_state(void)
+{
+ u32 value1 = 0;
+ u32 mask1 = LPSS_GPIODEF0_DMA_D3_MASK;
+ u32 value2 = LPSS_PMCSR_D3hot;
+ u32 mask2 = LPSS_PMCSR_Dx_MASK;
+ /*
+ * PMC provides an information about actual status of the LPSS devices.
+ * Here we read the values related to LPSS power island, i.e. LPSS
+ * devices, excluding both LPSS DMA controllers, along with SCC domain.
+ */
+ u32 func_dis, d3_sts_0, pmc_status, pmc_mask = 0xfe000ffe;
+ int ret;
+
+ ret = pmc_atom_read(PMC_FUNC_DIS, &func_dis);
+ if (ret)
+ return;
+
+ mutex_lock(&lpss_iosf_mutex);
+
+ ret = pmc_atom_read(PMC_D3_STS_0, &d3_sts_0);
+ if (ret)
+ goto exit;
+
+ /*
+ * Get the status of entire LPSS power island per device basis.
+ * Shutdown both LPSS DMA controllers if and only if all other devices
+ * are already in D3hot.
+ */
+ pmc_status = (~(d3_sts_0 | func_dis)) & pmc_mask;
+ if (pmc_status)
+ goto exit;
+
+ iosf_mbi_modify(LPSS_IOSF_UNIT_LPIO1, MBI_CFG_WRITE,
+ LPSS_IOSF_PMCSR, value2, mask2);
+
+ iosf_mbi_modify(LPSS_IOSF_UNIT_LPIO2, MBI_CFG_WRITE,
+ LPSS_IOSF_PMCSR, value2, mask2);
+
+ iosf_mbi_modify(LPSS_IOSF_UNIT_LPIOEP, MBI_CR_WRITE,
+ LPSS_IOSF_GPIODEF0, value1, mask1);
+exit:
+ mutex_unlock(&lpss_iosf_mutex);
+}
+
+static void lpss_iosf_exit_d3_state(void)
+{
+ u32 value1 = LPSS_GPIODEF0_DMA1_D3 | LPSS_GPIODEF0_DMA2_D3;
+ u32 mask1 = LPSS_GPIODEF0_DMA_D3_MASK;
+ u32 value2 = LPSS_PMCSR_D0;
+ u32 mask2 = LPSS_PMCSR_Dx_MASK;
+
+ mutex_lock(&lpss_iosf_mutex);
+
+ iosf_mbi_modify(LPSS_IOSF_UNIT_LPIOEP, MBI_CR_WRITE,
+ LPSS_IOSF_GPIODEF0, value1, mask1);
+
+ iosf_mbi_modify(LPSS_IOSF_UNIT_LPIO2, MBI_CFG_WRITE,
+ LPSS_IOSF_PMCSR, value2, mask2);
+
+ iosf_mbi_modify(LPSS_IOSF_UNIT_LPIO1, MBI_CFG_WRITE,
+ LPSS_IOSF_PMCSR, value2, mask2);
+
+ mutex_unlock(&lpss_iosf_mutex);
+}
+
/* UART Component Parameter Register */
#define LPSS_UART_CPR 0xF4
#define LPSS_UART_CPR_AFCE BIT(4)
@@ -196,13 +300,21 @@ static const struct lpss_device_desc bsw_i2c_dev_desc = {
.setup = byt_i2c_setup,
};
-static struct lpss_device_desc bsw_spi_dev_desc = {
+static const struct lpss_device_desc bsw_spi_dev_desc = {
.flags = LPSS_CLK | LPSS_CLK_GATE | LPSS_CLK_DIVIDER | LPSS_SAVE_CTX
| LPSS_NO_D3_DELAY,
.prv_offset = 0x400,
.setup = lpss_deassert_reset,
};
+#define ICPU(model) { X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, }
+
+static const struct x86_cpu_id lpss_cpu_ids[] = {
+ ICPU(0x37), /* Valleyview, Bay Trail */
+ ICPU(0x4c), /* Braswell, Cherry Trail */
+ {}
+};
+
#else
#define LPSS_ADDR(desc) (0UL)
@@ -645,7 +757,17 @@ static int acpi_lpss_runtime_suspend(struct device *dev)
if (pdata->dev_desc->flags & LPSS_SAVE_CTX)
acpi_lpss_save_ctx(dev, pdata);
- return acpi_dev_runtime_suspend(dev);
+ ret = acpi_dev_runtime_suspend(dev);
+
+ /*
+ * This call must be last in the sequence, otherwise PMC will return
+ * wrong status for devices being about to be powered off. See
+ * lpss_iosf_enter_d3_state() for further information.
+ */
+ if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
+ lpss_iosf_enter_d3_state();
+
+ return ret;
}
static int acpi_lpss_runtime_resume(struct device *dev)
@@ -653,6 +775,13 @@ static int acpi_lpss_runtime_resume(struct device *dev)
struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
int ret;
+ /*
+ * This call is kept first to be in symmetry with
+ * acpi_lpss_runtime_suspend() one.
+ */
+ if (lpss_quirks & LPSS_QUIRK_ALWAYS_POWER_ON && iosf_mbi_available())
+ lpss_iosf_exit_d3_state();
+
ret = acpi_dev_runtime_resume(dev);
if (ret)
return ret;
@@ -766,10 +895,19 @@ static struct acpi_scan_handler lpss_handler = {
void __init acpi_lpss_init(void)
{
- if (!lpt_clk_init()) {
- bus_register_notifier(&platform_bus_type, &acpi_lpss_nb);
- acpi_scan_add_handler(&lpss_handler);
- }
+ const struct x86_cpu_id *id;
+ int ret;
+
+ ret = lpt_clk_init();
+ if (ret)
+ return;
+
+ id = x86_match_cpu(lpss_cpu_ids);
+ if (id)
+ lpss_quirks |= LPSS_QUIRK_ALWAYS_POWER_ON;
+
+ bus_register_notifier(&platform_bus_type, &acpi_lpss_nb);
+ acpi_scan_add_handler(&lpss_handler);
}
#else
--
2.6.2
We have to call dw_dma_disable() to stop any ongoing transfer. On some
platforms we can't do that since DMA device is powered off. Moreover we have no
possibility at that point to check if the platform is affected or not. That's
why we call pm_runtime_get_sync() / pm_runtime_put() unconditionally. On the
other hand we can't use pm_runtime_suspended() because runtime PM framework is
not fully used by the driver.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/dma/dw/platform.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
index 68a4815..d0734e9 100644
--- a/drivers/dma/dw/platform.c
+++ b/drivers/dma/dw/platform.c
@@ -239,7 +239,19 @@ static void dw_shutdown(struct platform_device *pdev)
{
struct dw_dma_chip *chip = platform_get_drvdata(pdev);
+ /*
+ * We have to call dw_dma_disable() to stop any ongoing transfer. On
+ * some platforms we can't do that since DMA device is powered off.
+ * Moreover we have no possibility to check if the platform is affected
+ * or not. That's why we call pm_runtime_get_sync() / pm_runtime_put()
+ * unconditionally. On the other hand we can't use
+ * pm_runtime_suspended() because runtime PM framework is not fully
+ * used by the driver.
+ */
+ pm_runtime_get_sync(chip->dev);
dw_dma_disable(chip);
+ pm_runtime_put_sync_suspend(chip->dev);
+
clk_disable_unprepare(chip->clk);
}
--
2.6.2
There is no need to bother the hardware when all channels are idle. We have not
to get any interrupts.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/dma/dw/core.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index 7067b6d..8b20930 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -622,12 +622,17 @@ static void dw_dma_tasklet(unsigned long data)
static irqreturn_t dw_dma_interrupt(int irq, void *dev_id)
{
struct dw_dma *dw = dev_id;
- u32 status = dma_readl(dw, STATUS_INT);
+ u32 status;
+ /* Check if we have any interrupt from the DMAC which is not in use */
+ if (!dw->in_use)
+ return IRQ_NONE;
+
+ status = dma_readl(dw, STATUS_INT);
dev_vdbg(dw->dma.dev, "%s: status=0x%x\n", __func__, status);
/* Check if we have any interrupt from the DMAC */
- if (!status || !dw->in_use)
+ if (!status)
return IRQ_NONE;
/*
--
2.6.2
Since we have a work around to prevent a system hangup we don't need to provide
a platform data explicitly anymore.
This reverts commit 175267b389f781748e2bbb6c737e76b5c9bc4c88.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/dma/dw/platform.c | 17 +----------------
1 file changed, 1 insertion(+), 16 deletions(-)
diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
index d0734e9..127093a 100644
--- a/drivers/dma/dw/platform.c
+++ b/drivers/dma/dw/platform.c
@@ -155,7 +155,6 @@ static int dw_probe(struct platform_device *pdev)
struct dw_dma_chip *chip;
struct device *dev = &pdev->dev;
struct resource *mem;
- const struct acpi_device_id *id;
struct dw_dma_platform_data *pdata;
int err;
@@ -179,11 +178,6 @@ static int dw_probe(struct platform_device *pdev)
pdata = dev_get_platdata(dev);
if (!pdata)
pdata = dw_dma_parse_dt(pdev);
- if (!pdata && has_acpi_companion(dev)) {
- id = acpi_match_device(dev->driver->acpi_match_table, dev);
- if (id)
- pdata = (struct dw_dma_platform_data *)id->driver_data;
- }
chip->dev = dev;
@@ -264,17 +258,8 @@ MODULE_DEVICE_TABLE(of, dw_dma_of_id_table);
#endif
#ifdef CONFIG_ACPI
-static struct dw_dma_platform_data dw_dma_acpi_pdata = {
- .nr_channels = 8,
- .is_private = true,
- .chan_allocation_order = CHAN_ALLOCATION_ASCENDING,
- .chan_priority = CHAN_PRIORITY_ASCENDING,
- .block_size = 4095,
- .nr_masters = 2,
-};
-
static const struct acpi_device_id dw_dma_acpi_id_table[] = {
- { "INTL9C60", (kernel_ulong_t)&dw_dma_acpi_pdata },
+ { "INTL9C60", 0 },
{ }
};
MODULE_DEVICE_TABLE(acpi, dw_dma_acpi_id_table);
--
2.6.2
On 11/26/2015 05:19 PM, Andy Shevchenko wrote:
> This is an amendment to previously pushed commit 01ac170ba29a (ACPI / LPSS:
> allow to use specific PM domain during ->probe()). We can't assign anything to
> the platform device on ADD_DEVICE stage since it might be changed during
> unbound / bind cycle.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/acpi/acpi_lpss.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index f9e0d09..e840229 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -705,8 +705,14 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
> }
>
> switch (action) {
> - case BUS_NOTIFY_ADD_DEVICE:
> + case BUS_NOTIFY_BIND_DRIVER:
> pdev->dev.pm_domain = &acpi_lpss_pm_domain;
> + break;
> + case BUS_NOTIFY_BIND_DRIVER_ERROR:
> + case BUS_NOTIFY_UNBOUND_DRIVER:
> + pdev->dev.pm_domain = NULL;
> + break;
> + case BUS_NOTIFY_ADD_DEVICE:
This won't fix like revert of original commit does. Primary problem here
is that there is no explicit power on at all during LPSS device probe
because dev->pm_domain is set before probing.
driver_probe_device
platform_drv_prove
dev_pm_domain_attach
acpi_dev_pm_attach
returns instantly because of dev->pm_domain is set
Problem got unnoticed because devices are on during boot but
rmmod/modprobe cycle will hit it.
With this patch situation remains the same but works if you assign the
domain after probing when BUS_NOTIFY_BOUND_DRIVER notification comes
(s/BUS_NOTIFY_BIND_DRIVER/BUS_NOTIFY_BOUND_DRIVER/ above).
--
Jarkko
On Thu, 2015-11-26 at 18:30 +0200, Jarkko Nikula wrote:
> On 11/26/2015 05:19 PM, Andy Shevchenko wrote:
> > This is an amendment to previously pushed commit 01ac170ba29a (ACPI
> > / LPSS:
> > allow to use specific PM domain during ->probe()). We can't assign
> > anything to
> > the platform device on ADD_DEVICE stage since it might be changed
> > during
> > unbound / bind cycle.
> >
> > Signed-off-by: Andy Shevchenko <[email protected]>
> > ---
> > drivers/acpi/acpi_lpss.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> > index f9e0d09..e840229 100644
> > --- a/drivers/acpi/acpi_lpss.c
> > +++ b/drivers/acpi/acpi_lpss.c
> > @@ -705,8 +705,14 @@ static int acpi_lpss_platform_notify(struct
> > notifier_block *nb,
> > }
> >
> > switch (action) {
> > - case BUS_NOTIFY_ADD_DEVICE:
> > + case BUS_NOTIFY_BIND_DRIVER:
> > pdev->dev.pm_domain = &acpi_lpss_pm_domain;
> > + break;
> > + case BUS_NOTIFY_BIND_DRIVER_ERROR:
> > + case BUS_NOTIFY_UNBOUND_DRIVER:
> > + pdev->dev.pm_domain = NULL;
> > + break;
> > + case BUS_NOTIFY_ADD_DEVICE:
>
> This won't fix like revert of original commit does. Primary problem
> here
> is that there is no explicit power on at all during LPSS device probe
> because dev->pm_domain is set before probing.
And we can't do this as in very original code of acpi_lpss.c since DMA
has to be sure it's powered on while probing. We could guarantee this
only in case when PM domain is assigned already and we do our quirk for
it.
>From my point of view we have to fix hang first since it's most painful
case for users and their experience. Though I'm open to any better
solution if you have any in mind.
>
> driver_probe_device
> platform_drv_prove
> dev_pm_domain_attach
> acpi_dev_pm_attach
> returns instantly because of dev->pm_domain is set
>
> Problem got unnoticed because devices are on during boot but
> rmmod/modprobe cycle will hit it.
>
> With this patch situation remains the same but works if you assign
> the
> domain after probing when BUS_NOTIFY_BOUND_DRIVER notification comes
> (s/BUS_NOTIFY_BIND_DRIVER/BUS_NOTIFY_BOUND_DRIVER/ above).
>
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
On Thu, Nov 26, 2015 at 05:19:11PM +0200, Andy Shevchenko wrote:
> We have to call dw_dma_disable() to stop any ongoing transfer.
Ok
> On some platforms we can't do that since DMA device is powered off.
Umm, you said we have ongoing transfer which means DMA should be on..!!
> Moreover we have no
> possibility at that point to check if the platform is affected or not. That's
> why we call pm_runtime_get_sync() / pm_runtime_put() unconditionally. On the
> other hand we can't use pm_runtime_suspended() because runtime PM framework is
> not fully used by the driver.
Shouldn't that be fixed?
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/dma/dw/platform.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
> index 68a4815..d0734e9 100644
> --- a/drivers/dma/dw/platform.c
> +++ b/drivers/dma/dw/platform.c
> @@ -239,7 +239,19 @@ static void dw_shutdown(struct platform_device *pdev)
> {
> struct dw_dma_chip *chip = platform_get_drvdata(pdev);
>
> + /*
> + * We have to call dw_dma_disable() to stop any ongoing transfer. On
> + * some platforms we can't do that since DMA device is powered off.
> + * Moreover we have no possibility to check if the platform is affected
> + * or not. That's why we call pm_runtime_get_sync() / pm_runtime_put()
> + * unconditionally. On the other hand we can't use
> + * pm_runtime_suspended() because runtime PM framework is not fully
> + * used by the driver.
> + */
> + pm_runtime_get_sync(chip->dev);
> dw_dma_disable(chip);
> + pm_runtime_put_sync_suspend(chip->dev);
> +
> clk_disable_unprepare(chip->clk);
> }
>
> --
> 2.6.2
>
--
~Vinod
On Thu, 2015-11-26 at 22:31 +0530, Vinod Koul wrote:
> On Thu, Nov 26, 2015 at 05:19:11PM +0200, Andy Shevchenko wrote:
> > We have to call dw_dma_disable() to stop any ongoing transfer.
>
> Ok
>
> > On some platforms we can't do that since DMA device is powered off.
>
> Umm, you said we have ongoing transfer which means DMA should be
> on..!!
Yes, that's true for HSW/BDW and non-affected BYT/CHT.
Like I mentioned here is no possibility to know which platform we run
on.
Would you like to test this on a real device? We can provide you a
login.
>
> > Moreover we have no
> > possibility at that point to check if the platform is affected or
> > not. That's
> > why we call pm_runtime_get_sync() / pm_runtime_put()
> > unconditionally. On the
> > other hand we can't use pm_runtime_suspended() because runtime PM
> > framework is
> > not fully used by the driver.
>
> Shouldn't that be fixed?
Do you have any solution how?
Rough approach is to turn on it on channel allocation and turn off on
freeing resources. The obvious downside of this solution is power
consumption of idling device.
In any case it might be done in the future and it's not a scope of this
series.
>
> >
> > Signed-off-by: Andy Shevchenko <[email protected]>
> > ---
> > drivers/dma/dw/platform.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
> > index 68a4815..d0734e9 100644
> > --- a/drivers/dma/dw/platform.c
> > +++ b/drivers/dma/dw/platform.c
> > @@ -239,7 +239,19 @@ static void dw_shutdown(struct platform_device
> > *pdev)
> > {
> > struct dw_dma_chip *chip = platform_get_drvdata(pdev);
> >
> > + /*
> > + * We have to call dw_dma_disable() to stop any ongoing
> > transfer. On
> > + * some platforms we can't do that since DMA device is
> > powered off.
> > + * Moreover we have no possibility to check if the
> > platform is affected
> > + * or not. That's why we call pm_runtime_get_sync() /
> > pm_runtime_put()
> > + * unconditionally. On the other hand we can't use
> > + * pm_runtime_suspended() because runtime PM framework is
> > not fully
> > + * used by the driver.
> > + */
> > + pm_runtime_get_sync(chip->dev);
> > dw_dma_disable(chip);
> > + pm_runtime_put_sync_suspend(chip->dev);
> > +
> > clk_disable_unprepare(chip->clk);
> > }
> >
> > --
> > 2.6.2
> >
>
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
On Thu, Nov 26, 2015 at 07:24:48PM +0200, Andy Shevchenko wrote:
> On Thu, 2015-11-26 at 22:31 +0530, Vinod Koul wrote:
> > On Thu, Nov 26, 2015 at 05:19:11PM +0200, Andy Shevchenko wrote:
> > > We have to call dw_dma_disable() to stop any ongoing transfer.
> >
> > Ok
> >
> > > On some platforms we can't do that since DMA device is powered off.
> >
> > Umm, you said we have ongoing transfer which means DMA should be
> > on..!!
>
> Yes, that's true for HSW/BDW and non-affected BYT/CHT.
Can you please explain even when DMA is in use how can device be powered
off? That does not sound right to me. Is this on GP DMA on BYT/CHT or
something else?
>
> Like I mentioned here is no possibility to know which platform we run
> on.
>
> Would you like to test this on a real device? We can provide you a
> login.
>
> >
> > > Moreover we have no
> > > possibility at that point to check if the platform is affected or
> > > not. That's
> > > why we call pm_runtime_get_sync() / pm_runtime_put()
> > > unconditionally. On the
> > > other hand we can't use pm_runtime_suspended() because runtime PM
> > > framework is
> > > not fully used by the driver.
> >
> > Shouldn't that be fixed?
>
> Do you have any solution how?
>
> Rough approach is to turn on it on channel allocation and turn off on
> freeing resources. The obvious downside of this solution is power
> consumption of idling device.
But in that case, the clients should not hold ref of dma chan when idle and
allocate only when required which is a resonable expectation
>
> In any case it might be done in the future and it's not a scope of this
> series.
Sounds fine to me
>
> >
> > >
> > > Signed-off-by: Andy Shevchenko <[email protected]>
> > > ---
> > > ?drivers/dma/dw/platform.c | 12 ++++++++++++
> > > ?1 file changed, 12 insertions(+)
> > >
> > > diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
> > > index 68a4815..d0734e9 100644
> > > --- a/drivers/dma/dw/platform.c
> > > +++ b/drivers/dma/dw/platform.c
> > > @@ -239,7 +239,19 @@ static void dw_shutdown(struct platform_device
> > > *pdev)
> > > ?{
> > > ? struct dw_dma_chip *chip = platform_get_drvdata(pdev);
> > > ?
> > > + /*
> > > + ?* We have to call dw_dma_disable() to stop any ongoing
> > > transfer. On
> > > + ?* some platforms we can't do that since DMA device is
> > > powered off.
> > > + ?* Moreover we have no possibility to check if the
> > > platform is affected
> > > + ?* or not. That's why we call pm_runtime_get_sync() /
> > > pm_runtime_put()
> > > + ?* unconditionally. On the other hand we can't use
> > > + ?* pm_runtime_suspended() because runtime PM framework is
> > > not fully
> > > + ?* used by the driver.
> > > + ?*/
> > > + pm_runtime_get_sync(chip->dev);
> > > ? dw_dma_disable(chip);
> > > + pm_runtime_put_sync_suspend(chip->dev);
> > > +
> > > ? clk_disable_unprepare(chip->clk);
> > > ?}
> > > ?
> > > --
> > > 2.6.2
> > >
> >
>
> --
> Andy Shevchenko <[email protected]>
> Intel Finland Oy
>
--
~Vinod
On Thu, 2015-11-26 at 23:11 +0530, Vinod Koul wrote:
> On Thu, Nov 26, 2015 at 07:24:48PM +0200, Andy Shevchenko wrote:
> > On Thu, 2015-11-26 at 22:31 +0530, Vinod Koul wrote:
> > > On Thu, Nov 26, 2015 at 05:19:11PM +0200, Andy Shevchenko wrote:
> > > > We have to call dw_dma_disable() to stop any ongoing transfer.
> > >
> > > Ok
> > >
> > > > On some platforms we can't do that since DMA device is powered
> > > > off.
> > >
> > > Umm, you said we have ongoing transfer which means DMA should be
> > > on..!!
> >
> > Yes, that's true for HSW/BDW and non-affected BYT/CHT.
>
> Can you please explain even when DMA is in use how can device be
> powered
> off? That does not sound right to me.
It can't, but the problem is we can't distinguish that in this routine!
We simple do *not* know the actual power state of DMA.
These calls *ensures* that DMA is powered on. Yes, the call to
dw_dma_off() when it used to be powered off sounds silly, I agree,
though I see no upstreamable (non-hackish) solution for that.
Previously I proposed to remove .shutdown hook completely, you were
objecting.
> Is this on GP DMA on BYT/CHT or
> something else?
Correct. Affected platforms are BYT-T and some or all of BSW/CHT
depending on firmware in use.
>
> > Like I mentioned here is no possibility to know which platform we
> > run
> > on.
> >
> > Would you like to test this on a real device? We can provide you a
> > login.
> >
> > >
> > > > Moreover we have no
> > > > possibility at that point to check if the platform is affected
> > > > or
> > > > not. That's
> > > > why we call pm_runtime_get_sync() / pm_runtime_put()
> > > > unconditionally. On the
> > > > other hand we can't use pm_runtime_suspended() because runtime
> > > > PM
> > > > framework is
> > > > not fully used by the driver.
> > >
> > > Shouldn't that be fixed?
> >
> > Do you have any solution how?
> >
> > Rough approach is to turn on it on channel allocation and turn off
> > on
> > freeing resources. The obvious downside of this solution is power
> > consumption of idling device.
>
> But in that case, the clients should not hold ref of dma chan when
> idle and
> allocate only when required which is a resonable expectation
There is not the case for few drivers. At least for us it's spi-pxa2xx
one. It requires channels on its ->probe() stage. Jarkko is Cc'ed here,
you may ask him as he is our maintainer for the SPI.
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
On Thu, 2015-11-26 at 19:58 +0200, Andy Shevchenko wrote:
> On Thu, 2015-11-26 at 23:11 +0530, Vinod Koul wrote:
> > On Thu, Nov 26, 2015 at 07:24:48PM +0200, Andy Shevchenko wrote:
> > > On Thu, 2015-11-26 at 22:31 +0530, Vinod Koul wrote:
> > > > On Thu, Nov 26, 2015 at 05:19:11PM +0200, Andy Shevchenko
> > > > wrote:
> > > > > We have to call dw_dma_disable() to stop any ongoing
> > > > > transfer.
> > > >
> > > > Ok
> > > >
> > > > > On some platforms we can't do that since DMA device is
> > > > > powered
> > > > > off.
> > > >
> > > > Umm, you said we have ongoing transfer which means DMA should
> > > > be
> > > > on..!!
> > >
> > > Yes, that's true for HSW/BDW and non-affected BYT/CHT.
> >
> > Can you please explain even when DMA is in use how can device be
> > powered
> > off? That does not sound right to me.
>
> It can't, but the problem is we can't distinguish that in this
> routine!
> We simple do *not* know the actual power state of DMA.
>
> These calls *ensures* that DMA is powered on. Yes, the call to
> dw_dma_off() when it used to be powered off sounds silly, I agree,
> though I see no upstreamable (non-hackish) solution for that.
>
> Previously I proposed to remove .shutdown hook completely, you were
> objecting.
And second approach with PMC involved was also rejected by you since it
was too hackish, which I completely agree with.
>
> > Is this on GP DMA on BYT/CHT or
> > something else?
>
> Correct. Affected platforms are BYT-T and some or all of BSW/CHT
> depending on firmware in use.
>
> >
> > > Like I mentioned here is no possibility to know which platform we
> > > run
> > > on.
> > >
> > > Would you like to test this on a real device? We can provide you
> > > a
> > > login.
> > >
> > > >
> > > > > Moreover we have no
> > > > > possibility at that point to check if the platform is
> > > > > affected
> > > > > or
> > > > > not. That's
> > > > > why we call pm_runtime_get_sync() / pm_runtime_put()
> > > > > unconditionally. On the
> > > > > other hand we can't use pm_runtime_suspended() because
> > > > > runtime
> > > > > PM
> > > > > framework is
> > > > > not fully used by the driver.
> > > >
> > > > Shouldn't that be fixed?
> > >
> > > Do you have any solution how?
> > >
> > > Rough approach is to turn on it on channel allocation and turn
> > > off
> > > on
> > > freeing resources. The obvious downside of this solution is power
> > > consumption of idling device.
> >
> > But in that case, the clients should not hold ref of dma chan when
> > idle and
> > allocate only when required which is a resonable expectation
>
> There is not the case for few drivers. At least for us it's spi-
> pxa2xx
> one. It requires channels on its ->probe() stage. Jarkko is Cc'ed
> here,
> you may ask him as he is our maintainer for the SPI.
>
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Thursday, November 26, 2015 05:19:07 PM Andy Shevchenko wrote:
> In case ->probe() fails the notifier does not inform a subscriber about this.
> In the result it might happend that some resources that had been allocated will
> stay allocated and therefore lead to resource leak.
>
> Introduce a new notification to inform the subscriber that ->probe() failed.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
I'd rather say the problem is that the users of BUS_NOTIFY_BIND_DRIVER have no
chance to do any cleanup in case of a probe failure (there may be problems even
if resources aren't leaked).
> ---
> drivers/base/dd.c | 8 ++++++--
> include/linux/device.h | 1 +
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index a641cf3..ac071a5 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -290,7 +290,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
> /* If using pinctrl, bind pins now before probing */
> ret = pinctrl_bind_pins(dev);
> if (ret)
> - goto probe_failed;
> + goto pinctrl_bind_failed;
>
> if (driver_sysfs_add(dev)) {
> printk(KERN_ERR "%s: driver_sysfs_add(%s) failed\n",
> @@ -334,6 +334,11 @@ static int really_probe(struct device *dev, struct device_driver *drv)
> goto done;
>
> probe_failed:
> + if (dev->bus)
> + blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> + BUS_NOTIFY_BIND_DRIVER_ERROR,
> + dev);
Well, if we do that, device_bind_driver() needs to send that notification too
in case it doesn't call driver_bound().
> +pinctrl_bind_failed:
> devres_release_all(dev);
> driver_sysfs_remove(dev);
> dev->driver = NULL;
> @@ -701,7 +706,6 @@ static void __device_release_driver(struct device *dev)
> blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> BUS_NOTIFY_UNBOUND_DRIVER,
> dev);
> -
> }
> }
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index b8f411b..87cf423 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -191,6 +191,7 @@ extern int bus_unregister_notifier(struct bus_type *bus,
> unbound */
> #define BUS_NOTIFY_UNBOUND_DRIVER 0x00000007 /* driver is unbound
> from the device */
> +#define BUS_NOTIFY_BIND_DRIVER_ERROR 0x80000004 /* driver fails to be bound */
I'd call it BUS_NOTIFY_DRIVER_NOT_BOUND.
>
> extern struct kset *bus_get_kset(struct bus_type *bus);
> extern struct klist *bus_get_device_klist(struct bus_type *bus);
>
Thanks,
Rafael
On Thursday, November 26, 2015 06:45:17 PM Andy Shevchenko wrote:
> On Thu, 2015-11-26 at 18:30 +0200, Jarkko Nikula wrote:
> > On 11/26/2015 05:19 PM, Andy Shevchenko wrote:
> > > This is an amendment to previously pushed commit 01ac170ba29a (ACPI
> > > / LPSS:
> > > allow to use specific PM domain during ->probe()). We can't assign
> > > anything to
> > > the platform device on ADD_DEVICE stage since it might be changed
> > > during
> > > unbound / bind cycle.
> > >
> > > Signed-off-by: Andy Shevchenko <[email protected]>
> > > ---
> > > drivers/acpi/acpi_lpss.c | 9 +++++++--
> > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> > > index f9e0d09..e840229 100644
> > > --- a/drivers/acpi/acpi_lpss.c
> > > +++ b/drivers/acpi/acpi_lpss.c
> > > @@ -705,8 +705,14 @@ static int acpi_lpss_platform_notify(struct
> > > notifier_block *nb,
> > > }
> > >
> > > switch (action) {
> > > - case BUS_NOTIFY_ADD_DEVICE:
> > > + case BUS_NOTIFY_BIND_DRIVER:
> > > pdev->dev.pm_domain = &acpi_lpss_pm_domain;
> > > + break;
> > > + case BUS_NOTIFY_BIND_DRIVER_ERROR:
> > > + case BUS_NOTIFY_UNBOUND_DRIVER:
> > > + pdev->dev.pm_domain = NULL;
> > > + break;
> > > + case BUS_NOTIFY_ADD_DEVICE:
> >
> > This won't fix like revert of original commit does.
Which commit exactly are you talking about, for reference?
> > Primary problem here
> > is that there is no explicit power on at all during LPSS device probe
> > because dev->pm_domain is set before probing.
>
> And we can't do this as in very original code of acpi_lpss.c since DMA
> has to be sure it's powered on while probing. We could guarantee this
> only in case when PM domain is assigned already and we do our quirk for
> it.
>
> From my point of view we have to fix hang first since it's most painful
> case for users and their experience. Though I'm open to any better
> solution if you have any in mind.
>
> >
> > driver_probe_device
> > platform_drv_prove
> > dev_pm_domain_attach
> > acpi_dev_pm_attach
> > returns instantly because of dev->pm_domain is set
This looks like a candidate for the new PM domain callbacks, ->activate and
->dismiss.
->activate() is called before the probe, so it may power up things.
->dismiss() in turn is called in the failed probe case, so it can do the
cleanup.
Have you considered using these?
Thanks,
Rafael
On 11/26/2015 06:45 PM, Andy Shevchenko wrote:
> On Thu, 2015-11-26 at 18:30 +0200, Jarkko Nikula wrote:
>> On 11/26/2015 05:19 PM, Andy Shevchenko wrote:
>>> This is an amendment to previously pushed commit 01ac170ba29a (ACPI
>>> / LPSS:
>>> allow to use specific PM domain during ->probe()). We can't assign
>>> anything to
>>> the platform device on ADD_DEVICE stage since it might be changed
>>> during
>>> unbound / bind cycle.
>>>
>>> Signed-off-by: Andy Shevchenko <[email protected]>
>>> ---
>>> drivers/acpi/acpi_lpss.c | 9 +++++++--
>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
>>> index f9e0d09..e840229 100644
>>> --- a/drivers/acpi/acpi_lpss.c
>>> +++ b/drivers/acpi/acpi_lpss.c
>>> @@ -705,8 +705,14 @@ static int acpi_lpss_platform_notify(struct
>>> notifier_block *nb,
>>> }
>>>
>>> switch (action) {
>>> - case BUS_NOTIFY_ADD_DEVICE:
>>> + case BUS_NOTIFY_BIND_DRIVER:
>>> pdev->dev.pm_domain = &acpi_lpss_pm_domain;
>>> + break;
>>> + case BUS_NOTIFY_BIND_DRIVER_ERROR:
>>> + case BUS_NOTIFY_UNBOUND_DRIVER:
>>> + pdev->dev.pm_domain = NULL;
>>> + break;
>>> + case BUS_NOTIFY_ADD_DEVICE:
>>
>> This won't fix like revert of original commit does. Primary problem
>> here
>> is that there is no explicit power on at all during LPSS device probe
>> because dev->pm_domain is set before probing.
>
> And we can't do this as in very original code of acpi_lpss.c since DMA
> has to be sure it's powered on while probing. We could guarantee this
> only in case when PM domain is assigned already and we do our quirk for
> it.
>
I'm not sure do I follow here. Is the power on chain different for LPSS
DMA because setting the domain at BIND stage prevents the call to
acpi_dev_pm_full_power() before driver probe? See below.
driver_probe_device
really_probe
driver_sysfs_add -> BUS_NOTIFY_BIND_DRIVER
-> platform_drv_prove
dev_pm_domain_attach
acpi_dev_pm_attach
if (dev->pm_domain) return -EEXIST;
...
if (power_on) { acpi_dev_pm_full_power(adev);... }
-> probe
driver_bound -> BUS_NOTIFY_BOUND_DRIVER
> From my point of view we have to fix hang first since it's most painful
> case for users and their experience. Though I'm open to any better
> solution if you have any in mind.
>
Sure.
--
Jarkko
On Fri, 2015-11-27 at 00:09 +0100, Rafael J. Wysocki wrote:
> On Thursday, November 26, 2015 05:19:07 PM Andy Shevchenko wrote:
> > In case ->probe() fails the notifier does not inform a subscriber
> > about this.
> > In the result it might happend that some resources that had been
> > allocated will
> > stay allocated and therefore lead to resource leak.
> >
> > Introduce a new notification to inform the subscriber that
> > ->probe() failed.
> >
> > Signed-off-by: Andy Shevchenko <[email protected]>
>
> I'd rather say the problem is that the users of
> BUS_NOTIFY_BIND_DRIVER have no
> chance to do any cleanup in case of a probe failure (there may be
> problems even
> if resources aren't leaked).
Thanks, Rafael, all of your comments sound reasonable for me. Will be
taken into consideration in next version.
>
> > ---
> > drivers/base/dd.c | 8 ++++++--
> > include/linux/device.h | 1 +
> > 2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index a641cf3..ac071a5 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -290,7 +290,7 @@ static int really_probe(struct device *dev,
> > struct device_driver *drv)
> > /* If using pinctrl, bind pins now before probing */
> > ret = pinctrl_bind_pins(dev);
> > if (ret)
> > - goto probe_failed;
> > + goto pinctrl_bind_failed;
> >
> > if (driver_sysfs_add(dev)) {
> > printk(KERN_ERR "%s: driver_sysfs_add(%s)
> > failed\n",
> > @@ -334,6 +334,11 @@ static int really_probe(struct device *dev,
> > struct device_driver *drv)
> > goto done;
> >
> > probe_failed:
> > + if (dev->bus)
> > + blocking_notifier_call_chain(&dev->bus->p-
> > >bus_notifier,
> > + BUS_NOTIFY_BIND_DRIVE
> > R_ERROR,
> > + dev);
>
> Well, if we do that, device_bind_driver() needs to send that
> notification too
> in case it doesn't call driver_bound().
>
> > +pinctrl_bind_failed:
> > devres_release_all(dev);
> > driver_sysfs_remove(dev);
> > dev->driver = NULL;
> > @@ -701,7 +706,6 @@ static void __device_release_driver(struct
> > device *dev)
> > blocking_notifier_call_chain(&dev->bus->p-
> > >bus_notifier,
> > BUS_NOTIFY_UN
> > BOUND_DRIVER,
> > dev);
> > -
> > }
> > }
> >
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index b8f411b..87cf423 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -191,6 +191,7 @@ extern int bus_unregister_notifier(struct
> > bus_type *bus,
> > unbound */
> > #define BUS_NOTIFY_UNBOUND_DRIVER 0x00000007 /* driver is
> > unbound
> > from the
> > device */
> > +#define BUS_NOTIFY_BIND_DRIVER_ERROR 0x80000004 /* driver
> > fails to be bound */
>
> I'd call it BUS_NOTIFY_DRIVER_NOT_BOUND.
>
> >
> > extern struct kset *bus_get_kset(struct bus_type *bus);
> > extern struct klist *bus_get_device_klist(struct bus_type *bus);
> >
>
> Thanks,
> Rafael
>
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
On Fri, 2015-11-27 at 00:15 +0100, Rafael J. Wysocki wrote:
> On Thursday, November 26, 2015 06:45:17 PM Andy Shevchenko wrote:
> > On Thu, 2015-11-26 at 18:30 +0200, Jarkko Nikula wrote:
> > > On 11/26/2015 05:19 PM, Andy Shevchenko wrote:
> > > > This is an amendment to previously pushed commit 01ac170ba29a
> > > > (ACPI
> > > > / LPSS:
> > > > allow to use specific PM domain during ->probe()). We can't
> > > > assign
> > > > anything to
> > > > the platform device on ADD_DEVICE stage since it might be
> > > > changed
> > > > during
> > > > unbound / bind cycle.
> > > >
> > > > Signed-off-by: Andy Shevchenko <[email protected]
> > > > om>
> > > > ---
> > > > drivers/acpi/acpi_lpss.c | 9 +++++++--
> > > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/acpi_lpss.c
> > > > b/drivers/acpi/acpi_lpss.c
> > > > index f9e0d09..e840229 100644
> > > > --- a/drivers/acpi/acpi_lpss.c
> > > > +++ b/drivers/acpi/acpi_lpss.c
> > > > @@ -705,8 +705,14 @@ static int
> > > > acpi_lpss_platform_notify(struct
> > > > notifier_block *nb,
> > > > }
> > > >
> > > > switch (action) {
> > > > - case BUS_NOTIFY_ADD_DEVICE:
> > > > + case BUS_NOTIFY_BIND_DRIVER:
> > > > pdev->dev.pm_domain = &acpi_lpss_pm_domain;
> > > > + break;
> > > > + case BUS_NOTIFY_BIND_DRIVER_ERROR:
> > > > + case BUS_NOTIFY_UNBOUND_DRIVER:
> > > > + pdev->dev.pm_domain = NULL;
> > > > + break;
> > > > + case BUS_NOTIFY_ADD_DEVICE:
> > >
> > > This won't fix like revert of original commit does.
>
> Which commit exactly are you talking about, for reference?
commit 01ac170ba29a9903ee590e1ef2d8e6b27b49a16c
Author: Andy Shevchenko <[email protected]>
Date: Wed Nov 5 18:34:46 2014 +0200
ACPI / LPSS: allow to use specific PM domain during ->probe()
> > > Primary problem here
> > > is that there is no explicit power on at all during LPSS device
> > > probe
> > > because dev->pm_domain is set before probing.
> >
> > And we can't do this as in very original code of acpi_lpss.c since
> > DMA
> > has to be sure it's powered on while probing. We could guarantee
> > this
> > only in case when PM domain is assigned already and we do our quirk
> > for
> > it.
> >
> > From my point of view we have to fix hang first since it's most
> > painful
> > case for users and their experience. Though I'm open to any better
> > solution if you have any in mind.
> >
> > >
> > > driver_probe_device
> > > platform_drv_prove
> > > dev_pm_domain_attach
> > > acpi_dev_pm_attach
> > > returns instantly because of dev->pm_domain is set
>
> This looks like a candidate for the new PM domain callbacks,
> ->activate and
> ->dismiss.
>
> ->activate() is called before the probe, so it may power up things.
>
> ->dismiss() in turn is called in the failed probe case, so it can do
> the
> cleanup.
>
> Have you considered using these?
Thanks for the hint. We will check this.
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
On Fri, 2015-11-27 at 09:05 +0200, Jarkko Nikula wrote:
> On 11/26/2015 06:45 PM, Andy Shevchenko wrote:
> > On Thu, 2015-11-26 at 18:30 +0200, Jarkko Nikula wrote:
> > > On 11/26/2015 05:19 PM, Andy Shevchenko wrote:
> > > > This is an amendment to previously pushed commit 01ac170ba29a
> > > > (ACPI
> > > > / LPSS:
> > > > allow to use specific PM domain during ->probe()). We can't
> > > > assign
> > > > anything to
> > > > the platform device on ADD_DEVICE stage since it might be
> > > > changed
> > > > during
> > > > unbound / bind cycle.
> > > >
> > > > Signed-off-by: Andy Shevchenko <[email protected]
> > > > om>
> > > > ---
> > > > drivers/acpi/acpi_lpss.c | 9 +++++++--
> > > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/acpi_lpss.c
> > > > b/drivers/acpi/acpi_lpss.c
> > > > index f9e0d09..e840229 100644
> > > > --- a/drivers/acpi/acpi_lpss.c
> > > > +++ b/drivers/acpi/acpi_lpss.c
> > > > @@ -705,8 +705,14 @@ static int
> > > > acpi_lpss_platform_notify(struct
> > > > notifier_block *nb,
> > > > }
> > > >
> > > > switch (action) {
> > > > - case BUS_NOTIFY_ADD_DEVICE:
> > > > + case BUS_NOTIFY_BIND_DRIVER:
> > > > pdev->dev.pm_domain = &acpi_lpss_pm_domain;
> > > > + break;
> > > > + case BUS_NOTIFY_BIND_DRIVER_ERROR:
> > > > + case BUS_NOTIFY_UNBOUND_DRIVER:
> > > > + pdev->dev.pm_domain = NULL;
> > > > + break;
> > > > + case BUS_NOTIFY_ADD_DEVICE:
> > >
> > > This won't fix like revert of original commit does. Primary
> > > problem
> > > here
> > > is that there is no explicit power on at all during LPSS device
> > > probe
> > > because dev->pm_domain is set before probing.
> >
> > And we can't do this as in very original code of acpi_lpss.c since
> > DMA
> > has to be sure it's powered on while probing. We could guarantee
> > this
> > only in case when PM domain is assigned already and we do our quirk
> > for
> > it.
> >
> I'm not sure do I follow here. Is the power on chain different for
> LPSS
> DMA because setting the domain at BIND stage prevents the call to
> acpi_dev_pm_full_power() before driver probe? See below.
>
> driver_probe_device
> really_probe
> driver_sysfs_add -> BUS_NOTIFY_BIND_DRIVER
> -> platform_drv_prove
> dev_pm_domain_attach
> acpi_dev_pm_attach
> if (dev->pm_domain) return -EEXIST;
> ...
> if (power_on) { acpi_dev_pm_full_power(adev);... }
And how exactly does it enable a power on DMA controller that doesn't
have _PS0 / _PS3 / _PSC methods?
Maybe I missed something obvious.
> -> probe
> driver_bound -> BUS_NOTIFY_BOUND_DRIVER
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
On Fri, 2015-11-27 at 11:56 +0200, Andy Shevchenko wrote:
> > > > >
> On Fri, 2015-11-27 at 00:15 +0100, Rafael J. Wysocki wrote:
> > On Thursday, November 26, 2015 06:45:17 PM Andy Shevchenko wrote:
> > > On Thu, 2015-11-26 at 18:30 +0200, Jarkko Nikula wrote:
> > > > On 11/26/2015 05:19 PM, Andy Shevchenko wrote:
> > > > This won't fix like revert of original commit does.
Jarkko, I will split this one to the revert (with Fixes tag) and new
patch to target DMA issue.
> > > > Primary problem here
> > > > is that there is no explicit power on at all during LPSS device
> > > > probe
> > > > because dev->pm_domain is set before probing.
> > >
> > > And we can't do this as in very original code of acpi_lpss.c
> > > since
> > > DMA
> > > has to be sure it's powered on while probing. We could guarantee
> > > this
> > > only in case when PM domain is assigned already and we do our
> > > quirk
> > > for
> > > it.
> > >
> > > From my point of view we have to fix hang first since it's most
> > > painful
> > > case for users and their experience. Though I'm open to any
> > > better
> > > solution if you have any in mind.
> > >
> > > >
> > > > driver_probe_device
> > > > platform_drv_prove
> > > > dev_pm_domain_attach
> > > > acpi_dev_pm_attach
> > > > returns instantly because of dev->pm_domain is set
> >
> > This looks like a candidate for the new PM domain callbacks,
> > ->activate and
> > ->dismiss.
> >
> > ->activate() is called before the probe, so it may power up things.
> >
> > ->dismiss() in turn is called in the failed probe case, so it can
> > do
> > the
> > cleanup.
> >
> > Have you considered using these?
>
> Thanks for the hint. We will check this.
I briefly checked this for DMA issue. It will not help anyhow, so we
*have to* move a power domain assignment to the BIND stage.
For I2C and rest LPSS devices this might help (though didn't look
deeply). My understanding that we assign those callbacks in the LPSS
custom PM domain and call them explicitly in acpi_lpss.c.
The code will be the same as we are using now to bring device from
runtime suspend resume. This means whenever we call probe for e.g. I2C
we end up in a sequence similar to:
pm_runtime_resume(I2C);
->probe(I2C);
pm_runtime_suspend(I2C);
I will try to mock up this and check if it will work, though have no
idea what to do if I2C during probe calls pm_runtime_forbid().
Jarkko, what do you think?
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 12/03/2015 09:29 PM, Shevchenko, Andriy wrote:
> I briefly checked this for DMA issue. It will not help anyhow, so we
> *have to* move a power domain assignment to the BIND stage.
>
> For I2C and rest LPSS devices this might help (though didn't look
> deeply). My understanding that we assign those callbacks in the LPSS
> custom PM domain and call them explicitly in acpi_lpss.c.
>
> The code will be the same as we are using now to bring device from
> runtime suspend resume. This means whenever we call probe for e.g. I2C
> we end up in a sequence similar to:
> pm_runtime_resume(I2C);
> ->probe(I2C);
> pm_runtime_suspend(I2C);
>
> I will try to mock up this and check if it will work, though have no
> idea what to do if I2C during probe calls pm_runtime_forbid().
>
> Jarkko, what do you think?
>
I suppose device core will handle it. If the runtime PM is forbidden or
not initialized at all the device shouldn't idle.
--
Jarkko