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

[Jarkko Sakkinen]

On Sun, Nov 02, 2014 at 02:33:05PM -0700, Jason Gunthorpe wrote:
> On Sat, Nov 01, 2014 at 11:01:35AM +0200, Jarkko Sakkinen wrote:
> > Added own class for TPM devices that is used for TPM 2.0 and onwards.
> > For TPM1 old device structure is kept for backwards compatibility.
> > 
> > Each struct tpm_chip represents a character device that is associated
> > to the tpm device class.
> 
> I think we should hang back on this untill we can answer a few
> questions..
> 
> I certainly thing both TPM1 and TPM2 should create the class, at the
> worst only tpm2 uses it for the chardev?

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.

> 
> > @@ -63,7 +65,7 @@ static int tpm_open(struct inode *inode, struct file *file)
> >  	 * by the check of is_open variable, which is protected
> >  	 * by driver_lock. */
> >  	if (test_and_set_bit(0, &chip->is_open)) {
> > -		dev_dbg(chip->dev, "Another process owns this TPM\n");
> > +		dev_dbg(chip->pdev, "Another process owns this TPM\n");
> >  		return -EBUSY;
> 
> I actually think these are mostly wrong, and are part of why I think
> tpm1 should create the class too.
> 
> Except for the probe and remove function everything should log using
> the TPM device name (eg tpm0) and not the platform device name, so all
> of these debug statements should stay chip->dev...

I tend to agree but here applies my previous comment.

> 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?

It's fairly mechanical, just a matter of replacing string
"chip->dev" with "chip->pdev".
> 
> > +static void tpm_dev_release(struct device *dev)
> > +{
> > +}
> 
> And all of this is upside down, the devm interm stuff should hold a
> get_device() on the struct device and the kfree should live here.

Agreed but didn't want diverge from TPM1 sequences (yet!).
I think FIXME comment might be appropriate.

> > +	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().

> > +	rc = cdev_add(&chip->cdev, chip->dev.devt, 1);
> 
> Can we/should we re-use the misc dev minor number assigned to TPM for
> tpm0?
> 
> To me altering the dev major:minor  a far more visible change than
> moving the 'dev' file in sysfs. udev will handle the latter
> transparently, but the former will break non-udev systems..

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).

> Jason

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