Re: [PATCH] PM / Domains: Fix initial default state of the need_restore flag

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Friday, November 07, 2014 02:27:27 PM Ulf Hansson wrote:
> The initial state of the device's need_restore flag should'nt depend on
> the current state of the PM domain. For example it should be perfectly
> valid to attach an inactive device to a powered PM domain.
> 
> The pm_genpd_dev_need_restore() API allow us to update the need_restore
> flag to somewhat cope with such scenarios. Typically that should have
> been done from drivers/buses ->probe() since it's those that put the
> requirements on the value of the need_restore flag.
> 
> Until recently, the Exynos SOCs were the only user of the
> pm_genpd_dev_need_restore() API, though invoking it from a centralized
> location while adding devices to their PM domains.
> 
> Due to that Exynos now have swithed to the generic OF-based PM domain
> look-up, it's no longer possible to invoke the API from a centralized
> location. The reason is because devices are now added to their PM
> domains during the probe sequence.
> 
> Commit "ARM: exynos: Move to generic PM domain DT bindings"
> did the switch for Exynos to the generic OF-based PM domain look-up,
> but it also removed the call to pm_genpd_dev_need_restore(). This
> caused a regression for some of the Exynos drivers.
> 
> To handle things more properly in the generic PM domain, let's change
> the default initial value of the need_restore flag to reflect that the
> state is unknown. As soon as some of the runtime PM callbacks gets
> invoked, update the initial value accordingly.
> 
> Moreover, since the generic PM domain is verifying that all device's
> are both runtime PM enabled and suspended, using pm_runtime_suspended()
> while pm_genpd_poweroff() is invoked from the scheduled work, we can be
> sure of that the PM domain won't be powering off while having active
> devices.
> 
> Do note that, the generic PM domain can still only know about active
> devices which has been activated through invoking its runtime PM resume
> callback. In other words, buses/drivers using pm_runtime_set_active()
> during ->probe() will still suffer from a race condition, potentially
> probing a device without having its PM domain being powered. That issue
> will have to be solved using a different approach.
> 
> This a log from the boot regression for Exynos5, which is being fixed in
> this patch.
> 
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 308 at ../drivers/clk/clk.c:851 clk_disable+0x24/0x30()
> Modules linked in:
> CPU: 0 PID: 308 Comm: kworker/0:1 Not tainted 3.18.0-rc3-00569-gbd9449f-dirty #10
> Workqueue: pm pm_runtime_work
> [<c0013c64>] (unwind_backtrace) from [<c0010dec>] (show_stack+0x10/0x14)
> [<c0010dec>] (show_stack) from [<c03ee4cc>] (dump_stack+0x70/0xbc)
> [<c03ee4cc>] (dump_stack) from [<c0020d34>] (warn_slowpath_common+0x64/0x88)
> [<c0020d34>] (warn_slowpath_common) from [<c0020d74>] (warn_slowpath_null+0x1c/0x24)
> [<c0020d74>] (warn_slowpath_null) from [<c03107b0>] (clk_disable+0x24/0x30)
> [<c03107b0>] (clk_disable) from [<c02cc834>] (gsc_runtime_suspend+0x128/0x160)
> [<c02cc834>] (gsc_runtime_suspend) from [<c0249024>] (pm_generic_runtime_suspend+0x2c/0x38)
> [<c0249024>] (pm_generic_runtime_suspend) from [<c024f44c>] (pm_genpd_default_save_state+0x2c/0x8c)
> [<c024f44c>] (pm_genpd_default_save_state) from [<c024ff2c>] (pm_genpd_poweroff+0x224/0x3ec)
> [<c024ff2c>] (pm_genpd_poweroff) from [<c02501b4>] (pm_genpd_runtime_suspend+0x9c/0xcc)
> [<c02501b4>] (pm_genpd_runtime_suspend) from [<c024a4f8>] (__rpm_callback+0x2c/0x60)
> [<c024a4f8>] (__rpm_callback) from [<c024a54c>] (rpm_callback+0x20/0x74)
> [<c024a54c>] (rpm_callback) from [<c024a930>] (rpm_suspend+0xd4/0x43c)
> [<c024a930>] (rpm_suspend) from [<c024bbcc>] (pm_runtime_work+0x80/0x90)
> [<c024bbcc>] (pm_runtime_work) from [<c0032a9c>] (process_one_work+0x12c/0x314)
> [<c0032a9c>] (process_one_work) from [<c0032cf4>] (worker_thread+0x3c/0x4b0)
> [<c0032cf4>] (worker_thread) from [<c003747c>] (kthread+0xcc/0xe8)
> [<c003747c>] (kthread) from [<c000e738>] (ret_from_fork+0x14/0x3c)
> ---[ end trace 40cd58bcd6988f12 ]---
> 
> Fixes: a4a8c2c4962bb655 (ARM: exynos: Move to generic PM domain DT bindings)
> Reported-by: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
> Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> ---
> 
> This patch is intended as fix for 3.18 rc[n] due to the regression for Exynos
> SOCs.
> 
> I would also like to call for help in getting this thoroughly tested.
> 
> I have tested this on Arndale Dual, Exynos 5250. According the log attached in
> the commit message as well.
> 
> I have tested this on UX500, which support for the generic PM domain is about
> to be queued for 3.19. Since UX500 uses the AMBA bus/drivers, which uses
> pm_runtime_set_active() instead of pm_runtime_get_sync() during ->probe(), I
> could also verify that this behavior is maintained.
> 
> Finally I have also tested this patchset on UX500 and using the below patchset
> to make sure the approach is suitable long term wise as well.
> [PATCH v3 0/9] PM / Domains: Fix race conditions during boot
> http://www.spinics.net/lists/arm-kernel/msg369409.html

