[PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Andrey Pronin]

Resetting TPM while processing a command may lead to issues
on the next boot. Ensure that we don't have any ongoing
commands, and that no further commands can be sent to the chip
by unregistering the device in the shutdown handler.
tpm_chip_unregister() waits for the completion of an ongoing
command, if any, and then clears out chip->ops and unregisters
sysfs entities.

Signed-off-by: Andrey Pronin <apronin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 drivers/char/tpm/tpm_i2c_infineon.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
index 62ee44e57ddc..0c829fe26561 100644
--- a/drivers/char/tpm/tpm_i2c_infineon.c
+++ b/drivers/char/tpm/tpm_i2c_infineon.c
@@ -689,14 +689,18 @@ static int tpm_tis_i2c_probe(struct i2c_client *client,
 	return rc;
 }
 
-static int tpm_tis_i2c_remove(struct i2c_client *client)
+static void tpm_tis_i2c_shutdown(struct i2c_client *client)
 {
 	struct tpm_chip *chip = tpm_dev.chip;
 
 	tpm_chip_unregister(chip);
 	release_locality(chip, tpm_dev.locality, 1);
 	tpm_dev.client = NULL;
+}
 
+static int tpm_tis_i2c_remove(struct i2c_client *client)
+{
+	tpm_tis_i2c_shutdown(client);
 	return 0;
 }
 
@@ -704,6 +708,7 @@ static struct i2c_driver tpm_tis_i2c_driver = {
 	.id_table = tpm_tis_i2c_table,
 	.probe = tpm_tis_i2c_probe,
 	.remove = tpm_tis_i2c_remove,
+	.shutdown = tpm_tis_i2c_shutdown,
 	.driver = {
 		   .name = "tpm_i2c_infineon",
 		   .pm = &tpm_tis_i2c_ops,
-- 
2.11.0.483.g087da7b7c-goog


------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi

[PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Andrey Pronin]

Resetting TPM while processing a command may lead to issues
on the next boot. Ensure that we don't have any ongoing
commands, and that no further commands can be sent to the chip
by unregistering the device in the shutdown handler.
tpm_chip_unregister() waits for the completion of an ongoing
command, if any, and then clears out chip->ops and unregisters
sysfs entities.

Signed-off-by: Andrey Pronin <apronin@chromium.org>
---
 drivers/char/tpm/tpm_i2c_infineon.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
index 62ee44e57ddc..0c829fe26561 100644
--- a/drivers/char/tpm/tpm_i2c_infineon.c
+++ b/drivers/char/tpm/tpm_i2c_infineon.c
@@ -689,14 +689,18 @@ static int tpm_tis_i2c_probe(struct i2c_client *client,
 	return rc;
 }
 
-static int tpm_tis_i2c_remove(struct i2c_client *client)
+static void tpm_tis_i2c_shutdown(struct i2c_client *client)
 {
 	struct tpm_chip *chip = tpm_dev.chip;
 
 	tpm_chip_unregister(chip);
 	release_locality(chip, tpm_dev.locality, 1);
 	tpm_dev.client = NULL;
+}
 
+static int tpm_tis_i2c_remove(struct i2c_client *client)
+{
+	tpm_tis_i2c_shutdown(client);
 	return 0;
 }
 
@@ -704,6 +708,7 @@ static struct i2c_driver tpm_tis_i2c_driver = {
 	.id_table = tpm_tis_i2c_table,
 	.probe = tpm_tis_i2c_probe,
 	.remove = tpm_tis_i2c_remove,
+	.shutdown = tpm_tis_i2c_shutdown,
 	.driver = {
 		   .name = "tpm_i2c_infineon",
 		   .pm = &tpm_tis_i2c_ops,
-- 
2.11.0.483.g087da7b7c-goog

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Jason Gunthorpe]

On Fri, Jan 13, 2017 at 04:09:54PM -0800, Andrey Pronin wrote:
> Resetting TPM while processing a command may lead to issues
> on the next boot. Ensure that we don't have any ongoing
> commands, and that no further commands can be sent to the chip
> by unregistering the device in the shutdown handler.
> tpm_chip_unregister() waits for the completion of an ongoing
> command, if any, and then clears out chip->ops and unregisters
> sysfs entities.

Unregistering in a shutdown handler seems very strange, it also waits
for userspace things, so I wonder if it could be problematic?

Maybe just use

   down_write(&chip->ops_sem);
   chip->ops = NULL;
   up_write(&chip->ops_sem);

In the shutdown handler?

Jason

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Jason Gunthorpe]

On Fri, Jan 13, 2017 at 04:09:54PM -0800, Andrey Pronin wrote:
> Resetting TPM while processing a command may lead to issues
> on the next boot. Ensure that we don't have any ongoing
> commands, and that no further commands can be sent to the chip
> by unregistering the device in the shutdown handler.
> tpm_chip_unregister() waits for the completion of an ongoing
> command, if any, and then clears out chip->ops and unregisters
> sysfs entities.

Unregistering in a shutdown handler seems very strange, it also waits
for userspace things, so I wonder if it could be problematic?

Maybe just use

   down_write(&chip->ops_sem);
   chip->ops = NULL;
   up_write(&chip->ops_sem);

In the shutdown handler?

Jason

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Andrey Pronin]

On Fri, Jan 13, 2017 at 05:28:57PM -0700, Jason Gunthorpe wrote:
> On Fri, Jan 13, 2017 at 04:09:54PM -0800, Andrey Pronin wrote:
> > Resetting TPM while processing a command may lead to issues
> > on the next boot. Ensure that we don't have any ongoing
> > commands, and that no further commands can be sent to the chip
> > by unregistering the device in the shutdown handler.
> > tpm_chip_unregister() waits for the completion of an ongoing
> > command, if any, and then clears out chip->ops and unregisters
> > sysfs entities.
> 
> Unregistering in a shutdown handler seems very strange, it also waits
> for userspace things, so I wonder if it could be problematic?
> 
> Maybe just use
> 
>    down_write(&chip->ops_sem);
>    chip->ops = NULL;
>    up_write(&chip->ops_sem);
> 
> In the shutdown handler?

down_write(&chip->ops_sem) would still wait for completing the initiated
writes, since tpm_write() in tpm-dev.c calls tpm_try_get_ops().
Also, tpm-sysfs.c calls chip->ops directly, so sysfs should be
unregistered first.
And the last thing, this driver supports TPM 1.2, but if it was a 2.0
chip, it'd also need to send TPM2_Shutdown(CLEAR) from its shutdown
handler (or get an unorderly shutdown and DA counter increment).

All these things are handled by tpm_chip_unregister(). I thought about
creating a tpm_chip_shutdown routine that could be called from shutdown
handlers of the drivers that need it (and I'd do it for every driver,
especially in 2.0 case). But decided that it's better to reuse the
existing tpm_chip_unregister() that already does what's needed.

> 
> Jason

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Jarkko Sakkinen]

On Fri, Jan 13, 2017 at 04:42:30PM -0800, Andrey Pronin wrote:
> On Fri, Jan 13, 2017 at 05:28:57PM -0700, Jason Gunthorpe wrote:
> > On Fri, Jan 13, 2017 at 04:09:54PM -0800, Andrey Pronin wrote:
> > > Resetting TPM while processing a command may lead to issues
> > > on the next boot. Ensure that we don't have any ongoing
> > > commands, and that no further commands can be sent to the chip
> > > by unregistering the device in the shutdown handler.
> > > tpm_chip_unregister() waits for the completion of an ongoing
> > > command, if any, and then clears out chip->ops and unregisters
> > > sysfs entities.
> > 
> > Unregistering in a shutdown handler seems very strange, it also waits
> > for userspace things, so I wonder if it could be problematic?
> > 
> > Maybe just use
> > 
> >    down_write(&chip->ops_sem);
> >    chip->ops = NULL;
> >    up_write(&chip->ops_sem);
> > 
> > In the shutdown handler?
> 
> down_write(&chip->ops_sem) would still wait for completing the initiated
> writes, since tpm_write() in tpm-dev.c calls tpm_try_get_ops().
> Also, tpm-sysfs.c calls chip->ops directly, so sysfs should be
> unregistered first.

Why don't you fix the tpm-sysfs issue but rather misusing
tpm_chip_unregister?

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Jarkko Sakkinen]

On Mon, Jan 16, 2017 at 11:33:18AM +0200, Jarkko Sakkinen wrote:
> On Fri, Jan 13, 2017 at 04:42:30PM -0800, Andrey Pronin wrote:
> > On Fri, Jan 13, 2017 at 05:28:57PM -0700, Jason Gunthorpe wrote:
> > > On Fri, Jan 13, 2017 at 04:09:54PM -0800, Andrey Pronin wrote:
> > > > Resetting TPM while processing a command may lead to issues
> > > > on the next boot. Ensure that we don't have any ongoing
> > > > commands, and that no further commands can be sent to the chip
> > > > by unregistering the device in the shutdown handler.
> > > > tpm_chip_unregister() waits for the completion of an ongoing
> > > > command, if any, and then clears out chip->ops and unregisters
> > > > sysfs entities.
> > > 
> > > Unregistering in a shutdown handler seems very strange, it also waits
> > > for userspace things, so I wonder if it could be problematic?
> > > 
> > > Maybe just use
> > > 
> > >    down_write(&chip->ops_sem);
> > >    chip->ops = NULL;
> > >    up_write(&chip->ops_sem);
> > > 
> > > In the shutdown handler?
> > 
> > down_write(&chip->ops_sem) would still wait for completing the initiated
> > writes, since tpm_write() in tpm-dev.c calls tpm_try_get_ops().
> > Also, tpm-sysfs.c calls chip->ops directly, so sysfs should be
> > unregistered first.
> 
> Why don't you fix the tpm-sysfs issue but rather misusing
> tpm_chip_unregister?

Ignore this. I wasn't following this thread properly.

Only now had time read carefully through the discussion.

/Jarkko

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Jason Gunthorpe]

On Fri, Jan 13, 2017 at 04:42:30PM -0800, Andrey Pronin wrote:
> On Fri, Jan 13, 2017 at 05:28:57PM -0700, Jason Gunthorpe wrote:
> > On Fri, Jan 13, 2017 at 04:09:54PM -0800, Andrey Pronin wrote:
> > > Resetting TPM while processing a command may lead to issues
> > > on the next boot. Ensure that we don't have any ongoing
> > > commands, and that no further commands can be sent to the chip
> > > by unregistering the device in the shutdown handler.
> > > tpm_chip_unregister() waits for the completion of an ongoing
> > > command, if any, and then clears out chip->ops and unregisters
> > > sysfs entities.
> > 
> > Unregistering in a shutdown handler seems very strange, it also waits
> > for userspace things, so I wonder if it could be problematic?
> > 
> > Maybe just use
> > 
> >    down_write(&chip->ops_sem);
> >    chip->ops = NULL;
> >    up_write(&chip->ops_sem);
> > 
> > In the shutdown handler?
> 
> down_write(&chip->ops_sem) would still wait for completing the initiated
> writes, since tpm_write() in tpm-dev.c calls tpm_try_get_ops().

Yes, but that is a timeout limited wait. unregister waits for sysfs
files to be closed which is potentially unbounded.

> Yes, but it doesn't wait for sysfs
> Also, tpm-sysfs.c calls chip->ops directly, so sysfs should be
> unregistered first.

Yes, sorry, I should have mentioned that.. Maybe that is too much to
fix..

> And the last thing, this driver supports TPM 1.2, but if it was a 2.0
> chip, it'd also need to send TPM2_Shutdown(CLEAR) from its shutdown
> handler (or get an unorderly shutdown and DA counter increment).

I'm confused - doesn't your system reset the TPM when it reboots?
Isn't that required so the firmware starts with known PCRs? Doesn't
reset trump unorderly shutdown?

In any event that seems like an all-chips problem not a chip specific
bug fix?

> All these things are handled by tpm_chip_unregister(). I thought about
> creating a tpm_chip_shutdown routine that could be called from shutdown
> handlers of the drivers that need it (and I'd do it for every driver,
> especially in 2.0 case). But decided that it's better to reuse the
> existing tpm_chip_unregister() that already does what's needed.

If for some reason we need this for every driver then this is probably
a better approach - but that seems very, very strange to me.

Jason

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Andrey Pronin]

On Mon, Jan 16, 2017 at 09:19:19AM -0700, Jason Gunthorpe wrote:
> On Fri, Jan 13, 2017 at 04:42:30PM -0800, Andrey Pronin wrote:
> > On Fri, Jan 13, 2017 at 05:28:57PM -0700, Jason Gunthorpe wrote:
> > > On Fri, Jan 13, 2017 at 04:09:54PM -0800, Andrey Pronin wrote:
> > > > Resetting TPM while processing a command may lead to issues
> > > > on the next boot. Ensure that we don't have any ongoing
> > > > commands, and that no further commands can be sent to the chip
> > > > by unregistering the device in the shutdown handler.
> > > > tpm_chip_unregister() waits for the completion of an ongoing
> > > > command, if any, and then clears out chip->ops and unregisters
> > > > sysfs entities.
> > > 
> > > Unregistering in a shutdown handler seems very strange, it also waits
> > > for userspace things, so I wonder if it could be problematic?
> > > 
> > > Maybe just use
> > > 
> > >    down_write(&chip->ops_sem);
> > >    chip->ops = NULL;
> > >    up_write(&chip->ops_sem);
> > > 
> > > In the shutdown handler?
> > 
> > down_write(&chip->ops_sem) would still wait for completing the initiated
> > writes, since tpm_write() in tpm-dev.c calls tpm_try_get_ops().
> 
> Yes, but that is a timeout limited wait. unregister waits for sysfs
> files to be closed which is potentially unbounded.
> 
> > Yes, but it doesn't wait for sysfs
> > Also, tpm-sysfs.c calls chip->ops directly, so sysfs should be
> > unregistered first.
> 
> Yes, sorry, I should have mentioned that.. Maybe that is too much to
> fix..
> 

If we fix sysfs to go through tpm_try_get_ops, then all we can do for
shutdown is indeed something like

	down_write(&chip->ops_sem);
	if (chip->ops && chip->flags & TPM_CHIP_FLAG_TPM2)
		tpm2_shutdown(chip, TPM2_SU_CLEAR);
	chip->ops = NULL;
	up_write(&chip->ops_sem);

Does that sound like a good plan?
If we don't want sysfs to increment/decrement the reference count for
the device, we can still make it go through

	down_write(&chip->ops_sem);
	if (chip->ops) {
		...
	}
	up_write(&chip->ops_sem);


> > And the last thing, this driver supports TPM 1.2, but if it was a 2.0
> > chip, it'd also need to send TPM2_Shutdown(CLEAR) from its shutdown
> > handler (or get an unorderly shutdown and DA counter increment).
> 
> I'm confused - doesn't your system reset the TPM when it reboots?
> Isn't that required so the firmware starts with known PCRs? Doesn't
> reset trump unorderly shutdown?
> 

That's right, the TPM is reset when the system reboots. However, for
TPM 2.0, if it resets w/o Shutdown(CLEAR) first, it will detect it
during Startup, and mark as unorderly shutdown. Shutdown(CLEAR) is
the signal to the TPM to save its state to nvram and prepare to reset.
If it was not done, it is possible that something was not saved (e.g.
the DA counter), and the chip correctly marks it as a potential DA
problem.

> In any event that seems like an all-chips problem not a chip specific
> bug fix?
>
The part about TPM 2.0 Shutdown(CLEAR) above is an all-chip (actually,
all-2.0-chip) problem. The part where we prevent TPM from being reset
in the middle of a command (potentially) may or may not affect other
chips - I simply have no knowledge if it leads to issues anywhere else.

> 
> > All these things are handled by tpm_chip_unregister(). I thought about
> > creating a tpm_chip_shutdown routine that could be called from shutdown
> > handlers of the drivers that need it (and I'd do it for every driver,
> > especially in 2.0 case). But decided that it's better to reuse the
> > existing tpm_chip_unregister() that already does what's needed.
> 
> If for some reason we need this for every driver then this is probably
> a better approach - but that seems very, very strange to me.

As described above, we can do a smaller tpm_chip_shutdown() that the
drivers that need it (2.0 or susceptible to issues if reset in the
middle of command) can call.
I'll do it, if it sounds like the right plan to you.

Andrey

> 
> Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Andrey Pronin]

On Mon, Jan 16, 2017 at 09:19:19AM -0700, Jason Gunthorpe wrote:
> On Fri, Jan 13, 2017 at 04:42:30PM -0800, Andrey Pronin wrote:
> > On Fri, Jan 13, 2017 at 05:28:57PM -0700, Jason Gunthorpe wrote:
> > > On Fri, Jan 13, 2017 at 04:09:54PM -0800, Andrey Pronin wrote:
> > > > Resetting TPM while processing a command may lead to issues
> > > > on the next boot. Ensure that we don't have any ongoing
> > > > commands, and that no further commands can be sent to the chip
> > > > by unregistering the device in the shutdown handler.
> > > > tpm_chip_unregister() waits for the completion of an ongoing
> > > > command, if any, and then clears out chip->ops and unregisters
> > > > sysfs entities.
> > > 
> > > Unregistering in a shutdown handler seems very strange, it also waits
> > > for userspace things, so I wonder if it could be problematic?
> > > 
> > > Maybe just use
> > > 
> > >    down_write(&chip->ops_sem);
> > >    chip->ops = NULL;
> > >    up_write(&chip->ops_sem);
> > > 
> > > In the shutdown handler?
> > 
> > down_write(&chip->ops_sem) would still wait for completing the initiated
> > writes, since tpm_write() in tpm-dev.c calls tpm_try_get_ops().
> 
> Yes, but that is a timeout limited wait. unregister waits for sysfs
> files to be closed which is potentially unbounded.
> 
> > Yes, but it doesn't wait for sysfs
> > Also, tpm-sysfs.c calls chip->ops directly, so sysfs should be
> > unregistered first.
> 
> Yes, sorry, I should have mentioned that.. Maybe that is too much to
> fix..
> 

If we fix sysfs to go through tpm_try_get_ops, then all we can do for
shutdown is indeed something like

	down_write(&chip->ops_sem);
	if (chip->ops && chip->flags & TPM_CHIP_FLAG_TPM2)
		tpm2_shutdown(chip, TPM2_SU_CLEAR);
	chip->ops = NULL;
	up_write(&chip->ops_sem);

Does that sound like a good plan?
If we don't want sysfs to increment/decrement the reference count for
the device, we can still make it go through

	down_write(&chip->ops_sem);
	if (chip->ops) {
		...
	}
	up_write(&chip->ops_sem);


> > And the last thing, this driver supports TPM 1.2, but if it was a 2.0
> > chip, it'd also need to send TPM2_Shutdown(CLEAR) from its shutdown
> > handler (or get an unorderly shutdown and DA counter increment).
> 
> I'm confused - doesn't your system reset the TPM when it reboots?
> Isn't that required so the firmware starts with known PCRs? Doesn't
> reset trump unorderly shutdown?
> 

That's right, the TPM is reset when the system reboots. However, for
TPM 2.0, if it resets w/o Shutdown(CLEAR) first, it will detect it
during Startup, and mark as unorderly shutdown. Shutdown(CLEAR) is
the signal to the TPM to save its state to nvram and prepare to reset.
If it was not done, it is possible that something was not saved (e.g.
the DA counter), and the chip correctly marks it as a potential DA
problem.

> In any event that seems like an all-chips problem not a chip specific
> bug fix?
>
The part about TPM 2.0 Shutdown(CLEAR) above is an all-chip (actually,
all-2.0-chip) problem. The part where we prevent TPM from being reset
in the middle of a command (potentially) may or may not affect other
chips - I simply have no knowledge if it leads to issues anywhere else.

> 
> > All these things are handled by tpm_chip_unregister(). I thought about
> > creating a tpm_chip_shutdown routine that could be called from shutdown
> > handlers of the drivers that need it (and I'd do it for every driver,
> > especially in 2.0 case). But decided that it's better to reuse the
> > existing tpm_chip_unregister() that already does what's needed.
> 
> If for some reason we need this for every driver then this is probably
> a better approach - but that seems very, very strange to me.

As described above, we can do a smaller tpm_chip_shutdown() that the
drivers that need it (2.0 or susceptible to issues if reset in the
middle of command) can call.
I'll do it, if it sounds like the right plan to you.

Andrey

> 
> Jason

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Jason Gunthorpe]

On Tue, Jan 17, 2017 at 09:58:27AM -0800, Andrey Pronin wrote:
> > Yes, sorry, I should have mentioned that.. Maybe that is too much to
> > fix..
> 
> If we fix sysfs to go through tpm_try_get_ops, then all we can do for
> shutdown is indeed something like

Maybe yes, I also had at one point a thought to push the read side of
the ops_sem all the way down to the transmit_cmd level... But that
complicates calling shutdown.

> 	down_write(&chip->ops_sem);
> 	if (chip->ops && chip->flags & TPM_CHIP_FLAG_TPM2)
> 		tpm2_shutdown(chip, TPM2_SU_CLEAR);
> 	chip->ops = NULL;
> 	up_write(&chip->ops_sem);
> 
> Does that sound like a good plan?
> If we don't want sysfs to increment/decrement the reference count for
> the device, we can still make it go through

Grabbing the extra kref is harmless..

> > I'm confused - doesn't your system reset the TPM when it reboots?
> > Isn't that required so the firmware starts with known PCRs? Doesn't
> > reset trump unorderly shutdown?
> > 
> 
> That's right, the TPM is reset when the system reboots. However, for
> TPM 2.0, if it resets w/o Shutdown(CLEAR) first, it will detect it
> during Startup, and mark as unorderly shutdown. Shutdown(CLEAR) is
> the signal to the TPM to save its state to nvram and prepare to reset.
> If it was not done, it is possible that something was not saved (e.g.
> the DA counter), and the chip correctly marks it as a potential DA
> problem.

Okay, that makes sense, and needs to go in a comment someplace!

> > > All these things are handled by tpm_chip_unregister(). I thought about
> > > creating a tpm_chip_shutdown routine that could be called from shutdown
> > > handlers of the drivers that need it (and I'd do it for every driver,
> > > especially in 2.0 case). But decided that it's better to reuse the
> > > existing tpm_chip_unregister() that already does what's needed.
> > 
> > If for some reason we need this for every driver then this is probably
> > a better approach - but that seems very, very strange to me.
> 
> As described above, we can do a smaller tpm_chip_shutdown() that the
> drivers that need it (2.0 or susceptible to issues if reset in the
> middle of command) can call.
> I'll do it, if it sounds like the right plan to you.

Yes please..

Is there some way we can have the TPM core do this without requiring
the driver to add a shutdown the struct driver?

Maybe we could put something in chip->dev->driver? Not sure..

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Jason Gunthorpe]

On Tue, Jan 17, 2017 at 09:58:27AM -0800, Andrey Pronin wrote:
> > Yes, sorry, I should have mentioned that.. Maybe that is too much to
> > fix..
> 
> If we fix sysfs to go through tpm_try_get_ops, then all we can do for
> shutdown is indeed something like

Maybe yes, I also had at one point a thought to push the read side of
the ops_sem all the way down to the transmit_cmd level... But that
complicates calling shutdown.

> 	down_write(&chip->ops_sem);
> 	if (chip->ops && chip->flags & TPM_CHIP_FLAG_TPM2)
> 		tpm2_shutdown(chip, TPM2_SU_CLEAR);
> 	chip->ops = NULL;
> 	up_write(&chip->ops_sem);
> 
> Does that sound like a good plan?
> If we don't want sysfs to increment/decrement the reference count for
> the device, we can still make it go through

Grabbing the extra kref is harmless..

> > I'm confused - doesn't your system reset the TPM when it reboots?
> > Isn't that required so the firmware starts with known PCRs? Doesn't
> > reset trump unorderly shutdown?
> > 
> 
> That's right, the TPM is reset when the system reboots. However, for
> TPM 2.0, if it resets w/o Shutdown(CLEAR) first, it will detect it
> during Startup, and mark as unorderly shutdown. Shutdown(CLEAR) is
> the signal to the TPM to save its state to nvram and prepare to reset.
> If it was not done, it is possible that something was not saved (e.g.
> the DA counter), and the chip correctly marks it as a potential DA
> problem.

Okay, that makes sense, and needs to go in a comment someplace!

> > > All these things are handled by tpm_chip_unregister(). I thought about
> > > creating a tpm_chip_shutdown routine that could be called from shutdown
> > > handlers of the drivers that need it (and I'd do it for every driver,
> > > especially in 2.0 case). But decided that it's better to reuse the
> > > existing tpm_chip_unregister() that already does what's needed.
> > 
> > If for some reason we need this for every driver then this is probably
> > a better approach - but that seems very, very strange to me.
> 
> As described above, we can do a smaller tpm_chip_shutdown() that the
> drivers that need it (2.0 or susceptible to issues if reset in the
> middle of command) can call.
> I'll do it, if it sounds like the right plan to you.

Yes please..

Is there some way we can have the TPM core do this without requiring
the driver to add a shutdown the struct driver?

Maybe we could put something in chip->dev->driver? Not sure..

Jason

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Andrey Pronin]

On Tue, Jan 17, 2017 at 12:27:28PM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 17, 2017 at 09:58:27AM -0800, Andrey Pronin wrote:
> > > Yes, sorry, I should have mentioned that.. Maybe that is too much to
> > > fix..
> > 
> > If we fix sysfs to go through tpm_try_get_ops, then all we can do for
> > shutdown is indeed something like
> 
> Maybe yes, I also had at one point a thought to push the read side of
> the ops_sem all the way down to the transmit_cmd level... But that
> complicates calling shutdown.
> 
> > 	down_write(&chip->ops_sem);
> > 	if (chip->ops && chip->flags & TPM_CHIP_FLAG_TPM2)
> > 		tpm2_shutdown(chip, TPM2_SU_CLEAR);
> > 	chip->ops = NULL;
> > 	up_write(&chip->ops_sem);
> > 
> > Does that sound like a good plan?
> > If we don't want sysfs to increment/decrement the reference count for
> > the device, we can still make it go through
> 
> Grabbing the extra kref is harmless..
> 
> > > I'm confused - doesn't your system reset the TPM when it reboots?
> > > Isn't that required so the firmware starts with known PCRs? Doesn't
> > > reset trump unorderly shutdown?
> > > 
> > 
> > That's right, the TPM is reset when the system reboots. However, for
> > TPM 2.0, if it resets w/o Shutdown(CLEAR) first, it will detect it
> > during Startup, and mark as unorderly shutdown. Shutdown(CLEAR) is
> > the signal to the TPM to save its state to nvram and prepare to reset.
> > If it was not done, it is possible that something was not saved (e.g.
> > the DA counter), and the chip correctly marks it as a potential DA
> > problem.
> 
> Okay, that makes sense, and needs to go in a comment someplace!
> 
> > > > All these things are handled by tpm_chip_unregister(). I thought about
> > > > creating a tpm_chip_shutdown routine that could be called from shutdown
> > > > handlers of the drivers that need it (and I'd do it for every driver,
> > > > especially in 2.0 case). But decided that it's better to reuse the
> > > > existing tpm_chip_unregister() that already does what's needed.
> > > 
> > > If for some reason we need this for every driver then this is probably
> > > a better approach - but that seems very, very strange to me.
> > 
> > As described above, we can do a smaller tpm_chip_shutdown() that the
> > drivers that need it (2.0 or susceptible to issues if reset in the
> > middle of command) can call.
> > I'll do it, if it sounds like the right plan to you.
> 
> Yes please..
> 
> Is there some way we can have the TPM core do this without requiring
> the driver to add a shutdown the struct driver?
> 
> Maybe we could put something in chip->dev->driver? Not sure..

I can play more with it. We can check in tpm_chip_register() if
chip->dev->driver->shutdown is NULL, and, if so, set it to a default
handler. Or, do register_reboot_notifier() instead, to avoid messing
with struct device_driver from tpm-chip.c. Not sure if that's a
consideration at alli - any reason not to mess with those structures?

In any case, driver->shutdown or register_reboot_notifier, if we
still export that same common tpm_shutdown for those drivers that
want to do their custom shutdown handlers and register them through
module_driver(), we should be ok.

Whatever we do, we should allow the drivers to still send
(vendor-specific) commands from their shutdown handlers.

At some point, we actually used to have a register_reboot_notifier()
in the common tpm-chip.c code to make sure that it is done during
shutdown. But it is called before .shutdown, so a driver can't do
device-specific things with the device (or it can, but through
re-implementing the common transfer routines). That's why I
switched to a solution where a driver calls this common handler
itself, when it is ready for it. Similarly to what's done for
tpm_pm_suspend/resume().

But, yes, setting a default handler through chip->dev->driver
might just be good enough.

> 
> Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Andrey Pronin]

On Tue, Jan 17, 2017 at 12:27:28PM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 17, 2017 at 09:58:27AM -0800, Andrey Pronin wrote:
> > > Yes, sorry, I should have mentioned that.. Maybe that is too much to
> > > fix..
> > 
> > If we fix sysfs to go through tpm_try_get_ops, then all we can do for
> > shutdown is indeed something like
> 
> Maybe yes, I also had at one point a thought to push the read side of
> the ops_sem all the way down to the transmit_cmd level... But that
> complicates calling shutdown.
> 
> > 	down_write(&chip->ops_sem);
> > 	if (chip->ops && chip->flags & TPM_CHIP_FLAG_TPM2)
> > 		tpm2_shutdown(chip, TPM2_SU_CLEAR);
> > 	chip->ops = NULL;
> > 	up_write(&chip->ops_sem);
> > 
> > Does that sound like a good plan?
> > If we don't want sysfs to increment/decrement the reference count for
> > the device, we can still make it go through
> 
> Grabbing the extra kref is harmless..
> 
> > > I'm confused - doesn't your system reset the TPM when it reboots?
> > > Isn't that required so the firmware starts with known PCRs? Doesn't
> > > reset trump unorderly shutdown?
> > > 
> > 
> > That's right, the TPM is reset when the system reboots. However, for
> > TPM 2.0, if it resets w/o Shutdown(CLEAR) first, it will detect it
> > during Startup, and mark as unorderly shutdown. Shutdown(CLEAR) is
> > the signal to the TPM to save its state to nvram and prepare to reset.
> > If it was not done, it is possible that something was not saved (e.g.
> > the DA counter), and the chip correctly marks it as a potential DA
> > problem.
> 
> Okay, that makes sense, and needs to go in a comment someplace!
> 
> > > > All these things are handled by tpm_chip_unregister(). I thought about
> > > > creating a tpm_chip_shutdown routine that could be called from shutdown
> > > > handlers of the drivers that need it (and I'd do it for every driver,
> > > > especially in 2.0 case). But decided that it's better to reuse the
> > > > existing tpm_chip_unregister() that already does what's needed.
> > > 
> > > If for some reason we need this for every driver then this is probably
> > > a better approach - but that seems very, very strange to me.
> > 
> > As described above, we can do a smaller tpm_chip_shutdown() that the
> > drivers that need it (2.0 or susceptible to issues if reset in the
> > middle of command) can call.
> > I'll do it, if it sounds like the right plan to you.
> 
> Yes please..
> 
> Is there some way we can have the TPM core do this without requiring
> the driver to add a shutdown the struct driver?
> 
> Maybe we could put something in chip->dev->driver? Not sure..

I can play more with it. We can check in tpm_chip_register() if
chip->dev->driver->shutdown is NULL, and, if so, set it to a default
handler. Or, do register_reboot_notifier() instead, to avoid messing
with struct device_driver from tpm-chip.c. Not sure if that's a
consideration at alli - any reason not to mess with those structures?

In any case, driver->shutdown or register_reboot_notifier, if we
still export that same common tpm_shutdown for those drivers that
want to do their custom shutdown handlers and register them through
module_driver(), we should be ok.

Whatever we do, we should allow the drivers to still send
(vendor-specific) commands from their shutdown handlers.

At some point, we actually used to have a register_reboot_notifier()
in the common tpm-chip.c code to make sure that it is done during
shutdown. But it is called before .shutdown, so a driver can't do
device-specific things with the device (or it can, but through
re-implementing the common transfer routines). That's why I
switched to a solution where a driver calls this common handler
itself, when it is ready for it. Similarly to what's done for
tpm_pm_suspend/resume().

But, yes, setting a default handler through chip->dev->driver
might just be good enough.

> 
> Jason

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Jason Gunthorpe]

On Tue, Jan 17, 2017 at 12:13:36PM -0800, Andrey Pronin wrote:
> > Is there some way we can have the TPM core do this without requiring
> > the driver to add a shutdown the struct driver?
> > 
> > Maybe we could put something in chip->dev->driver? Not sure..
> 
> I can play more with it. We can check in tpm_chip_register() if
> chip->dev->driver->shutdown is NULL, and, if so, set it to a default
> handler. Or, do register_reboot_notifier() instead, to avoid messing
> with struct device_driver from tpm-chip.c. Not sure if that's a
> consideration at alli - any reason not to mess with those structures?

I think ordering is important here, the TPM core has to do any
shutdown before the driver shutdown method. That restriction might
entirely preclude using a reboot_notifier.

> Whatever we do, we should allow the drivers to still send
> (vendor-specific) commands from their shutdown handlers.

A vendor specific command should be done via a new core TPM
mechanism. I really want to keep access drivers (eg i2c, lpc, spi,
etc) out of the buisness of *assuming* they are connected to any
specific chip.

So, the core should detect chip XYZ and then issue the required
vendor-specific command in some way.

The driver shutdown would be used to close the access interface in
some way.

> But, yes, setting a default handler through chip->dev->driver
> might just be good enough.

Probably the *best* thing would be to add shutdown to 'struct class'
in the driver core like suspend/resume?

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Jason Gunthorpe]

On Tue, Jan 17, 2017 at 12:13:36PM -0800, Andrey Pronin wrote:
> > Is there some way we can have the TPM core do this without requiring
> > the driver to add a shutdown the struct driver?
> > 
> > Maybe we could put something in chip->dev->driver? Not sure..
> 
> I can play more with it. We can check in tpm_chip_register() if
> chip->dev->driver->shutdown is NULL, and, if so, set it to a default
> handler. Or, do register_reboot_notifier() instead, to avoid messing
> with struct device_driver from tpm-chip.c. Not sure if that's a
> consideration at alli - any reason not to mess with those structures?

I think ordering is important here, the TPM core has to do any
shutdown before the driver shutdown method. That restriction might
entirely preclude using a reboot_notifier.

> Whatever we do, we should allow the drivers to still send
> (vendor-specific) commands from their shutdown handlers.

A vendor specific command should be done via a new core TPM
mechanism. I really want to keep access drivers (eg i2c, lpc, spi,
etc) out of the buisness of *assuming* they are connected to any
specific chip.

So, the core should detect chip XYZ and then issue the required
vendor-specific command in some way.

The driver shutdown would be used to close the access interface in
some way.

> But, yes, setting a default handler through chip->dev->driver
> might just be good enough.

Probably the *best* thing would be to add shutdown to 'struct class'
in the driver core like suspend/resume?

Jason

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Andrey Pronin]

On Tue, Jan 17, 2017 at 01:59:33PM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 17, 2017 at 12:13:36PM -0800, Andrey Pronin wrote:
> > > Is there some way we can have the TPM core do this without requiring
> > > the driver to add a shutdown the struct driver?
> > > 
> > > Maybe we could put something in chip->dev->driver? Not sure..
> > 
> > I can play more with it. We can check in tpm_chip_register() if
> > chip->dev->driver->shutdown is NULL, and, if so, set it to a default
> > handler. Or, do register_reboot_notifier() instead, to avoid messing
> > with struct device_driver from tpm-chip.c. Not sure if that's a
> > consideration at alli - any reason not to mess with those structures?
> 
> I think ordering is important here, the TPM core has to do any
> shutdown before the driver shutdown method. That restriction might
> entirely preclude using a reboot_notifier.

Actually, reboot_notifier is called before the driver shutdown method,
so if TPM core registers it, it does its thing first.

However, if the driver wants to send vendor-specific commands (using
tpm_send to avoid re-implementing tpm_transmit_cmd on its side), it
actually wants to do it in the following order:
1) Send vendor specific commands.
2) Call common tpm_shutdown to send Shutdown(CLEAR) and prevent further
access to the device.
3) Close the low-level interface in the device-specific way, if/as
needed.
And for this sequence reboot_notifier gets in the way.

So, I'd still allow the driver to explicitly call tpm_shutdown from
its shutdown handler, or signal to tpm_tis_core that it wants the
default handler - either by leaving .shutdown = NULL, or, for example,
by setting a new flag in tpm_tis_data.

> 
> > Whatever we do, we should allow the drivers to still send
> > (vendor-specific) commands from their shutdown handlers.
> 
> A vendor specific command should be done via a new core TPM
> mechanism. I really want to keep access drivers (eg i2c, lpc, spi,
> etc) out of the buisness of *assuming* they are connected to any
> specific chip.
>

Here I was talking not about tpm_tis_spi or tpm_tis. Those can
continue relying on the core, or register the default handler using
.shutdown = tpm_shutdown.
I'm talking about the driver like tpm_i2c_infineon, which although
uses the core, is created for a specific family of chips. So, it
can assume that it needs to send vendor-specific commands.

> So, the core should detect chip XYZ and then issue the required
> vendor-specific command in some way.
> 

Do I get it right that you propose to create this new core tpm
mechanism for handling shutdowns? I didn't find anything that'd
allow to call vendor-specifc hooks from the core during shutdown,
but may be I'm missing something.

> The driver shutdown would be used to close the access interface in
> some way.
> 
> > But, yes, setting a default handler through chip->dev->driver
> > might just be good enough.
> 
> Probably the *best* thing would be to add shutdown to 'struct class'
> in the driver core like suspend/resume?
>

Yes, if that could be added to struct class, and then device_shutdown()
would call the class suspend, if the driver suspend is NULL, that'd
solve it. Then the core can register the default shutdown in class,
and an individual access driver can override it by registering its own
shutdown callback. Still, due to the ordering issues discussed above,
it should be either/or, not first-driver-then-class, if both exist.

