Re: [PATCH v5 7/7] tpm: create TPM 2.0 devices using own device class

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

On Mon, 2014-11-03 at 14:38 -0700, Jason Gunthorpe wrote:
> On Mon, Nov 03, 2014 at 07:41:01AM +0200, Jarkko Sakkinen wrote:
> 
> > I used the class 'tpm' only for TPM 2.x because I didn't want to
> > break the binary compatibility for TPM 1.x anyway. In ideal situtation
> > both would be character devices inside the class 'tpm' and there would
> > be sysfs attribute such as 'family' to mark the protocol to be used.
> 
> You can create the class without moving away from miscdev...
> 
> Not having the device creates way to much difference that has to be
> supported, way too messy.

I have to admit that I'm not quite following here but I assume that
you restated this below in more verbose way and this is basically the
same argument :)

> > > And considering the volume of changes it might be better to leave
> > > 'dev' as a pointer to the tpm class rather than try and tackle that in
> > > this giant patch..
> > 
> > Maybe, or maybe I could make the rename a separate patch?
> 
> Pointer then rename?
> 
> > It's fairly mechanical, just a matter of replacing string
> > "chip->dev" with "chip->pdev".
> 
> Not everyone should be replaced :|

I think the current variable name "dev" is miss-leading. The
use of "pdev" would document better the appropriate use for that
field (i.e. for the most cases DON'T use it).

> > > > +	chip->cdev.owner = chip->pdev->driver->owner;
> > > 
> > > Is that right? the cdev fops is in this module, not the driver's
> > > module..
> > 
> > tpm.ko defines the interface but TPM device driver module owns the
> > character device. I think this is right and similar logic is also
> > for example rtc_device_register().
> 
> Hmm, yes, I see that in rtc_dev_prepare - but I don't have time to
> figure out why that might be :)
> 
> On the surface, it doesn't seem to make sense: The cdev layer never
> calls into a function that goes to the chip module - all functions go
> to the fops in tpm.ko, and tmp.ko is (eventually) responsible for
> ensuring that ops never points to an unloaded module.
> 
> So why should it care what the driver module is??

Fully agreed, I think you got a point here. I think it makes sense
as a guideline to kind of "centralize" robustness to tpm.ko and
leave as little responsibility as possible to device drivers.

> > I could understand in the context of a misc device but don't really
> > when TPM 2.0 devices have their own device class. Using a 'tpm' class
> > would in all cases break non-udev systems because major number is no
> > longer 10 (misc).
> 
> I'm just saying it would be nice to force the major/minor to misc.tpm
> for tmp0.
> 
> No idea if that is OK or not.

I think that would be a mess. The way things are done in this v5
patch set is actually quite coherent and it does not break backwards
compatibility because the "proper" device hierarchy is only used for
TPM 2.0 devices.

I think the improvement for v6 would be to add a sysctl attribute
to disable legacy device hierarchy and sysfs attributes. If that
attribute is unset, the old misc hierarchy would be used for any
TPM version and TPM 1.0 devices would use existing sysfs attributes.

If the flag is set, then the new proper device hierarchy would be
used for any TPM device and we would have also chance to redefine
sysfs attributes.

This would add some legacy clutter to the stack but would be a
piss-off-free and non-ABI-breaking solution.

> Jason

PS. I feel that drivers/char/tpm is a like a desolated town. How 
many of the people currently marked as maintainers are actually
participating to the subsystem development let alone driving it?

This subsystem is increasingly important to us given that major
software platforms like Chrome OS make extensive use of it and
I've heard that even bug fixes have taken some times over a year
to get through. Does not look good at all.

/Jarkko