| Message ID | 1826096.Y1k7UfFlRi@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
+ Jerome Blin Jerome, this is new version of patch from Rafael that I mentioned in LCK-1599. Please take it instead. On Wed, 2015-01-21 at 16:03 +0100, Rafael J. Wysocki wrote: > On Wednesday, January 21, 2015 03:37:43 AM Rafael J. Wysocki wrote: > > On Tuesday, January 20, 2015 03:45:36 PM Rafael J. Wysocki wrote: > > > On Thursday, January 15, 2015 01:10:44 PM Andy Shevchenko wrote: > > > > We can't rely on the first enumerated device since it might be not probed yet > > > > when DMA driver wants to resume the device. This patch changes the place where > > > > we assign the 'proxy' device. From now on the first resumed device will be > > > > recognized as a 'proxy' because it is probed. > > > > > > I was about to apply this, but I realized that we couldn't do it this way. -> > > > > > > > Reported-by: Jerome Blin <jerome.blin@intel.com> > > > > Fixes: 6c17ee44d524 (ACPI / LPSS: introduce a 'proxy' device to power on LPSS for DMA) > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > --- > > > > drivers/acpi/acpi_lpss.c | 17 +++++++++++------ > > > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c > > > > index 4f3febf..d851290 100644 > > > > --- a/drivers/acpi/acpi_lpss.c > > > > +++ b/drivers/acpi/acpi_lpss.c > > > > @@ -374,8 +374,6 @@ static int acpi_lpss_create_device(struct acpi_device *adev, > > > > adev->driver_data = pdata; > > > > pdev = acpi_create_platform_device(adev); > > > > if (!IS_ERR_OR_NULL(pdev)) { > > > > - if (!proxy_device && dev_desc->flags & LPSS_DEV_PROXY) > > > > - proxy_device = &pdev->dev; > > > > return 1; > > > > } > > > > > > > > @@ -615,10 +613,17 @@ static int acpi_lpss_runtime_resume(struct device *dev) > > > > struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev)); > > > > int ret; > > > > > > > > - if (pdata->dev_desc->flags & LPSS_PROXY_REQ && proxy_device) { > > > > - ret = pm_runtime_get_sync(proxy_device); > > > > - if (ret) > > > > - return ret; > > > > + if (!proxy_device && pdata->dev_desc->flags & LPSS_DEV_PROXY) > > > > + proxy_device = dev; > > > > > > -> OK, so what if the driver of proxy_device is removed later? > > > > > > > + > > > > + if (pdata->dev_desc->flags & LPSS_PROXY_REQ) { > > > > + if (proxy_device) { > > > > + ret = pm_runtime_get_sync(proxy_device); > > > > + if (ret) > > > > + return ret; > > > > + } else { > > > > + return -EPROBE_DEFER; > > > > > > The error code returned here will disable runtime PM for dev forever, > > > even if proxy_device becomes available at one point. You could return > > > -EBUSY instead, but why don't we add BUS_NOTIFY_BOUND_DRIVER and > > > BUS_NOTIFY_UNBIND_DRIVER handling to acpi_lpss_platform_notify() and > > > set/unset proxy_device from there? > > > > Well, that's more complicated, because we need to be careful when we switch > > proxies while the old one has been reference counted. > > > > Maybe something like this (untested)? > > That sill had a few problems. > > The one below should be better (still untested, though). It assumes that > the devices needing a proxy will never start as runtime-suspended and will > always be resumed before removal. > > --- > drivers/acpi/acpi_lpss.c | 126 ++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 113 insertions(+), 13 deletions(-) > > Index: linux-pm/drivers/acpi/acpi_lpss.c > =================================================================== > --- linux-pm.orig/drivers/acpi/acpi_lpss.c > +++ linux-pm/drivers/acpi/acpi_lpss.c > @@ -72,12 +72,21 @@ struct lpss_device_desc { > void (*setup)(struct lpss_private_data *pdata); > }; > > -static struct device *proxy_device; > - > static struct lpss_device_desc lpss_dma_desc = { > .flags = LPSS_CLK | LPSS_PROXY_REQ, > }; > > +struct lpss_proxy { > + struct device *dev; > + struct list_head node; > +}; > + > +static struct lpss_proxy *current_proxy; > +static unsigned int proxy_count; > +static unsigned int proxy_suspend_count; > +static LIST_HEAD(proxy_list); > +static DEFINE_MUTEX(proxy_lock); > + > struct lpss_private_data { > void __iomem *mmio_base; > resource_size_t mmio_size; > @@ -85,6 +94,7 @@ struct lpss_private_data { > struct clk *clk; > const struct lpss_device_desc *dev_desc; > u32 prv_reg_ctx[LPSS_PRV_REG_COUNT]; > + struct lpss_proxy proxy_entry; > }; > > /* UART Component Parameter Register */ > @@ -373,11 +383,8 @@ static int acpi_lpss_create_device(struc > > adev->driver_data = pdata; > pdev = acpi_create_platform_device(adev); > - if (!IS_ERR_OR_NULL(pdev)) { > - if (!proxy_device && dev_desc->flags & LPSS_DEV_PROXY) > - proxy_device = &pdev->dev; > + if (!IS_ERR_OR_NULL(pdev)) > return 1; > - } > > ret = PTR_ERR(pdev); > adev->driver_data = NULL; > @@ -591,23 +598,41 @@ static int acpi_lpss_resume_early(struct > static int acpi_lpss_runtime_suspend(struct device *dev) > { > struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev)); > + bool use_proxy; > int ret; > > + if (pdata->dev_desc->flags & LPSS_PROXY_REQ) { > + use_proxy = true; > + mutex_lock(&proxy_lock); > + if (!current_proxy) { > + ret = -EBUSY; > + goto out; > + } > + } else { > + use_proxy = false; > + } > + > ret = pm_generic_runtime_suspend(dev); > if (ret) > - return ret; > + goto out; > > if (pdata->dev_desc->flags & LPSS_SAVE_CTX) > acpi_lpss_save_ctx(dev, pdata); > > ret = acpi_dev_runtime_suspend(dev); > if (ret) > - return ret; > + goto out; > > - if (pdata->dev_desc->flags & LPSS_PROXY_REQ && proxy_device) > - return pm_runtime_put_sync_suspend(proxy_device); > + if (use_proxy) { > + proxy_suspend_count++; > + ret = pm_runtime_put_sync_suspend(current_proxy->dev); > + } > > - return 0; > + out: > + if (use_proxy) > + mutex_unlock(&proxy_lock); > + > + return ret; > } > > static int acpi_lpss_runtime_resume(struct device *dev) > @@ -615,8 +640,16 @@ static int acpi_lpss_runtime_resume(stru > struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev)); > int ret; > > - if (pdata->dev_desc->flags & LPSS_PROXY_REQ && proxy_device) { > - ret = pm_runtime_get_sync(proxy_device); > + if (pdata->dev_desc->flags & LPSS_PROXY_REQ) { > + mutex_lock(&proxy_lock); > + if (current_proxy) { > + ret = pm_runtime_get_sync(current_proxy->dev); > + if (!ret) > + proxy_suspend_count--; > + } else { > + ret = -EBUSY; > + } > + mutex_unlock(&proxy_lock); > if (ret) > return ret; > } > @@ -652,6 +685,29 @@ static struct dev_pm_domain acpi_lpss_pm > }, > }; > > +static void acpi_lpss_switch_proxy(void) > +{ > + struct device *dev = current_proxy ? current_proxy->dev : NULL; > + unsigned int count = proxy_count - proxy_suspend_count; > + > + if (list_empty(&proxy_list)) { > + current_proxy = NULL; > + } else { > + struct lpss_private_data *lpd; > + > + lpd = list_first_entry(&proxy_list, struct lpss_private_data, > + proxy_entry.node); > + current_proxy = &lpd->proxy_entry; > + } > + while (count--) { > + if (dev) > + pm_runtime_put_noidle(dev); > + > + if (current_proxy) > + pm_runtime_get_sync(current_proxy->dev); > + } > +} > + > static int acpi_lpss_platform_notify(struct notifier_block *nb, > unsigned long action, void *data) > { > @@ -680,15 +736,59 @@ static int acpi_lpss_platform_notify(str > switch (action) { > case BUS_NOTIFY_ADD_DEVICE: > pdev->dev.pm_domain = &acpi_lpss_pm_domain; > + if (pdata->dev_desc->flags & LPSS_DEV_PROXY) { > + mutex_lock(&proxy_lock); > + > + /* This assimes the device to be powered up. */ > + proxy_count++; > + if (current_proxy) > + pm_runtime_get_sync(current_proxy->dev); > + > + mutex_unlock(&proxy_lock); > + } > if (pdata->dev_desc->flags & LPSS_LTR) > return sysfs_create_group(&pdev->dev.kobj, > &lpss_attr_group); > break; > case BUS_NOTIFY_DEL_DEVICE: > + if (pdata->dev_desc->flags & LPSS_DEV_PROXY) { > + mutex_lock(&proxy_lock); > + > + /* This assumes that the device is not suspended. */ > + proxy_count--; > + if (current_proxy) > + pm_runtime_put(current_proxy->dev); > + > + mutex_unlock(&proxy_lock); > + } > if (pdata->dev_desc->flags & LPSS_LTR) > sysfs_remove_group(&pdev->dev.kobj, &lpss_attr_group); > pdev->dev.pm_domain = NULL; > break; > + case BUS_NOTIFY_BOUND_DRIVER: > + if (pdata->dev_desc->flags & LPSS_DEV_PROXY) { > + mutex_lock(&proxy_lock); > + > + INIT_LIST_HEAD(&pdata->proxy_entry.node); > + pdata->proxy_entry.dev = &pdev->dev; > + list_add_tail(&pdata->proxy_entry.node, &proxy_list); > + if (!current_proxy) > + acpi_lpss_switch_proxy(); > + > + mutex_unlock(&proxy_lock); > + } > + break; > + case BUS_NOTIFY_UNBIND_DRIVER: > + if (pdata->dev_desc->flags & LPSS_DEV_PROXY) { > + mutex_lock(&proxy_lock); > + > + list_del(&pdata->proxy_entry.node); > + if (current_proxy == &pdata->proxy_entry) > + acpi_lpss_switch_proxy(); > + > + mutex_unlock(&proxy_lock); > + } > + break; > default: > break; > } >
Index: linux-pm/drivers/acpi/acpi_lpss.c =================================================================== --- linux-pm.orig/drivers/acpi/acpi_lpss.c +++ linux-pm/drivers/acpi/acpi_lpss.c @@ -72,12 +72,21 @@ struct lpss_device_desc { void (*setup)(struct lpss_private_data *pdata); }; -static struct device *proxy_device; - static struct lpss_device_desc lpss_dma_desc = { .flags = LPSS_CLK | LPSS_PROXY_REQ, }; +struct lpss_proxy { + struct device *dev; + struct list_head node; +}; + +static struct lpss_proxy *current_proxy; +static unsigned int proxy_count; +static unsigned int proxy_suspend_count; +static LIST_HEAD(proxy_list); +static DEFINE_MUTEX(proxy_lock); + struct lpss_private_data { void __iomem *mmio_base; resource_size_t mmio_size; @@ -85,6 +94,7 @@ struct lpss_private_data { struct clk *clk; const struct lpss_device_desc *dev_desc; u32 prv_reg_ctx[LPSS_PRV_REG_COUNT]; + struct lpss_proxy proxy_entry; }; /* UART Component Parameter Register */ @@ -373,11 +383,8 @@ static int acpi_lpss_create_device(struc adev->driver_data = pdata; pdev = acpi_create_platform_device(adev); - if (!IS_ERR_OR_NULL(pdev)) { - if (!proxy_device && dev_desc->flags & LPSS_DEV_PROXY) - proxy_device = &pdev->dev; + if (!IS_ERR_OR_NULL(pdev)) return 1; - } ret = PTR_ERR(pdev); adev->driver_data = NULL; @@ -591,23 +598,41 @@ static int acpi_lpss_resume_early(struct static int acpi_lpss_runtime_suspend(struct device *dev) { struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev)); + bool use_proxy; int ret; + if (pdata->dev_desc->flags & LPSS_PROXY_REQ) { + use_proxy = true; + mutex_lock(&proxy_lock); + if (!current_proxy) { + ret = -EBUSY; + goto out; + } + } else { + use_proxy = false; + } + ret = pm_generic_runtime_suspend(dev); if (ret) - return ret; + goto out; if (pdata->dev_desc->flags & LPSS_SAVE_CTX) acpi_lpss_save_ctx(dev, pdata); ret = acpi_dev_runtime_suspend(dev); if (ret) - return ret; + goto out; - if (pdata->dev_desc->flags & LPSS_PROXY_REQ && proxy_device) - return pm_runtime_put_sync_suspend(proxy_device); + if (use_proxy) { + proxy_suspend_count++; + ret = pm_runtime_put_sync_suspend(current_proxy->dev); + } - return 0; + out: + if (use_proxy) + mutex_unlock(&proxy_lock); + + return ret; } static int acpi_lpss_runtime_resume(struct device *dev) @@ -615,8 +640,16 @@ static int acpi_lpss_runtime_resume(stru struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev)); int ret; - if (pdata->dev_desc->flags & LPSS_PROXY_REQ && proxy_device) { - ret = pm_runtime_get_sync(proxy_device); + if (pdata->dev_desc->flags & LPSS_PROXY_REQ) { + mutex_lock(&proxy_lock); + if (current_proxy) { + ret = pm_runtime_get_sync(current_proxy->dev); + if (!ret) + proxy_suspend_count--; + } else { + ret = -EBUSY; + } + mutex_unlock(&proxy_lock); if (ret) return ret; } @@ -652,6 +685,29 @@ static struct dev_pm_domain acpi_lpss_pm }, }; +static void acpi_lpss_switch_proxy(void) +{ + struct device *dev = current_proxy ? current_proxy->dev : NULL; + unsigned int count = proxy_count - proxy_suspend_count; + + if (list_empty(&proxy_list)) { + current_proxy = NULL; + } else { + struct lpss_private_data *lpd; + + lpd = list_first_entry(&proxy_list, struct lpss_private_data, + proxy_entry.node); + current_proxy = &lpd->proxy_entry; + } + while (count--) { + if (dev) + pm_runtime_put_noidle(dev); + + if (current_proxy) + pm_runtime_get_sync(current_proxy->dev); + } +} + static int acpi_lpss_platform_notify(struct notifier_block *nb, unsigned long action, void *data) { @@ -680,15 +736,59 @@ static int acpi_lpss_platform_notify(str switch (action) { case BUS_NOTIFY_ADD_DEVICE: pdev->dev.pm_domain = &acpi_lpss_pm_domain; + if (pdata->dev_desc->flags & LPSS_DEV_PROXY) { + mutex_lock(&proxy_lock); + + /* This assimes the device to be powered up. */ + proxy_count++; + if (current_proxy) + pm_runtime_get_sync(current_proxy->dev); + + mutex_unlock(&proxy_lock); + } if (pdata->dev_desc->flags & LPSS_LTR) return sysfs_create_group(&pdev->dev.kobj, &lpss_attr_group); break; case BUS_NOTIFY_DEL_DEVICE: + if (pdata->dev_desc->flags & LPSS_DEV_PROXY) { + mutex_lock(&proxy_lock); + + /* This assumes that the device is not suspended. */ + proxy_count--; + if (current_proxy) + pm_runtime_put(current_proxy->dev); + + mutex_unlock(&proxy_lock); + } if (pdata->dev_desc->flags & LPSS_LTR) sysfs_remove_group(&pdev->dev.kobj, &lpss_attr_group); pdev->dev.pm_domain = NULL; break; + case BUS_NOTIFY_BOUND_DRIVER: + if (pdata->dev_desc->flags & LPSS_DEV_PROXY) { + mutex_lock(&proxy_lock); + + INIT_LIST_HEAD(&pdata->proxy_entry.node); + pdata->proxy_entry.dev = &pdev->dev; + list_add_tail(&pdata->proxy_entry.node, &proxy_list); + if (!current_proxy) + acpi_lpss_switch_proxy(); + + mutex_unlock(&proxy_lock); + } + break; + case BUS_NOTIFY_UNBIND_DRIVER: + if (pdata->dev_desc->flags & LPSS_DEV_PROXY) { + mutex_lock(&proxy_lock); + + list_del(&pdata->proxy_entry.node); + if (current_proxy == &pdata->proxy_entry) + acpi_lpss_switch_proxy(); + + mutex_unlock(&proxy_lock); + } + break; default: break; }