linux-kernel - Re: Device core removal ordering brokenness

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.44L0.0905111000001.2960-100000@iolanthe.rowland.org>
Date:	Mon, 11 May 2009 10:02:04 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Benjamin Herrenschmidt <benh@...nel.crashing.org>
cc:	Greg KH <greg@...ah.com>,
	Linux Kernel list <linux-kernel@...r.kernel.org>,
	David Woodhouse <dwmw2@...radead.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: Device core removal ordering brokenness

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

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