Re: [PATCH] ACPI / LPSS: set first resuming device as a 'proxy'

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

+ 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;
>  	}
> 


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy