Re: [tpmdd-devel] [PATCH] tpm: unified PPI interface for TPM 1.x/2.0 devices

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

Gave this some rethought :)

On Wed, Apr 08, 2015 at 10:26:07AM +0300, Jarkko Sakkinen wrote:
> On Wed, Apr 01, 2015 at 12:19:25PM -0600, Jason Gunthorpe wrote:
> > On Wed, Apr 01, 2015 at 03:28:52PM +0300, Jarkko Sakkinen wrote:
> > > Added PPI interface to the character device. PPI interface is also kept
> > > in the pdev for backwards compatibility.
> > 
> > Could you look at just completely moving the PPI interface to the char
> > dev and then adding a symlink from the pdev? That would be really
> > ideal.
> > 
> > symlinks have the advantage that they actually fully fix the lifetime
> > issues.
> > 
> > This seems doable, if we replace the ppi_attrs group with a bunch of
> > calls to sysfs_create_link it should work ?
> 
> If we follow the pattern in [1] by the book, how would you use
> sysfs_create_link()? To be more specific, how would you get the driver
> core to create the symlinks for you? 
> 
> If we decide not to follow [1] by the book, then this might be doable 
> (thinking off my head, that's the reason why I use *might be* instead
> of *is*). Wouldn't we get non-racy behavior if sysfs_create_link()'s
> are executed after device_initialize() and before device_add()?

Here I tend to lean towards for creating a separate set of attributes
instead. I would keep the legacy stuff completely separated of the sysfs
attributes for the character devices and not do any clever things in the
sysfs.

> > > +static struct tpm_chip *ppi_dev_to_chip(struct device *dev)
> > > +{
> > > +	struct tpm_chip *chip = dev_get_drvdata(dev);
> > > +
> > > +	if (chip == NULL)
> > > +		chip = to_tpm_chip(chip);
> > > +
> > > +	return chip;
> > > +}
> > 
> > If symlinks don't work out, we should probably just set the drvdata on
> > the tpm_chip itself to avoid this.
> 
> I'll experiment with this. Thanks for the comment.
> 
> > > +	if (!(chip->flags & TPM_CHIP_FLAG_PPI))
> > > +		return -EINVAL;
> > 
> > Hum, I don't think the PPI files should be created if there is no PPI
> > support..
> 
> Again, following [1] by the book. And again, I think we could just as
> well do our sysfs stuff in-between device_initialize() and device_add()
> and get the non-racy behavior.

I do not think it would be a bad idea to always create them when the
kernel is compiled with CONFIG_ACPI. Maybe it would be abetter idea to
return -ENOSYS? Device Model in the Linux kernel seems to recommend
through the defaults APIs a flat set of attributes for each device 
node.

> > > +void __init tpm_ppi_init(struct class *tpm_class)
> > > +{
> > > +	tpm_class->dev_groups = tpm_groups;
> > >  }
> > 
> > So this shouldn't be unconditional.
> > 
> > Also, ultimately PPI can't just claim the dev_groups, other parts of
> > the driver will need to add groups too.
> > 
> > I think it makes more sense to do
> > 
> > struct attribute_group *tpm_ppi_get_sysfs(struct tpm_chip *chip)
> > {
> > }
> > 
> > And take care of building the list in the caller.
> > 
> > And tpm_ppi_get_sysfs should be called after the driver is readied but
> > before adding the device.
> 
> I don't think this would matter. Things could be refactored when more
> sysfs attributes are needed.
> 
> > Jason

/Jarkko