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

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

On Wed, 2015-04-08 at 10:28 -0600, Jason Gunthorpe wrote:
> 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?
> 
> The driver core does not create symlinks, it creates the real files,
> which is the tpm_class->dev_groups part of your patch. That is fine..
> 
> The symlinks replace the broken legacy files under the
> platform_device. These are already racy, and different versions of the
> kernel have had different kind of races here. It wasn't until your
> 'tpm: fix call order in tpm-chip.c' that the ordering here started to
> make any kind of sense.
> 
> So, I'm inclined to say the legacy paths don't much matter. They have
> rarely worked race free so nothing out there can be depending on
> them.
> 
> I'd rather see the legacy paths be turned into symlinkes because that
> means we can close the use-after-free oops possibility the current
> code has, and that is a more serious bug than the user space race
> which has always existed anyhow.

OK, I'll consider doing this for the next iteration.

> > 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()?
> 
> That would at least preserve the latest semantic that the uevent is
> created after all the sysfs is in place. It is the best we can
> do.
> 
> Since this seems to address the race problem why do you think this is
> not worthwhile?
> 
> > > > +	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.
> 
> Not relying on the class default seems reasonable for ppi to me.
> 
> > 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?
> 
> This is not really consistent with other uses of sysfs, user space
> tooling has a harder time detecting ENOSYS than it does a file that
> doesn't exist.
> 
> It is also a change from the current PPI behavior, so I don't think we
> should do this without a very good reason.

Agreed.

> > Device Model in the Linux kernel seems to recommend
> > through the defaults APIs a flat set of attributes for each device
> > node.
> 
> No, that is just the defaults scheme, there are other ways to
> create attributes that are conditional based on device capabilities.
> 
> Greg notes:
> 
> > Sometimes you don't have control over the driver either, or want
> > different sysfs files for different devices controlled by your driver
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> drivers/firewire/core-device.c has an example of this, the
> config_rom_attributes are pruned to only expose the ones that actually
> exist using the struct device groups scheme.

Thanks for noting this.

> Jason

OK based on this discussion I'll iterate the patch.

/Jarkko