Re: [RFC Add no_suspend attribute V2] Let the driver know if it's in use

[Michael Trimarchi <trimarchi@gandalf.sssup.it>]

Rafael J. Wysocki wrote:
> On Saturday 25 April 2009, Michael Trimarchi wrote:
>   
>> Greg KH wrote:
>>     
>>> On Fri, Apr 24, 2009 at 07:55:55PM +0200, Michael Trimarchi wrote:
>>>   
>>>       
>>>>>  /**
>>>>> + *	device_set_no_suspend_enable - Mark the device as used by userspace
>>>>> + *	application
>>>>> + */
>>>>>       
>>>>>           
>>> This is not proper kernel-doc, please fix this up.
>>>
>>> And "no_suspend_enable" is ackward, drop the "enable" part?
>>>
>>>
>>>   
>>>       
>>>>> +void device_set_no_suspend_enable(struct device *dev, bool enable)
>>>>> +{
>>>>> +	struct device *next;
>>>>> +
>>>>> +	mutex_lock(&dpm_list_mtx);
>>>>> +
>>>>> +	/* the new status is equal the old one */
>>>>> +	if (dev->power.no_suspend == !!enable)
>>>>> +		goto out;
>>>>> +
>>>>> +	/* change the device status */
>>>>> +	dev->power.no_suspend = !!enable;
>>>>> +	if (dev->power.no_suspend)
>>>>> +		dev->power.subtree_no_suspend = 0;
>>>>>   
>>>>>       
>>>>>           
>>>> I find a bug here, i will fix.
>>>> It can be ok the rest of the code?
>>>>     
>>>>         
>>>>> +
>>>>> +	list_for_each_entry_reverse(next, &dev->power.entry, power.entry) {
>>>>> +		/* 
>>>>> +		 * exit if we find a node with the same parent of the start
>>>>> +		 * device
>>>>> +		 */
>>>>> +		if (dev->parent && next->parent == dev->parent)
>>>>> +			break;
>>>>> +
>>>>> +		if (next->parent) {
>>>>> +			/* Propagate the status */
>>>>> +			next->power.subtree_no_suspend =
>>>>> +				device_no_suspend_enable(next->parent);
>>>>> +		}
>>>>> +	}
>>>>> +out:
>>>>> +	mutex_unlock(&dpm_list_mtx);
>>>>> +	return;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(device_set_no_suspend_enable);
>>>>> +
>>>>> +/**
>>>>>   *	device_pm_add - add a device to the list of active devices
>>>>>   *	@dev:	Device to be added to the list
>>>>>   */
>>>>> @@ -78,6 +117,11 @@ void device_pm_add(struct device *dev)
>>>>>  		if (dev->parent->power.status >= DPM_SUSPENDING)
>>>>>  			dev_warn(dev, "parent %s should not be sleeping\n",
>>>>>  				 dev_name(dev->parent));
>>>>> +		if (device_no_suspend_enable(dev->parent)) {
>>>>> +			/* if the parent has suspend disable, propagate it
>>>>> +			 * to the new child */
>>>>> +			dev->power.subtree_no_suspend = 1;
>>>>> +		}
>>>>>  	} else if (transition_started) {
>>>>>  		/*
>>>>>  		 * We refuse to register parentless devices while a PM
>>>>> @@ -87,7 +131,15 @@ void device_pm_add(struct device *dev)
>>>>>  		dev_WARN(dev, "Parentless device registered during a PM transaction\n");
>>>>>  	}
>>>>>  
>>>>> -	list_add_tail(&dev->power.entry, &dpm_list);
>>>>> +	if (dev->parent) {
>>>>> +		/*
>>>>> +		 * if the device has a parent insert just before it.
>>>>> +		 */
>>>>> +		list_add_tail(&dev->power.entry, &(dev->parent)->power.entry);
>>>>>           
Anyway, I revert this beacuse is wrong but is it correct to use here the
list_add instead the list_add_tail, it put the child node
after the parent stricly and don't create hole in subtree visit. Now I 
see only a dump
reason for debugging and a simple exit in flag update.
The important thing is that the children follow the parent,
and in this way is like visiting the tree. Nothing change for me because 
is difficult
to isolate only a subtree, if you have a list, but I'm sure that if I 
find a subtree with
the same parent, my subtree is finish, and if someone add a new device, 
the system put
in the correct position.
>>>>> +	}
>>>>> +	else
>>>>> +		list_add_tail(&dev->power.entry, &dpm_list);
>>>>> +
>>>>>       
>>>>>           
>>> Why are you changing the ordering for when we add devices to the list?
>>> This seems like you are adding stuff now in backwards order, why make
>>> this change?
>>>   
>>>       
>> Sorry you are right the children must be discovere after and not  before 
>> parent.
>> I was convinced that children go to suspend before parent.
>>     
>
> They generally do, but that's not precise.
>
> More precisely, there are three walks of dpm_list during suspend.  First,
> dpm_prepare() walks the list in the straight order, so if the parents are
> in the list before their children, they will be visited first.  This is the
> phase you can use to propagate your "no_suspend" flags from the parents to the
> children, IMO.
>
> Then, dpm_suspend() and device_power_down() walk the list in the reverse order,
> and that's where the device drivers' suspend callbacks are executed.
>   
Ok it's clear... Thank's Rafael for the support. You give me a big help 
when you invite
me to check the code again. Now I try to produce a new patch.
> Thanks,
> Rafael
>
>   
Thanks.
Michael