[tpmdd-devel,v5,4/5] Initialize TPM and get durations and timeouts
diff mbox

Message ID 1454959628-30582-5-git-send-email-stefanb@linux.vnet.ibm.com
State New
Headers show

Commit Message

Stefan Berger Feb. 8, 2016, 7:27 p.m. UTC
Add the retrieval of TPM 1.2 durations and timeouts. Since this requires
the startup of the TPM, do this for TPM 1.2 and TPM 2.

To not allow to interleave with commands from user space, so send the
TPM_Startup as the first command. The timeouts can then be gotten at any
later time, even interleaved with commands from user space.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm-vtpm.c | 58 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

Comments

Jason Gunthorpe Feb. 9, 2016, 5:33 a.m. UTC | #1
On Mon, Feb 08, 2016 at 02:27:07PM -0500, Stefan Berger wrote:
> Add the retrieval of TPM 1.2 durations and timeouts. Since this requires
> the startup of the TPM, do this for TPM 1.2 and TPM 2.
> 
> To not allow to interleave with commands from user space, so send the
> TPM_Startup as the first command. The timeouts can then be gotten at any
> later time, even interleaved with commands from user space.

Do not call tpm_register until get_timeouts has completed and this
will naturally be avoided the same way every other TPM driver does.

The long term goal is to move the get_timeouts call into tpm_register.

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Stefan Berger Feb. 9, 2016, 4:19 p.m. UTC | #2
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/09/2016 
12:33:23 AM:


> 
> On Mon, Feb 08, 2016 at 02:27:07PM -0500, Stefan Berger wrote:
> > Add the retrieval of TPM 1.2 durations and timeouts. Since this 
requires
> > the startup of the TPM, do this for TPM 1.2 and TPM 2.
> > 
> > To not allow to interleave with commands from user space, so send the
> > TPM_Startup as the first command. The timeouts can then be gotten at 
any
> > later time, even interleaved with commands from user space.
> 
> Do not call tpm_register until get_timeouts has completed and this
> will naturally be avoided the same way every other TPM driver does.

Getting the timeouts cannot complete before the TPM emulator has started, 
which in turn cannot start before the ioctl returns. We don't know whether 
user space starts a client first on /dev/tpm1 or the TPM emulator starts 
first on the file descriptor. We can control which command gets *queued* 
for the TPM emulator and that's what I am doing by having the kernel queue 
the startup command first.

I moved the start of the work call before the tpm_chip_register, so that 
we *queue* the TPM_Startup before user space can send its first command.

> 
> The long term goal is to move the get_timeouts call into tpm_register.

That may become a problem then.

   Stefan

> 
> Jason
> 
> 
------------------------------------------------------------------------------
> Site24x7 APM Insight: Get Deep Visibility into Application Performance
> APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
> Monitor end-to-end web transactions and take corrective actions now
> Troubleshoot faster and improve end-user experience. Signup Now!
> http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
>
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Jason Gunthorpe Feb. 9, 2016, 4:52 p.m. UTC | #3
On Tue, Feb 09, 2016 at 11:19:15AM -0500, Stefan Berger wrote:
> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/09/2016 12:33:23
> AM:
> 
> 
> >
> > On Mon, Feb 08, 2016 at 02:27:07PM -0500, Stefan Berger wrote:
> > > Add the retrieval of TPM 1.2 durations and timeouts. Since this requires
> > > the startup of the TPM, do this for TPM 1.2 and TPM 2.
> > >
> > > To not allow to interleave with commands from user space, so send the
> > > TPM_Startup as the first command. The timeouts can then be gotten at any
> > > later time, even interleaved with commands from user space.
> >
> > Do not call tpm_register until get_timeouts has completed and this
> > will naturally be avoided the same way every other TPM driver does.
> 
> Getting the timeouts cannot complete before the TPM emulator has started, which
> in turn cannot start before the ioctl returns. We don't know whether user space
> starts a client first on /dev/tpm1 or the TPM emulator starts first on the file
> descriptor. We can control which command gets *queued* for the TPM emulator and
> that's what I am doing by having the kernel queue the startup command first.

I keep saying this same solution - start a work queue just before the
ioctl returns and do the get_timeouts and registration in there.

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Stefan Berger Feb. 9, 2016, 5:45 p.m. UTC | #4
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/09/2016 
11:52:28 AM:

> 
> On Tue, Feb 09, 2016 at 11:19:15AM -0500, Stefan Berger wrote:
> > Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/09/
> 2016 12:33:23
> > AM:
> > 
> > 
> > >
> > > On Mon, Feb 08, 2016 at 02:27:07PM -0500, Stefan Berger wrote: 
> > > > Add the retrieval of TPM 1.2 durations and timeouts. Since this 
requires
> > > > the startup of the TPM, do this for TPM 1.2 and TPM 2.
> > > >
> > > > To not allow to interleave with commands from user space, so send 
the
> > > > TPM_Startup as the first command. The timeouts can then be gotten 
at any
> > > > later time, even interleaved with commands from user space.
> > >
> > > Do not call tpm_register until get_timeouts has completed and this
> > > will naturally be avoided the same way every other TPM driver does.
> > 
> > Getting the timeouts cannot complete before the TPM emulator has 
> started, which
> > in turn cannot start before the ioctl returns. We don't know 
> whether user space
> > starts a client first on /dev/tpm1 or the TPM emulator starts 
> first on the file
> > descriptor. We can control which command gets *queued* for the TPM
> emulator and
> > that's what I am doing by having the kernel queue the startup command 
first.
> 
> I keep saying this same solution - start a work queue just before the
> ioctl returns and do the get_timeouts and registration in there.

