Device core removal ordering brokenness

Device core removal ordering brokenness

[Benjamin Herrenschmidt]

Hi Alan !

I was looking at git history regarding the various BUS_NOTIFY_*
notifiers (since David needs some stuff for his DMA debug code that
isn't provided by the current set) when I noticed that commit of yours:

ec0676ee28528dc8dda13a93ee4b1f215a0c2f9d

Unless I'm mistaken, which is very possible, this moves the
BUS_NOTIFY_DEL_DEVICE callback to -before- the driver remove() callback
is invoked. This sounds very illogical and potentially dangerous to me.

In fact, the original ordering and the only one that, to me, makes sense
in term of semantics is:

ADD / ->probe() / BOUND ... UNBIND / ->remove() / DEL

And not the current (since your patch):

ADD / ->probe() / BOUND ... DEL / UNBIND / ->remove()

IE. The DEL callback might tear down data structures used by the driver,
such as DMA mapping stuff etc...  (In fact, that's pretty much the whole
point of this callback). ADD/DEL should be invoked while no driver is
active on the device.

Now if I look at the reason for your change, I discover what look to me
like added brokenness in the core, but again, I may be missing something
obvious. IE. The addition and removal path don't look symetric to me,
and you moved the DEL callback because in the first place, the core
tears down various things (such as PM or sysfs related data structures)
before the driver is unbound from the device.

Whatever you guys think is the right approach for those sysfs and PM
structures, I do believe that moving around the DEL callback was a
mistake and I can see that becoming an issue on various platforms (if
not already).

Cheers,
Ben.

Re: Device core removal ordering brokenness

[Alan Stern]

On Sun, 10 May 2009, Benjamin Herrenschmidt wrote:

> Hi Alan !

Hi!

> I was looking at git history regarding the various BUS_NOTIFY_*
> notifiers (since David needs some stuff for his DMA debug code that
> isn't provided by the current set) when I noticed that commit of yours:
> 
> ec0676ee28528dc8dda13a93ee4b1f215a0c2f9d
> 
> Unless I'm mistaken, which is very possible, this moves the
> BUS_NOTIFY_DEL_DEVICE callback to -before- the driver remove() callback
> is invoked. This sounds very illogical and potentially dangerous to me.
> 
> In fact, the original ordering and the only one that, to me, makes sense
> in term of semantics is:
> 
> ADD / ->probe() / BOUND ... UNBIND / ->remove() / DEL
> 
> And not the current (since your patch):
> 
> ADD / ->probe() / BOUND ... DEL / UNBIND / ->remove()
> 
> IE. The DEL callback might tear down data structures used by the driver,
> such as DMA mapping stuff etc...  (In fact, that's pretty much the whole
> point of this callback). ADD/DEL should be invoked while no driver is
> active on the device.
> 
> Now if I look at the reason for your change, I discover what look to me
> like added brokenness in the core, but again, I may be missing something
> obvious. IE. The addition and removal path don't look symetric to me,
> and you moved the DEL callback because in the first place, the core
> tears down various things (such as PM or sysfs related data structures)
> before the driver is unbound from the device.
> 
> Whatever you guys think is the right approach for those sysfs and PM
> structures, I do believe that moving around the DEL callback was a
> mistake and I can see that becoming an issue on various platforms (if
> not already).

Before the patch, the ordering was like this:

device_add: 	ADD  dpm_sysfs_add()  ->probe
device_del:	dpm_sysfs_remove()  ->remove  DEL

Now the ordering is like this:

device_add:	dpm_sysfs_add()  ADD  ->probe
device_del:	DEL  dpm_sysfs_remove()  ->remove

Okay, yes, it's not symmetrical.  But the point of the patch was to put
the DEL before the dpm_sysfs_remove(), and in any case the code wasn't
symmetrical even before the patch.  I gather that you'd prefer to see

device_del:	->remove  DEL  dpm_sysfs_remove()

Offhand I can't think of any reason not to do this.  Maybe someone else 
can; this code has a lot of undocumented constraints.  (Hmm, what 
happens if a system suspend occurs after the device has been 
unregistered from its bus but before it has been taken off the dpm 
list?  It's probably okay but worth checking...)

If you'd like to submit a patch moving the "if (dev->bus)...", 
device_pm_remove(), and dpm_sysfs_remove() stuff after the call to 
bus_remove_device(), go ahead.

Alan Stern

Re: Device core removal ordering brokenness

[Benjamin Herrenschmidt]

On Sun, 2009-05-10 at 10:58 -0400, Alan Stern wrote:

> Before the patch, the ordering was like this:
> 
> device_add: 	ADD  dpm_sysfs_add()  ->probe
> device_del:	dpm_sysfs_remove()  ->remove  DEL

Right.

> Now the ordering is like this:
> 
> device_add:	dpm_sysfs_add()  ADD  ->probe
> device_del:	DEL  dpm_sysfs_remove()  ->remove
> 
> Okay, yes, it's not symmetrical.  But the point of the patch was to put
> the DEL before the dpm_sysfs_remove(), and in any case the code wasn't
> symmetrical even before the patch.

How so ? It does definitely look symetrical above :-) That's not a big
deal per-se though, it's just that I want to be able to tear down data
structures in DEL that may be indirectly used by the driver (DMA mapping
related or even MMIO related internal arch stuff).

>   I gather that you'd prefer to see
> 
> device_del:	->remove  DEL  dpm_sysfs_remove()

I don't actually care that much about where drm_sysfs_remove() is vs.
DEL, but you seem to want to adjust the sysfs files in ADD and DEL, so
that would make sense.

> Offhand I can't think of any reason not to do this.  Maybe someone else 
> can; this code has a lot of undocumented constraints.  (Hmm, what 
> happens if a system suspend occurs after the device has been 
> unregistered from its bus but before it has been taken off the dpm 
> list?  It's probably okay but worth checking...)
> 
> If you'd like to submit a patch moving the "if (dev->bus)...", 
> device_pm_remove(), and dpm_sysfs_remove() stuff after the call to 
> bus_remove_device(), go ahead.

I first want to "probe" you guys in case there's some nasty skeleton
waiting around the corner, but yeah I'll probably do that. One other
option is to split DEL in two.

Ben

Re: Device core removal ordering brokenness

[Alan Stern]

On Mon, 11 May 2009, Benjamin Herrenschmidt wrote:

> On Sun, 2009-05-10 at 10:58 -0400, Alan Stern wrote:
> 
> > Before the patch, the ordering was like this:
> > 
> > device_add: 	ADD  dpm_sysfs_add()  ->probe
> > device_del:	dpm_sysfs_remove()  ->remove  DEL
> 
> Right.
> 
> > Now the ordering is like this:
> > 
> > device_add:	dpm_sysfs_add()  ADD  ->probe
> > device_del:	DEL  dpm_sysfs_remove()  ->remove
> > 
> > Okay, yes, it's not symmetrical.  But the point of the patch was to put
> > the DEL before the dpm_sysfs_remove(), and in any case the code wasn't
> > symmetrical even before the patch.
> 
> How so ? It does definitely look symetrical above :-)

The "ADD / ->probe / ->remove / DEL" sequence is symmetrical, but the
"dpm_sysfs_add() / ->probe / dpm_sysfs_remove() / ->remove" sequence 
isn't.

>  That's not a big
> deal per-se though, it's just that I want to be able to tear down data
> structures in DEL that may be indirectly used by the driver (DMA mapping
> related or even MMIO related internal arch stuff).

Okay, that's understandable.

> >   I gather that you'd prefer to see
> > 
> > device_del:	->remove  DEL  dpm_sysfs_remove()
> 
> I don't actually care that much about where drm_sysfs_remove() is vs.
> DEL, but you seem to want to adjust the sysfs files in ADD and DEL, so
> that would make sense.

Correct.

> > Offhand I can't think of any reason not to do this.  Maybe someone else 
> > can; this code has a lot of undocumented constraints.  (Hmm, what 
> > happens if a system suspend occurs after the device has been 
> > unregistered from its bus but before it has been taken off the dpm 
> > list?  It's probably okay but worth checking...)
> > 
> > If you'd like to submit a patch moving the "if (dev->bus)...", 
> > device_pm_remove(), and dpm_sysfs_remove() stuff after the call to 
> > bus_remove_device(), go ahead.
> 
> I first want to "probe" you guys in case there's some nasty skeleton
> waiting around the corner, but yeah I'll probably do that. One other
> option is to split DEL in two.

There doesn't appear to be any skeleton hanging around.  Go for it!

Alan Stern