I'm generally fine with the approach, but ->

> ---
>  drivers/base/power/domain.c | 38 +++++++++++++++++++++++++++++++-------
>  include/linux/pm_domain.h   |  2 +-
>  2 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index b520687..a9523a3 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -361,9 +361,19 @@ static int __pm_genpd_save_device(struct pm_domain_data *pdd,
>  	struct device *dev = pdd->dev;
>  	int ret = 0;
>  
> -	if (gpd_data->need_restore)
> +	if (gpd_data->need_restore == 1)

-> I'd prefer checks for the sign rather than for a specific value, like

	if (gpd_data->need_restore > 0)

>  		return 0;
>  

and so on.

> +	/*
> +	 * If the value of the need_restore flag is still unknown at this point,
> +	 * we trust that pm_genpd_poweroff() has verified that the device is
> +	 * already runtime PM suspended.
> +	 */
> +	if (gpd_data->need_restore == -1) {
> +		gpd_data->need_restore = 1;
> +		return 0;
> +	}
> +
>  	mutex_unlock(&genpd->lock);
>  
>  	genpd_start_dev(genpd, dev);
> @@ -373,7 +383,7 @@ static int __pm_genpd_save_device(struct pm_domain_data *pdd,
>  	mutex_lock(&genpd->lock);
>  
>  	if (!ret)
> -		gpd_data->need_restore = true;
> +		gpd_data->need_restore = 1;
>  
>  	return ret;
>  }
> @@ -389,13 +399,17 @@ static void __pm_genpd_restore_device(struct pm_domain_data *pdd,
>  {
>  	struct generic_pm_domain_data *gpd_data = to_gpd_data(pdd);
>  	struct device *dev = pdd->dev;
> -	bool need_restore = gpd_data->need_restore;
> +	int need_restore = gpd_data->need_restore;
>  
> -	gpd_data->need_restore = false;
> +	gpd_data->need_restore = 0;
>  	mutex_unlock(&genpd->lock);
>  
>  	genpd_start_dev(genpd, dev);
> -	if (need_restore)
> +	/*
> +	 * Make sure to also restore those devices which we until now, didn't
> +	 * know the state for.
> +	 */
> +	if (need_restore != 0)

and here you can do

	if (need_restore)

>  		genpd_restore_dev(genpd, dev);
>  
>  	mutex_lock(&genpd->lock);
> @@ -603,6 +617,7 @@ static void genpd_power_off_work_fn(struct work_struct *work)
>  static int pm_genpd_runtime_suspend(struct device *dev)
>  {
>  	struct generic_pm_domain *genpd;
> +	struct generic_pm_domain_data *gpd_data;
>  	bool (*stop_ok)(struct device *__dev);
>  	int ret;
>  
> @@ -628,6 +643,15 @@ static int pm_genpd_runtime_suspend(struct device *dev)
>  		return 0;
>  
>  	mutex_lock(&genpd->lock);
> +	/*
> +	 * If we have an unknown state of the need_restore flag, it means none
> +	 * of the runtime PM callbacks has been invoked yet. Let's update the
> +	 * flag to reflect that the current state is active.
> +	 */
> +	gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
> +	if (gpd_data->need_restore == -1)
> +		gpd_data->need_restore = 0;
> +
>  	genpd->in_progress++;
>  	pm_genpd_poweroff(genpd);
>  	genpd->in_progress--;
> @@ -1442,7 +1466,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>  	mutex_lock(&gpd_data->lock);
>  	gpd_data->base.dev = dev;
>  	list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
> -	gpd_data->need_restore = genpd->status == GPD_STATE_POWER_OFF;
> +	gpd_data->need_restore = -1;
>  	gpd_data->td.constraint_changed = true;
>  	gpd_data->td.effective_constraint_ns = -1;
>  	mutex_unlock(&gpd_data->lock);
> @@ -1546,7 +1570,7 @@ void pm_genpd_dev_need_restore(struct device *dev, bool val)
>  
>  	psd = dev_to_psd(dev);
>  	if (psd && psd->domain_data)
> -		to_gpd_data(psd->domain_data)->need_restore = val;
> +		to_gpd_data(psd->domain_data)->need_restore = val ? 1 : 0;
>  
>  	spin_unlock_irqrestore(&dev->power.lock, flags);
>  }
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index b3ed776..2e0e06d 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -106,7 +106,7 @@ struct generic_pm_domain_data {
>  	struct notifier_block nb;
>  	struct mutex lock;
>  	unsigned int refcount;
> -	bool need_restore;
> +	int need_restore;
>  };
>  
>  #ifdef CONFIG_PM_GENERIC_DOMAINS
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux