> On Tue, Oct 04, 2016 at 08:19:46AM +0300, Jarkko Sakkinen wrote:
>
> > > Make the driver uncallable first. The worst race that can happen is
> > > that open("/dev/tpm0", ...) returns -EPIPE. I do not consider this
> > > fatal at all.
> >
> > No responses for this reasonable proposal so I'll show what I mean:
>
> How is this any better than what Thomas proposed? It seems much worse to
> me since now we have even more stuff in the wrong order.
>
> There are three purposes to the ordering as it stands today
> 1) To guarantee that tpm2_shutdown is the last command delivered to
> the TPM. When it is issued all other ways to access the device
> are hard fenced off.
I'm not sure where are you taking this requirements from simple bit is just
enough to make the HW inaccessible if the interface is designed right.
> 2) To hard fence the tpm subsystem for the 'platform' driver. Once
> tpm_del_char_device completes no callback into the driver
> is possible *at all*. The driver can destroy everything
> (iounmap, dereg irq, etc) and the driver module can be unloaded.
There is some wrong terminology character device is related to user space only,
a device driver can function w/o it.
> 3) To prevent oopsing with the sysfs code. Recall this comment
>
> /* The sysfs routines rely on an implicit tpm_try_get_ops, device_del
> * is called before ops is null'd and the sysfs core synchronizes this
> * removal so that no callbacks are running or can run again
> */
>
> device_del is what eliminates the sysfs access path, so
> ordering device_del after ops = null is just unconditionally
> wrong.
The ordering can be resolved, like this
down_write(&chip->ops_sem);
if (chip->flags & TPM_CHIP_FLAG_TPM2)
tpm2_shutdown(chip, TPM2_SU_CLEAR);
up_write(&chip->ops_sem);
device_del(&chip->dev);
down_write(&chip->ops_sem);
chip->ops = NULL;
up_write(&chip->ops_sem);
>
> I still haven't heard an explanation why Thomas's other patches need this, or
> why trying to change this ordering makes any sense at all considering how the
> subsystem is constructed.
I thought it's quite clear form the commit message, the device_del naturally
toggles runtime_pm of the parent device, it tries to resume the parent device
so it can perform denationalization and then suspend the parent device back
which caused tpm2_shutdown to fail.
>
> Further, if tpm_crb now needs a registered device, how on earth do all the
> chip ops we call work *before* registration? Or is that another bug?
>
> Why can't tpm_crb return to the pre-registration operating state in the driver
> remove function before calling unregister?
>
> None of this makes any sense to me.
I general we can not to implement power management via runtime_pm and resolve
the issue within tpm_crb driver but it's not abouth tpm_crb.
tpm2_shutdown is a tpm stack call it's not tpm_crb function, it uses
tpm_transmit_cmd and friends it should have valid tpm_chip initialized and
valid.
I'm not sure what could be more clearer than that.
> This whole thing was very carefully constructed to work *correctly* during
> unregister. Many other subsystems have races and bugs during remove (eg see
> the securityfs discussion). TPM has a hard requirement to support safe
> unregister due to the vtpm stuff, so we don't get to screw it up just to
> support
> one driver.
I have to admit that I'm not sure what the vtpm does yet, but I have a feeling
that a simple flag can fix this.
Thanks
Tomas
------------------------------------------------------------------------------
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
