+ 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@xxxxxxxxx>
> > > > Fixes: 6c17ee44d524 (ACPI / LPSS: introduce a 'proxy' device to power on LPSS for DMA)
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > > > ---
> > > > 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;
> }
>
--
Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
