Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1494539 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [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

Page 1 of 2  [1] 2  Next page →

#1494539 — [PATCH] tpm: don't destroy chip device prematurely

In tpm_del_char_device device_del is called
prior to tpm2_shutdown where it is still used.

Fortunately, so far chip->dev was used only for printouts
int tpm2_shutdown flow, hence system didn't crash. But with
the introduction of runtime power management it will result in
shutting down the parent device while it still in use.

Fixes: 20e0152393b41 ("tpm: fix crash in tpm_tis deinitialization")
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/char/tpm/tpm-chip.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index e5950131bd90..b1cb0aae8e66 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -261,7 +261,6 @@ static int tpm_add_char_device(struct tpm_chip *chip)
 static void tpm_del_char_device(struct tpm_chip *chip)
 {
 	cdev_del(&chip->cdev);
-	device_del(&chip->dev);
 
 	/* Make the chip unavailable. */
 	mutex_lock(&idr_lock);
@@ -274,6 +273,8 @@ static void tpm_del_char_device(struct tpm_chip *chip)
 		tpm2_shutdown(chip, TPM2_SU_CLEAR);
 	chip->ops = NULL;
 	up_write(&chip->ops_sem);
+
+	device_del(&chip->dev);
 }
 
 static int tpm1_chip_register(struct tpm_chip *chip)
-- 
2.7.4

[toc] | [next] | [standalone]

#1494547

On Sun, Oct 02, 2016 at 10:39:31AM +0300, Tomas Winkler wrote:
> In tpm_del_char_device device_del is called
> prior to tpm2_shutdown where it is still used.
> 
> Fortunately, so far chip->dev was used only for printouts
> int tpm2_shutdown flow, hence system didn't crash. But with
> the introduction of runtime power management it will result in
> shutting down the parent device while it still in use.
> 
> Fixes: 20e0152393b41 ("tpm: fix crash in tpm_tis deinitialization")
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>

Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

> ---
>  drivers/char/tpm/tpm-chip.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index e5950131bd90..b1cb0aae8e66 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -261,7 +261,6 @@ static int tpm_add_char_device(struct tpm_chip *chip)
>  static void tpm_del_char_device(struct tpm_chip *chip)
>  {
>  	cdev_del(&chip->cdev);
> -	device_del(&chip->dev);
>  
>  	/* Make the chip unavailable. */
>  	mutex_lock(&idr_lock);
> @@ -274,6 +273,8 @@ static void tpm_del_char_device(struct tpm_chip *chip)
>  		tpm2_shutdown(chip, TPM2_SU_CLEAR);
>  	chip->ops = NULL;
>  	up_write(&chip->ops_sem);
> +
> +	device_del(&chip->dev);
>  }
>  
>  static int tpm1_chip_register(struct tpm_chip *chip)
> -- 
> 2.7.4
> 

[toc] | [prev] | [next] | [standalone]

#1494548

On Sun, Oct 02, 2016 at 01:17:55PM +0300, Jarkko Sakkinen wrote:
> On Sun, Oct 02, 2016 at 10:39:31AM +0300, Tomas Winkler wrote:
> > In tpm_del_char_device device_del is called
> > prior to tpm2_shutdown where it is still used.
> > 
> > Fortunately, so far chip->dev was used only for printouts
> > int tpm2_shutdown flow, hence system didn't crash. But with
> > the introduction of runtime power management it will result in
> > shutting down the parent device while it still in use.
> > 
> > Fixes: 20e0152393b41 ("tpm: fix crash in tpm_tis deinitialization")
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> 
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Applied.

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1494611

On Sun, Oct 02, 2016 at 01:24:55PM +0300, Jarkko Sakkinen wrote:
> On Sun, Oct 02, 2016 at 01:17:55PM +0300, Jarkko Sakkinen wrote:
> > On Sun, Oct 02, 2016 at 10:39:31AM +0300, Tomas Winkler wrote:
> > > In tpm_del_char_device device_del is called
> > > prior to tpm2_shutdown where it is still used.
> > > 
> > > Fortunately, so far chip->dev was used only for printouts
> > > int tpm2_shutdown flow, hence system didn't crash. But with
> > > the introduction of runtime power management it will result in
> > > shutting down the parent device while it still in use.
> > > 
> > > Fixes: 20e0152393b41 ("tpm: fix crash in tpm_tis deinitialization")
> > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > 
> > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> Applied.

This patch is wrong, I though the comments were clear. All entry
points to find the device must be deleted before we commit to shutting
down the device.

You need to figure out some other way to solve your problem.

Jason

[toc] | [prev] | [next] | [standalone]

#1494715

> On Sun, Oct 02, 2016 at 01:24:55PM +0300, Jarkko Sakkinen wrote:
> > On Sun, Oct 02, 2016 at 01:17:55PM +0300, Jarkko Sakkinen wrote:
> > > On Sun, Oct 02, 2016 at 10:39:31AM +0300, Tomas Winkler wrote:
> > > > In tpm_del_char_device device_del is called prior to tpm2_shutdown
> > > > where it is still used.
> > > >
> > > > Fortunately, so far chip->dev was used only for printouts int
> > > > tpm2_shutdown flow, hence system didn't crash. But with the
> > > > introduction of runtime power management it will result in
> > > > shutting down the parent device while it still in use.
> > > >
> > > > Fixes: 20e0152393b41 ("tpm: fix crash in tpm_tis
> > > > deinitialization")
> > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > >
> > > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> >
> > Applied.
> 
> This patch is wrong, I though the comments were clear. All entry points to find
> the device must be deleted before we commit to shutting down the device.
> 
> You need to figure out some other way to solve your problem.

Please  be more specific regarding flows you think will be wrong  with this patch, you must agree that the current code is broken even w/o runtime pm.
Thanks
Tomas

[toc] | [prev] | [next] | [standalone]

#1494721

> 
> > On Sun, Oct 02, 2016 at 01:24:55PM +0300, Jarkko Sakkinen wrote:
> > > On Sun, Oct 02, 2016 at 01:17:55PM +0300, Jarkko Sakkinen wrote:
> > > > On Sun, Oct 02, 2016 at 10:39:31AM +0300, Tomas Winkler wrote:
> > > > > In tpm_del_char_device device_del is called prior to
> > > > > tpm2_shutdown where it is still used.
> > > > >
> > > > > Fortunately, so far chip->dev was used only for printouts int
> > > > > tpm2_shutdown flow, hence system didn't crash. But with the
> > > > > introduction of runtime power management it will result in
> > > > > shutting down the parent device while it still in use.
> > > > >
> > > > > Fixes: 20e0152393b41 ("tpm: fix crash in tpm_tis
> > > > > deinitialization")
> > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > >
> > > > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > >
> > > Applied.
> >
> > This patch is wrong, I though the comments were clear. All entry
> > points to find the device must be deleted before we commit to shutting
> down the device.
> >
> > You need to figure out some other way to solve your problem.
> 
> Please  be more specific regarding flows you think will be wrong  with this
> patch, you must agree that the current code is broken even w/o runtime pm.

I've looked to the registration code and it indeed has few more issues
Maybe TPM_CHIP_FLAG_REGISTERED can be used for sealing the access to the device during deregistration, current usage is void. 

> 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

[toc] | [prev] | [next] | [standalone]

#1494863

On Mon, Oct 03, 2016 at 07:38:44AM +0000, Winkler, Tomas wrote:
> > 
> > > On Sun, Oct 02, 2016 at 01:24:55PM +0300, Jarkko Sakkinen wrote:
> > > > On Sun, Oct 02, 2016 at 01:17:55PM +0300, Jarkko Sakkinen wrote:
> > > > > On Sun, Oct 02, 2016 at 10:39:31AM +0300, Tomas Winkler wrote:
> > > > > > In tpm_del_char_device device_del is called prior to
> > > > > > tpm2_shutdown where it is still used.
> > > > > >
> > > > > > Fortunately, so far chip->dev was used only for printouts int
> > > > > > tpm2_shutdown flow, hence system didn't crash. But with the
> > > > > > introduction of runtime power management it will result in
> > > > > > shutting down the parent device while it still in use.
> > > > > >
> > > > > > Fixes: 20e0152393b41 ("tpm: fix crash in tpm_tis
> > > > > > deinitialization")
> > > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > > >
> > > > > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > >
> > > > Applied.
> > >
> > > This patch is wrong, I though the comments were clear. All entry
> > > points to find the device must be deleted before we commit to shutting
> > down the device.
> > >
> > > You need to figure out some other way to solve your problem.
> > 
> > Please  be more specific regarding flows you think will be wrong  with this
> > patch, you must agree that the current code is broken even w/o runtime pm.
> 
> I've looked to the registration code and it indeed has few more issues
> Maybe TPM_CHIP_FLAG_REGISTERED can be used for sealing the access to the device during deregistration, current usage is void. 

Good catch BTW. This flag has gone quite obsolote.

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1494927

On Mon, Oct 03, 2016 at 03:42:25PM +0300, Jarkko Sakkinen wrote:

> > I've looked to the registration code and it indeed has few more issues

?

> > Maybe TPM_CHIP_FLAG_REGISTERED can be used for sealing the access
> > to the device during deregistration, current usage is void.

This is done via chip->ops = NULL and the rwlock scheme.
 
> Good catch BTW. This flag has gone quite obsolote.

I think all the drivers have been updated at this point so we can
probably get rid of it entirely.

Jason

[toc] | [prev] | [next] | [standalone]

#1494982

> 
> On Mon, Oct 03, 2016 at 03:42:25PM +0300, Jarkko Sakkinen wrote:
> 
> > > I've looked to the registration code and it indeed has few more
> > > issues
> 
> ?
> 
> > > Maybe TPM_CHIP_FLAG_REGISTERED can be used for sealing the access to
> > > the device during deregistration, current usage is void.
> 
> This is done via chip->ops = NULL and the rwlock scheme.
I'm not this is the best choice, kind of unusual in the subsystems.  
> 
> > Good catch BTW. This flag has gone quite obsolote.
> 
> I think all the drivers have been updated at this point so we can probably get
> rid of it entirely.

I would actually keep it for the tpm2_shutdown exception, not sure this can be handled by ops  = NULL and rwlock.


Thanks
Tomas

> 
> Jason

[toc] | [prev] | [next] | [standalone]

#1494862

On Mon, Oct 03, 2016 at 07:05:48AM +0000, Winkler, Tomas wrote:
> 
> > On Sun, Oct 02, 2016 at 01:24:55PM +0300, Jarkko Sakkinen wrote:
> > > On Sun, Oct 02, 2016 at 01:17:55PM +0300, Jarkko Sakkinen wrote:
> > > > On Sun, Oct 02, 2016 at 10:39:31AM +0300, Tomas Winkler wrote:
> > > > > In tpm_del_char_device device_del is called prior to tpm2_shutdown
> > > > > where it is still used.
> > > > >
> > > > > Fortunately, so far chip->dev was used only for printouts int
> > > > > tpm2_shutdown flow, hence system didn't crash. But with the
> > > > > introduction of runtime power management it will result in
> > > > > shutting down the parent device while it still in use.
> > > > >
> > > > > Fixes: 20e0152393b41 ("tpm: fix crash in tpm_tis
> > > > > deinitialization")
> > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > >
> > > > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > >
> > > Applied.
> > 
> > This patch is wrong, I though the comments were clear. All entry points to find
> > the device must be deleted before we commit to shutting down the device.
> > 
> > You need to figure out some other way to solve your problem.
> 
> Please  be more specific regarding flows you think will be wrong  with
> this patch, you must agree that the current code is broken even w/o
> runtime pm.

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.

> Thanks
> Tomas

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1495202

On Mon, Oct 03, 2016 at 03:48:36PM +0300, Jarkko Sakkinen wrote:
> On Mon, Oct 03, 2016 at 07:05:48AM +0000, Winkler, Tomas wrote:
> > 
> > > On Sun, Oct 02, 2016 at 01:24:55PM +0300, Jarkko Sakkinen wrote:
> > > > On Sun, Oct 02, 2016 at 01:17:55PM +0300, Jarkko Sakkinen wrote:
> > > > > On Sun, Oct 02, 2016 at 10:39:31AM +0300, Tomas Winkler wrote:
> > > > > > In tpm_del_char_device device_del is called prior to tpm2_shutdown
> > > > > > where it is still used.
> > > > > >
> > > > > > Fortunately, so far chip->dev was used only for printouts int
> > > > > > tpm2_shutdown flow, hence system didn't crash. But with the
> > > > > > introduction of runtime power management it will result in
> > > > > > shutting down the parent device while it still in use.
> > > > > >
> > > > > > Fixes: 20e0152393b41 ("tpm: fix crash in tpm_tis
> > > > > > deinitialization")
> > > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > > >
> > > > > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > >
> > > > Applied.
> > > 
> > > This patch is wrong, I though the comments were clear. All entry points to find
> > > the device must be deleted before we commit to shutting down the device.
> > > 
> > > You need to figure out some other way to solve your problem.
> > 
> > Please  be more specific regarding flows you think will be wrong  with
> > this patch, you must agree that the current code is broken even w/o
> > runtime pm.
> 
> 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:

/* Make the driver uncallable. */
down_write(&chip->ops_sem);
if (chip->flags & TPM_CHIP_FLAG_TPM2)
	tpm2_shutdown(chip, TPM2_SU_CLEAR);
chip->ops = NULL;
up_write(&chip->ops_sem);

cdev_del(&chip->cdev);
device_del(&chip->dev);

/* Make the chip unavailable. */
mutex_lock(&idr_lock);
idr_replace(&dev_nums_idr, NULL, chip->dev_num);
mutex_unlock(&idr_lock);

The worst thing that can happen is -EPIPE.

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1495522

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

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.

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.

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.

Jason

[toc] | [prev] | [next] | [standalone]

#1495615

> 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

[toc] | [prev] | [next] | [standalone]

#1495625

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

[toc] | [prev] | [next] | [standalone]

#1495741

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

Will try to do better. 

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

Right, but that's why we need the TPM_CHIP_FLAG_REGISTERED bit to stay.
Second, tmp2_shutdown only assure that the tpm state is saved,  we are taking too hardline here.  If another command is issued, this is a problem of the upper layers and it has to be fixed in the upper layer.
On the other hand it is much worse if tpm2_shutdown is not sent at all.

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

This is the problem statement form the commit message: 

' But with the introduction of runtime power management it will result in
shutting down the parent device while it still in use.' 

We are talking about 
https://lkml.org/lkml/2016/9/12/352 
https://sourceforge.net/p/tpmdd/mailman/message/35395799/

But again, the real bug is in design, where a device is used after device_del()  is called.

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

 "chip-dev.parent''
 dev_get_drvdata(&chip->dev);

> 
> What code fails and why?

   device_del(dev)
          bus_remove_device(dev)
                device_release_driver(dev)
                       __device_release_driver(dev)
                                      pm_runtime_reinit(dev) {
                                            if (dev->parent)
                                                   pm_runtime_put(dev->parent);
                                       }

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

I rephrase it again as well, this requirement is  just abusing of the device interface.

> device_del only unregisters the dev, it does not deinitialize it, nor does it free
> any memory. 

Someone will do a legitimate fix in device_del and all you house will crash on you, the device should not be used after device_del is called. 
There is nothing in the interface that promises that nothing is destructed. 

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.

There is device hierarchy which is important for power management, please read the code.

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

TPM_CHIP_FLAG_REGISTERED

Tomas

[toc] | [prev] | [next] | [standalone]

#1495916

On Wed, Oct 05, 2016 at 07:48:59AM +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??)
> 
> Will try to do better. 

Jason, sorry your question slipped while going through the dicussion
:-)

I think I'll take the standpoint that I'll wait for the next version.

The important thing is to notice that runtime PM requires the device
to be "alive" and in the device hierarchy. It's a constraint...

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1495959

On Wed, Oct 05, 2016 at 06:15:26PM +0300, Jarkko Sakkinen wrote:

> The important thing is to notice that runtime PM requires the device
> to be "alive" and in the device hierarchy. It's a constraint...

There are two devices.

The chip->dev and the chip->dev.parent (aka the acpi_device)

Runtime PM is *only* attached to the chip->dev.parent - it does not
interact in any significant way with the chip->dev.

device_del is on the chip->dev. The acpi_device remains intact, and
fully functional.

This is why the whole patch is so confusing to me.

Jason

[toc] | [prev] | [next] | [standalone]

#1495974

On Wed, Oct 05, 2016 at 07:48:59AM +0000, Winkler, Tomas wrote:
> > > 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.
> 
> Right, but that's why we need the TPM_CHIP_FLAG_REGISTERED bit to
> stay.

How does that help?

We already null ops to signal the driver is removed, but we don't
check ops in all places today. Notably sysfs doesn't do the test, and
relies on the hard fence device_del provides.

So at a minimum to go forward with this approach you have to fix sysfs
to be safe - which I don't think is worthwhile..

> Second, tmp2_shutdown only assure that the tpm state is saved, we
> are taking too hardline here.  If another command is issued, this is
> a problem of the upper layers and it has to be fixed in the upper
> layer.

We can't fix it at the upper layers, this is classic removal
race. Whatever happens it must not cause the kernel to malfunction.

> This is the problem statement form the commit message: 
> 
> ' But with the introduction of runtime power management it will result in
> shutting down the parent device while it still in use.' 

> https://sourceforge.net/p/tpmdd/mailman/message/35395799/

Great, your commit message should include the klog message this patch
is fixing.

> > But again, the real bug is in design, where a device is used after
> > device_del()  is called.

No, I don't think so..

> > What code actually fails? I don't see anything in the runtime pm patch that
> > relies on chip->dev at all.
> 
>  "chip-dev.parent''
>  dev_get_drvdata(&chip->dev);

Also chip->dev.name because we can call dev_log/etc(&chip->dev..)

These are all fine, obviously. Todays kernel retains those values
across device_del and we set those values in tpmm_chip_alloc/etc. So
correct values are present as long as the chip memory exists. tpm
continues to hold a kref on the chip so the memory will be around.

'dev' values are only being used for clarity and convenience, if ever
the kernel changes behavior and nulls those values during device_del
then we will copy the values into chip and stop using dev. No
algorithm needs to change, and we don't need a registered 'dev'
to function.

However, I find such a possible change to be deeply unlikely. If you
look around the kernel I think you will find that many subsystems
subtly depend on these same invariants for corectness during various
unregister races.

> > What code fails and why?
> 
>    device_del(dev)
>           bus_remove_device(dev)
>                 device_release_driver(dev)
>                        __device_release_driver(dev)
>                                       pm_runtime_reinit(dev) {
>                                             if (dev->parent)
>                                                    pm_runtime_put(dev->parent);

Eh?? The full code is:

void pm_runtime_reinit(struct device *dev)
{
 if (!pm_runtime_enabled(dev)) {
     if (dev->power.irq_safe) {
         if (dev->parent)
              pm_runtime_put(dev->parent);

And irq_safe is only set by pm_runtime_irq_safe(). I can't find
any place that looks like that is called on chip->dev

Is there some other PM path where dev->parent becomes invovled?

Are you just guessing this solves a problem, or were you able to
reproduce Jarkko's report?

Considering that Jarkko cannot reliably reproduce the original bug,
I'm deeply skeptical this patch actually does anything more than
fiddle with timing around some kind of undiscovered race condition
scenario.

Even if that pm_runtime_put is happening, why doesn't the

+    pm_runtime_get_sync(chip->dev.parent);

The runtime_pm patch adds to tpm_transmit take care of bringing the
device back?

I'm still not hearing from you an explanation for what is actually
happening to cause the 325 error.. What does that error code decode
to anyhow?

Jason

[toc] | [prev] | [next] | [standalone]

#1496028

> 
> On Wed, Oct 05, 2016 at 07:48:59AM +0000, Winkler, Tomas wrote:
> > > > 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.
> >
> > Right, but that's why we need the TPM_CHIP_FLAG_REGISTERED bit to
> > stay.
> 
> How does that help?
> 
> We already null ops to signal the driver is removed, but we don't check ops in
> all places today. Notably sysfs doesn't do the test, and relies on the hard fence
> device_del provides.
> 
> So at a minimum to go forward with this approach you have to fix sysfs to be
> safe - which I don't think is worthwhile..

Yes, if this approach is taken this has to go across the whole stack, there is no question about it. 


> > Second, tmp2_shutdown only assure that the tpm state is saved, we are
> > taking too hardline here.  If another command is issued, this is a
> > problem of the upper layers and it has to be fixed in the upper layer.
> 
> We can't fix it at the upper layers, this is classic removal race. Whatever
> happens it must not cause the kernel to malfunction.
> 
> > This is the problem statement form the commit message:
> >
> > ' But with the introduction of runtime power management it will result
> > in shutting down the parent device while it still in use.'
> 
> > https://sourceforge.net/p/tpmdd/mailman/message/35395799/
> 
> Great, your commit message should include the klog message this patch is
> fixing.
>

It could, but that  patch was not merged yet, and I believe even if the issue is exposed only with runtime_pm currently, we have a bug in design even w/o runtime pm.

> > > But again, the real bug is in design, where a device is used after
> > > device_del()  is called.
> 
> No, I don't think so..

I do :)
> 
> > > What code actually fails? I don't see anything in the runtime pm
> > > patch that relies on chip->dev at all.
> >
> >  "chip-dev.parent''
> >  dev_get_drvdata(&chip->dev);
> 
> Also chip->dev.name because we can call dev_log/etc(&chip->dev..)

Yes, of course but this was not the point of disagreement
 
> These are all fine, obviously. Todays kernel retains those values across
> device_del and we set those values in tpmm_chip_alloc/etc. So correct values
> are present as long as the chip memory exists. tpm continues to hold a kref on
> the chip so the memory will be around.

I'm not saying they are not, but calling deep into the tpm stack and even to the parent device with unutilized device is not sane. 

> 'dev' values are only being used for clarity and convenience, if ever the kernel
> changes behavior and nulls those values during device_del then we will copy
> the values into chip and stop using dev. No algorithm needs to change, and we
> don't need a registered 'dev'
> to function.

Frankly I would suggest to stay with the device, it let you add tpm spec attributes (sysfs) if needed to the stack, you cannot hang those on the air.
You should distinguish between chip->dev and cdev though, those are not the same things. 

> However, I find such a possible change to be deeply unlikely. If you look
> around the kernel I think you will find that many subsystems subtly depend on
> these same invariants for corectness during various unregister races.

Yes, but the code around is aware that it's unregistering, you have device_del, put_device  pair enclosing device removal in the same function., unlike tmp2_shutdown which requires most of the stack functional. 
 
> > > What code fails and why?
> >
> >    device_del(dev)
> >           bus_remove_device(dev)
> >                 device_release_driver(dev)
> >                        __device_release_driver(dev)
> >                                       pm_runtime_reinit(dev) {
> >                                             if (dev->parent)
> >
> > pm_runtime_put(dev->parent);
> 
> Eh?? The full code is:
> 
> void pm_runtime_reinit(struct device *dev) {  if (!pm_runtime_enabled(dev)) {
>      if (dev->power.irq_safe) {
>          if (dev->parent)
>               pm_runtime_put(dev->parent);
> 
> And irq_safe is only set by pm_runtime_irq_safe(). I can't find any place that
> looks like that is called on chip->dev
> 
> Is there some other PM path where dev->parent becomes invovled?

Of course, the power management utilize the device hierarch, it assumes there is power dependencies between parents and child devices, such as bus controllers and the devices on that bus. 

> Are you just guessing this solves a problem, or were you able to reproduce
> Jarkko's report?

No, guesses are not my style :), this solves the issue, as you see this was also validated by Jarrko on his setup.

> Considering that Jarkko cannot reliably reproduce the original bug, I'm deeply
> skeptical this patch actually does anything more than fiddle with timing
> around some kind of undiscovered race condition scenario.
> 
> Even if that pm_runtime_put is happening, why doesn't the
> 
> +    pm_runtime_get_sync(chip->dev.parent);
> 
> The runtime_pm patch adds to tpm_transmit take care of bringing the device
> back?

Unfortunately not because, because it gets out of sync  and what is actually happening 
is that idle callback is called and device is put to idle between send and receive.

Now we can find a trick to fix this, but this would be rather w/o while we know what the real issue is.

> I'm still not hearing from you an explanation for what is actually happening to
> cause the 325 error.. What does that error code decode to anyhow?

Error code 0x145 RC_AUTH_CONTEXT, but the error code may vary, basically we lost context because the device went to the idle state. 
Thanks
Tomas

[toc] | [prev] | [next] | [standalone]

#1496059

On Wed, Oct 05, 2016 at 08:09:17PM +0000, Winkler, Tomas wrote:

> It could, but that patch was not merged yet, and I believe even if
> the issue is exposed only with runtime_pm currently, we have a bug
> in design even w/o runtime pm.

Please don't make changes without any justification :(

> > These are all fine, obviously. Todays kernel retains those values across
> > device_del and we set those values in tpmm_chip_alloc/etc. So correct values
> > are present as long as the chip memory exists. tpm continues to hold a kref on
> > the chip so the memory will be around.
> 
> I'm not saying they are not, but calling deep into the tpm stack and
> even to the parent device with unutilized device is not sane.

You keep asserting that, but it just isn't true at all.

For a long time the tpm subsystem didn't even have a
'struct device'. That is something Jarkko and I added.

The *ONLY* thing it does is act as the anchor for user space - eg it
holds the sysfs, contains the 'dev' file for the cdev, etc, etc. This
was an important clean up.

Internally to the tpm core, and the drivers, the chip->dev does
*NOTHING* except hold the few variables you pointed out. That is it.

We could go back to the old code, without the 'dev' and things
could still work correctly.

This is why your assertion the struct device needs to be registered
makes no sense.

If the runtime_pm patches change this, then we have a very serious
problem. Removing this assumption is much harder than a one line patch
moving device_del.

I actually have no idea how you'd do it, since we call all sorts of
tpm ops between device_init and device_add - again device_del is the
least of the problems if runtime pm insists the chip->dev be
registered when running transmit_cmd.

So, I again, strongly advise you to give up on this idea, it is too
hard for TPM, and does not seem technically needed at this time. Even
it it does seem to make some kind of intuitive sense.

> > Is there some other PM path where dev->parent becomes invovled?
> 
> Of course, the power management utilize the device hierarch, it
> assumes there is power dependencies between parents and child
> devices, such as bus controllers and the devices on that bus.

Sure, but that relationship only maters if something does a PM call on
the chip->dev, and AFAIK, nothing does that.

Do you know differently?

You pointed at something that isn't even run and said it is the source
of the problem.. You really need to set up here and explain exactly
what is happening.

> > Are you just guessing this solves a problem, or were you able to reproduce
> > Jarkko's report?
> 
> No, guesses are not my style :), this solves the issue, as you see
> this was also validated by Jarrko on his setup.

In the thread you pointed to Jarkko said he could not reproduce the
original issue. Jarkko can you clarify??

> > Even if that pm_runtime_put is happening, why doesn't the
> > 
> > +    pm_runtime_get_sync(chip->dev.parent);
> > 
> > The runtime_pm patch adds to tpm_transmit take care of bringing the device
> > back?
> 
> Unfortunately not because, because it gets out of sync and what is
> actually happening is that idle callback is called and device is put
> to idle between send and receive.

What? As far as I understand this PM stuff, I would call that a very
serious bug.

If a PM transition during transmit_cmd causes the TPM to abort/fail
command execution then it *MUST* be prevented. Period.

pm_runtime_get_sync appears to be the correct thing to get the
guarentee, so I'm very confused by your statement.

> Now we can find a trick to fix this, but this would be rather w/o
> while we know what the real issue is.

don't understand this.. Are you saying that going into idle during
tpm_transmit is not a bug?

Sounds like there is some sort of race condition with the pm stuff
that needs fixing.

I still don't see what your actual bug is, other than what I already
knew - somehow PM causes transmit_cmd to fail.

Jason

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web