So, we still need to export the common tpm_shutdown(). So, I'd suggest
to start with that: create and export the common tpm_shutdown() from
the core, that send Shutdown(CLEAR) for 2.0 and null-ifies chip->ops
(and fix sysfs to acquire chip->ops_sem) and let the interested drivers
call it. If we end up with having class shutdown, we can also register
the default handler through that mechanism. For now, we may or may not
register the default callback from core through register_reboot_notifier
iff chip->dev->driver->shutdown == NULL. That can be debated further.

WDYT?

Andrey

> 
> Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Andrey Pronin]

On Tue, Jan 17, 2017 at 01:59:33PM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 17, 2017 at 12:13:36PM -0800, Andrey Pronin wrote:
> > > Is there some way we can have the TPM core do this without requiring
> > > the driver to add a shutdown the struct driver?
> > > 
> > > Maybe we could put something in chip->dev->driver? Not sure..
> > 
> > I can play more with it. We can check in tpm_chip_register() if
> > chip->dev->driver->shutdown is NULL, and, if so, set it to a default
> > handler. Or, do register_reboot_notifier() instead, to avoid messing
> > with struct device_driver from tpm-chip.c. Not sure if that's a
> > consideration at alli - any reason not to mess with those structures?
> 
> I think ordering is important here, the TPM core has to do any
> shutdown before the driver shutdown method. That restriction might
> entirely preclude using a reboot_notifier.

Actually, reboot_notifier is called before the driver shutdown method,
so if TPM core registers it, it does its thing first.

However, if the driver wants to send vendor-specific commands (using
tpm_send to avoid re-implementing tpm_transmit_cmd on its side), it
actually wants to do it in the following order:
1) Send vendor specific commands.
2) Call common tpm_shutdown to send Shutdown(CLEAR) and prevent further
access to the device.
3) Close the low-level interface in the device-specific way, if/as
needed.
And for this sequence reboot_notifier gets in the way.

So, I'd still allow the driver to explicitly call tpm_shutdown from
its shutdown handler, or signal to tpm_tis_core that it wants the
default handler - either by leaving .shutdown = NULL, or, for example,
by setting a new flag in tpm_tis_data.

> 
> > Whatever we do, we should allow the drivers to still send
> > (vendor-specific) commands from their shutdown handlers.
> 
> A vendor specific command should be done via a new core TPM
> mechanism. I really want to keep access drivers (eg i2c, lpc, spi,
> etc) out of the buisness of *assuming* they are connected to any
> specific chip.
>

Here I was talking not about tpm_tis_spi or tpm_tis. Those can
continue relying on the core, or register the default handler using
.shutdown = tpm_shutdown.
I'm talking about the driver like tpm_i2c_infineon, which although
uses the core, is created for a specific family of chips. So, it
can assume that it needs to send vendor-specific commands.

> So, the core should detect chip XYZ and then issue the required
> vendor-specific command in some way.
> 

Do I get it right that you propose to create this new core tpm
mechanism for handling shutdowns? I didn't find anything that'd
allow to call vendor-specifc hooks from the core during shutdown,
but may be I'm missing something.

> The driver shutdown would be used to close the access interface in
> some way.
> 
> > But, yes, setting a default handler through chip->dev->driver
> > might just be good enough.
> 
> Probably the *best* thing would be to add shutdown to 'struct class'
> in the driver core like suspend/resume?
>

Yes, if that could be added to struct class, and then device_shutdown()
would call the class suspend, if the driver suspend is NULL, that'd
solve it. Then the core can register the default shutdown in class,
and an individual access driver can override it by registering its own
shutdown callback. Still, due to the ordering issues discussed above,
it should be either/or, not first-driver-then-class, if both exist.

So, we still need to export the common tpm_shutdown(). So, I'd suggest
to start with that: create and export the common tpm_shutdown() from
the core, that send Shutdown(CLEAR) for 2.0 and null-ifies chip->ops
(and fix sysfs to acquire chip->ops_sem) and let the interested drivers
call it. If we end up with having class shutdown, we can also register
the default handler through that mechanism. For now, we may or may not
register the default callback from core through register_reboot_notifier
iff chip->dev->driver->shutdown == NULL. That can be debated further.

WDYT?

Andrey

> 
> Jason

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Jason Gunthorpe]

On Tue, Jan 17, 2017 at 03:00:53PM -0800, Andrey Pronin wrote:

> Here I was talking not about tpm_tis_spi or tpm_tis. Those can
> continue relying on the core, or register the default handler using
> .shutdown = tpm_shutdown.  I'm talking about the driver like
> tpm_i2c_infineon, which although uses the core, is created for a
> specific family of chips. So, it can assume that it needs to send
> vendor-specific commands.

But this is exactly what I'm talking about, infineon chips come in
lots of interface types, and if their legacy i2c interface need a
vendor command then likely their chips that use a common interface do
as well.

Conflating interface and bus is something I have been ripping out over
the years..

> > So, the core should detect chip XYZ and then issue the required
> > vendor-specific command in some way.
> 
> Do I get it right that you propose to create this new core tpm
> mechanism for handling shutdowns? I didn't find anything that'd
> allow to call vendor-specifc hooks from the core during shutdown,
> but may be I'm missing something.

It would be simple to add to the core path:

if (chip->id == 1234)
    tpm_vendor_1234_shutdown(...);

We don't need to involve the driver in this. If it becomes a very
complex thing down then road then we may need *bus* and *chip*
drivers, but for now the 'chip' driver(s) are just inlined in the core
code..

But if there is no actual need to do this right now then don't worry
about overdesigning things..

> > Probably the *best* thing would be to add shutdown to 'struct class'
> > in the driver core like suspend/resume?
> 
> Yes, if that could be added to struct class, and then device_shutdown()
> would call the class suspend, if the driver suspend is NULL, that'd
> solve it.

Won't know if it is possible until someone sends a patch to Greg/etc :)

> Then the core can register the default shutdown in class, and an
> individual access driver can override it by registering its own
> shutdown callback. Still, due to the ordering issues discussed
> above, it should be either/or, not first-driver-then-class, if both
> exist.

First class then driver is the usual model, which is OK for TPM.

> So, we still need to export the common tpm_shutdown().

No need to export if no driver is calling it, like I said don't
overdesign here, it is trivial to change if someone needs to do
something different later.

> to start with that: create and export the common tpm_shutdown() from
> the core, that send Shutdown(CLEAR) for 2.0 and null-ifies chip->ops
> (and fix sysfs to acquire chip->ops_sem) and let the interested drivers
> call it.

I think you should do this and use the reboot_notifier or
class->shutdown approach

I'm not completely sure why you are worrying about sending a
vendor-specific command at shutdown. Do you actually need that?

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Jason Gunthorpe]

On Tue, Jan 17, 2017 at 03:00:53PM -0800, Andrey Pronin wrote:

> Here I was talking not about tpm_tis_spi or tpm_tis. Those can
> continue relying on the core, or register the default handler using
> .shutdown = tpm_shutdown.  I'm talking about the driver like
> tpm_i2c_infineon, which although uses the core, is created for a
> specific family of chips. So, it can assume that it needs to send
> vendor-specific commands.

But this is exactly what I'm talking about, infineon chips come in
lots of interface types, and if their legacy i2c interface need a
vendor command then likely their chips that use a common interface do
as well.

Conflating interface and bus is something I have been ripping out over
the years..

> > So, the core should detect chip XYZ and then issue the required
> > vendor-specific command in some way.
> 
> Do I get it right that you propose to create this new core tpm
> mechanism for handling shutdowns? I didn't find anything that'd
> allow to call vendor-specifc hooks from the core during shutdown,
> but may be I'm missing something.

It would be simple to add to the core path:

if (chip->id == 1234)
    tpm_vendor_1234_shutdown(...);

We don't need to involve the driver in this. If it becomes a very
complex thing down then road then we may need *bus* and *chip*
drivers, but for now the 'chip' driver(s) are just inlined in the core
code..

But if there is no actual need to do this right now then don't worry
about overdesigning things..

> > Probably the *best* thing would be to add shutdown to 'struct class'
> > in the driver core like suspend/resume?
> 
> Yes, if that could be added to struct class, and then device_shutdown()
> would call the class suspend, if the driver suspend is NULL, that'd
> solve it.

Won't know if it is possible until someone sends a patch to Greg/etc :)

> Then the core can register the default shutdown in class, and an
> individual access driver can override it by registering its own
> shutdown callback. Still, due to the ordering issues discussed
> above, it should be either/or, not first-driver-then-class, if both
> exist.

First class then driver is the usual model, which is OK for TPM.

> So, we still need to export the common tpm_shutdown().

No need to export if no driver is calling it, like I said don't
overdesign here, it is trivial to change if someone needs to do
something different later.

> to start with that: create and export the common tpm_shutdown() from
> the core, that send Shutdown(CLEAR) for 2.0 and null-ifies chip->ops
> (and fix sysfs to acquire chip->ops_sem) and let the interested drivers
> call it.

I think you should do this and use the reboot_notifier or
class->shutdown approach

I'm not completely sure why you are worrying about sending a
vendor-specific command at shutdown. Do you actually need that?

Jason

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Andrey Pronin]

On Tue, Jan 17, 2017 at 04:22:07PM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 17, 2017 at 03:00:53PM -0800, Andrey Pronin wrote:
> 
> > Here I was talking not about tpm_tis_spi or tpm_tis. Those can
> > continue relying on the core, or register the default handler using
> > .shutdown = tpm_shutdown.  I'm talking about the driver like
> > tpm_i2c_infineon, which although uses the core, is created for a
> > specific family of chips. So, it can assume that it needs to send
> > vendor-specific commands.
> 
> But this is exactly what I'm talking about, infineon chips come in
> lots of interface types, and if their legacy i2c interface need a
> vendor command then likely their chips that use a common interface do
> as well.
> 
> Conflating interface and bus is something I have been ripping out over
> the years..
> 

Thanks, Jason. OK, I'll try to follow that path then with my patches.

> > > So, the core should detect chip XYZ and then issue the required
> > > vendor-specific command in some way.
> > 
> > Do I get it right that you propose to create this new core tpm
> > mechanism for handling shutdowns? I didn't find anything that'd
> > allow to call vendor-specifc hooks from the core during shutdown,
> > but may be I'm missing something.
> 
> It would be simple to add to the core path:
> 
> if (chip->id == 1234)
>     tpm_vendor_1234_shutdown(...);
> 
> We don't need to involve the driver in this. If it becomes a very
> complex thing down then road then we may need *bus* and *chip*
> drivers, but for now the 'chip' driver(s) are just inlined in the core
> code..
> 
> But if there is no actual need to do this right now then don't worry
> about overdesigning things..

OK, I can live with chip->id specific logic in probe/shutdown, if that's
the current approach.

> 
> > > Probably the *best* thing would be to add shutdown to 'struct class'
> > > in the driver core like suspend/resume?
> > 
> > Yes, if that could be added to struct class, and then device_shutdown()
> > would call the class suspend, if the driver suspend is NULL, that'd
> > solve it.
> 
> Won't know if it is possible until someone sends a patch to Greg/etc :)
> 
> > Then the core can register the default shutdown in class, and an
> > individual access driver can override it by registering its own
> > shutdown callback. Still, due to the ordering issues discussed
> > above, it should be either/or, not first-driver-then-class, if both
> > exist.
> 
> First class then driver is the usual model, which is OK for TPM.

If "first class then driver", then the already existing
register_reboot_notifier() can play the role of the class handler,
since those hooks are called before drivers' shutdown handlers.

> 
> > So, we still need to export the common tpm_shutdown().
> 
> No need to export if no driver is calling it, like I said don't
> overdesign here, it is trivial to change if someone needs to do
> something different later.
> 

So, I started putting together an alternative patch (decided to go
with a new patch instead of a new version for this one, since it's
no longer limited to Infineon driver). The new patch is just going
to do the following
	down_write(&chip->ops_sem);
	if (chip->ops) {
		if (chip->flags & TPM_CHIP_FLAG_TPM2)
			tpm2_shutdown(chip, TPM2_SU_CLEAR);
		chip->ops = NULL;
	}
	up_write(&chip->ops_sem);
on shutdown in registered "reboot notifier".

I went through the places that access chip->ops to see what's
going on at the moment with protecting them with tpm_try_get_ops().
Here is the current state:

 - tpm_transmit/tpm_transmit_cmd. Not exported and are only called
   by the core after tpm_try_get_ops() except for one place in
   sysfs - pubek_show().

 - sysfs also directly accesses chip->ops in cancel_store(), but
   that routine doesn't seem to be used anywhere. Shall it be
   just removed?

 - tpm_get_timeouts. Besides the core is called by xen-tpmfront,
   but before tpm_chip_register(), so no harm possible as of now.

 - wait_for_tpm_stat. Besides the core is called by xen-tpmfront.
   It is called from inside chip->ops handlers only, which presumably
   can happen only when the ops_sem is hold (once sysfs is fixed).

 - st33zp24 has it's own wait_for_stat() function that goes directly
   to chip->ops. It happens inside chip->ops->recv/send (as for Xen),
   which is fine. And also inside its resume handler, which is fine
   as long as resume can never happen after shutdown (I believe it's
   true).

So, if I add going through tpm_try_get_ops() to pubek_show and
delete cancel_store(), that'll fix sysfs, and be enough for the things
to work for now.

But it looks a bit brittle. So, before I submit my next patch:
Do you think it's ok to leave wait_for_tpm_stat() and
tpm_get_timeouts() and just continue be mindful when using those
low-level functions? Or, shall we instead move acquiring ops_sem
and checking for ops == NULL inside those low-level functions:
tpm_transmit, wait_for_tpm_stat, tpm_get_timeout. It then should
probably be separated from get_device(), which can be kept inside
tpm_try_get_ops().

> > to start with that: create and export the common tpm_shutdown() from
> > the core, that send Shutdown(CLEAR) for 2.0 and null-ifies chip->ops
> > (and fix sysfs to acquire chip->ops_sem) and let the interested drivers
> > call it.
> 
> I think you should do this and use the reboot_notifier or
> class->shutdown approach
> 
> I'm not completely sure why you are worrying about sending a
> vendor-specific command at shutdown. Do you actually need that?
>

Yes, I do need that to send sleep-control vendor commands to the
device in my case during shutdown (as well as suspend and resume).
It makes much more sense to send them using tpm_transfer_cmd, which
relies on chip->ops, than reimplementing the same functionality in
the device driver.

Again, I can live with "if (chip->id == 1234)" logic in core to
achieve that for now, if that's the chosen course. (Or, just
register a device-specific "reboot notifier" with lower priority
to be called before the "class-level" shutdown logic.)

> 
> Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Andrey Pronin]

On Tue, Jan 17, 2017 at 04:22:07PM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 17, 2017 at 03:00:53PM -0800, Andrey Pronin wrote:
> 
> > Here I was talking not about tpm_tis_spi or tpm_tis. Those can
> > continue relying on the core, or register the default handler using
> > .shutdown = tpm_shutdown.  I'm talking about the driver like
> > tpm_i2c_infineon, which although uses the core, is created for a
> > specific family of chips. So, it can assume that it needs to send
> > vendor-specific commands.
> 
> But this is exactly what I'm talking about, infineon chips come in
> lots of interface types, and if their legacy i2c interface need a
> vendor command then likely their chips that use a common interface do
> as well.
> 
> Conflating interface and bus is something I have been ripping out over
> the years..
> 

Thanks, Jason. OK, I'll try to follow that path then with my patches.

> > > So, the core should detect chip XYZ and then issue the required
> > > vendor-specific command in some way.
> > 
> > Do I get it right that you propose to create this new core tpm
> > mechanism for handling shutdowns? I didn't find anything that'd
> > allow to call vendor-specifc hooks from the core during shutdown,
> > but may be I'm missing something.
> 
> It would be simple to add to the core path:
> 
> if (chip->id == 1234)
>     tpm_vendor_1234_shutdown(...);
> 
> We don't need to involve the driver in this. If it becomes a very
> complex thing down then road then we may need *bus* and *chip*
> drivers, but for now the 'chip' driver(s) are just inlined in the core
> code..
> 
> But if there is no actual need to do this right now then don't worry
> about overdesigning things..

OK, I can live with chip->id specific logic in probe/shutdown, if that's
the current approach.

> 
> > > Probably the *best* thing would be to add shutdown to 'struct class'
> > > in the driver core like suspend/resume?
> > 
> > Yes, if that could be added to struct class, and then device_shutdown()
> > would call the class suspend, if the driver suspend is NULL, that'd
> > solve it.
> 
> Won't know if it is possible until someone sends a patch to Greg/etc :)
> 
> > Then the core can register the default shutdown in class, and an
> > individual access driver can override it by registering its own
> > shutdown callback. Still, due to the ordering issues discussed
> > above, it should be either/or, not first-driver-then-class, if both
> > exist.
> 
> First class then driver is the usual model, which is OK for TPM.

If "first class then driver", then the already existing
register_reboot_notifier() can play the role of the class handler,
since those hooks are called before drivers' shutdown handlers.

> 
> > So, we still need to export the common tpm_shutdown().
> 
> No need to export if no driver is calling it, like I said don't
> overdesign here, it is trivial to change if someone needs to do
> something different later.
> 

So, I started putting together an alternative patch (decided to go
with a new patch instead of a new version for this one, since it's
no longer limited to Infineon driver). The new patch is just going
to do the following
	down_write(&chip->ops_sem);
	if (chip->ops) {
		if (chip->flags & TPM_CHIP_FLAG_TPM2)
			tpm2_shutdown(chip, TPM2_SU_CLEAR);
		chip->ops = NULL;
	}
	up_write(&chip->ops_sem);
on shutdown in registered "reboot notifier".

I went through the places that access chip->ops to see what's
going on at the moment with protecting them with tpm_try_get_ops().
Here is the current state:

 - tpm_transmit/tpm_transmit_cmd. Not exported and are only called
   by the core after tpm_try_get_ops() except for one place in
   sysfs - pubek_show().

 - sysfs also directly accesses chip->ops in cancel_store(), but
   that routine doesn't seem to be used anywhere. Shall it be
   just removed?

 - tpm_get_timeouts. Besides the core is called by xen-tpmfront,
   but before tpm_chip_register(), so no harm possible as of now.

 - wait_for_tpm_stat. Besides the core is called by xen-tpmfront.
   It is called from inside chip->ops handlers only, which presumably
   can happen only when the ops_sem is hold (once sysfs is fixed).

 - st33zp24 has it's own wait_for_stat() function that goes directly
   to chip->ops. It happens inside chip->ops->recv/send (as for Xen),
   which is fine. And also inside its resume handler, which is fine
   as long as resume can never happen after shutdown (I believe it's
   true).

So, if I add going through tpm_try_get_ops() to pubek_show and
delete cancel_store(), that'll fix sysfs, and be enough for the things
to work for now.

But it looks a bit brittle. So, before I submit my next patch:
Do you think it's ok to leave wait_for_tpm_stat() and
tpm_get_timeouts() and just continue be mindful when using those
low-level functions? Or, shall we instead move acquiring ops_sem
and checking for ops == NULL inside those low-level functions:
tpm_transmit, wait_for_tpm_stat, tpm_get_timeout. It then should
probably be separated from get_device(), which can be kept inside
tpm_try_get_ops().

> > to start with that: create and export the common tpm_shutdown() from
> > the core, that send Shutdown(CLEAR) for 2.0 and null-ifies chip->ops
> > (and fix sysfs to acquire chip->ops_sem) and let the interested drivers
> > call it.
> 
> I think you should do this and use the reboot_notifier or
> class->shutdown approach
> 
> I'm not completely sure why you are worrying about sending a
> vendor-specific command at shutdown. Do you actually need that?
>

Yes, I do need that to send sleep-control vendor commands to the
device in my case during shutdown (as well as suspend and resume).
It makes much more sense to send them using tpm_transfer_cmd, which
relies on chip->ops, than reimplementing the same functionality in
the device driver.

Again, I can live with "if (chip->id == 1234)" logic in core to
achieve that for now, if that's the chosen course. (Or, just
register a device-specific "reboot notifier" with lower priority
to be called before the "class-level" shutdown logic.)

> 
> Jason

Re: [tpmdd-devel] [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Andrey Pronin]

On Mon, Jan 23, 2017 at 12:02:46PM -0800, Andrey Pronin wrote:
> On Tue, Jan 17, 2017 at 04:22:07PM -0700, Jason Gunthorpe wrote:
> > On Tue, Jan 17, 2017 at 03:00:53PM -0800, Andrey Pronin wrote:
> > 
> > > Here I was talking not about tpm_tis_spi or tpm_tis. Those can
> > > continue relying on the core, or register the default handler using
> > > .shutdown = tpm_shutdown.  I'm talking about the driver like
> > > tpm_i2c_infineon, which although uses the core, is created for a
> > > specific family of chips. So, it can assume that it needs to send
> > > vendor-specific commands.
> > 
> > But this is exactly what I'm talking about, infineon chips come in
> > lots of interface types, and if their legacy i2c interface need a
> > vendor command then likely their chips that use a common interface do
> > as well.
> > 
> > Conflating interface and bus is something I have been ripping out over
> > the years..
> > 
> 
> Thanks, Jason. OK, I'll try to follow that path then with my patches.
> 
> > > > So, the core should detect chip XYZ and then issue the required
> > > > vendor-specific command in some way.
> > > 
> > > Do I get it right that you propose to create this new core tpm
> > > mechanism for handling shutdowns? I didn't find anything that'd
> > > allow to call vendor-specifc hooks from the core during shutdown,
> > > but may be I'm missing something.
> > 
> > It would be simple to add to the core path:
> > 
> > if (chip->id == 1234)
> >     tpm_vendor_1234_shutdown(...);
> > 
> > We don't need to involve the driver in this. If it becomes a very
> > complex thing down then road then we may need *bus* and *chip*
> > drivers, but for now the 'chip' driver(s) are just inlined in the core
> > code..
> > 
> > But if there is no actual need to do this right now then don't worry
> > about overdesigning things..
> 
> OK, I can live with chip->id specific logic in probe/shutdown, if that's
> the current approach.
> 
> > 
> > > > Probably the *best* thing would be to add shutdown to 'struct class'
> > > > in the driver core like suspend/resume?
> > > 
> > > Yes, if that could be added to struct class, and then device_shutdown()
> > > would call the class suspend, if the driver suspend is NULL, that'd
> > > solve it.
> > 
> > Won't know if it is possible until someone sends a patch to Greg/etc :)
> > 
> > > Then the core can register the default shutdown in class, and an
> > > individual access driver can override it by registering its own
> > > shutdown callback. Still, due to the ordering issues discussed
> > > above, it should be either/or, not first-driver-then-class, if both
> > > exist.
> > 
> > First class then driver is the usual model, which is OK for TPM.
> 
> If "first class then driver", then the already existing
> register_reboot_notifier() can play the role of the class handler,
> since those hooks are called before drivers' shutdown handlers.
> 
> > 
> > > So, we still need to export the common tpm_shutdown().
> > 
> > No need to export if no driver is calling it, like I said don't
> > overdesign here, it is trivial to change if someone needs to do
> > something different later.
> > 
> 
> So, I started putting together an alternative patch (decided to go
> with a new patch instead of a new version for this one, since it's
> no longer limited to Infineon driver). The new patch is just going
> to do the following
> 	down_write(&chip->ops_sem);
> 	if (chip->ops) {
> 		if (chip->flags & TPM_CHIP_FLAG_TPM2)
> 			tpm2_shutdown(chip, TPM2_SU_CLEAR);
> 		chip->ops = NULL;
> 	}
> 	up_write(&chip->ops_sem);
> on shutdown in registered "reboot notifier".
> 
> I went through the places that access chip->ops to see what's
> going on at the moment with protecting them with tpm_try_get_ops().
> Here is the current state:
> 
>  - tpm_transmit/tpm_transmit_cmd. Not exported and are only called
>    by the core after tpm_try_get_ops() except for one place in
>    sysfs - pubek_show().
> 
>  - sysfs also directly accesses chip->ops in cancel_store(), but
>    that routine doesn't seem to be used anywhere. Shall it be
>    just removed?

My bad, need more coffee. Of course, cancel_store() is used. So, should
fix that as well.

> 
>  - tpm_get_timeouts. Besides the core is called by xen-tpmfront,
>    but before tpm_chip_register(), so no harm possible as of now.
> 
>  - wait_for_tpm_stat. Besides the core is called by xen-tpmfront.
>    It is called from inside chip->ops handlers only, which presumably
>    can happen only when the ops_sem is hold (once sysfs is fixed).
> 
>  - st33zp24 has it's own wait_for_stat() function that goes directly
>    to chip->ops. It happens inside chip->ops->recv/send (as for Xen),
>    which is fine. And also inside its resume handler, which is fine
>    as long as resume can never happen after shutdown (I believe it's
>    true).
> 
> So, if I add going through tpm_try_get_ops() to pubek_show and
> delete cancel_store(), that'll fix sysfs, and be enough for the things
> to work for now.
> 
> But it looks a bit brittle. So, before I submit my next patch:
> Do you think it's ok to leave wait_for_tpm_stat() and
> tpm_get_timeouts() and just continue be mindful when using those
> low-level functions? Or, shall we instead move acquiring ops_sem
> and checking for ops == NULL inside those low-level functions:
> tpm_transmit, wait_for_tpm_stat, tpm_get_timeout. It then should
> probably be separated from get_device(), which can be kept inside
> tpm_try_get_ops().
> 
> > > to start with that: create and export the common tpm_shutdown() from
> > > the core, that send Shutdown(CLEAR) for 2.0 and null-ifies chip->ops
> > > (and fix sysfs to acquire chip->ops_sem) and let the interested drivers
> > > call it.
> > 
> > I think you should do this and use the reboot_notifier or
> > class->shutdown approach
> > 
> > I'm not completely sure why you are worrying about sending a
> > vendor-specific command at shutdown. Do you actually need that?
> >
> 
> Yes, I do need that to send sleep-control vendor commands to the
> device in my case during shutdown (as well as suspend and resume).
> It makes much more sense to send them using tpm_transfer_cmd, which
> relies on chip->ops, than reimplementing the same functionality in
> the device driver.
> 
> Again, I can live with "if (chip->id == 1234)" logic in core to
> achieve that for now, if that's the chosen course. (Or, just
> register a device-specific "reboot notifier" with lower priority
> to be called before the "class-level" shutdown logic.)
> 
> > 
> > Jason
> 
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Jason Gunthorpe]

On Mon, Jan 23, 2017 at 12:02:46PM -0800, Andrey Pronin wrote:

> > But if there is no actual need to do this right now then don't worry
> > about overdesigning things..
> 
> OK, I can live with chip->id specific logic in probe/shutdown, if that's
> the current approach.

It is where we are heading.. If it becomes prevelent we will need chip
and bus drivers.

> > First class then driver is the usual model, which is OK for TPM.
> 
> If "first class then driver", then the already existing
> register_reboot_notifier() can play the role of the class handler,
> since those hooks are called before drivers' shutdown handlers.

Okay.. But maybe fire off patch doing this via the device code, as if
that is accepted it is better than the reboot handler in terms of
guaranteeing ordering/etc.

> 	down_write(&chip->ops_sem);
> 	if (chip->ops) {
> 		if (chip->flags & TPM_CHIP_FLAG_TPM2)
> 			tpm2_shutdown(chip, TPM2_SU_CLEAR);
> 		chip->ops = NULL;
> 	}
> 	up_write(&chip->ops_sem);
> on shutdown in registered "reboot notifier".

Sure, seems like a good starting point.

> I went through the places that access chip->ops to see what's
> going on at the moment with protecting them with tpm_try_get_ops().
> Here is the current state:
> 
>  - tpm_transmit/tpm_transmit_cmd. Not exported and are only called
>    by the core after tpm_try_get_ops() except for one place in
>    sysfs - pubek_show().

Most of sysfs calls functions like tpm_pcr_read_dev, tpm_getcap, etc,
etc that boil down the tpm_transmit_cmd, so many more need it than you
listed.

> But it looks a bit brittle. So, before I submit my next patch:
> Do you think it's ok to leave wait_for_tpm_stat() and
> tpm_get_timeouts() and just continue be mindful when using those
> low-level functions?

For xenfront we should someday move it to use TPM_OPS_AUTO_STARTUP and
drop the tpm_get_timeouts.

I think it is fine to leave those two low level commands as is..

Like I said, it would be more robust to acquire a lock directly in
places like transmit_cmd, but I suspect that is too hard to retrofit
at this time...

> and checking for ops == NULL inside those low-level functions:
> tpm_transmit, wait_for_tpm_stat, tpm_get_timeout. It then should
> probably be separated from get_device(), which can be kept inside
> tpm_try_get_ops().

No sense checking for ops=null if you also can't guarentee the lock is
held, it is just confusing.

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Jason Gunthorpe]

On Mon, Jan 23, 2017 at 12:02:46PM -0800, Andrey Pronin wrote:

