Date: Wed, 8 Apr 2015 10:28:33 -0600
From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.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
Subject: Re: [PATCH] tpm: unified PPI interface for TPM 1.x/2.0 devices
Message-ID: <20150408162833.GA26497@obsidianresearch.com>
References: <1427891332-16709-1-git-send-email-jarkko.sakkinen@linux.intel.com>
 <20150401181925.GA20550@obsidianresearch.com>
 <20150408072607.GA7994@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20150408072607.GA7994@intel.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3932
Lines: 96

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.

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

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

Jason
--
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/