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