> > But if there is no actual need to do this right now then don't worry
> > about overdesigning things..
> 
> OK, I can live with chip->id specific logic in probe/shutdown, if that's
> the current approach.

It is where we are heading.. If it becomes prevelent we will need chip
and bus drivers.

> > First class then driver is the usual model, which is OK for TPM.
> 
> If "first class then driver", then the already existing
> register_reboot_notifier() can play the role of the class handler,
> since those hooks are called before drivers' shutdown handlers.

Okay.. But maybe fire off patch doing this via the device code, as if
that is accepted it is better than the reboot handler in terms of
guaranteeing ordering/etc.

> 	down_write(&chip->ops_sem);
> 	if (chip->ops) {
> 		if (chip->flags & TPM_CHIP_FLAG_TPM2)
> 			tpm2_shutdown(chip, TPM2_SU_CLEAR);
> 		chip->ops = NULL;
> 	}
> 	up_write(&chip->ops_sem);
> on shutdown in registered "reboot notifier".

Sure, seems like a good starting point.

> I went through the places that access chip->ops to see what's
> going on at the moment with protecting them with tpm_try_get_ops().
> Here is the current state:
> 
>  - tpm_transmit/tpm_transmit_cmd. Not exported and are only called
>    by the core after tpm_try_get_ops() except for one place in
>    sysfs - pubek_show().

Most of sysfs calls functions like tpm_pcr_read_dev, tpm_getcap, etc,
etc that boil down the tpm_transmit_cmd, so many more need it than you
listed.

> But it looks a bit brittle. So, before I submit my next patch:
> Do you think it's ok to leave wait_for_tpm_stat() and
> tpm_get_timeouts() and just continue be mindful when using those
> low-level functions?

For xenfront we should someday move it to use TPM_OPS_AUTO_STARTUP and
drop the tpm_get_timeouts.

I think it is fine to leave those two low level commands as is..

Like I said, it would be more robust to acquire a lock directly in
places like transmit_cmd, but I suspect that is too hard to retrofit
at this time...

> and checking for ops == NULL inside those low-level functions:
> tpm_transmit, wait_for_tpm_stat, tpm_get_timeout. It then should
> probably be separated from get_device(), which can be kept inside
> tpm_try_get_ops().

No sense checking for ops=null if you also can't guarentee the lock is
held, it is just confusing.

Jason

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Andrey Pronin]

On Mon, Jan 23, 2017 at 01:39:01PM -0700, Jason Gunthorpe wrote:
> On Mon, Jan 23, 2017 at 12:02:46PM -0800, Andrey Pronin wrote:
> 
> > > But if there is no actual need to do this right now then don't worry
> > > about overdesigning things..
> > 
> > OK, I can live with chip->id specific logic in probe/shutdown, if that's
> > the current approach.
> 
> It is where we are heading.. If it becomes prevelent we will need chip
> and bus drivers.
> 
> > > First class then driver is the usual model, which is OK for TPM.
> > 
> > If "first class then driver", then the already existing
> > register_reboot_notifier() can play the role of the class handler,
> > since those hooks are called before drivers' shutdown handlers.
> 
> Okay.. But maybe fire off patch doing this via the device code, as if
> that is accepted it is better than the reboot handler in terms of
> guaranteeing ordering/etc.
>

You mean introducing shutdown in struct class? Hmm, we can try,
but...

If we do something in 'struct class' for shutdown, that should
probably be modeled after power management, for which class
handlers are already supported. So, I looked more closely at
how this logic is implemented for suspend in
drivers/base/power/main.c.

Simplifying to class vs driver handlers, it checks class suspend
first. And if it exists, calls it. Otherwise, it calls driver
suspend. So, it's "either-or", not "first one, then another".
Unless I'm missing something obvious, of course...

If we want "first one, then another" it might be better to
go the register_reboot_notifier() path, instead of trying
to add something inconsistent with how it is handled for
power management to the core logic. I suspect those
notifiers is the mechanism of choice if we need to do
both class-level and device-level handling.

And I don't want to implement a class-level handler that'd
prevent driver handlers from doing device-specific handling,
if it is needed. Even if we reverse the order and do "driver
first, and iff it doesn't exist, then class", we'd still
need to create and export 'tpm_shutdown' for drivers to use
in their custom handlers. Otherwise, there's no way for
them to zero out chip->ops and prevent further access to
the chip.

Thus, after looking at that, I'd still suggest doing one of
the following:

We can go the register_reboot_notifier() route. Which already
explicitly supports priorites for those notify handlers, so one
can be sure that lower-prio handlers are called first. And,
in any case, all these handlers are called before driver->shutdown
routines. So, the ordering is very predictable, and we can design
whatever fits our needs with it.

Or, again, keep it even simpler, but require changes to all tpm
drivers. Create and export 'tpm_shutdown' in the core, and let
all the tpm drivers register their own shutdown handlers and
call it from there, adding their own logic to it, if they want.
Sorry, f we go this way, looks like we made the full cycle. But
I only now got to checking what's there in kernel for power
management.

I don't have strong preferences. In both cases, driver writers
need to be aware of the shutdown convention. Either (1) know
that there's a registered notifier, and they won't be able to
use tpm_transmit from driver->shutdown. Or, (2) know that they
need to implement driver->shutdown if they support TPM 2.0 or
can have problems if the chip is reset in the middle of command.
(1) sounds better (fewer drivers will be affected), but not
decisively.

> 
> > 	down_write(&chip->ops_sem);
> > 	if (chip->ops) {
> > 		if (chip->flags & TPM_CHIP_FLAG_TPM2)
> > 			tpm2_shutdown(chip, TPM2_SU_CLEAR);
> > 		chip->ops = NULL;
> > 	}
> > 	up_write(&chip->ops_sem);
> > on shutdown in registered "reboot notifier".
> 
> Sure, seems like a good starting point.
> 
> > I went through the places that access chip->ops to see what's
> > going on at the moment with protecting them with tpm_try_get_ops().
> > Here is the current state:
> > 
> >  - tpm_transmit/tpm_transmit_cmd. Not exported and are only called
> >    by the core after tpm_try_get_ops() except for one place in
> >    sysfs - pubek_show().
> 
> Most of sysfs calls functions like tpm_pcr_read_dev, tpm_getcap, etc,
> etc that boil down the tpm_transmit_cmd, so many more need it than you
> listed.
> 

Yes, you're right, of course. I definitely needed more coffee before
writing that. I ended up adding tpm_try_get_ops/tpm_put_ops to almost
every sysfs call once I started looking into them.

> > But it looks a bit brittle. So, before I submit my next patch:
> > Do you think it's ok to leave wait_for_tpm_stat() and
> > tpm_get_timeouts() and just continue be mindful when using those
> > low-level functions?
> 
> For xenfront we should someday move it to use TPM_OPS_AUTO_STARTUP and
> drop the tpm_get_timeouts.
> 
> I think it is fine to leave those two low level commands as is..
> 
> Like I said, it would be more robust to acquire a lock directly in
> places like transmit_cmd, but I suspect that is too hard to retrofit
> at this time...
> 
> > and checking for ops == NULL inside those low-level functions:
> > tpm_transmit, wait_for_tpm_stat, tpm_get_timeout. It then should
> > probably be separated from get_device(), which can be kept inside
> > tpm_try_get_ops().
> 
> No sense checking for ops=null if you also can't guarentee the lock is
> held, it is just confusing.

Oh, absolutely. I just meant calling tpm_try_get_ops(), which does both:
acquires the lock and checks that ops != NULL.

> 
> Jason

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Jason Gunthorpe]

On Mon, Jan 23, 2017 at 02:19:29PM -0800, Andrey Pronin wrote:

> Simplifying to class vs driver handlers, it checks class suspend
> first. And if it exists, calls it. Otherwise, it calls driver
> suspend. So, it's "either-or", not "first one, then another".
> Unless I'm missing something obvious, of course...

I belive the typical approach is that the class handler then chains to
the driver handler at the proper point in the shutdown flow. (ie this
way class code can execute before and after the driver power
management method)

> if it is needed. Even if we reverse the order and do "driver
> first, and iff it doesn't exist, then class", we'd still
> need to create and export 'tpm_shutdown' for drivers to use

The purpose of the driver shutdown would be to shutdown the bus, not
the chip. So it would be called with ops already disabled by the core
code, there is never a reason for a bus driver to call tpm_shutdown.

> We can go the register_reboot_notifier() route. Which already
> explicitly supports priorites for those notify handlers, so one

This is probably OK too, but it just feels weird to ignore the basic
core code pattern and fall back to notifiers, but I understand it can
be hard to make those changes.. But still worth at least one attempt,
IMHO.

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

[Jason Gunthorpe]

On Mon, Jan 23, 2017 at 02:19:29PM -0800, Andrey Pronin wrote:

> Simplifying to class vs driver handlers, it checks class suspend
> first. And if it exists, calls it. Otherwise, it calls driver
> suspend. So, it's "either-or", not "first one, then another".
> Unless I'm missing something obvious, of course...

I belive the typical approach is that the class handler then chains to
the driver handler at the proper point in the shutdown flow. (ie this
way class code can execute before and after the driver power
management method)

> if it is needed. Even if we reverse the order and do "driver
> first, and iff it doesn't exist, then class", we'd still
> need to create and export 'tpm_shutdown' for drivers to use

The purpose of the driver shutdown would be to shutdown the bus, not
the chip. So it would be called with ops already disabled by the core
code, there is never a reason for a bus driver to call tpm_shutdown.

> We can go the register_reboot_notifier() route. Which already
> explicitly supports priorites for those notify handlers, so one

This is probably OK too, but it just feels weird to ignore the basic
core code pattern and fall back to notifiers, but I understand it can
be hard to make those changes.. But still worth at least one attempt,
IMHO.

Jason