Failure of device_add() and parent's child_count

Failure of device_add() and parent's child_count

[Alan Stern]

Rafael:

The Runtime PM interface is a little awkward concerning failure of 
device_add().  Let's consider the normal case where a newly-discovered 
device is being registered, and it is initially powered on.  Assuming 
that we want the PM core to know the device's true state while it is 
being probed, the registration code will have to look like this:

	...
	dev->parent = ...
	...
	pm_runtime_set_active(dev);
	pm_runtime_enable(dev);
	...
	rc = device_add(dev);

The assignment to dev->parent has to come first, so that 
pm_runtime_set_active() will increment the parent's child_count.  The 
child_count gets decremented again when device_del() calls 
device_pm_remove().

This means that following a failure of device_add(), the caller has to
be responsible for making sure the parent's child_count is correct.  So 
after calling device_add(), we need to do:

	if (rc < 0) {
		dev_warn(dev, ...);
		pm_runtime_disable(dev);
		pm_runtime_set_suspended(dev);
		put_device(dev);
	}

I doubt that people using the Runtime PM interface are aware of this.  
Did you do it in your new PCI runtime-PM code?

It would be good to remove this awkwardness, but I don't know what is
the best way to do it.  Perhaps __pm_runtime_set_status() shouldn't
adjust the parent's child_count unless the device is registered.  Then
device_pm_add() could adjust the child_count as needed.

Alan Stern

Re: Failure of device_add() and parent's child_count

[Rafael J. Wysocki]

On Wednesday 25 November 2009, Alan Stern wrote:
> Rafael:
> 
> The Runtime PM interface is a little awkward concerning failure of 
> device_add().  Let's consider the normal case where a newly-discovered 
> device is being registered, and it is initially powered on.  Assuming 
> that we want the PM core to know the device's true state while it is 
> being probed, the registration code will have to look like this:
> 
> 	...
> 	dev->parent = ...
> 	...
> 	pm_runtime_set_active(dev);
> 	pm_runtime_enable(dev);
> 	...
> 	rc = device_add(dev);
> 
> The assignment to dev->parent has to come first, so that 
> pm_runtime_set_active() will increment the parent's child_count.  The 
> child_count gets decremented again when device_del() calls 
> device_pm_remove().
> 
> This means that following a failure of device_add(), the caller has to
> be responsible for making sure the parent's child_count is correct.  So 
> after calling device_add(), we need to do:
> 
> 	if (rc < 0) {
> 		dev_warn(dev, ...);
> 		pm_runtime_disable(dev);
> 		pm_runtime_set_suspended(dev);
> 		put_device(dev);
> 	}
> 
> I doubt that people using the Runtime PM interface are aware of this.  

I'm quite sure they aren't.

> Did you do it in your new PCI runtime-PM code?

No, I don't.  In the case of a network adapter I've been working on recently,
it's sufficient to enable/disable the runtime PM in _open()/_close().

> It would be good to remove this awkwardness, but I don't know what is
> the best way to do it.  Perhaps __pm_runtime_set_status() shouldn't
> adjust the parent's child_count unless the device is registered.  Then
> device_pm_add() could adjust the child_count as needed.

It seems fine at first sight, but I need to think a bit more about that.

Thanks,
Rafael

Re: Failure of device_add() and parent's child_count

[Alan Stern]

On Fri, 27 Nov 2009, Rafael J. Wysocki wrote:

> > This means that following a failure of device_add(), the caller has to
> > be responsible for making sure the parent's child_count is correct.  So 
> > after calling device_add(), we need to do:
> > 
> > 	if (rc < 0) {
> > 		dev_warn(dev, ...);
> > 		pm_runtime_disable(dev);
> > 		pm_runtime_set_suspended(dev);
> > 		put_device(dev);
> > 	}
> > 
> > I doubt that people using the Runtime PM interface are aware of this.  
> 
> I'm quite sure they aren't.
> 
> > Did you do it in your new PCI runtime-PM code?
> 
> No, I don't.  In the case of a network adapter I've been working on recently,
> it's sufficient to enable/disable the runtime PM in _open()/_close().
> 
> > It would be good to remove this awkwardness, but I don't know what is
> > the best way to do it.  Perhaps __pm_runtime_set_status() shouldn't
> > adjust the parent's child_count unless the device is registered.  Then
> > device_pm_add() could adjust the child_count as needed.
> 
> It seems fine at first sight, but I need to think a bit more about that.

Is anybody going to do runtime PM on a device (or allow its parent to
be suspended) before registering it with the driver core?  That's the 
case to worry about.

Failures of device_add() are going to be pretty rare in any case.  So
maybe this doesn't much matter.

Still, a short paragraph could be added to the end of the documentation
file, showing the right way to do this.

Alan Stern