diff mbox

[tpmdd-devel,v6,1/2] tpm: Issue a TPM2_Shutdown for TPM2 devices.

Message ID 20170616172531.28464-1-joshz@google.com
State New
Headers show

Commit Message

Josh Zimmerman via tpmdd-devel June 16, 2017, 5:25 p.m. UTC
If a TPM2 loses power without a TPM2_Shutdown command being issued (a
"disorderly reboot"), it may lose some state that has yet to be
persisted to NVRam, and will increment the DA counter. After the DA
counter gets sufficiently large, the TPM will lock the user out.

NOTE: This only changes behavior on TPM2 devices. Since TPM1 uses sysfs,
and sysfs relies on implicit locking on chip->ops, it is not safe to
allow this code to run in TPM1, or to add sysfs support to TPM2, until
that locking is made explicit.

Signed-off-by: Josh Zimmerman <joshz@google.com>
Cc: stable@vger.kernel.org

----
v2:
  - Properly split changes between this and another commit
  - Use proper locking primitive.
  - Fix commenting style
v3:
  - Re-fix commenting style
v4:
  - Update description and tags (Reviewed-by, Cc).
v5:
  - Update documentation.
v6:
  - Call device and/or bus shutdown from tpm_shutdown.
---
 drivers/char/tpm/tpm-chip.c  | 31 +++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm-sysfs.c |  3 +++
 2 files changed, 34 insertions(+)

Comments

Jarkko Sakkinen June 18, 2017, 9:53 p.m. UTC | #1
On Fri, 2017-06-16 at 10:25 -0700, Josh Zimmerman wrote:
> If a TPM2 loses power without a TPM2_Shutdown command being issued (a
> "disorderly reboot"), it may lose some state that has yet to be
> persisted to NVRam, and will increment the DA counter. After the DA
> counter gets sufficiently large, the TPM will lock the user out.

> NOTE: This only changes behavior on TPM2 devices. Since TPM1 uses
sysfs,
> and sysfs relies on implicit locking on chip->ops, it is not safe to
> allow this code to run in TPM1, or to add sysfs support to TPM2,
until
> that locking is made explicit.

> Signed-off-by: Josh Zimmerman <joshz@google.com>
> Cc: stable@vger.kernel.org

> ----
> v2:
>   - Properly split changes between this and another commit
>   - Use proper locking primitive.
>   - Fix commenting style
> v3:
>   - Re-fix commenting style
> v4:
>   - Update description and tags (Reviewed-by, Cc).
> v5:
>   - Update documentation.
> v6:
>   - Call device and/or bus shutdown from tpm_shutdown.

The change log should be after the dashes *below* right before diff
stat. Since this is 1/2 I should be abe to assume that I could apply on
this and not apply 2/2.

Why this patch set does not have a cover letter?

Anyway, I applied these to my master and if they don't break anything
I'm including them to my 4.13 pull request. I left the commit messages
to the form that they turn out if I just git am -3 the mbox files.

Next time, do a git am sanity check if you have changelogs in your
patches.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jarkko Sakkinen June 18, 2017, 11:21 p.m. UTC | #2
Hold on.

On Fri, 2017-06-16 at 10:25 -0700, Josh Zimmerman wrote:
> If a TPM2 loses power without a TPM2_Shutdown command being issued (a
> "disorderly reboot"), it may lose some state that has yet to be
> persisted to NVRam, and will increment the DA counter. After the DA
> counter gets sufficiently large, the TPM will lock the user out.

> NOTE: This only changes behavior on TPM2 devices. Since TPM1 uses
sysfs,
> and sysfs relies on implicit locking on chip->ops, it is not safe to
> allow this code to run in TPM1, or to add sysfs support to TPM2,
until
> that locking is made explicit.

> Signed-off-by: Josh Zimmerman <joshz@google.com>
> Cc: stable@vger.kernel.org

