On Tue, Nov 05, 2013 at 08:34:00PM +0100, Arnd Bergmann wrote: > On Tuesday 05 November 2013, Jason Gunthorpe wrote: > > The issue is sysfs, the TPM core attaches attributes to the driver's > > struct device, which means it has to convert from struct device * to > > its own private data. It is totally wrong, but that is the way > > it has been for ever. :( > > I've just had a very brief look at that subsystem, and the first driver > I looked at (tpm_atmel) actually creates a child platform_device for > the sole purpose of registering that with the tpm subsystem, which seems > to avoid this problem. The TPM subsystem requires a struct device to bind onto (for sysfs at least), so drivers that don't have one are required to create one :| atmel is unusual as most drivers have a pre-existing device. > Maybe the same can be done for the other drivers as > well. Unfortunately, that will change the sysfs structure, which > might break user space relying on the current path to the > device. AFAIK the correct fix is to have the TPM subsystem core create a struct device for its own use (eg tpm0), under its own class, which is what other subsystems I looked at do. This way the subsystem can have access to a release function that is properly tied to the actual object lifetime, and it can use container_of to retrieve its own private data from, eg sysfs. Combined with get_ops/put_ops style access I think you can solve the unload lifetime issues with sysfs.. However, even if all that is done, compatibility sysfs's under the driver's struct device will still be needed, which AFAICT will still require the drvdata be owned by the subsystem not the driver :( > The TPM subsystem definely seems a bit unusual in this regard, so I > hope not too many other parts of the kernel have this particular > problem. It is just old, it was never updated to modern standards, hopefully that is rare :| > (side note: another unusual aspect of the TPM subsystem is the use of a > custom 'release' function override. Seems harmless, but very weird). Near as I can tell that is a hack that was put in to avoid panics around unload lifetime issues - it doesn't solve any problems, just makes them less common.. This is why I looked so closely at devm, I thought 'lets just remove that release function hack and use devm', but they are not equivalent. :) Thanks for looking at this, a good chunk of the first round of my cleanups will hit 3.13, next on the list are these problems. https://lkml.org/lkml/2013/9/23/444 Jason -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
