Re: [linux-pm] [GIT PULL] One more power management fix for 2.6.37

[Alan Stern]

On Thu, 4 Nov 2010, Rafael J. Wysocki wrote:

> OK, so I think we can relax the locking in dpm_[suspend/resume]_noirq() to
> avoid executing callbacks under dpm_list_mtx, like in the (untested) patch
> below.
> 
> Alan, do you see any immediate problem with that?
> 
> Rafael
> 
> ---
>  drivers/base/power/main.c |   17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/drivers/base/power/main.c
> ===================================================================
> --- linux-2.6.orig/drivers/base/power/main.c
> +++ linux-2.6/drivers/base/power/main.c
> @@ -480,15 +480,23 @@ void dpm_resume_noirq(pm_message_t state
>  
>  	mutex_lock(&dpm_list_mtx);
>  	transition_started = false;
> -	list_for_each_entry(dev, &dpm_list, power.entry)
> +	list_for_each_entry(dev, &dpm_list, power.entry) {
> +		get_device(dev);
>  		if (dev->power.status > DPM_OFF) {
>  			int error;
>  
>  			dev->power.status = DPM_OFF;
> +
> +			mutex_unlock(&dpm_list_mtx);
> +
>  			error = device_resume_noirq(dev, state);
> +
> +			mutex_lock(&dpm_list_mtx);
>  			if (error)
>  				pm_dev_err(dev, state, " early", error);
>  		}
> +		put_device(dev);
> +	}
>  	mutex_unlock(&dpm_list_mtx);
>  	dpm_show_time(starttime, state, "early");
>  	resume_device_irqs();
> @@ -796,12 +804,19 @@ int dpm_suspend_noirq(pm_message_t state
>  	suspend_device_irqs();
>  	mutex_lock(&dpm_list_mtx);
>  	list_for_each_entry_reverse(dev, &dpm_list, power.entry) {
> +		get_device(dev);
> +		mutex_unlock(&dpm_list_mtx);
> +
>  		error = device_suspend_noirq(dev, state);
> +
> +		mutex_lock(&dpm_list_mtx);
>  		if (error) {
>  			pm_dev_err(dev, state, " late", error);
> +			put_device(dev);
>  			break;
>  		}
>  		dev->power.status = DPM_OFF_IRQ;
> +		put_device(dev);
>  	}
>  	mutex_unlock(&dpm_list_mtx);
>  	if (error)

This won't work if the callback tries to unregister the device, but of
course the old code wouldn't work in that case either.  We should
document this requirement.  It means that your get_device and
put_device calls aren't needed.

Aside from that I don't see any problems.  In principle there shouldn't 
be any other processes running at this time that would want to access 
either the device or the dpm_list.  Maybe this means the socket locking 
isn't needed in the pcmcia late-suspend and early-resume routines, 
which would be a simpler solution.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/