> ----
> v2:
>   - Properly split changes between this and another commit
>   - Use proper locking primitive.
>   - Fix commenting style
> v3:
>   - Re-fix commenting style
> v4:
>   - Update description and tags (Reviewed-by, Cc).
> v5:
>   - Update documentation.
> v6:
>   - Call device and/or bus shutdown from tpm_shutdown.
> ---
>  drivers/char/tpm/tpm-chip.c  | 31 +++++++++++++++++++++++++++++++
>  drivers/char/tpm/tpm-sysfs.c |  3 +++
>  2 files changed, 34 insertions(+)

> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-
chip.c
> index 9dec9f551b83..62691d266f22 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -142,6 +142,36 @@ static void tpm_devs_release(struct device *dev)
>  	put_device(&chip->dev);
>  }
>  
> +/**
> + * tpm_shutdown() - prepare the TPM device for loss of power.
> + * @dev: device to which the chip is associated.
> + *
> + * Issues a TPM2_Shutdown command prior to loss of power, as
required by the
> + * TPM 2.0 spec.
> + *
> + * XXX: This codepath relies on the fact that sysfs is not enabled
for
> + * TPM2: sysfs uses an implicit lock on chip->ops, so this could
race if TPM2
> + * has sysfs support enabled before TPM sysfs's implicit locking is
fixed.
> + */
> +static void tpm_shutdown(struct device *dev)
> +{
> +	struct tpm_chip *chip = container_of(dev, struct tpm_chip,
dev);
> +
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +		down_write(&chip->ops_sem);
> +		tpm2_shutdown(chip, TPM_SU_CLEAR);
> +		chip->ops = NULL;
> +		up_write(&chip->ops_sem);
> +	}
> +	// Allow bus- and device-specific code to run. Note: since
chip->ops
> +	// is NULL, more-specific shutdown code will not be able to
issue TPM
> +	// commands.
> +	if (dev->bus->shutdown)
> +		dev->bus->shutdown(dev);
> +	else if (dev->driver && dev->driver->shutdown)
> +		dev->driver->shutdown(dev);

WTF is this part.

And why we have now duplicate code??

I'm dropping these patches for now from my master branch. I don't
understand what is going on...

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jarkko Sakkinen June 18, 2017, 11:26 p.m. UTC | #3
On Mon, 2017-06-19 at 01:21 +0200, Jarkko Sakkinen wrote:
> Hold on.
> 
> On Fri, 2017-06-16 at 10:25 -0700, Josh Zimmerman wrote:
> > If a TPM2 loses power without a TPM2_Shutdown command being issued
> > (a
> > "disorderly reboot"), it may lose some state that has yet to be
> > persisted to NVRam, and will increment the DA counter. After the DA
> > counter gets sufficiently large, the TPM will lock the user out.
> >  
> > NOTE: This only changes behavior on TPM2 devices. Since TPM1 uses
> 
> sysfs,
> > and sysfs relies on implicit locking on chip->ops, it is not safe
> > to
> > allow this code to run in TPM1, or to add sysfs support to TPM2,
> 
> until
> > that locking is made explicit.
> >  
> > Signed-off-by: Josh Zimmerman <joshz@google.com>
> > Cc: stable@vger.kernel.org
> >  
> > ----
> > v2:
> >    - Properly split changes between this and another commit
> >    - Use proper locking primitive.
> >    - Fix commenting style
> > v3:
> >    - Re-fix commenting style
> > v4:
> >    - Update description and tags (Reviewed-by, Cc).
> > v5:
> >    - Update documentation.
> > v6:
> >    - Call device and/or bus shutdown from tpm_shutdown.
> > ---
> >   drivers/char/tpm/tpm-chip.c  | 31 +++++++++++++++++++++++++++++++
> >   drivers/char/tpm/tpm-sysfs.c |  3 +++
> >   2 files changed, 34 insertions(+)
> >  
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-
> 
> chip.c
> > index 9dec9f551b83..62691d266f22 100644
> > --- a/drivers/char/tpm/tpm-chip.c
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -142,6 +142,36 @@ static void tpm_devs_release(struct device
> > *dev)
> >   	put_device(&chip->dev);
> >   }
> >   
> > +/**
> > + * tpm_shutdown() - prepare the TPM device for loss of power.
> > + * @dev: device to which the chip is associated.
> > + *
> > + * Issues a TPM2_Shutdown command prior to loss of power, as
> 
> required by the
> > + * TPM 2.0 spec.
> > + *
> > + * XXX: This codepath relies on the fact that sysfs is not enabled
> 
> for
> > + * TPM2: sysfs uses an implicit lock on chip->ops, so this could
> 
> race if TPM2
> > + * has sysfs support enabled before TPM sysfs's implicit locking
> > is
> 
> fixed.
> > + */
> > +static void tpm_shutdown(struct device *dev)
> > +{
> > +	struct tpm_chip *chip = container_of(dev, struct tpm_chip,
> 
> dev);
> > +
> > +	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > +		down_write(&chip->ops_sem);
> > +		tpm2_shutdown(chip, TPM_SU_CLEAR);
> > +		chip->ops = NULL;
> > +		up_write(&chip->ops_sem);
> > +	}
> > +	// Allow bus- and device-specific code to run. Note: since
> 
> chip->ops
> > +	// is NULL, more-specific shutdown code will not be able
> > to
> 
> issue TPM
> > +	// commands.
> > +	if (dev->bus->shutdown)
> > +		dev->bus->shutdown(dev);
> > +	else if (dev->driver && dev->driver->shutdown)
> > +		dev->driver->shutdown(dev);
> 
> WTF is this part.
> 
> And why we have now duplicate code??
> 
> I'm dropping these patches for now from my master branch. I don't
> understand what is going on...
> 
> /Jarkko


