Re: [PATCH 1/2 v4] tpm: cmd_ready command can be issued only after granting locality

2018-02-26 Thread Jarkko Sakkinen 
On Sun, 2018-02-25 at 14:43 +, Winkler, Tomas wrote:
> > 
> > On Sun, Feb 25, 2018 at 10:37:08AM +, Winkler, Tomas wrote:
> > > > 
> > > > On Sat, Feb 24, 2018 at 05:43:16PM +0200, Tomas Winkler wrote:
> > > > > The correct sequence is to first request locality and only after
> > > > > that perform cmd_ready handshake, otherwise the hardware will drop
> > > > > the subsequent message as from the device point of view the
> > > > > cmd_ready handshake wasn't performed. Symmetrically locality has
> > > > > to be relinquished only after going idle handshake has completed,
> > > > > this requires that go_idle has to poll for the completion and as
> > > > > well locality relinquish has to poll for completion so it is not
> > > > > overridden in back to back commands flow.
> > > > > 
> > > > > The issue is only visible on devices that support multiple localities.
> > > > > 
> > > > > Signed-off-by: Tomas Winkler 
> > > > > Tested-by: Jarkko Sakkinen 
> > > > > ---
> > > > > V2: poll for locality relinquish completion
> > > > > V3: 1. Print error message upon locality relinquish failure
> > > > > 2. Don't override rc code on error path with locality
> > > > > relinquish
> > > > > V4: 3. Don't capture locality relinquish error code in rc, just print
> > > > >   the error message.
> > > > > 
> > > > >return value.
> > > > >  drivers/char/tpm/tpm-interface.c |  20 +---
> > > > >  drivers/char/tpm/tpm_crb.c   | 108 +++-
> > 
> > -
> > > > --
> > > > >  drivers/char/tpm/tpm_tis_core.c  |   4 +-
> > > > >  include/linux/tpm.h  |   2 +-
> > > > >  4 files changed, 93 insertions(+), 41 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/char/tpm/tpm-interface.c
> > > > > b/drivers/char/tpm/tpm-interface.c
> > > > > index 9e80a953d693..4d74bacca5a1 100644
> > > > > --- a/drivers/char/tpm/tpm-interface.c
> > > > > +++ b/drivers/char/tpm/tpm-interface.c
> > > > > @@ -422,8 +422,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip,
> > > > > struct
> > > > 
> > > > tpm_space *space,
> > > > >   if (!(flags & TPM_TRANSMIT_UNLOCKED))
> > > > >   mutex_lock(>tpm_mutex);
> > > > > 
> > > > > - if (chip->dev.parent)
> > > > > - pm_runtime_get_sync(chip->dev.parent);
> > > > > 
> > > > >   if (chip->ops->clk_enable != NULL)
> > > > >   chip->ops->clk_enable(chip, true); @@ -439,6 +437,9
> > > > > @@
> > 
> > ssize_t
> > > > > tpm_transmit(struct tpm_chip *chip, struct
> > > > 
> > > > tpm_space *space,
> > > > >   chip->locality = rc;
> > > > >   }
> > > > > 
> > > > > + if (chip->dev.parent)
> > > > > + pm_runtime_get_sync(chip->dev.parent);
> > > > > +
> > > > >   rc = tpm2_prepare_space(chip, space, ordinal, buf);
> > > > >   if (rc)
> > > > >   goto out;
> > > > > @@ -499,17 +500,24 @@ ssize_t tpm_transmit(struct tpm_chip *chip,
> > > > 
> > > > struct tpm_space *space,
> > > > >   rc = tpm2_commit_space(chip, space, ordinal, buf, );
> > > > > 
> > > > >  out:
> > > > > + if (chip->dev.parent)
> > > > > + pm_runtime_put_sync(chip->dev.parent);
> > > > > +
> > > > >   if (need_locality && chip->ops->relinquish_locality) {
> > > > > - chip->ops->relinquish_locality(chip, chip->locality);
> > > > > + /* this coud be on error path, don't override error
> > > > > code */
> > > > > + int l_rc = chip->ops->relinquish_locality(chip,
> > > > > +chip->locality);
> > > > 
> > > > Declaration should be in the beginning of the function.
> > > 
> > > No, it shouldn't. I cannot find any reference to this statement, I've
> > > already
> > 
> > explained my reasoning in a previous mail.
> > > 
> > > > 
> > > > > +
> > > > > + if (l_rc) {
> > > > > + dev_err(>dev, "%s: relinquish_locality:
> > > > > error
> > > > 
> > > > %d\n",
> > > > > + __func__, l_rc);
> > > > > + }
> > > > 
> > > > In kernel coding style, this should be w/o curly braces.
> > > 
> > > Yep, missed that, will resubmit
> > > 
> > > 
> > > > I can fix these cosmetic issues
> > > > 
> > > > Reviewed-by: Jarkko Sakkinen 
> > > > 
> > > > Doesn't this need
> > > > 
> > > > Fixes: 877c57d0d0ca ("tpm_crb: request and relinquish locality 0")
> > > > And shouldn't this also have
> > > > 
> > > > Cc: sta...@vger.kernel.org
> > > > 
> > > > ?
> > > 
> > > Good points
> > > Thanks
> > > Tomas
> > 
> > Tomas, I updated v4 myself and pushed it to master/next. Please tell me if
> > there is anything wrong and I will fix it.
> > 
> 
> Please, take my fix v5. I didn't agree to moving l_rc out its scope. 
> Thanks
> Tomas

OK, I removed the commit. Thanks.

/Jarkko

Re: [PATCH 1/2 v4] tpm: cmd_ready command can be issued only after granting locality

2018-02-26 Thread Jarkko Sakkinen 
On Sun, 2018-02-25 at 14:43 +, Winkler, Tomas wrote:
> > 
> > On Sun, Feb 25, 2018 at 10:37:08AM +, Winkler, Tomas wrote:
> > > > 
> > > > On Sat, Feb 24, 2018 at 05:43:16PM +0200, Tomas Winkler wrote:
> > > > > The correct sequence is to first request locality and only after
> > > > > that perform cmd_ready handshake, otherwise the hardware will drop
> > > > > the subsequent message as from the device point of view the
> > > > > cmd_ready handshake wasn't performed. Symmetrically locality has
> > > > > to be relinquished only after going idle handshake has completed,
> > > > > this requires that go_idle has to poll for the completion and as
> > > > > well locality relinquish has to poll for completion so it is not
> > > > > overridden in back to back commands flow.
> > > > > 
> > > > > The issue is only visible on devices that support multiple localities.
> > > > > 
> > > > > Signed-off-by: Tomas Winkler 
> > > > > Tested-by: Jarkko Sakkinen 
> > > > > ---
> > > > > V2: poll for locality relinquish completion
> > > > > V3: 1. Print error message upon locality relinquish failure
> > > > > 2. Don't override rc code on error path with locality
> > > > > relinquish
> > > > > V4: 3. Don't capture locality relinquish error code in rc, just print
> > > > >   the error message.
> > > > > 
> > > > >return value.
> > > > >  drivers/char/tpm/tpm-interface.c |  20 +---
> > > > >  drivers/char/tpm/tpm_crb.c   | 108 +++-
> > 
> > -
> > > > --
> > > > >  drivers/char/tpm/tpm_tis_core.c  |   4 +-
> > > > >  include/linux/tpm.h  |   2 +-
> > > > >  4 files changed, 93 insertions(+), 41 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/char/tpm/tpm-interface.c
> > > > > b/drivers/char/tpm/tpm-interface.c
> > > > > index 9e80a953d693..4d74bacca5a1 100644
> > > > > --- a/drivers/char/tpm/tpm-interface.c
> > > > > +++ b/drivers/char/tpm/tpm-interface.c
> > > > > @@ -422,8 +422,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip,
> > > > > struct
> > > > 
> > > > tpm_space *space,
> > > > >   if (!(flags & TPM_TRANSMIT_UNLOCKED))
> > > > >   mutex_lock(>tpm_mutex);
> > > > > 
> > > > > - if (chip->dev.parent)
> > > > > - pm_runtime_get_sync(chip->dev.parent);
> > > > > 
> > > > >   if (chip->ops->clk_enable != NULL)
> > > > >   chip->ops->clk_enable(chip, true); @@ -439,6 +437,9
> > > > > @@
> > 
> > ssize_t
> > > > > tpm_transmit(struct tpm_chip *chip, struct
> > > > 
> > > > tpm_space *space,
> > > > >   chip->locality = rc;
> > > > >   }
> > > > > 
> > > > > + if (chip->dev.parent)
> > > > > + pm_runtime_get_sync(chip->dev.parent);
> > > > > +
> > > > >   rc = tpm2_prepare_space(chip, space, ordinal, buf);
> > > > >   if (rc)
> > > > >   goto out;
> > > > > @@ -499,17 +500,24 @@ ssize_t tpm_transmit(struct tpm_chip *chip,
> > > > 
> > > > struct tpm_space *space,
> > > > >   rc = tpm2_commit_space(chip, space, ordinal, buf, );
> > > > > 
> > > > >  out:
> > > > > + if (chip->dev.parent)
> > > > > + pm_runtime_put_sync(chip->dev.parent);
> > > > > +
> > > > >   if (need_locality && chip->ops->relinquish_locality) {
> > > > > - chip->ops->relinquish_locality(chip, chip->locality);
> > > > > + /* this coud be on error path, don't override error
> > > > > code */
> > > > > + int l_rc = chip->ops->relinquish_locality(chip,
> > > > > +chip->locality);
> > > > 
> > > > Declaration should be in the beginning of the function.
> > > 
> > > No, it shouldn't. I cannot find any reference to this statement, I've
> > > already
> > 
> > explained my reasoning in a previous mail.
> > > 
> > > > 
> > > > > +
> > > > > + if (l_rc) {
> > > > > + dev_err(>dev, "%s: relinquish_locality:
> > > > > error
> > > > 
> > > > %d\n",
> > > > > + __func__, l_rc);
> > > > > + }
> > > > 
> > > > In kernel coding style, this should be w/o curly braces.
> > > 
> > > Yep, missed that, will resubmit
> > > 
> > > 
> > > > I can fix these cosmetic issues
> > > > 
> > > > Reviewed-by: Jarkko Sakkinen 
> > > > 
> > > > Doesn't this need
> > > > 
> > > > Fixes: 877c57d0d0ca ("tpm_crb: request and relinquish locality 0")
> > > > And shouldn't this also have
> > > > 
> > > > Cc: sta...@vger.kernel.org
> > > > 
> > > > ?
> > > 
> > > Good points
> > > Thanks
> > > Tomas
> > 
> > Tomas, I updated v4 myself and pushed it to master/next. Please tell me if
> > there is anything wrong and I will fix it.
> > 
> 
> Please, take my fix v5. I didn't agree to moving l_rc out its scope. 
> Thanks
> Tomas

OK, I removed the commit. Thanks.

/Jarkko

RE: [PATCH 1/2 v4] tpm: cmd_ready command can be issued only after granting locality

2018-02-25 Thread Winkler, Tomas 

> >
> > On Sun, Feb 25, 2018 at 10:37:08AM +, Winkler, Tomas wrote:
> > > >
> > > > On Sat, Feb 24, 2018 at 05:43:16PM +0200, Tomas Winkler wrote:
> > > > > The correct sequence is to first request locality and only after
> > > > > that perform cmd_ready handshake, otherwise the hardware will
> > > > > drop the subsequent message as from the device point of view the
> > > > > cmd_ready handshake wasn't performed. Symmetrically locality has
> > > > > to be relinquished only after going idle handshake has
> > > > > completed, this requires that go_idle has to poll for the
> > > > > completion and as well locality relinquish has to poll for
> > > > > completion so it is not overridden in back to back commands flow.
> > > > >
> > > > > The issue is only visible on devices that support multiple localities.
> > > > >
> > > > > Signed-off-by: Tomas Winkler 
> > > > > Tested-by: Jarkko Sakkinen 
> > > > > ---
> > > > > V2: poll for locality relinquish completion
> > > > > V3: 1. Print error message upon locality relinquish failure
> > > > > 2. Don't override rc code on error path with locality
> > > > > relinquish
> > > > > V4: 3. Don't capture locality relinquish error code in rc, just print
> > > > >   the error message.
> > > > >
> > > > >return value.
> > > > >  drivers/char/tpm/tpm-interface.c |  20 +---
> > > > >  drivers/char/tpm/tpm_crb.c   | 108
> +++-
> > -
> > > > --
> > > > >  drivers/char/tpm/tpm_tis_core.c  |   4 +-
> > > > >  include/linux/tpm.h  |   2 +-
> > > > >  4 files changed, 93 insertions(+), 41 deletions(-)
> > > > >
> > > > > diff --git a/drivers/char/tpm/tpm-interface.c
> > > > > b/drivers/char/tpm/tpm-interface.c
> > > > > index 9e80a953d693..4d74bacca5a1 100644
> > > > > --- a/drivers/char/tpm/tpm-interface.c
> > > > > +++ b/drivers/char/tpm/tpm-interface.c
> > > > > @@ -422,8 +422,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip,
> > > > > struct
> > > > tpm_space *space,
> > > > >   if (!(flags & TPM_TRANSMIT_UNLOCKED))
> > > > >   mutex_lock(>tpm_mutex);
> > > > >
> > > > > - if (chip->dev.parent)
> > > > > - pm_runtime_get_sync(chip->dev.parent);
> > > > >
> > > > >   if (chip->ops->clk_enable != NULL)
> > > > >   chip->ops->clk_enable(chip, true); @@ -439,6 +437,9
> @@
> > ssize_t
> > > > > tpm_transmit(struct tpm_chip *chip, struct
> > > > tpm_space *space,
> > > > >   chip->locality = rc;
> > > > >   }
> > > > >
> > > > > + if (chip->dev.parent)
> > > > > + pm_runtime_get_sync(chip->dev.parent);
> > > > > +
> > > > >   rc = tpm2_prepare_space(chip, space, ordinal, buf);
> > > > >   if (rc)
> > > > >   goto out;
> > > > > @@ -499,17 +500,24 @@ ssize_t tpm_transmit(struct tpm_chip
> > > > > *chip,
> > > > struct tpm_space *space,
> > > > >   rc = tpm2_commit_space(chip, space, ordinal, buf, );
> > > > >
> > > > >  out:
> > > > > + if (chip->dev.parent)
> > > > > + pm_runtime_put_sync(chip->dev.parent);
> > > > > +
> > > > >   if (need_locality && chip->ops->relinquish_locality) {
> > > > > - chip->ops->relinquish_locality(chip, chip->locality);
> > > > > + /* this coud be on error path, don't override error
> code */
> > > > > + int l_rc = chip->ops->relinquish_locality(chip,
> > > > > +chip->locality);
> > > >
> > > > Declaration should be in the beginning of the function.
> > >
> > > No, it shouldn't. I cannot find any reference to this statement,
> > > I've already
> > explained my reasoning in a previous mail.
> > >
> > > >
> > > > > +
> > > > > + if (l_rc) {
> > > > > + dev_err(>dev, "%s: relinquish_locality:
> error
> > > > %d\n",
> > > > > + __func__, l_rc);
> > > > > + }
> > > >
> > > > In kernel coding style, this should be w/o curly braces.
> > >
> > > Yep, missed that, will resubmit
> > >
> > >
> > > > I can fix these cosmetic issues
> > > >
> > > > Reviewed-by: Jarkko Sakkinen 
> > > >
> > > > Doesn't this need
> > > >
> > > > Fixes: 877c57d0d0ca ("tpm_crb: request and relinquish locality 0")
> > > > And shouldn't this also have
> > > >
> > > > Cc: sta...@vger.kernel.org
> > > >
> > > > ?
> > > Good points
> > > Thanks
> > > Tomas
> >
> > Tomas, I updated v4 myself and pushed it to master/next. Please tell
> > me if there is anything wrong and I will fix it.
> >
> Please, take my fix v5. I didn't agree to moving l_rc out its scope.

Also it doesn't apply to stable w/o rework.

Thanks
Tomas

RE: [PATCH 1/2 v4] tpm: cmd_ready command can be issued only after granting locality

2018-02-25 Thread Winkler, Tomas 

> >
> > On Sun, Feb 25, 2018 at 10:37:08AM +, Winkler, Tomas wrote:
> > > >
> > > > On Sat, Feb 24, 2018 at 05:43:16PM +0200, Tomas Winkler wrote:
> > > > > The correct sequence is to first request locality and only after
> > > > > that perform cmd_ready handshake, otherwise the hardware will
> > > > > drop the subsequent message as from the device point of view the
> > > > > cmd_ready handshake wasn't performed. Symmetrically locality has
> > > > > to be relinquished only after going idle handshake has
> > > > > completed, this requires that go_idle has to poll for the
> > > > > completion and as well locality relinquish has to poll for
> > > > > completion so it is not overridden in back to back commands flow.
> > > > >
> > > > > The issue is only visible on devices that support multiple localities.
> > > > >
> > > > > Signed-off-by: Tomas Winkler 
> > > > > Tested-by: Jarkko Sakkinen 
> > > > > ---
> > > > > V2: poll for locality relinquish completion
> > > > > V3: 1. Print error message upon locality relinquish failure
> > > > > 2. Don't override rc code on error path with locality
> > > > > relinquish
> > > > > V4: 3. Don't capture locality relinquish error code in rc, just print
> > > > >   the error message.
> > > > >
> > > > >return value.
> > > > >  drivers/char/tpm/tpm-interface.c |  20 +---
> > > > >  drivers/char/tpm/tpm_crb.c   | 108
> +++-
> > -
> > > > --
> > > > >  drivers/char/tpm/tpm_tis_core.c  |   4 +-
> > > > >  include/linux/tpm.h  |   2 +-
> > > > >  4 files changed, 93 insertions(+), 41 deletions(-)
> > > > >
> > > > > diff --git a/drivers/char/tpm/tpm-interface.c
> > > > > b/drivers/char/tpm/tpm-interface.c
> > > > > index 9e80a953d693..4d74bacca5a1 100644
> > > > > --- a/drivers/char/tpm/tpm-interface.c
> > > > > +++ b/drivers/char/tpm/tpm-interface.c
> > > > > @@ -422,8 +422,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip,
> > > > > struct
> > > > tpm_space *space,
> > > > >   if (!(flags & TPM_TRANSMIT_UNLOCKED))
> > > > >   mutex_lock(>tpm_mutex);
> > > > >
> > > > > - if (chip->dev.parent)
> > > > > - pm_runtime_get_sync(chip->dev.parent);
> > > > >
> > > > >   if (chip->ops->clk_enable != NULL)
> > > > >   chip->ops->clk_enable(chip, true); @@ -439,6 +437,9
> @@
> > ssize_t
> > > > > tpm_transmit(struct tpm_chip *chip, struct
> > > > tpm_space *space,
> > > > >   chip->locality = rc;
> > > > >   }
> > > > >
> > > > > + if (chip->dev.parent)
> > > > > + pm_runtime_get_sync(chip->dev.parent);
> > > > > +
> > > > >   rc = tpm2_prepare_space(chip, space, ordinal, buf);
> > > > >   if (rc)
> > > > >   goto out;
> > > > > @@ -499,17 +500,24 @@ ssize_t tpm_transmit(struct tpm_chip
> > > > > *chip,
> > > > struct tpm_space *space,
> > > > >   rc = tpm2_commit_space(chip, space, ordinal, buf, );
> > > > >
> > > > >  out:
> > > > > + if (chip->dev.parent)
> > > > > + pm_runtime_put_sync(chip->dev.parent);
> > > > > +
> > > > >   if (need_locality && chip->ops->relinquish_locality) {
> > > > > - chip->ops->relinquish_locality(chip, chip->locality);
> > > > > + /* this coud be on error path, don't override error
> code */
> > > > > + int l_rc = chip->ops->relinquish_locality(chip,
> > > > > +chip->locality);
> > > >
> > > > Declaration should be in the beginning of the function.
> > >
> > > No, it shouldn't. I cannot find any reference to this statement,
> > > I've already
> > explained my reasoning in a previous mail.
> > >
> > > >
> > > > > +
> > > > > + if (l_rc) {
> > > > > + dev_err(>dev, "%s: relinquish_locality:
> error
> > > > %d\n",
> > > > > + __func__, l_rc);
> > > > > + }
> > > >
> > > > In kernel coding style, this should be w/o curly braces.
> > >
> > > Yep, missed that, will resubmit
> > >
> > >
> > > > I can fix these cosmetic issues
> > > >
> > > > Reviewed-by: Jarkko Sakkinen 
> > > >
> > > > Doesn't this need
> > > >
> > > > Fixes: 877c57d0d0ca ("tpm_crb: request and relinquish locality 0")
> > > > And shouldn't this also have
> > > >
> > > > Cc: sta...@vger.kernel.org
> > > >
> > > > ?
> > > Good points
> > > Thanks
> > > Tomas
> >
> > Tomas, I updated v4 myself and pushed it to master/next. Please tell
> > me if there is anything wrong and I will fix it.
> >
> Please, take my fix v5. I didn't agree to moving l_rc out its scope.

Also it doesn't apply to stable w/o rework.

Thanks
Tomas

RE: [PATCH 1/2 v4] tpm: cmd_ready command can be issued only after granting locality

2018-02-25 Thread Winkler, Tomas 
> 
> On Sun, Feb 25, 2018 at 10:37:08AM +, Winkler, Tomas wrote:
> > >
> > > On Sat, Feb 24, 2018 at 05:43:16PM +0200, Tomas Winkler wrote:
> > > > The correct sequence is to first request locality and only after
> > > > that perform cmd_ready handshake, otherwise the hardware will drop
> > > > the subsequent message as from the device point of view the
> > > > cmd_ready handshake wasn't performed. Symmetrically locality has
> > > > to be relinquished only after going idle handshake has completed,
> > > > this requires that go_idle has to poll for the completion and as
> > > > well locality relinquish has to poll for completion so it is not
> > > > overridden in back to back commands flow.
> > > >
> > > > The issue is only visible on devices that support multiple localities.
> > > >
> > > > Signed-off-by: Tomas Winkler 
> > > > Tested-by: Jarkko Sakkinen 
> > > > ---
> > > > V2: poll for locality relinquish completion
> > > > V3: 1. Print error message upon locality relinquish failure
> > > > 2. Don't override rc code on error path with locality
> > > > relinquish
> > > > V4: 3. Don't capture locality relinquish error code in rc, just print
> > > > the error message.
> > > >
> > > >return value.
> > > >  drivers/char/tpm/tpm-interface.c |  20 +---
> > > >  drivers/char/tpm/tpm_crb.c   | 108 +++-
> -
> > > --
> > > >  drivers/char/tpm/tpm_tis_core.c  |   4 +-
> > > >  include/linux/tpm.h  |   2 +-
> > > >  4 files changed, 93 insertions(+), 41 deletions(-)
> > > >
> > > > diff --git a/drivers/char/tpm/tpm-interface.c
> > > > b/drivers/char/tpm/tpm-interface.c
> > > > index 9e80a953d693..4d74bacca5a1 100644
> > > > --- a/drivers/char/tpm/tpm-interface.c
> > > > +++ b/drivers/char/tpm/tpm-interface.c
> > > > @@ -422,8 +422,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip,
> > > > struct
> > > tpm_space *space,
> > > > if (!(flags & TPM_TRANSMIT_UNLOCKED))
> > > > mutex_lock(>tpm_mutex);
> > > >
> > > > -   if (chip->dev.parent)
> > > > -   pm_runtime_get_sync(chip->dev.parent);
> > > >
> > > > if (chip->ops->clk_enable != NULL)
> > > > chip->ops->clk_enable(chip, true); @@ -439,6 +437,9 @@
> ssize_t
> > > > tpm_transmit(struct tpm_chip *chip, struct
> > > tpm_space *space,
> > > > chip->locality = rc;
> > > > }
> > > >
> > > > +   if (chip->dev.parent)
> > > > +   pm_runtime_get_sync(chip->dev.parent);
> > > > +
> > > > rc = tpm2_prepare_space(chip, space, ordinal, buf);
> > > > if (rc)
> > > > goto out;
> > > > @@ -499,17 +500,24 @@ ssize_t tpm_transmit(struct tpm_chip *chip,
> > > struct tpm_space *space,
> > > > rc = tpm2_commit_space(chip, space, ordinal, buf, );
> > > >
> > > >  out:
> > > > +   if (chip->dev.parent)
> > > > +   pm_runtime_put_sync(chip->dev.parent);
> > > > +
> > > > if (need_locality && chip->ops->relinquish_locality) {
> > > > -   chip->ops->relinquish_locality(chip, chip->locality);
> > > > +   /* this coud be on error path, don't override error 
> > > > code */
> > > > +   int l_rc = chip->ops->relinquish_locality(chip,
> > > > +chip->locality);
> > >
> > > Declaration should be in the beginning of the function.
> >
> > No, it shouldn't. I cannot find any reference to this statement, I've 
> > already
> explained my reasoning in a previous mail.
> >
> > >
> > > > +
> > > > +   if (l_rc) {
> > > > +   dev_err(>dev, "%s: relinquish_locality: 
> > > > error
> > > %d\n",
> > > > +   __func__, l_rc);
> > > > +   }
> > >
> > > In kernel coding style, this should be w/o curly braces.
> >
> > Yep, missed that, will resubmit
> >
> >
> > > I can fix these cosmetic issues
> > >
> > > Reviewed-by: Jarkko Sakkinen 
> > >
> > > Doesn't this need
> > >
> > > Fixes: 877c57d0d0ca ("tpm_crb: request and relinquish locality 0")
> > > And shouldn't this also have
> > >
> > > Cc: sta...@vger.kernel.org
> > >
> > > ?
> > Good points
> > Thanks
> > Tomas
> 
> Tomas, I updated v4 myself and pushed it to master/next. Please tell me if
> there is anything wrong and I will fix it.
> 
Please, take my fix v5. I didn't agree to moving l_rc out its scope. 
Thanks
Tomas

RE: [PATCH 1/2 v4] tpm: cmd_ready command can be issued only after granting locality

2018-02-25 Thread Winkler, Tomas 
> 
> On Sun, Feb 25, 2018 at 10:37:08AM +, Winkler, Tomas wrote:
> > >
> > > On Sat, Feb 24, 2018 at 05:43:16PM +0200, Tomas Winkler wrote:
> > > > The correct sequence is to first request locality and only after
> > > > that perform cmd_ready handshake, otherwise the hardware will drop
> > > > the subsequent message as from the device point of view the
> > > > cmd_ready handshake wasn't performed. Symmetrically locality has
> > > > to be relinquished only after going idle handshake has completed,
> > > > this requires that go_idle has to poll for the completion and as
> > > > well locality relinquish has to poll for completion so it is not
> > > > overridden in back to back commands flow.
> > > >
> > > > The issue is only visible on devices that support multiple localities.
> > > >
> > > > Signed-off-by: Tomas Winkler 
> > > > Tested-by: Jarkko Sakkinen 
> > > > ---
> > > > V2: poll for locality relinquish completion
> > > > V3: 1. Print error message upon locality relinquish failure
> > > > 2. Don't override rc code on error path with locality
> > > > relinquish
> > > > V4: 3. Don't capture locality relinquish error code in rc, just print
> > > > the error message.
> > > >
> > > >return value.
> > > >  drivers/char/tpm/tpm-interface.c |  20 +---
> > > >  drivers/char/tpm/tpm_crb.c   | 108 +++-
> -
> > > --
> > > >  drivers/char/tpm/tpm_tis_core.c  |   4 +-
> > > >  include/linux/tpm.h  |   2 +-
> > > >  4 files changed, 93 insertions(+), 41 deletions(-)
> > > >
> > > > diff --git a/drivers/char/tpm/tpm-interface.c
> > > > b/drivers/char/tpm/tpm-interface.c
> > > > index 9e80a953d693..4d74bacca5a1 100644
> > > > --- a/drivers/char/tpm/tpm-interface.c
> > > > +++ b/drivers/char/tpm/tpm-interface.c
> > > > @@ -422,8 +422,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip,
> > > > struct
> > > tpm_space *space,
> > > > if (!(flags & TPM_TRANSMIT_UNLOCKED))
> > > > mutex_lock(>tpm_mutex);
> > > >
> > > > -   if (chip->dev.parent)
> > > > -   pm_runtime_get_sync(chip->dev.parent);
> > > >
> > > > if (chip->ops->clk_enable != NULL)
> > > > chip->ops->clk_enable(chip, true); @@ -439,6 +437,9 @@
> ssize_t
> > > > tpm_transmit(struct tpm_chip *chip, struct
> > > tpm_space *space,
> > > > chip->locality = rc;
> > > > }
> > > >
> > > > +   if (chip->dev.parent)
> > > > +   pm_runtime_get_sync(chip->dev.parent);
> > > > +
> > > > rc = tpm2_prepare_space(chip, space, ordinal, buf);
> > > > if (rc)
> > > > goto out;
> > > > @@ -499,17 +500,24 @@ ssize_t tpm_transmit(struct tpm_chip *chip,
> > > struct tpm_space *space,
> > > > rc = tpm2_commit_space(chip, space, ordinal, buf, );
> > > >
> > > >  out:
> > > > +   if (chip->dev.parent)
> > > > +   pm_runtime_put_sync(chip->dev.parent);
> > > > +
> > > > if (need_locality && chip->ops->relinquish_locality) {
> > > > -   chip->ops->relinquish_locality(chip, chip->locality);
> > > > +   /* this coud be on error path, don't override error 
> > > > code */
> > > > +   int l_rc = chip->ops->relinquish_locality(chip,
> > > > +chip->locality);
> > >
> > > Declaration should be in the beginning of the function.
> >
> > No, it shouldn't. I cannot find any reference to this statement, I've 
> > already
> explained my reasoning in a previous mail.
> >
> > >
> > > > +
> > > > +   if (l_rc) {
> > > > +   dev_err(>dev, "%s: relinquish_locality: 
> > > > error
> > > %d\n",
> > > > +   __func__, l_rc);
> > > > +   }
> > >
> > > In kernel coding style, this should be w/o curly braces.
> >
> > Yep, missed that, will resubmit
> >
> >
> > > I can fix these cosmetic issues
> > >
> > > Reviewed-by: Jarkko Sakkinen 
> > >
> > > Doesn't this need
> > >
> > > Fixes: 877c57d0d0ca ("tpm_crb: request and relinquish locality 0")
> > > And shouldn't this also have
> > >
> > > Cc: sta...@vger.kernel.org
> > >
> > > ?
> > Good points
> > Thanks
> > Tomas
> 
> Tomas, I updated v4 myself and pushed it to master/next. Please tell me if
> there is anything wrong and I will fix it.
> 
Please, take my fix v5. I didn't agree to moving l_rc out its scope. 
Thanks
Tomas

Re: [PATCH 1/2 v4] tpm: cmd_ready command can be issued only after granting locality

2018-02-25 Thread Jarkko Sakkinen 
On Sun, Feb 25, 2018 at 10:37:08AM +, Winkler, Tomas wrote:
> > 
> > On Sat, Feb 24, 2018 at 05:43:16PM +0200, Tomas Winkler wrote:
> > > The correct sequence is to first request locality and only after that
> > > perform cmd_ready handshake, otherwise the hardware will drop the
> > > subsequent message as from the device point of view the cmd_ready
> > > handshake wasn't performed. Symmetrically locality has to be
> > > relinquished only after going idle handshake has completed, this
> > > requires that go_idle has to poll for the completion and as well
> > > locality relinquish has to poll for completion so it is not overridden
> > > in back to back commands flow.
> > >
> > > The issue is only visible on devices that support multiple localities.
> > >
> > > Signed-off-by: Tomas Winkler 
> > > Tested-by: Jarkko Sakkinen 
> > > ---
> > > V2: poll for locality relinquish completion
> > > V3: 1. Print error message upon locality relinquish failure
> > > 2. Don't override rc code on error path with locality relinquish
> > > V4: 3. Don't capture locality relinquish error code in rc, just print
> > >   the error message.
> > >
> > >return value.
> > >  drivers/char/tpm/tpm-interface.c |  20 +---
> > >  drivers/char/tpm/tpm_crb.c   | 108 +++--
> > --
> > >  drivers/char/tpm/tpm_tis_core.c  |   4 +-
> > >  include/linux/tpm.h  |   2 +-
> > >  4 files changed, 93 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/drivers/char/tpm/tpm-interface.c
> > > b/drivers/char/tpm/tpm-interface.c
> > > index 9e80a953d693..4d74bacca5a1 100644
> > > --- a/drivers/char/tpm/tpm-interface.c
> > > +++ b/drivers/char/tpm/tpm-interface.c
> > > @@ -422,8 +422,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct
> > tpm_space *space,
> > >   if (!(flags & TPM_TRANSMIT_UNLOCKED))
> > >   mutex_lock(>tpm_mutex);
> > >
> > > - if (chip->dev.parent)
> > > - pm_runtime_get_sync(chip->dev.parent);
> > >
> > >   if (chip->ops->clk_enable != NULL)
> > >   chip->ops->clk_enable(chip, true);
> > > @@ -439,6 +437,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct
> > tpm_space *space,
> > >   chip->locality = rc;
> > >   }
> > >
> > > + if (chip->dev.parent)
> > > + pm_runtime_get_sync(chip->dev.parent);
> > > +
> > >   rc = tpm2_prepare_space(chip, space, ordinal, buf);
> > >   if (rc)
> > >   goto out;
> > > @@ -499,17 +500,24 @@ ssize_t tpm_transmit(struct tpm_chip *chip,
> > struct tpm_space *space,
> > >   rc = tpm2_commit_space(chip, space, ordinal, buf, );
> > >
> > >  out:
> > > + if (chip->dev.parent)
> > > + pm_runtime_put_sync(chip->dev.parent);
> > > +
> > >   if (need_locality && chip->ops->relinquish_locality) {
> > > - chip->ops->relinquish_locality(chip, chip->locality);
> > > + /* this coud be on error path, don't override error code */
> > > + int l_rc = chip->ops->relinquish_locality(chip, chip->locality);
> > 
> > Declaration should be in the beginning of the function.
> 
> No, it shouldn't. I cannot find any reference to this statement, I've already 
> explained my reasoning in a previous mail. 
>   
> > 
> > > +
> > > + if (l_rc) {
> > > + dev_err(>dev, "%s: relinquish_locality: error
> > %d\n",
> > > + __func__, l_rc);
> > > + }
> > 
> > In kernel coding style, this should be w/o curly braces.
> 
> Yep, missed that, will resubmit
> 
> 
> > I can fix these cosmetic issues
> > 
> > Reviewed-by: Jarkko Sakkinen 
> > 
> > Doesn't this need
> > 
> > Fixes: 877c57d0d0ca ("tpm_crb: request and relinquish locality 0")
> > And shouldn't this also have
> > 
> > Cc: sta...@vger.kernel.org
> > 
> > ?
> Good points
> Thanks
> Tomas

Tomas, I updated v4 myself and pushed it to master/next. Please tell
me if there is anything wrong and I will fix it.

/Jarkko

Re: [PATCH 1/2 v4] tpm: cmd_ready command can be issued only after granting locality

2018-02-25 Thread Jarkko Sakkinen 
On Sun, Feb 25, 2018 at 10:37:08AM +, Winkler, Tomas wrote:
> > 
> > On Sat, Feb 24, 2018 at 05:43:16PM +0200, Tomas Winkler wrote:
> > > The correct sequence is to first request locality and only after that
> > > perform cmd_ready handshake, otherwise the hardware will drop the
> > > subsequent message as from the device point of view the cmd_ready
> > > handshake wasn't performed. Symmetrically locality has to be
> > > relinquished only after going idle handshake has completed, this
> > > requires that go_idle has to poll for the completion and as well
> > > locality relinquish has to poll for completion so it is not overridden
> > > in back to back commands flow.
> > >
> > > The issue is only visible on devices that support multiple localities.
> > >
> > > Signed-off-by: Tomas Winkler 
> > > Tested-by: Jarkko Sakkinen 
> > > ---
> > > V2: poll for locality relinquish completion
> > > V3: 1. Print error message upon locality relinquish failure
> > > 2. Don't override rc code on error path with locality relinquish
> > > V4: 3. Don't capture locality relinquish error code in rc, just print
> > >   the error message.
> > >
> > >return value.
> > >  drivers/char/tpm/tpm-interface.c |  20 +---
> > >  drivers/char/tpm/tpm_crb.c   | 108 +++--
> > --
> > >  drivers/char/tpm/tpm_tis_core.c  |   4 +-
> > >  include/linux/tpm.h  |   2 +-
> > >  4 files changed, 93 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/drivers/char/tpm/tpm-interface.c
> > > b/drivers/char/tpm/tpm-interface.c
> > > index 9e80a953d693..4d74bacca5a1 100644
> > > --- a/drivers/char/tpm/tpm-interface.c
> > > +++ b/drivers/char/tpm/tpm-interface.c
> > > @@ -422,8 +422,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct
> > tpm_space *space,
> > >   if (!(flags & TPM_TRANSMIT_UNLOCKED))
> > >   mutex_lock(>tpm_mutex);
> > >
> > > - if (chip->dev.parent)
> > > - pm_runtime_get_sync(chip->dev.parent);
> > >
> > >   if (chip->ops->clk_enable != NULL)
> > >   chip->ops->clk_enable(chip, true);
> > > @@ -439,6 +437,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct
> > tpm_space *space,
> > >   chip->locality = rc;
> > >   }
> > >
> > > + if (chip->dev.parent)
> > > + pm_runtime_get_sync(chip->dev.parent);
> > > +
> > >   rc = tpm2_prepare_space(chip, space, ordinal, buf);
> > >   if (rc)
> > >   goto out;
> > > @@ -499,17 +500,24 @@ ssize_t tpm_transmit(struct tpm_chip *chip,
> > struct tpm_space *space,
> > >   rc = tpm2_commit_space(chip, space, ordinal, buf, );
> > >
> > >  out:
> > > + if (chip->dev.parent)
> > > + pm_runtime_put_sync(chip->dev.parent);
> > > +
> > >   if (need_locality && chip->ops->relinquish_locality) {
> > > - chip->ops->relinquish_locality(chip, chip->locality);
> > > + /* this coud be on error path, don't override error code */
> > > + int l_rc = chip->ops->relinquish_locality(chip, chip->locality);
> > 
> > Declaration should be in the beginning of the function.
> 
> No, it shouldn't. I cannot find any reference to this statement, I've already 
> explained my reasoning in a previous mail. 
>   
> > 
> > > +
> > > + if (l_rc) {
> > > + dev_err(>dev, "%s: relinquish_locality: error
> > %d\n",
> > > + __func__, l_rc);
> > > + }
> > 
> > In kernel coding style, this should be w/o curly braces.
> 
> Yep, missed that, will resubmit
> 
> 
> > I can fix these cosmetic issues
> > 
> > Reviewed-by: Jarkko Sakkinen 
> > 
> > Doesn't this need
> > 
> > Fixes: 877c57d0d0ca ("tpm_crb: request and relinquish locality 0")
> > And shouldn't this also have
> > 
> > Cc: sta...@vger.kernel.org
> > 
> > ?
> Good points
> Thanks
> Tomas

Tomas, I updated v4 myself and pushed it to master/next. Please tell
me if there is anything wrong and I will fix it.

/Jarkko

RE: [PATCH 1/2 v4] tpm: cmd_ready command can be issued only after granting locality

2018-02-25 Thread Winkler, Tomas 
> 
> On Sat, Feb 24, 2018 at 05:43:16PM +0200, Tomas Winkler wrote:
> > The correct sequence is to first request locality and only after that
> > perform cmd_ready handshake, otherwise the hardware will drop the
> > subsequent message as from the device point of view the cmd_ready
> > handshake wasn't performed. Symmetrically locality has to be
> > relinquished only after going idle handshake has completed, this
> > requires that go_idle has to poll for the completion and as well
> > locality relinquish has to poll for completion so it is not overridden
> > in back to back commands flow.
> >
> > The issue is only visible on devices that support multiple localities.
> >
> > Signed-off-by: Tomas Winkler 
> > Tested-by: Jarkko Sakkinen 
> > ---
> > V2: poll for locality relinquish completion
> > V3: 1. Print error message upon locality relinquish failure
> > 2. Don't override rc code on error path with locality relinquish
> > V4: 3. Don't capture locality relinquish error code in rc, just print
> > the error message.
> >
> >return value.
> >  drivers/char/tpm/tpm-interface.c |  20 +---
> >  drivers/char/tpm/tpm_crb.c   | 108 +++--
> --
> >  drivers/char/tpm/tpm_tis_core.c  |   4 +-
> >  include/linux/tpm.h  |   2 +-
> >  4 files changed, 93 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-interface.c
> > b/drivers/char/tpm/tpm-interface.c
> > index 9e80a953d693..4d74bacca5a1 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -422,8 +422,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct
> tpm_space *space,
> > if (!(flags & TPM_TRANSMIT_UNLOCKED))
> > mutex_lock(>tpm_mutex);
> >
> > -   if (chip->dev.parent)
> > -   pm_runtime_get_sync(chip->dev.parent);
> >
> > if (chip->ops->clk_enable != NULL)
> > chip->ops->clk_enable(chip, true);
> > @@ -439,6 +437,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct
> tpm_space *space,
> > chip->locality = rc;
> > }
> >
> > +   if (chip->dev.parent)
> > +   pm_runtime_get_sync(chip->dev.parent);
> > +
> > rc = tpm2_prepare_space(chip, space, ordinal, buf);
> > if (rc)
> > goto out;
> > @@ -499,17 +500,24 @@ ssize_t tpm_transmit(struct tpm_chip *chip,
> struct tpm_space *space,
> > rc = tpm2_commit_space(chip, space, ordinal, buf, );
> >
> >  out:
> > +   if (chip->dev.parent)
> > +   pm_runtime_put_sync(chip->dev.parent);
> > +
> > if (need_locality && chip->ops->relinquish_locality) {
> > -   chip->ops->relinquish_locality(chip, chip->locality);
> > +   /* this coud be on error path, don't override error code */
> > +   int l_rc = chip->ops->relinquish_locality(chip, chip->locality);
> 
> Declaration should be in the beginning of the function.

No, it shouldn't. I cannot find any reference to this statement, I've already 
explained my reasoning in a previous mail. 
  
> 
> > +
> > +   if (l_rc) {
> > +   dev_err(>dev, "%s: relinquish_locality: error
> %d\n",
> > +   __func__, l_rc);
> > +   }
> 
> In kernel coding style, this should be w/o curly braces.

Yep, missed that, will resubmit


> I can fix these cosmetic issues
> 
> Reviewed-by: Jarkko Sakkinen 
> 
> Doesn't this need
> 
> Fixes: 877c57d0d0ca ("tpm_crb: request and relinquish locality 0")
> And shouldn't this also have
> 
> Cc: sta...@vger.kernel.org
> 
> ?
Good points
Thanks
Tomas

RE: [PATCH 1/2 v4] tpm: cmd_ready command can be issued only after granting locality

2018-02-25 Thread Winkler, Tomas 
> 
> On Sat, Feb 24, 2018 at 05:43:16PM +0200, Tomas Winkler wrote:
> > The correct sequence is to first request locality and only after that
> > perform cmd_ready handshake, otherwise the hardware will drop the
> > subsequent message as from the device point of view the cmd_ready
> > handshake wasn't performed. Symmetrically locality has to be
> > relinquished only after going idle handshake has completed, this
> > requires that go_idle has to poll for the completion and as well
> > locality relinquish has to poll for completion so it is not overridden
> > in back to back commands flow.
> >
> > The issue is only visible on devices that support multiple localities.
> >
> > Signed-off-by: Tomas Winkler 
> > Tested-by: Jarkko Sakkinen 
> > ---
> > V2: poll for locality relinquish completion
> > V3: 1. Print error message upon locality relinquish failure
> > 2. Don't override rc code on error path with locality relinquish
> > V4: 3. Don't capture locality relinquish error code in rc, just print
> > the error message.
> >
> >return value.
> >  drivers/char/tpm/tpm-interface.c |  20 +---
> >  drivers/char/tpm/tpm_crb.c   | 108 +++--
> --
> >  drivers/char/tpm/tpm_tis_core.c  |   4 +-
> >  include/linux/tpm.h  |   2 +-
> >  4 files changed, 93 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-interface.c
> > b/drivers/char/tpm/tpm-interface.c
> > index 9e80a953d693..4d74bacca5a1 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -422,8 +422,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct
> tpm_space *space,
> > if (!(flags & TPM_TRANSMIT_UNLOCKED))
> > mutex_lock(>tpm_mutex);
> >
> > -   if (chip->dev.parent)
> > -   pm_runtime_get_sync(chip->dev.parent);
> >
> > if (chip->ops->clk_enable != NULL)
> > chip->ops->clk_enable(chip, true);
> > @@ -439,6 +437,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct
> tpm_space *space,
> > chip->locality = rc;
> > }
> >
> > +   if (chip->dev.parent)
> > +   pm_runtime_get_sync(chip->dev.parent);
> > +
> > rc = tpm2_prepare_space(chip, space, ordinal, buf);
> > if (rc)
> > goto out;
> > @@ -499,17 +500,24 @@ ssize_t tpm_transmit(struct tpm_chip *chip,
> struct tpm_space *space,
> > rc = tpm2_commit_space(chip, space, ordinal, buf, );
> >
> >  out:
> > +   if (chip->dev.parent)
> > +   pm_runtime_put_sync(chip->dev.parent);
> > +
> > if (need_locality && chip->ops->relinquish_locality) {
> > -   chip->ops->relinquish_locality(chip, chip->locality);
> > +   /* this coud be on error path, don't override error code */
> > +   int l_rc = chip->ops->relinquish_locality(chip, chip->locality);
> 
> Declaration should be in the beginning of the function.

No, it shouldn't. I cannot find any reference to this statement, I've already 
explained my reasoning in a previous mail. 
  
> 
> > +
> > +   if (l_rc) {
> > +   dev_err(>dev, "%s: relinquish_locality: error
> %d\n",
> > +   __func__, l_rc);
> > +   }
> 
> In kernel coding style, this should be w/o curly braces.

Yep, missed that, will resubmit


> I can fix these cosmetic issues
> 
> Reviewed-by: Jarkko Sakkinen 
> 
> Doesn't this need
> 
> Fixes: 877c57d0d0ca ("tpm_crb: request and relinquish locality 0")
> And shouldn't this also have
> 
> Cc: sta...@vger.kernel.org
> 
> ?
Good points
Thanks
Tomas

Re: [PATCH 1/2 v4] tpm: cmd_ready command can be issued only after granting locality

2018-02-25 Thread Jarkko Sakkinen 
On Sat, Feb 24, 2018 at 05:43:16PM +0200, Tomas Winkler wrote:
> The correct sequence is to first request locality and only after
> that perform cmd_ready handshake, otherwise the hardware will drop
> the subsequent message as from the device point of view the cmd_ready
> handshake wasn't performed. Symmetrically locality has to be relinquished
> only after going idle handshake has completed, this requires that
> go_idle has to poll for the completion and as well locality
> relinquish has to poll for completion so it is not overridden
> in back to back commands flow.
> 
> The issue is only visible on devices that support multiple localities.
> 
> Signed-off-by: Tomas Winkler 
> Tested-by: Jarkko Sakkinen 
> ---
> V2: poll for locality relinquish completion
> V3: 1. Print error message upon locality relinquish failure
> 2. Don't override rc code on error path with locality relinquish
> V4: 3. Don't capture locality relinquish error code in rc, just print
>   the error message.
> 
>return value.
>  drivers/char/tpm/tpm-interface.c |  20 +---
>  drivers/char/tpm/tpm_crb.c   | 108 
> +++
>  drivers/char/tpm/tpm_tis_core.c  |   4 +-
>  include/linux/tpm.h  |   2 +-
>  4 files changed, 93 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c 
> b/drivers/char/tpm/tpm-interface.c
> index 9e80a953d693..4d74bacca5a1 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -422,8 +422,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct 
> tpm_space *space,
>   if (!(flags & TPM_TRANSMIT_UNLOCKED))
>   mutex_lock(>tpm_mutex);
>  
> - if (chip->dev.parent)
> - pm_runtime_get_sync(chip->dev.parent);
>  
>   if (chip->ops->clk_enable != NULL)
>   chip->ops->clk_enable(chip, true);
> @@ -439,6 +437,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct 
> tpm_space *space,
>   chip->locality = rc;
>   }
>  
> + if (chip->dev.parent)
> + pm_runtime_get_sync(chip->dev.parent);
> +
>   rc = tpm2_prepare_space(chip, space, ordinal, buf);
>   if (rc)
>   goto out;
> @@ -499,17 +500,24 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct 
> tpm_space *space,
>   rc = tpm2_commit_space(chip, space, ordinal, buf, );
>  
>  out:
> + if (chip->dev.parent)
> + pm_runtime_put_sync(chip->dev.parent);
> +
>   if (need_locality && chip->ops->relinquish_locality) {
> - chip->ops->relinquish_locality(chip, chip->locality);
> + /* this coud be on error path, don't override error code */
> + int l_rc = chip->ops->relinquish_locality(chip, chip->locality);

Declaration should be in the beginning of the function.

> +
> + if (l_rc) {
> + dev_err(>dev, "%s: relinquish_locality: error 
> %d\n",
> + __func__, l_rc);
> + }

In kernel coding style, this should be w/o curly braces.

I can fix these cosmetic issues

Reviewed-by: Jarkko Sakkinen 

Doesn't this need

Fixes: 877c57d0d0ca ("tpm_crb: request and relinquish locality 0")

And shouldn't this also have

Cc: sta...@vger.kernel.org

?

/Jarkko

Re: [PATCH 1/2 v4] tpm: cmd_ready command can be issued only after granting locality

2018-02-25 Thread Jarkko Sakkinen 
On Sat, Feb 24, 2018 at 05:43:16PM +0200, Tomas Winkler wrote:
> The correct sequence is to first request locality and only after
> that perform cmd_ready handshake, otherwise the hardware will drop
> the subsequent message as from the device point of view the cmd_ready
> handshake wasn't performed. Symmetrically locality has to be relinquished
> only after going idle handshake has completed, this requires that
> go_idle has to poll for the completion and as well locality
> relinquish has to poll for completion so it is not overridden
> in back to back commands flow.
> 
> The issue is only visible on devices that support multiple localities.
> 
> Signed-off-by: Tomas Winkler 
> Tested-by: Jarkko Sakkinen 
> ---
> V2: poll for locality relinquish completion
> V3: 1. Print error message upon locality relinquish failure
> 2. Don't override rc code on error path with locality relinquish
> V4: 3. Don't capture locality relinquish error code in rc, just print
>   the error message.
> 
>return value.
>  drivers/char/tpm/tpm-interface.c |  20 +---
>  drivers/char/tpm/tpm_crb.c   | 108 
> +++
>  drivers/char/tpm/tpm_tis_core.c  |   4 +-
>  include/linux/tpm.h  |   2 +-
>  4 files changed, 93 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c 
> b/drivers/char/tpm/tpm-interface.c
> index 9e80a953d693..4d74bacca5a1 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -422,8 +422,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct 
> tpm_space *space,
>   if (!(flags & TPM_TRANSMIT_UNLOCKED))
>   mutex_lock(>tpm_mutex);
>  
> - if (chip->dev.parent)
> - pm_runtime_get_sync(chip->dev.parent);
>  
>   if (chip->ops->clk_enable != NULL)
>   chip->ops->clk_enable(chip, true);
> @@ -439,6 +437,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct 
> tpm_space *space,
>   chip->locality = rc;
>   }
>  
> + if (chip->dev.parent)
> + pm_runtime_get_sync(chip->dev.parent);
> +
>   rc = tpm2_prepare_space(chip, space, ordinal, buf);
>   if (rc)
>   goto out;
> @@ -499,17 +500,24 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct 
> tpm_space *space,
>   rc = tpm2_commit_space(chip, space, ordinal, buf, );
>  
>  out:
> + if (chip->dev.parent)
> + pm_runtime_put_sync(chip->dev.parent);
> +
>   if (need_locality && chip->ops->relinquish_locality) {
> - chip->ops->relinquish_locality(chip, chip->locality);
> + /* this coud be on error path, don't override error code */
> + int l_rc = chip->ops->relinquish_locality(chip, chip->locality);

Declaration should be in the beginning of the function.

> +
> + if (l_rc) {
> + dev_err(>dev, "%s: relinquish_locality: error 
> %d\n",
> + __func__, l_rc);
> + }

In kernel coding style, this should be w/o curly braces.

I can fix these cosmetic issues

Reviewed-by: Jarkko Sakkinen 

Doesn't this need

Fixes: 877c57d0d0ca ("tpm_crb: request and relinquish locality 0")

And shouldn't this also have

Cc: sta...@vger.kernel.org

?

/Jarkko

[PATCH 1/2 v4] tpm: cmd_ready command can be issued only after granting locality

2018-02-24 Thread Tomas Winkler 
The correct sequence is to first request locality and only after
that perform cmd_ready handshake, otherwise the hardware will drop
the subsequent message as from the device point of view the cmd_ready
handshake wasn't performed. Symmetrically locality has to be relinquished
only after going idle handshake has completed, this requires that
go_idle has to poll for the completion and as well locality
relinquish has to poll for completion so it is not overridden
in back to back commands flow.

The issue is only visible on devices that support multiple localities.

Signed-off-by: Tomas Winkler 
Tested-by: Jarkko Sakkinen 
---
V2: poll for locality relinquish completion
V3: 1. Print error message upon locality relinquish failure
2. Don't override rc code on error path with locality relinquish
V4: 3. Don't capture locality relinquish error code in rc, just print
the error message.

   return value.
 drivers/char/tpm/tpm-interface.c |  20 +---
 drivers/char/tpm/tpm_crb.c   | 108 +++
 drivers/char/tpm/tpm_tis_core.c  |   4 +-
 include/linux/tpm.h  |   2 +-
 4 files changed, 93 insertions(+), 41 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 9e80a953d693..4d74bacca5a1 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -422,8 +422,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct 
tpm_space *space,
if (!(flags & TPM_TRANSMIT_UNLOCKED))
mutex_lock(>tpm_mutex);
 
-   if (chip->dev.parent)
-   pm_runtime_get_sync(chip->dev.parent);
 
if (chip->ops->clk_enable != NULL)
chip->ops->clk_enable(chip, true);
@@ -439,6 +437,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct 
tpm_space *space,
chip->locality = rc;
}
 
+   if (chip->dev.parent)
+   pm_runtime_get_sync(chip->dev.parent);
+
rc = tpm2_prepare_space(chip, space, ordinal, buf);
if (rc)
goto out;
@@ -499,17 +500,24 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct 
tpm_space *space,
rc = tpm2_commit_space(chip, space, ordinal, buf, );
 
 out:
+   if (chip->dev.parent)
+   pm_runtime_put_sync(chip->dev.parent);
+
if (need_locality && chip->ops->relinquish_locality) {
-   chip->ops->relinquish_locality(chip, chip->locality);
+   /* this coud be on error path, don't override error code */
+   int l_rc = chip->ops->relinquish_locality(chip, chip->locality);
+
+   if (l_rc) {
+   dev_err(>dev, "%s: relinquish_locality: error 
%d\n",
+   __func__, l_rc);
+   }
chip->locality = -1;
}
+
 out_no_locality:
if (chip->ops->clk_enable != NULL)
chip->ops->clk_enable(chip, false);
 
-   if (chip->dev.parent)
-   pm_runtime_put_sync(chip->dev.parent);
-
if (!(flags & TPM_TRANSMIT_UNLOCKED))
mutex_unlock(>tpm_mutex);
return rc ? rc : len;
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 7b3c2a8aa9de..497edd9848cd 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -112,6 +112,25 @@ struct tpm2_crb_smc {
u32 smc_func_id;
 };
 
+static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
+   unsigned long timeout)
+{
+   ktime_t start;
+   ktime_t stop;
+
+   start = ktime_get();
+   stop = ktime_add(start, ms_to_ktime(timeout));
+
+   do {
+   if ((ioread32(reg) & mask) == value)
+   return true;
+
+   usleep_range(50, 100);
+   } while (ktime_before(ktime_get(), stop));
+
+   return ((ioread32(reg) & mask) == value);
+}
+
 /**
  * crb_go_idle - request tpm crb device to go the idle state
  *
@@ -128,7 +147,7 @@ struct tpm2_crb_smc {
  *
  * Return: 0 always
  */
-static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv 
*priv)
+static int crb_go_idle(struct device *dev, struct crb_priv *priv)
 {
if ((priv->sm == ACPI_TPM2_START_METHOD) ||
(priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
@@ -136,30 +155,17 @@ static int __maybe_unused crb_go_idle(struct device *dev, 
struct crb_priv *priv)
return 0;
 
iowrite32(CRB_CTRL_REQ_GO_IDLE, >regs_t->ctrl_req);
-   /* we don't really care when this settles */
 
+   if (!crb_wait_for_reg_32(>regs_t->ctrl_req,
+CRB_CTRL_REQ_GO_IDLE/* mask */,
+0, /* value */
+TPM2_TIMEOUT_C)) {
+   dev_warn(dev, "goIdle timed out\n");
+   return -ETIME;
+   }
return 0;
 }
 
-static bool

[PATCH 1/2 v4] tpm: cmd_ready command can be issued only after granting locality

2018-02-24 Thread Tomas Winkler 
The correct sequence is to first request locality and only after
that perform cmd_ready handshake, otherwise the hardware will drop
the subsequent message as from the device point of view the cmd_ready
handshake wasn't performed. Symmetrically locality has to be relinquished
only after going idle handshake has completed, this requires that
go_idle has to poll for the completion and as well locality
relinquish has to poll for completion so it is not overridden
in back to back commands flow.

The issue is only visible on devices that support multiple localities.

Signed-off-by: Tomas Winkler 
Tested-by: Jarkko Sakkinen 
---
V2: poll for locality relinquish completion
V3: 1. Print error message upon locality relinquish failure
2. Don't override rc code on error path with locality relinquish
V4: 3. Don't capture locality relinquish error code in rc, just print
the error message.

   return value.
 drivers/char/tpm/tpm-interface.c |  20 +---
 drivers/char/tpm/tpm_crb.c   | 108 +++
 drivers/char/tpm/tpm_tis_core.c  |   4 +-
 include/linux/tpm.h  |   2 +-
 4 files changed, 93 insertions(+), 41 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 9e80a953d693..4d74bacca5a1 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -422,8 +422,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct 
tpm_space *space,
if (!(flags & TPM_TRANSMIT_UNLOCKED))
mutex_lock(>tpm_mutex);
 
-   if (chip->dev.parent)
-   pm_runtime_get_sync(chip->dev.parent);
 
if (chip->ops->clk_enable != NULL)
chip->ops->clk_enable(chip, true);
@@ -439,6 +437,9 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct 
tpm_space *space,
chip->locality = rc;
}
 
+   if (chip->dev.parent)
+   pm_runtime_get_sync(chip->dev.parent);
+
rc = tpm2_prepare_space(chip, space, ordinal, buf);
if (rc)
goto out;
@@ -499,17 +500,24 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct 
tpm_space *space,
rc = tpm2_commit_space(chip, space, ordinal, buf, );
 
 out:
+   if (chip->dev.parent)
+   pm_runtime_put_sync(chip->dev.parent);
+
if (need_locality && chip->ops->relinquish_locality) {
-   chip->ops->relinquish_locality(chip, chip->locality);
+   /* this coud be on error path, don't override error code */
+   int l_rc = chip->ops->relinquish_locality(chip, chip->locality);
+
+   if (l_rc) {
+   dev_err(>dev, "%s: relinquish_locality: error 
%d\n",
+   __func__, l_rc);
+   }
chip->locality = -1;
}
+
 out_no_locality:
if (chip->ops->clk_enable != NULL)
chip->ops->clk_enable(chip, false);
 
-   if (chip->dev.parent)
-   pm_runtime_put_sync(chip->dev.parent);
-
if (!(flags & TPM_TRANSMIT_UNLOCKED))
mutex_unlock(>tpm_mutex);
return rc ? rc : len;
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 7b3c2a8aa9de..497edd9848cd 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -112,6 +112,25 @@ struct tpm2_crb_smc {
u32 smc_func_id;
 };
 
+static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
+   unsigned long timeout)
+{
+   ktime_t start;
+   ktime_t stop;
+
+   start = ktime_get();
+   stop = ktime_add(start, ms_to_ktime(timeout));
+
+   do {
+   if ((ioread32(reg) & mask) == value)
+   return true;
+
+   usleep_range(50, 100);
+   } while (ktime_before(ktime_get(), stop));
+
+   return ((ioread32(reg) & mask) == value);
+}
+
 /**
  * crb_go_idle - request tpm crb device to go the idle state
  *
@@ -128,7 +147,7 @@ struct tpm2_crb_smc {
  *
  * Return: 0 always
  */
-static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv 
*priv)
+static int crb_go_idle(struct device *dev, struct crb_priv *priv)
 {
if ((priv->sm == ACPI_TPM2_START_METHOD) ||
(priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
@@ -136,30 +155,17 @@ static int __maybe_unused crb_go_idle(struct device *dev, 
struct crb_priv *priv)
return 0;
 
iowrite32(CRB_CTRL_REQ_GO_IDLE, >regs_t->ctrl_req);
-   /* we don't really care when this settles */
 
+   if (!crb_wait_for_reg_32(>regs_t->ctrl_req,
+CRB_CTRL_REQ_GO_IDLE/* mask */,
+0, /* value */
+TPM2_TIMEOUT_C)) {
+   dev_warn(dev, "goIdle timed out\n");
+   return -ETIME;
+   }
return 0;
 }
 
-static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
-