And how do we unroll if tpm_chip_register fails? We just close the fd that 
we gave to user space? 

I also don't see why it is a better solution than what I have here now:

https://github.com/stefanberger/linux/commit/954c7dece4a5c44525ca07a9569dc2f3b2b3d174


Stefan

> 
> Jason
>
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Jason Gunthorpe Feb. 9, 2016, 6:01 p.m. UTC | #5
On Tue, Feb 09, 2016 at 12:45:37PM -0500, Stefan Berger wrote:
> And how do we unroll if tpm_chip_register fails? We just close the fd that we
> gave to user space?

Arrange for the fd to become permanently readable and read returns
error, user space will close the fd.

> I also don't see why it is a better solution than what I have here now:
> 
> https://github.com/stefanberger/linux/commit/
> 954c7dece4a5c44525ca07a9569dc2f3b2b3d174

As I've said, this mucks up where we want to go with the core code,
and breaks our current invariants - the TPM hardware must be fully
operational before tpm_register is called.

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Stefan Berger Feb. 9, 2016, 6:11 p.m. UTC | #6
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/09/2016 
01:01:52 PM:


> 
> On Tue, Feb 09, 2016 at 12:45:37PM -0500, Stefan Berger wrote:
> > And how do we unroll if tpm_chip_register fails? We just close thefd 
that we
> > gave to user space?
> 
> Arrange for the fd to become permanently readable and read returns
> error, user space will close the fd.