This series is too broken to be merged :-( I expect cover letter for
the next version...

Feels weird that you have to call framework functions like that in the
driver. You must have brilliant reason to do so and that should be very
well documented in the code. This is terrible...

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jason Gunthorpe June 19, 2017, 3:51 p.m. UTC | #4
On Mon, Jun 19, 2017 at 01:26:47AM +0200, Jarkko Sakkinen wrote:
 
> Feels weird that you have to call framework functions like that in the
> driver. You must have brilliant reason to do so and that should be very
> well documented in the code. This is terrible...

This was all discussed on the list. It the way these callbacks work,
the higher levels in the callback stack call the lower levels, this
allows each level the place the next level's callback properly, eg do
things before/after as necessary.

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jarkko Sakkinen June 19, 2017, 10:12 p.m. UTC | #5
On Mon, Jun 19, 2017 at 09:51:22AM -0600, Jason Gunthorpe wrote:
> On Mon, Jun 19, 2017 at 01:26:47AM +0200, Jarkko Sakkinen wrote:
>  
> > Feels weird that you have to call framework functions like that in the
> > driver. You must have brilliant reason to do so and that should be very
> > well documented in the code. This is terrible...
> 
> This was all discussed on the list. It the way these callbacks work,
> the higher levels in the callback stack call the lower levels, this
> allows each level the place the next level's callback properly, eg do
> things before/after as necessary.
> 
> Jason

I tried to look up for discussion from the patchwork. These had appeared
in v6. I guess I have to backtrack the discussions from my maidir
because I honestly don't understand why class shutdown would have to
call bus callback explicitly. There's nothing in the commit message
about this nor  is there any comment in the code.

This must be fairly recent development that I've missed?

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jarkko Sakkinen June 19, 2017, 10:21 p.m. UTC | #6
On Tue, Jun 20, 2017 at 12:12:20AM +0200, Jarkko Sakkinen wrote:
> On Mon, Jun 19, 2017 at 09:51:22AM -0600, Jason Gunthorpe wrote:
> > On Mon, Jun 19, 2017 at 01:26:47AM +0200, Jarkko Sakkinen wrote:
> >  
> > > Feels weird that you have to call framework functions like that in the
> > > driver. You must have brilliant reason to do so and that should be very
> > > well documented in the code. This is terrible...
> > 
> > This was all discussed on the list. It the way these callbacks work,
> > the higher levels in the callback stack call the lower levels, this
> > allows each level the place the next level's callback properly, eg do
> > things before/after as necessary.
> > 
> > Jason
> 
> I tried to look up for discussion from the patchwork. These had appeared
> in v6. I guess I have to backtrack the discussions from my maidir
> because I honestly don't understand why class shutdown would have to
> call bus callback explicitly. There's nothing in the commit message
> about this nor  is there any comment in the code.
> 
> This must be fairly recent development that I've missed?
> 
> /Jarkko

Found it:

"Looking at this closer, now you definately have to change the TPM
patch to call through to the other shutdown methods. We can say
current TPM drivers have no driver->shutdown, but we cannot be sure
about the bus->shutdown, so may as well call both from tpm's
class->shutdown.

I would say this should be done after the tpm2_shutdown completes as
lower level shutdowns could remove register access.

Jason"

And makes sense.

This patch is a NAK for two reasons:

1. No comment explaining this.
2. Patches are broken and they are in wrong order and cover letter is
   missing

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jarkko Sakkinen June 19, 2017, 10:38 p.m. UTC | #7
On Tue, Jun 20, 2017 at 12:21:22AM +0200, Jarkko Sakkinen wrote:
> On Tue, Jun 20, 2017 at 12:12:20AM +0200, Jarkko Sakkinen wrote:
> > On Mon, Jun 19, 2017 at 09:51:22AM -0600, Jason Gunthorpe wrote:
> > > On Mon, Jun 19, 2017 at 01:26:47AM +0200, Jarkko Sakkinen wrote:
> > >  
> > > > Feels weird that you have to call framework functions like that in the
> > > > driver. You must have brilliant reason to do so and that should be very
> > > > well documented in the code. This is terrible...
> > > 
> > > This was all discussed on the list. It the way these callbacks work,
> > > the higher levels in the callback stack call the lower levels, this
> > > allows each level the place the next level's callback properly, eg do
> > > things before/after as necessary.
> > > 
> > > Jason
> > 
> > I tried to look up for discussion from the patchwork. These had appeared
> > in v6. I guess I have to backtrack the discussions from my maidir
> > because I honestly don't understand why class shutdown would have to
> > call bus callback explicitly. There's nothing in the commit message
> > about this nor  is there any comment in the code.
> > 
> > This must be fairly recent development that I've missed?
> > 
> > /Jarkko
> 
> Found it:
> 
> "Looking at this closer, now you definately have to change the TPM
> patch to call through to the other shutdown methods. We can say
> current TPM drivers have no driver->shutdown, but we cannot be sure
> about the bus->shutdown, so may as well call both from tpm's
> class->shutdown.
> 
> I would say this should be done after the tpm2_shutdown completes as
> lower level shutdowns could remove register access.
> 
> Jason"
> 
> And makes sense.
> 
> This patch is a NAK for two reasons:
> 
> 1. No comment explaining this.
> 2. Patches are broken and they are in wrong order and cover letter is
>    missing
> 
> /Jarkko

I *tried* to apply them myself after sending this in order to be helpful
but they have compilation errors:

drivers/char/tpm/tpm-chip.c: In function ‘tpm_shutdown’:
drivers/char/tpm/tpm-chip.c:162:23: error: ‘TPM_SU_CLEAR’ undeclared (first use in this function)
   tpm2_shutdown(chip, TPM_SU_CLEAR);
                       ^~~~~~~~~~~~
drivers/char/tpm/tpm-chip.c:162:23: note: each undeclared identifier is reported only once for each function it appears in
drivers/char/tpm/tpm-chip.c: In function ‘tpm_chip_alloc’:
drivers/char/tpm/tpm-chip.c:214:17: error: ‘chip->dev.class’ is a pointer; did you mean to use ‘->’?
  chip->dev.class.shutdown = tpm_shutdown;

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Josh Zimmerman via tpmdd-devel June 19, 2017, 10:44 p.m. UTC | #8
Sorry about the mess. I'm a bit swamped today, but I'll work on
cleaning up the patch formatting & commit message and fix the
compilation problem later today or tomorrow. (It did build on my
checkout of the tpmdd branch... guess I didn't pull in some important
change.)
Josh


On Mon, Jun 19, 2017 at 3:38 PM, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
> On Tue, Jun 20, 2017 at 12:21:22AM +0200, Jarkko Sakkinen wrote:
>> On Tue, Jun 20, 2017 at 12:12:20AM +0200, Jarkko Sakkinen wrote:
>> > On Mon, Jun 19, 2017 at 09:51:22AM -0600, Jason Gunthorpe wrote:
>> > > On Mon, Jun 19, 2017 at 01:26:47AM +0200, Jarkko Sakkinen wrote:
>> > >
>> > > > Feels weird that you have to call framework functions like that in the
>> > > > driver. You must have brilliant reason to do so and that should be very
>> > > > well documented in the code. This is terrible...
>> > >
>> > > This was all discussed on the list. It the way these callbacks work,
>> > > the higher levels in the callback stack call the lower levels, this
>> > > allows each level the place the next level's callback properly, eg do
>> > > things before/after as necessary.
>> > >
>> > > Jason
>> >
>> > I tried to look up for discussion from the patchwork. These had appeared
>> > in v6. I guess I have to backtrack the discussions from my maidir
>> > because I honestly don't understand why class shutdown would have to
>> > call bus callback explicitly. There's nothing in the commit message
>> > about this nor  is there any comment in the code.
>> >
>> > This must be fairly recent development that I've missed?
>> >
>> > /Jarkko
>>
>> Found it:
>>
>> "Looking at this closer, now you definately have to change the TPM
>> patch to call through to the other shutdown methods. We can say
>> current TPM drivers have no driver->shutdown, but we cannot be sure
>> about the bus->shutdown, so may as well call both from tpm's
>> class->shutdown.
>>
>> I would say this should be done after the tpm2_shutdown completes as
>> lower level shutdowns could remove register access.
>>
>> Jason"
>>
>> And makes sense.
>>
>> This patch is a NAK for two reasons:
>>
>> 1. No comment explaining this.
>> 2. Patches are broken and they are in wrong order and cover letter is
>>    missing
>>
>> /Jarkko
>
> I *tried* to apply them myself after sending this in order to be helpful
> but they have compilation errors:
>
> drivers/char/tpm/tpm-chip.c: In function ‘tpm_shutdown’:
> drivers/char/tpm/tpm-chip.c:162:23: error: ‘TPM_SU_CLEAR’ undeclared (first use in this function)
>    tpm2_shutdown(chip, TPM_SU_CLEAR);
>                        ^~~~~~~~~~~~
> drivers/char/tpm/tpm-chip.c:162:23: note: each undeclared identifier is reported only once for each function it appears in
> drivers/char/tpm/tpm-chip.c: In function ‘tpm_chip_alloc’:
> drivers/char/tpm/tpm-chip.c:214:17: error: ‘chip->dev.class’ is a pointer; did you mean to use ‘->’?
>   chip->dev.class.shutdown = tpm_shutdown;
>
> /Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jarkko Sakkinen June 20, 2017, 8:44 a.m. UTC | #9
On Tue, Jun 20, 2017 at 12:38:40AM +0200, Jarkko Sakkinen wrote:
> On Tue, Jun 20, 2017 at 12:21:22AM +0200, Jarkko Sakkinen wrote:
> > On Tue, Jun 20, 2017 at 12:12:20AM +0200, Jarkko Sakkinen wrote:
> > > On Mon, Jun 19, 2017 at 09:51:22AM -0600, Jason Gunthorpe wrote:
> > > > On Mon, Jun 19, 2017 at 01:26:47AM +0200, Jarkko Sakkinen wrote:
> > > >  
> > > > > Feels weird that you have to call framework functions like that in the
> > > > > driver. You must have brilliant reason to do so and that should be very
> > > > > well documented in the code. This is terrible...
> > > > 
> > > > This was all discussed on the list. It the way these callbacks work,
> > > > the higher levels in the callback stack call the lower levels, this
> > > > allows each level the place the next level's callback properly, eg do
> > > > things before/after as necessary.
> > > > 
> > > > Jason
> > > 
> > > I tried to look up for discussion from the patchwork. These had appeared
> > > in v6. I guess I have to backtrack the discussions from my maidir
> > > because I honestly don't understand why class shutdown would have to
> > > call bus callback explicitly. There's nothing in the commit message
> > > about this nor  is there any comment in the code.
> > > 
> > > This must be fairly recent development that I've missed?
> > > 
> > > /Jarkko
> > 
> > Found it:
> > 
> > "Looking at this closer, now you definately have to change the TPM
> > patch to call through to the other shutdown methods. We can say
> > current TPM drivers have no driver->shutdown, but we cannot be sure
> > about the bus->shutdown, so may as well call both from tpm's
> > class->shutdown.
> > 
> > I would say this should be done after the tpm2_shutdown completes as
> > lower level shutdowns could remove register access.
> > 
> > Jason"
> > 
> > And makes sense.
> > 
> > This patch is a NAK for two reasons:
> > 
> > 1. No comment explaining this.
> > 2. Patches are broken and they are in wrong order and cover letter is
> >    missing
> > 
> > /Jarkko
> 
> I *tried* to apply them myself after sending this in order to be helpful
> but they have compilation errors:
> 
> drivers/char/tpm/tpm-chip.c: In function ‘tpm_shutdown’:
> drivers/char/tpm/tpm-chip.c:162:23: error: ‘TPM_SU_CLEAR’ undeclared (first use in this function)
>    tpm2_shutdown(chip, TPM_SU_CLEAR);
>                        ^~~~~~~~~~~~
> drivers/char/tpm/tpm-chip.c:162:23: note: each undeclared identifier is reported only once for each function it appears in
> drivers/char/tpm/tpm-chip.c: In function ‘tpm_chip_alloc’:
> drivers/char/tpm/tpm-chip.c:214:17: error: ‘chip->dev.class’ is a pointer; did you mean to use ‘->’?
>   chip->dev.class.shutdown = tpm_shutdown;
> 
> /Jarkko

There's one more thing that I noticed. Before I can Cc these to the
stable these commits require the Fixes tag.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jarkko Sakkinen June 20, 2017, 3:05 p.m. UTC | #10
No worries. Just fix them and I will include to my next pull request!

/Jarkko

On Mon, Jun 19, 2017 at 03:44:55PM -0700, Josh Zimmerman wrote:
> Sorry about the mess. I'm a bit swamped today, but I'll work on
> cleaning up the patch formatting & commit message and fix the
> compilation problem later today or tomorrow. (It did build on my
> checkout of the tpmdd branch... guess I didn't pull in some important
> change.)
> Josh
> 
> 
> On Mon, Jun 19, 2017 at 3:38 PM, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> > On Tue, Jun 20, 2017 at 12:21:22AM +0200, Jarkko Sakkinen wrote:
> >> On Tue, Jun 20, 2017 at 12:12:20AM +0200, Jarkko Sakkinen wrote:
> >> > On Mon, Jun 19, 2017 at 09:51:22AM -0600, Jason Gunthorpe wrote:
> >> > > On Mon, Jun 19, 2017 at 01:26:47AM +0200, Jarkko Sakkinen wrote:
> >> > >
> >> > > > Feels weird that you have to call framework functions like that in the
> >> > > > driver. You must have brilliant reason to do so and that should be very
> >> > > > well documented in the code. This is terrible...
> >> > >
> >> > > This was all discussed on the list. It the way these callbacks work,
> >> > > the higher levels in the callback stack call the lower levels, this
> >> > > allows each level the place the next level's callback properly, eg do
> >> > > things before/after as necessary.
> >> > >
> >> > > Jason
> >> >
> >> > I tried to look up for discussion from the patchwork. These had appeared
> >> > in v6. I guess I have to backtrack the discussions from my maidir
> >> > because I honestly don't understand why class shutdown would have to
> >> > call bus callback explicitly. There's nothing in the commit message
> >> > about this nor  is there any comment in the code.
> >> >
> >> > This must be fairly recent development that I've missed?
> >> >
> >> > /Jarkko
> >>
> >> Found it:
> >>
> >> "Looking at this closer, now you definately have to change the TPM
> >> patch to call through to the other shutdown methods. We can say
> >> current TPM drivers have no driver->shutdown, but we cannot be sure
> >> about the bus->shutdown, so may as well call both from tpm's
> >> class->shutdown.
> >>
> >> I would say this should be done after the tpm2_shutdown completes as
> >> lower level shutdowns could remove register access.
> >>
> >> Jason"
> >>
> >> And makes sense.
> >>
> >> This patch is a NAK for two reasons:
> >>
> >> 1. No comment explaining this.
> >> 2. Patches are broken and they are in wrong order and cover letter is
> >>    missing
> >>
> >> /Jarkko
> >
> > I *tried* to apply them myself after sending this in order to be helpful
> > but they have compilation errors:
> >
> > drivers/char/tpm/tpm-chip.c: In function ‘tpm_shutdown’:
> > drivers/char/tpm/tpm-chip.c:162:23: error: ‘TPM_SU_CLEAR’ undeclared (first use in this function)
> >    tpm2_shutdown(chip, TPM_SU_CLEAR);
> >                        ^~~~~~~~~~~~
> > drivers/char/tpm/tpm-chip.c:162:23: note: each undeclared identifier is reported only once for each function it appears in
> > drivers/char/tpm/tpm-chip.c: In function ‘tpm_chip_alloc’:
> > drivers/char/tpm/tpm-chip.c:214:17: error: ‘chip->dev.class’ is a pointer; did you mean to use ‘->’?
> >   chip->dev.class.shutdown = tpm_shutdown;
> >
> > /Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jarkko Sakkinen June 21, 2017, 7:37 a.m. UTC | #11
While you still have to work on this, could you name the function as
tpm_class_shutdown() to better claarify its purpose? Otherwise, things
start to look a bit confusing. Thanks.

/Jarkko

On Tue, Jun 20, 2017 at 05:05:41PM +0200, Jarkko Sakkinen wrote:
> No worries. Just fix them and I will include to my next pull request!
> 
> /Jarkko
> 
> On Mon, Jun 19, 2017 at 03:44:55PM -0700, Josh Zimmerman wrote:
> > Sorry about the mess. I'm a bit swamped today, but I'll work on
> > cleaning up the patch formatting & commit message and fix the
> > compilation problem later today or tomorrow. (It did build on my
> > checkout of the tpmdd branch... guess I didn't pull in some important
> > change.)
> > Josh
> > 
> > 
> > On Mon, Jun 19, 2017 at 3:38 PM, Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > > On Tue, Jun 20, 2017 at 12:21:22AM +0200, Jarkko Sakkinen wrote:
> > >> On Tue, Jun 20, 2017 at 12:12:20AM +0200, Jarkko Sakkinen wrote:
> > >> > On Mon, Jun 19, 2017 at 09:51:22AM -0600, Jason Gunthorpe wrote:
> > >> > > On Mon, Jun 19, 2017 at 01:26:47AM +0200, Jarkko Sakkinen wrote:
> > >> > >
> > >> > > > Feels weird that you have to call framework functions like that in the
> > >> > > > driver. You must have brilliant reason to do so and that should be very
> > >> > > > well documented in the code. This is terrible...
> > >> > >
> > >> > > This was all discussed on the list. It the way these callbacks work,
> > >> > > the higher levels in the callback stack call the lower levels, this
> > >> > > allows each level the place the next level's callback properly, eg do
> > >> > > things before/after as necessary.
> > >> > >
> > >> > > Jason
> > >> >
> > >> > I tried to look up for discussion from the patchwork. These had appeared
> > >> > in v6. I guess I have to backtrack the discussions from my maidir
> > >> > because I honestly don't understand why class shutdown would have to
> > >> > call bus callback explicitly. There's nothing in the commit message
> > >> > about this nor  is there any comment in the code.
> > >> >
> > >> > This must be fairly recent development that I've missed?
> > >> >
> > >> > /Jarkko
> > >>
> > >> Found it:
> > >>
> > >> "Looking at this closer, now you definately have to change the TPM
> > >> patch to call through to the other shutdown methods. We can say
> > >> current TPM drivers have no driver->shutdown, but we cannot be sure
> > >> about the bus->shutdown, so may as well call both from tpm's
> > >> class->shutdown.
> > >>
> > >> I would say this should be done after the tpm2_shutdown completes as
> > >> lower level shutdowns could remove register access.
> > >>
> > >> Jason"
> > >>
> > >> And makes sense.
> > >>
> > >> This patch is a NAK for two reasons:
> > >>
> > >> 1. No comment explaining this.
> > >> 2. Patches are broken and they are in wrong order and cover letter is
> > >>    missing
> > >>
> > >> /Jarkko
> > >
> > > I *tried* to apply them myself after sending this in order to be helpful
> > > but they have compilation errors:
> > >
> > > drivers/char/tpm/tpm-chip.c: In function ‘tpm_shutdown’:
> > > drivers/char/tpm/tpm-chip.c:162:23: error: ‘TPM_SU_CLEAR’ undeclared (first use in this function)
> > >    tpm2_shutdown(chip, TPM_SU_CLEAR);
> > >                        ^~~~~~~~~~~~
> > > drivers/char/tpm/tpm-chip.c:162:23: note: each undeclared identifier is reported only once for each function it appears in
> > > drivers/char/tpm/tpm-chip.c: In function ‘tpm_chip_alloc’:
> > > drivers/char/tpm/tpm-chip.c:214:17: error: ‘chip->dev.class’ is a pointer; did you mean to use ‘->’?
> > >   chip->dev.class.shutdown = tpm_shutdown;
> > >
> > > /Jarkko
> 
> ------------------------------------------------------------------------------
> 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

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff mbox

Patch

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 9dec9f551b83..62691d266f22 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -142,6 +142,36 @@  static void tpm_devs_release(struct device *dev)
 	put_device(&chip->dev);
 }
 
