Message-ID: <1428559359.5321.12.camel@linux.intel.com>
Subject: Re: [PATCH] tpm: unified PPI interface for TPM 1.x/2.0 devices
From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Cc: Peter Huewe <peterhuewe@gmx.de>, Ashley Lai <ashley@ashleylai.com>,
        Marcel Selhorst <tpmdd@selhorst.net>,
        tpmdd-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org,
        christophe.ricard@gmail.com, jason.gunthorpe@obsidianresearch.com,
        stefanb@linux.vnet.ibm.com
Date: Thu, 09 Apr 2015 09:02:39 +0300
In-Reply-To: <20150408162833.GA26497@obsidianresearch.com>
References: <1427891332-16709-1-git-send-email-jarkko.sakkinen@linux.intel.com>
	 <20150401181925.GA20550@obsidianresearch.com>
	 <20150408072607.GA7994@intel.com>
	 <20150408162833.GA26497@obsidianresearch.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4318
Lines: 109

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


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