:-( User space will just pass the fd to another process, which is the TPM 
emulator, and that thing then starts failing on the broken file 
descriptor.

> 
> > I also don't see why it is a better solution than what I have here 
now:
> > 
> > https://github.com/stefanberger/linux/commit/
> > 954c7dece4a5c44525ca07a9569dc2f3b2b3d174
> 
> As I've said, this mucks up where we want to go with the core code,
> and breaks our current invariants - the TPM hardware must be fully
> operational before tpm_register is called.

Either way, I don't see how the TPM emulator can be fully operational 
before tpm_chip_register() is called. So far the code allows to start a 
TPM emulator on the file descript 'late'. The possibility for accomodating 
the solution in the future is there by leaving the possibility for a 
boolean where the driver can choose whether timeout retrieval is done as 
part of tpm_chip_register or it has to take care of this itself.

   Stefan
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Jason Gunthorpe Feb. 9, 2016, 6:20 p.m. UTC | #7
On Tue, Feb 09, 2016 at 01:11:58PM -0500, Stefan Berger wrote:

> :-( User space will just pass the fd to another process, which is the TPM
> emulator, and that thing then starts failing on the broken file descriptor.

.. and then it exits like any other failure and the caller has to sort
it out, which it already has to handle.

> Either way, I don't see how the TPM emulator can be fully operational before
> tpm_chip_register() is called.

You still haven't said what is wrong with doing all the work in a work
queue so user space can respond normally without all this ugly hacking.

> So far the code allows to start a TPM emulator on the file descript
> 'late'.

That breaks our invariants to user space. The /dev/tpmX, etc cannot appear
until get_timeouts/etc complete. This isn't optional. Ordering startup
isn't sufficient.

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Stefan Berger Feb. 9, 2016, 7:22 p.m. UTC | #8
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/09/2016 
01:20:13 PM:


> Subject: Re: [tpmdd-devel] [PATCH v5 4/5] Initialize TPM and get 
> durations and timeouts
> 
> On Tue, Feb 09, 2016 at 01:11:58PM -0500, Stefan Berger wrote:
> 
> > :-( User space will just pass the fd to another process, which is the 
TPM
> > emulator, and that thing then starts failing on the broken file 
descriptor.
> 
> .. and then it exits like any other failure and the caller has to sort
> it out, which it already has to handle.
> 
> > Either way, I don't see how the TPM emulator can be fully operational 
before
> > tpm_chip_register() is called.
> 
> You still haven't said what is wrong with doing all the work in a work
> queue so user space can respond normally without all this ugly hacking.

It doesn't seem right to simply close the file descriptor on the process 
if something goes wrong. The process couldn't close() on it since another 
thread could have gotten the same fd number for something else. I haven't 
seen a file descriptor that one must not close() in user space.

It doesn't seem to work to only close the file (fput()) in case something 
goes wrong while sending the commands to the TPM and before the 
tpm_chip_register(). In that case I see the message 'VFS: Close: file 
count is 0' once the application closes the fd.

Do you have any insights?

   Stefan
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Jason Gunthorpe Feb. 9, 2016, 7:36 p.m. UTC | #9
On Tue, Feb 09, 2016 at 02:22:21PM -0500, Stefan Berger wrote:
>    It doesn't seem right to simply close the file descriptor on the
>    process if something goes wrong. The process couldn't close() on it
>    since another thread could have gotten the same fd number for something
>    else. I haven't seen a file descriptor that one must not close() in
>    user space.

The kernel should never unilaterally close a fd, just make it
permanently error, like a far-end closed socket. Read fails, write
fails, and poll returns readable. User space will close it when it
detects read failure.

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Jarkko Sakkinen Feb. 10, 2016, 3:56 a.m. UTC | #10
On Tue, Feb 09, 2016 at 09:52:28AM -0700, Jason Gunthorpe wrote:
> On Tue, Feb 09, 2016 at 11:19:15AM -0500, Stefan Berger wrote:
> > Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/09/2016 12:33:23
> > AM:
> > 
> > 
> > >
> > > On Mon, Feb 08, 2016 at 02:27:07PM -0500, Stefan Berger wrote:
> > > > Add the retrieval of TPM 1.2 durations and timeouts. Since this requires
> > > > the startup of the TPM, do this for TPM 1.2 and TPM 2.
> > > >
> > > > To not allow to interleave with commands from user space, so send the
> > > > TPM_Startup as the first command. The timeouts can then be gotten at any
> > > > later time, even interleaved with commands from user space.
> > >
> > > Do not call tpm_register until get_timeouts has completed and this
> > > will naturally be avoided the same way every other TPM driver does.
> > 
> > Getting the timeouts cannot complete before the TPM emulator has started, which
> > in turn cannot start before the ioctl returns. We don't know whether user space
> > starts a client first on /dev/tpm1 or the TPM emulator starts first on the file
> > descriptor. We can control which command gets *queued* for the TPM emulator and
> > that's what I am doing by having the kernel queue the startup command first.
> 
> I keep saying this same solution - start a work queue just before the
> ioctl returns and do the get_timeouts and registration in there.

*Maybe* it would be worth to check David's patch:

https://github.com/PeterHuewe/linux-tpmdd/commit/9329f13c403daf1f4bd1e715d2ba0866e089fb1d

/Jarkko

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Stefan Berger Feb. 10, 2016, 5:15 a.m. UTC | #11
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote on 02/09/2016 
10:56:20 PM:

> 
> On Tue, Feb 09, 2016 at 09:52:28AM -0700, Jason Gunthorpe wrote:
> > On Tue, Feb 09, 2016 at 11:19:15AM -0500, Stefan Berger wrote:
> > > Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/
> 09/2016 12:33:23
> > > AM:
> > > 
> > > 
> > > >
> > > > On Mon, Feb 08, 2016 at 02:27:07PM -0500, Stefan Berger wrote:
> > > > > Add the retrieval of TPM 1.2 durations and timeouts. Since 
> this requires
> > > > > the startup of the TPM, do this for TPM 1.2 and TPM 2.
> > > > >
> > > > > To not allow to interleave with commands from user space, so 
send the
> > > > > TPM_Startup as the first command. The timeouts can then be 
> gotten at any
> > > > > later time, even interleaved with commands from user space.
> > > >
> > > > Do not call tpm_register until get_timeouts has completed and this
> > > > will naturally be avoided the same way every other TPM driver 
does.
> > > 
> > > Getting the timeouts cannot complete before the TPM emulator has
> started, which
> > > in turn cannot start before the ioctl returns. We don't know 
> whether user space
> > > starts a client first on /dev/tpm1 or the TPM emulator starts 
> first on the file
> > > descriptor. We can control which command gets *queued* for the 
> TPM emulator and
> > > that's what I am doing by having the kernel queue the startup 
> command first.
> > 
> > I keep saying this same solution - start a work queue just before the
> > ioctl returns and do the get_timeouts and registration in there.
> 
> *Maybe* it would be worth to check David's patch:
> 
> https://github.com/PeterHuewe/linux-tpmdd/commit/
> 9329f13c403daf1f4bd1e715d2ba0866e089fb1d


Redid that now.

https://github.com/stefanberger/linux/commit/83019eaab2cf5eb33f2665efdf9d2a117ed703b2
   Stefan
> 
> /Jarkko
>
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Jarkko Sakkinen Feb. 10, 2016, 8:12 a.m. UTC | #12
On Wed, Feb 10, 2016 at 12:15:44AM -0500, Stefan Berger wrote:
>    Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote on 02/09/2016
>    10:56:20 PM:
> 
>    >
>    > On Tue, Feb 09, 2016 at 09:52:28AM -0700, Jason Gunthorpe wrote:
>    > > On Tue, Feb 09, 2016 at 11:19:15AM -0500, Stefan Berger wrote:
>    > > > Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/
>    > 09/2016 12:33:23
>    > > > AM:
>    > > >
>    > > >
>    > > > >
>    > > > > On Mon, Feb 08, 2016 at 02:27:07PM -0500, Stefan Berger wrote:
>    > > > > > Add the retrieval of TPM 1.2 durations and timeouts. Since
>    > this requires
>    > > > > > the startup of the TPM, do this for TPM 1.2 and TPM 2.
>    > > > > >
>    > > > > > To not allow to interleave with commands from user space, so
>    send the
>    > > > > > TPM_Startup as the first command. The timeouts can then be
>    > gotten at any
>    > > > > > later time, even interleaved with commands from user space.
>    > > > >
>    > > > > Do not call tpm_register until get_timeouts has completed and this
>    > > > > will naturally be avoided the same way every other TPM driver
>    does.
>    > > >
>    > > > Getting the timeouts cannot complete before the TPM emulator has
>    > started, which
>    > > > in turn cannot start before the ioctl returns. We don't know
>    > whether user space
>    > > > starts a client first on /dev/tpm1 or the TPM emulator starts
>    > first on the file
>    > > > descriptor. We can control which command gets *queued* for the
>    > TPM emulator and
>    > > > that's what I am doing by having the kernel queue the startup
>    > command first.
>    > >
>    > > I keep saying this same solution - start a work queue just before the
>    > > ioctl returns and do the get_timeouts and registration in there.
>    >
>    > *Maybe* it would be worth to check David's patch:
>    >
>    > https://github.com/PeterHuewe/linux-tpmdd/commit/
>    > 9329f13c403daf1f4bd1e715d2ba0866e089fb1d
> 
>    Redid that now.
> 
>    https://github.com/stefanberger/linux/commit/83019eaab2cf5eb33f2665efdf9d2a117ed703b2

With very quick skim looks very similar, which is good because this
approach is tested and known to work thanks to David.

/Jarkko

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Jason Gunthorpe Feb. 10, 2016, 4:28 p.m. UTC | #13
On Wed, Feb 10, 2016 at 12:15:44AM -0500, Stefan Berger wrote:
>    Redid that now.
>     https://github.com/stefanberger/linux/commit/83019eaab2cf5eb33f2665efdf9d2a117ed703b2

Yeah, I think you've got that now.

Just a few other random commments..

This isn't great:

	vtpm_dev->dev_num = find_first_zero_bit(dev_mask, VTPM_NUM_DEVICES);

We shouldn't artificially limit the number of devices if
virtualization is the target. Use an idr, or figure out how to get rid
of it. Since it is only used here:

    dev_set_name(&vtpm_dev->dev, "vtpms%d", vtpm_dev->dev_num);

Maybe it could be adjusted to use chip->dev_num instead.

This:
	INIT_WORK(&vtpm_dev->work, vtpm_dev_work);
should be near the kzalloc

Drop the 1 line functions (vtpm_dev_work_start, vtpm_delete_vtpm_dev,

Use u32's here:

	u8 req_buf[TPM_BUFSIZE];     /* request buffer */

and related to ensure the buffer is sensibly aligned

Don't log on user space inputs, use debug or something:

		dev_err(&vtpm_dev->dev,
			"Invalid size in recv: count=%zd, req_len=%zd\n",
			count, len);

This:

	vtpm_dev_work_stop(vtpm_dev);

Doesn't kill a running work thread - more synchronization is needed
for that corner case

Make sure that devm is working for your specific case when this
happens:

 err:
	/* did not register chip */
	vtpm_dev->chip = NULL;

Instrument the core, devm should free the tpm chip when user space
closes the fd. If not the core needs to provide a non-devm alloc for
this driver. (easy)

This spin lock:

	spin_lock(&driver_lock);
	vtpm_dev->dev_num = find_first_zero_bit(dev_mask, VTPM_NUM_DEVICES);
	[..]
		err = device_register(&vtpm_dev->dev); /* does get_device */
	spin_unlock(&driver_lock);

I'm not comfortable with how big a region that is. If you keep
dev_mask, then only lock around the dev_mask manipulation not other
stuff. Relock to undo

fops_write should fail if op_recv isn't waiting for a buffer.

Looks pretty good really

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Stefan Berger Feb. 10, 2016, 9:45 p.m. UTC | #14
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/10/2016 
11:28:09 AM:

> 
> On Wed, Feb 10, 2016 at 12:15:44AM -0500, Stefan Berger wrote:
> >    Redid that now.
> >     https://github.com/stefanberger/linux/commit/
> 83019eaab2cf5eb33f2665efdf9d2a117ed703b2
> 
> Yeah, I think you've got that now.
> 
> Just a few other random commments..
> 
> This isn't great:
> 
>    vtpm_dev->dev_num = find_first_zero_bit(dev_mask, VTPM_NUM_DEVICES);
> 
> We shouldn't artificially limit the number of devices if
> virtualization is the target. Use an idr, or figure out how to get rid
> of it. Since it is only used here:
> 
>     dev_set_name(&vtpm_dev->dev, "vtpms%d", vtpm_dev->dev_num);
> 
> Maybe it could be adjusted to use chip->dev_num instead.

The chip->dev_num comes back from tpmm_chip_alloc() which is called with 
the device as a parameter. That's the device we're trying to create. So it 
seems we need to have two independency ways to generate unique device 
names. My suggestion would have been to increase the bitmap to the maximum 
size of the minor numbers but that's 20 bits, 1Mb. This would need to be 
done for both sides.

I don't know how idr applies here from what I read in an article. 
https://lwn.net/Articles/103209/

> 
> This:
>    INIT_WORK(&vtpm_dev->work, vtpm_dev_work);
> should be near the kzalloc

Ok, I'll move that.

> 
> Drop the 1 line functions (vtpm_dev_work_start, vtpm_delete_vtpm_dev,

We have these function pairs here:

vtpm_dev_work_start
vtpm_dev_work_stop

and

vtpm_create_vtpm_dev
vtpm_delete_vtpm_dev

Can we just keep them as pairs with the one function undoing what the 
other one has done. I prefix the one-liners with a 'static inline' and let 
the compiler optimize the code.


> 
> Use u32's here:
> 
>    u8 req_buf[TPM_BUFSIZE];     /* request buffer */
> 
> and related to ensure the buffer is sensibly aligned

May I ask why use u32's for a byte buffer? I am not sure what alignment we 
need here. 4 byte boundary for maximum memcpy performance?

http://lxr.free-electrons.com/source/drivers/char/tpm/tpm-dev.c#L34
http://lxr.free-electrons.com/source/drivers/char/tpm/tpm_i2c_infineon.c#L67



> 
> Don't log on user space inputs, use debug or something:
> 
>       dev_err(&vtpm_dev->dev,
>          "Invalid size in recv: count=%zd, req_len=%zd\n",
>          count, len);
> 
> This:
> 
>    vtpm_dev_work_stop(vtpm_dev);
> 
> Doesn't kill a running work thread - more synchronization is needed
> for that corner case

Do you mean another test for STATE_OPEN bit is needed before it enters the 
wait_event_interruptible in the code below?
Otherwise I tested this. What can happen is that the worker thread is 
stuck in the wait function:

static int vtpm_tpm_op_recv(struct tpm_chip *chip, u8 *buf, size_t count)
[...]
        /* wait for response or responder gone */
        sig = wait_event_interruptible(vtpm_dev->wq,
                (vtpm_dev->resp_len != 0
                || !test_bit(STATE_OPENED, &vtpm_dev->state)));
        if (sig)
                return -EINTR;

        /* process gone ? */
        if (!test_bit(STATE_OPENED, &vtpm_dev->state))
                return -EIO;
[...]

We loop while the task is still busy and kick it out of the above wait 
queue with this function here. Ok, so far we may kick multiple times since 
the condition may only be tested after waiting.

static void vtpm_fops_undo_open(struct vtpm_dev *vtpm_dev)
{
        clear_bit(STATE_OPENED, &vtpm_dev->state);

        /* no more TPM responses -- wake up anyone waiting for them */
        wake_up_interruptible(&vtpm_dev->wq);
}



> 
> Make sure that devm is working for your specific case when this
> happens:
> 

Just tested this and it seems to work. Chip is kfree'd once user space 
closes the fd.

>  err:
>    /* did not register chip */
>    vtpm_dev->chip = NULL;
> 
> Instrument the core, devm should free the tpm chip when user space
> closes the fd. If not the core needs to provide a non-devm alloc for
> this driver. (easy)


It does that. We only partly unwind in the thread by letting the file 
descriptor fail upon read and write and do the full unwinding upon file 
descriptor closure.


> 
> This spin lock:
> 
>    spin_lock(&driver_lock);
>    vtpm_dev->dev_num = find_first_zero_bit(dev_mask, VTPM_NUM_DEVICES);
>    [..]
>       err = device_register(&vtpm_dev->dev); /* does get_device */
>    spin_unlock(&driver_lock);
> 
> I'm not comfortable with how big a region that is. If you keep
> dev_mask, then only lock around the dev_mask manipulation not other
> stuff. Relock to undo

Ok.

> 
> fops_write should fail if op_recv isn't waiting for a buffer.

So we introduce another state flag whether we are in receiving or sending 
mode ? Return -EBADF in the case of an unexpected write() ? Though -EBADF 
indicates "fd is not a valid file descriptor or is not open for writing." 
So return -EIO?

> 
> Looks pretty good really

Yes. Thanks. And let's just keep the function pairs for the humans and let 
the compiler use the 'static inline' .

    Stefan


> 
> Jason
>
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Jason Gunthorpe Feb. 10, 2016, 10:23 p.m. UTC | #15
>    > We shouldn't artificially limit the number of devices if
>    > virtualization is the target. Use an idr, or figure out how to get rid
>    > of it. Since it is only used here:
>    >
>    >     dev_set_name(&vtpm_dev->dev, "vtpms%d", vtpm_dev->dev_num);
>    >
>    > Maybe it could be adjusted to use chip->dev_num instead.

>    The chip->dev_num comes back from tpmm_chip_alloc() which is called
>    with the device as a parameter. That's the device we're trying to

Hm, that is only needed for devm, could also do the tpm/tpmm split and
avoid needing a registered dev.

>    create. So it seems we need to have two independency ways to generate
>    unique device names. My suggestion would have been to increase the
>    bitmap to the maximum size of the minor numbers but that's 20 bits,
>    1Mb. This would need to be done for both sides.

No way

>    I don't know how idr applies here from what I read in an article.
>    [2]https://lwn.net/Articles/103209/

Most subsystems will use a idr to map the index number back to their
private number. idr efficiently allocates these numbers. Eg look at
how drivers/rtc/class.c uses the ida_ functions.

BTW, get rid of vtpm_list - it seems totally unusued.


>    >
>    > Drop the 1 line functions (vtpm_dev_work_start, vtpm_delete_vtpm_dev,
>    We have these function pairs here:
>    vtpm_dev_work_start
>    vtpm_dev_work_stop
>    and
>    vtpm_create_vtpm_dev
>    vtpm_delete_vtpm_dev
>    Can we just keep them as pairs with the one function undoing what the
>    other one has done. I prefix the one-liners with a 'static inline' and
>    let the compiler optimize the code.

I don't care that much either way.. But these sorts of obfuscating
wrappers are not really in line with good kernel practices.

Especially if they can only legally be called from one place. Ie the
work cleanup can only be properly called from fops release..


>    > Use u32's here:
>    >
>    >    u8 req_buf[TPM_BUFSIZE];     /* request buffer */
>    >
>    > and related to ensure the buffer is sensibly aligned
>    May I ask why use u32's for a byte buffer? I am not sure what alignment
>    we need here. 4 byte boundary for maximum memcpy performance?

Yes, it is nice to see the alignment guarenteed if memcpy is the
primary use of this memory. It almost always happens naturally..

>    > Doesn't kill a running work thread - more synchronization is needed
>    > for that corner case
>    Do you mean another test for STATE_OPEN bit is needed before it enters
>    the wait_event_interruptible in the code below?

No, just use a typical kernel idiom:

            clear_bit(STATE_OPENED, &vtpm_dev->state);
            wake_up_interruptible(&vtpm_dev->wq);
	    flush_workqueue(&vtpm_dev->work);

And it would be typical/desirable to see that open coded in the
one and only cleanup routine. That is what I usually look for when
looking at work queues.

I can't rember off hand if you need locking around dev->state to make
the above work, IIRC the bit ops are special.

Don't use work_busy in production code.

>    >
>    > fops_write should fail if op_recv isn't waiting for a buffer.
>    So we introduce another state flag whether we are in receiving or
>    sending mode ? Return -EBADF in the case of an unexpected write() ?
>    Though -EBADF indicates "fd is not a valid file descriptor or is not
>    open for writing." So return -EIO?

Probably yeah, pick an error code makes sense. EIO is probably OK, but
maybe shift the 'STATE_OPEN' code to EPIPE or something.

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Stefan Berger Feb. 11, 2016, 12:38 a.m. UTC | #16
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/10/2016 
05:23:13 PM:

>
> 
> >    > We shouldn't artificially limit the number of devices if
> >    > virtualization is the target. Use an idr, or figure out how to 
get rid
> >    > of it. Since it is only used here:
> >    >
> >    >     dev_set_name(&vtpm_dev->dev, "vtpms%d", vtpm_dev->dev_num);
> >    >
> >    > Maybe it could be adjusted to use chip->dev_num instead.
> 
> >    The chip->dev_num comes back from tpmm_chip_alloc() which is called
> >    with the device as a parameter. That's the device we're trying to
> 
> Hm, that is only needed for devm, could also do the tpm/tpmm split and
> avoid needing a registered dev.
> 

How about another patch on top of the ones I have so far only solving this 
problem ?

> 
> 
> >    > Use u32's here:
> >    >
> >    >    u8 req_buf[TPM_BUFSIZE];     /* request buffer */
> >    >
> >    > and related to ensure the buffer is sensibly aligned
> >    May I ask why use u32's for a byte buffer? I am not sure what 
alignment
> >    we need here. 4 byte boundary for maximum memcpy performance?
> 
> Yes, it is nice to see the alignment guarenteed if memcpy is the
> primary use of this memory. It almost always happens naturally..

We now have only one buffer and there's a size_t in front of it. So 
alignment will be at 4 bytes.

> 
> >    > Doesn't kill a running work thread - more synchronization is 
needed
> >    > for that corner case
> >    Do you mean another test for STATE_OPEN bit is needed before it 
enters
> >    the wait_event_interruptible in the code below?
> 
> No, just use a typical kernel idiom:
> 
>             clear_bit(STATE_OPENED, &vtpm_dev->state);
>             wake_up_interruptible(&vtpm_dev->wq);
>        flush_workqueue(&vtpm_dev->work);
> 
> And it would be typical/desirable to see that open coded in the
> one and only cleanup routine. That is what I usually look for when
> looking at work queues.
> 
> I can't rember off hand if you need locking around dev->state to make
> the above work, IIRC the bit ops are special.

These ops are atomic:

http://lxr.free-electrons.com/source/include/asm-generic/bitops/atomic.h#L86


   Stefan

> 
> Don't use work_busy in production code.
> 
> >    >
> >    > fops_write should fail if op_recv isn't waiting for a buffer.
> >    So we introduce another state flag whether we are in receiving or
> >    sending mode ? Return -EBADF in the case of an unexpected write() ?
> >    Though -EBADF indicates "fd is not a valid file descriptor or is 
not
> >    open for writing." So return -EIO?
> 
> Probably yeah, pick an error code makes sense. EIO is probably OK, but
> maybe shift the 'STATE_OPEN' code to EPIPE or something.
> 
> Jason
>
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Jarkko Sakkinen Feb. 11, 2016, 7:04 a.m. UTC | #17
On Wed, Feb 10, 2016 at 07:38:52PM -0500, Stefan Berger wrote:
>    Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/10/2016
>    05:23:13 PM:
> 
>    >
>    >
>    > >    > We shouldn't artificially limit the number of devices if
>    > >    > virtualization is the target. Use an idr, or figure out how to
>    get rid
>    > >    > of it. Since it is only used here:
>    > >    >
>    > >    >     dev_set_name(&vtpm_dev->dev, "vtpms%d", vtpm_dev->dev_num);
>    > >    >
>    > >    > Maybe it could be adjusted to use chip->dev_num instead.
>    >
>    > >    The chip->dev_num comes back from tpmm_chip_alloc() which is called
>    > >    with the device as a parameter. That's the device we're trying to
>    >
>    > Hm, that is only needed for devm, could also do the tpm/tpmm split and
>    > avoid needing a registered dev.
>    >
> 
>    How about another patch on top of the ones I have so far only solving this
>    problem ?

IDR has been on my backlog for a long time (over a year now).

If you'd create a patch that would migrate the subsystem to that and
do also do this split, I could take that patch as an independent
contribution and you would have a better baseline to develop on top
of. What do you think?

/Jarkko

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Stefan Berger Feb. 11, 2016, 3:24 p.m. UTC | #18
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote on 02/11/2016 
02:04:26 AM:

> From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> To: Stefan Berger/Watson/IBM@IBMUS
> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>, 
> dhowells@redhat.com, dwmw2@infradead.org, 
tpmdd-devel@lists.sourceforge.net
> Date: 02/11/2016 02:04 AM
> Subject: Re: [tpmdd-devel] [PATCH v5 4/5] Initialize TPM and get 
> durations and timeouts
> 
> On Wed, Feb 10, 2016 at 07:38:52PM -0500, Stefan Berger wrote:
> >    Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 
02/10/2016
> >    05:23:13 PM:
> > 
> >    >
> >    >
> >    > >    > We shouldn't artificially limit the number of devices if
> >    > >    > virtualization is the target. Use an idr, or figure out 
how to
> >    get rid
> >    > >    > of it. Since it is only used here:
> >    > >    >
> >    > >    >     dev_set_name(&vtpm_dev->dev, "vtpms%d", 
vtpm_dev->dev_num);
> >    > >    >
> >    > >    > Maybe it could be adjusted to use chip->dev_num instead.
> >    >
> >    > >    The chip->dev_num comes back from tpmm_chip_alloc() 
> which is called
> >    > >    with the device as a parameter. That's the device we're 
trying to
> >    >
> >    > Hm, that is only needed for devm, could also do the tpm/tpmm 
split and
> >    > avoid needing a registered dev.
> >    >
> > 
> >    How about another patch on top of the ones I have so far only 
> solving this
> >    problem ?
> 
> IDR has been on my backlog for a long time (over a year now).
> 
> If you'd create a patch that would migrate the subsystem to that and
> do also do this split, I could take that patch as an independent
> contribution and you would have a better baseline to develop on top
> of. What do you think?

We need two steps here. One to let tpmm_chip_alloc call two functions 
tpm_chip_alloc + tpmm_chip_dev (better name?), which are both going to be 
public and called by tpm-vtpm.c, and provide tpm_chip_free. Let me do 
that. Then we can get rid of the bitmap for the chip->dev_num 
independently and use idr there.


   Stefan
> 
> /Jarkko
>
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Stefan Berger Feb. 11, 2016, 4:51 p.m. UTC | #19
Stefan Berger/Watson/IBM@IBMUS wrote on 02/11/2016 10:24:18 AM:


> > If you'd create a patch that would migrate the subsystem to that and
> > do also do this split, I could take that patch as an independent
> > contribution and you would have a better baseline to develop on top
> > of. What do you think?
> 
> We need two steps here. One to let tpmm_chip_alloc call two 
> functions tpm_chip_alloc + tpmm_chip_dev (better name?), which are 
> both going to be public and called by tpm-vtpm.c, and provide 
> tpm_chip_free. Let me do that. Then we can get rid of the bitmap for
> the chip->dev_num independently and use idr there.
> 

Updated my branch now and based the driver on this function.

https://github.com/stefanberger/linux/commits/vtpm-driver.v3

No current driver needs this additional patch I think. So it can wait. I 
fixed a locking problem on the way, though...

    Stefan
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Jason Gunthorpe Feb. 11, 2016, 6:12 p.m. UTC | #20
On Thu, Feb 11, 2016 at 10:24:18AM -0500, Stefan Berger wrote:
>    We need two steps here. One to let tpmm_chip_alloc call two functions
>    tpm_chip_alloc + tpmm_chip_dev (better name?), which are both going to
>    be public and called by tpm-vtpm.c, and provide tpm_chip_free. Let me
>    do that. Then we can get rid of the bitmap for the chip->dev_num
>    independently and use idr there.

Make sense. Don't change the names all the drivers would have to be
churn'd. tpm_chip_alloc, tpmm_chip_alloc.

It is probably OK just to use put_device(&chip->dev), tpm_chip_put is
already taken for something else. Don't call it free, it isn't free.

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Stefan Berger Feb. 11, 2016, 7:10 p.m. UTC | #21
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote on 02/11/2016 
01:12:08 PM:


> 
> On Thu, Feb 11, 2016 at 10:24:18AM -0500, Stefan Berger wrote:
> >    We need two steps here. One to let tpmm_chip_alloc call two 
functions
> >    tpm_chip_alloc + tpmm_chip_dev (better name?), which are both going 
to
> >    be public and called by tpm-vtpm.c, and provide tpm_chip_free. Let 
me
> >    do that. Then we can get rid of the bitmap for the chip->dev_num
> >    independently and use idr there.
> 
> Make sense. Don't change the names all the drivers would have to be
> churn'd. tpm_chip_alloc, tpmm_chip_alloc.
> 

That's right:

struct tpm_chip *tpmm_chip_alloc(struct device *dev,
                                 const struct tpm_class_ops *ops)
{
        struct tpm_chip *chip;

        chip = tpm_chip_alloc(ops);
        if (IS_ERR(chip))
                return chip;

        tpmm_chip_dev(chip, dev);

        return chip;
}
EXPORT_SYMBOL_GPL(tpmm_chip_alloc);


> It is probably OK just to use put_device(&chip->dev), tpm_chip_put is
> already taken for something else. Don't call it free, it isn't free.

tpm_chip_free undoes what tpm_chip_alloc did.

/**
 * tpm_chip_alloc() - allocate a new struct tpm_chip instance
 * @dev: device to which the chip is associated
 * @ops: struct tpm_class_ops instance
 *
 * Allocates a new struct tpm_chip instance and assigns a free
 * device number for it.
 */
struct tpm_chip *tpm_chip_alloc(const struct tpm_class_ops *ops)
{
        struct tpm_chip *chip;

        chip = kzalloc(sizeof(*chip), GFP_KERNEL);
        if (chip == NULL)
                return ERR_PTR(-ENOMEM);

        mutex_init(&chip->tpm_mutex);
        INIT_LIST_HEAD(&chip->list);

        chip->ops = ops;

        spin_lock(&driver_lock);
        chip->dev_num = find_first_zero_bit(dev_mask, TPM_NUM_DEVICES);
        if (chip->dev_num < TPM_NUM_DEVICES)
                set_bit(chip->dev_num, dev_mask);
        spin_unlock(&driver_lock);

        if (chip->dev_num >= TPM_NUM_DEVICES) {
                pr_err("No available tpm device numbers\n");
                kfree(chip);
                return ERR_PTR(-ENOMEM);
        }

        scnprintf(chip->devname, sizeof(chip->devname), "tpm%d", 
chip->dev_num);

        return chip;
}
EXPORT_SYMBOL_GPL(tpm_chip_alloc);

/**
 * tpm_chip_free() - free tpm_chip previously allocated with 
tpm_chip_alloc
 * @chip: the tpm_chip to free
 */
void tpm_chip_free(struct tpm_chip *chip)
{
        spin_lock(&driver_lock);
        clear_bit(chip->dev_num, dev_mask);
        spin_unlock(&driver_lock);

        kfree(chip);
}
EXPORT_SYMBOL_GPL(tpm_chip_free);


Good?

   Stefan

> 
> Jason
>
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

Patch
diff mbox

diff --git a/drivers/char/tpm/tpm-vtpm.c b/drivers/char/tpm/tpm-vtpm.c
index 6ee1e44..d9fd797 100644
--- a/drivers/char/tpm/tpm-vtpm.c
+++ b/drivers/char/tpm/tpm-vtpm.c
@@ -41,6 +41,7 @@  struct vtpm_dev {
 
 	long state;
 #define STATE_OPENED_BIT   0
+#define STATE_INIT_VTPM    1
 
 	spinlock_t buf_lock;         /* lock for buffers */
 
@@ -52,6 +53,8 @@  struct vtpm_dev {
 	size_t resp_len;             /* length of queued TPM response */
 	u8 resp_buf[TPM_BUFSIZE];    /* response buffer */
 
+	struct work_struct work;     /* task that retrieves TPM timeouts */
+
 	struct list_head list;
 };
 
@@ -292,6 +295,9 @@  static int vtpm_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t count)
 
 	spin_unlock(&vtpm_dev->buf_lock);
 
+	/* sync for first startup command */
+	set_bit(STATE_INIT_VTPM, &vtpm_dev->state);
+
 	wake_up_interruptible(&vtpm_dev->wq);
 
 	return rc;
@@ -323,6 +329,52 @@  static const struct tpm_class_ops vtpm_tpm_ops = {
 };
 
 /*
+ * Code related to the startup of the TPM 2 and startup of TPM 1.2 +
+ * retrieval of timeouts and durations.
+ */
+
+static void vtpm_dev_work(struct work_struct *work)
+{
+	struct vtpm_dev *vtpm_dev = container_of(work, struct vtpm_dev, work);
+
+	if (vtpm_dev->flags & VTPM_FLAG_TPM2)
+		tpm2_startup(vtpm_dev->chip, TPM2_SU_CLEAR);
+	else {
+		/* we must send TPM_Startup() first */
+		tpm_startup(vtpm_dev->chip, TPM_ST_CLEAR);
+		tpm_get_timeouts(vtpm_dev->chip);
+	}
+}
+
+/*
+ * vtpm_dev_start_work: Schedule the work for TPM 1.2 & 2 initialization
+ */
+static int vtpm_dev_start_work(struct vtpm_dev *vtpm_dev)
+{
+	int sig;
+
+	INIT_WORK(&vtpm_dev->work, vtpm_dev_work);
+	schedule_work(&vtpm_dev->work);
+
+	/* make sure we send the 1st command before user space can */
+	sig = wait_event_interruptible(vtpm_dev->wq,
+		test_bit(STATE_INIT_VTPM, &vtpm_dev->state));
+	if (sig) {
+		cancel_work_sync(&vtpm_dev->work);
+		return -EINTR;
+	}
+	return 0;
+}
+
+/*
+ * vtpm_dev_stop_work: prevent the scheduled work from running
+ */
+static void vtpm_dev_stop_work(struct vtpm_dev *vtpm_dev)
+{
+	cancel_work_sync(&vtpm_dev->work);
+}
+
+/*
  * Code related to creation and deletion of device pairs
  */
 static void vtpm_dev_release(struct device *dev)
@@ -449,6 +501,10 @@  static struct file *vtpm_create_device_pair(
 	}
 	vtpm_dev->chip = chip;
 
+	rc = vtpm_dev_start_work(vtpm_dev);
+	if (rc)
+		goto err_vtpm_fput;
+
 	vtpm_new_pair->fd = fd;
 	vtpm_new_pair->major = MAJOR(vtpm_dev->chip->dev.devt);
 	vtpm_new_pair->minor = MINOR(vtpm_dev->chip->dev.devt);
@@ -476,6 +532,8 @@  err_delete_vtpm_dev:
  */
 static void vtpm_delete_device_pair(struct vtpm_dev *vtpm_dev)
 {
+	vtpm_dev_stop_work(vtpm_dev);
+
 	spin_lock(&driver_lock);
 	list_del_rcu(&vtpm_dev->list);
 	spin_unlock(&driver_lock);