Re: [PATCH] tpm: don't destroy chip device prematurely

From	Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Newsgroups	linux.kernel
Subject	Re: [PATCH] tpm: don't destroy chip device prematurely
Date	2016-10-05 01:20 +0200
Message-ID	<soHa9-Tn-3@gated-at.bofh.it> (permalink)
References	(4 earlier) <so5xT-Vq-1@gated-at.bofh.it> <soaQV-4YM-9@gated-at.bofh.it> <soqsF-6wU-1@gated-at.bofh.it> <soB4J-5is-11@gated-at.bofh.it> <soFUJ-8jf-7@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

On Tue, Oct 04, 2016 at 09:55:36PM +0000, Winkler, Tomas wrote:
> 
> > 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.

I'm having a hard time understanding the english in your
email. (Jarkko do you know what Tomas is talking about??)

> 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);

No, that is wrong as well, another thread can issue a TPM command
between the device_del and the ops = NULL. Presumably that will fail
the same as tpm2_shutdown does.

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

Not clear at all the commit message describes the 'solution' not the
problem.  This doesn't help..

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

What code actually fails? I don't see anything in the runtime pm patch
that relies on chip->dev at all.

What code fails and why?

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

I'll say it again, the tpm_transmit_cmd path must not require a
registered chip->dev.

device_del only unregisters the dev, it does not deinitialize it, nor
does it free any memory. I still don't understand how this has any
impact on the pm stuff when all the pm stuff is attached only to the
pdev.

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

What flag? Fix what?

Jason

Thread

[PATCH] tpm: don't destroy chip device prematurely Tomas Winkler <tomas.winkler@intel.com> - 2016-10-02 09:50 +0200
  Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-02 12:20 +0200
    Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-02 12:30 +0200
      Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-02 23:30 +0200
        RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-03 09:10 +0200
          RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-03 09:40 +0200
            Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-03 14:50 +0200
              Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-03 18:10 +0200
                RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-03 19:40 +0200
          Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-03 14:50 +0200
            Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-04 07:30 +0200
              Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-04 18:50 +0200
                RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-05 00:00 +0200
                Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-05 01:20 +0200
                RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-05 09:50 +0200
                Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-05 17:20 +0200
                Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-05 18:40 +0200
                Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-05 19:20 +0200
                RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-05 22:10 +0200
                Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-05 23:20 +0200
                RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-06 02:50 +0200
                Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-06 04:10 +0200
                RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-07 16:30 +0200
                Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-07 21:20 +0200
                RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-07 22:20 +0200
                Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-08 12:50 +0200
                Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-05 12:10 +0200
                Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-05 18:30 +0200
                Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-06 13:30 +0200
                Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-06 18:30 +0200
                Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-06 18:50 +0200
                Re: [PATCH] tpm: don't destroy chip device prematurely Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-10-05 12:10 +0200
          Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-03 18:10 +0200
            RE: [PATCH] tpm: don't destroy chip device prematurely "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-10-03 19:20 +0200
              Re: [PATCH] tpm: don't destroy chip device prematurely Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-10-03 19:40 +0200

csiph-web