+/**
+ * tpm_shutdown() - prepare the TPM device for loss of power.
+ * @dev: device to which the chip is associated.
+ *
+ * Issues a TPM2_Shutdown command prior to loss of power, as required by the
+ * TPM 2.0 spec.
+ *
+ * XXX: This codepath relies on the fact that sysfs is not enabled for
+ * TPM2: sysfs uses an implicit lock on chip->ops, so this could race if TPM2
+ * has sysfs support enabled before TPM sysfs's implicit locking is fixed.
+ */
+static void tpm_shutdown(struct device *dev)
+{
+	struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev);
+
+	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+		down_write(&chip->ops_sem);
+		tpm2_shutdown(chip, TPM_SU_CLEAR);
+		chip->ops = NULL;
+		up_write(&chip->ops_sem);
+	}
+	// Allow bus- and device-specific code to run. Note: since chip->ops
+	// is NULL, more-specific shutdown code will not be able to issue TPM
+	// commands.
+	if (dev->bus->shutdown)
+		dev->bus->shutdown(dev);
+	else if (dev->driver && dev->driver->shutdown)
+		dev->driver->shutdown(dev);
+}
+
 /**
  * tpm_chip_alloc() - allocate a new struct tpm_chip instance
  * @pdev: device to which the chip is associated
@@ -181,6 +211,7 @@  struct tpm_chip *tpm_chip_alloc(struct device *pdev,
 	device_initialize(&chip->devs);
 
 	chip->dev.class = tpm_class;
+	chip->dev.class.shutdown = tpm_shutdown;
 	chip->dev.release = tpm_dev_release;
 	chip->dev.parent = pdev;
 	chip->dev.groups = chip->groups;
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index 55405dbe43fa..59bd0b7b5959 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -294,6 +294,9 @@  static const struct attribute_group tpm_dev_group = {
 
 void tpm_sysfs_add_device(struct tpm_chip *chip)
 {
+	/* XXX: If you wish to remove this restriction, you must first update
+	 * tpm_sysfs to explicitly lock chip->ops.
+	 */
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		return;