[PATCH v5 2/2] Add "shutdown" to "struct class".

[PATCH v5 2/2] Add "shutdown" to "struct class".

[Josh Zimmerman]

The TPM class has some common shutdown code that must be executed for
all drivers. This adds some needed functionality for that.

Signed-off-by: Josh Zimmerman <joshz-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

-----
v2: Add Signed-off-by.
v3: Remove logically separate change.
v4: Add "acked-by" and "cc".
v5: Execute only one of the class/bus/driver's shutdown functions.
---
 drivers/base/core.c    | 6 +++++-
 include/linux/device.h | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 6bb60fb6a30b..9f426954681e 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2667,7 +2667,11 @@ void device_shutdown(void)
                pm_runtime_get_noresume(dev);
                pm_runtime_barrier(dev);

-               if (dev->bus && dev->bus->shutdown) {
+               if (dev->class && dev->class->shutdown) {
+                       if (initcall_debug)
+                               dev_info(dev, "shutdown\n");
+                       dev->class->shutdown(dev);
+               } else if (dev->bus && dev->bus->shutdown) {
                        if (initcall_debug)
                                dev_info(dev, "shutdown\n");
                        dev->bus->shutdown(dev);
diff --git a/include/linux/device.h b/include/linux/device.h
index 9ef518af5515..f240baac2001 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -378,6 +378,7 @@ int subsys_virtual_register(struct bus_type *subsys,
  * @suspend:   Used to put the device to sleep mode, usually to a low power
  *             state.
  * @resume:    Used to bring the device from the sleep mode.
+ * @shutdown:  Called at shut-down time to quiesce the device.
  * @ns_type:   Callbacks so sysfs can detemine namespaces.
  * @namespace: Namespace of the device belongs to this class.
  * @pm:                The default device power management operations
of this class.
@@ -407,6 +408,7 @@ struct class {

        int (*suspend)(struct device *dev, pm_message_t state);
        int (*resume)(struct device *dev);
+       int (*shutdown)(struct device *dev);

        const struct kobj_ns_type_operations *ns_type;
        const void *(*namespace)(struct device *dev);
-- 
2.13.0.219.gdb65acc882-goog

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

Re: [PATCH v5 2/2] Add "shutdown" to "struct class".

[Jason Gunthorpe]

On Thu, Jun 01, 2017 at 09:33:43AM -0700, Josh Zimmerman wrote:

> -               if (dev->bus && dev->bus->shutdown) {
> +               if (dev->class && dev->class->shutdown) {
> +                       if (initcall_debug)
> +                               dev_info(dev, "shutdown\n");
> +                       dev->class->shutdown(dev);
> +               } else if (dev->bus && dev->bus->shutdown) {
>                         if (initcall_debug)
>                                 dev_info(dev, "shutdown\n");
>                         dev->bus->shutdown(dev);

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

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

Re: [PATCH v5 2/2] Add "shutdown" to "struct class".

[Josh Zimmerman]

On Thu, Jun 1, 2017 at 9:39 AM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> On Thu, Jun 01, 2017 at 09:33:43AM -0700, Josh Zimmerman wrote:
>
>> -               if (dev->bus && dev->bus->shutdown) {
>> +               if (dev->class && dev->class->shutdown) {
>> +                       if (initcall_debug)
>> +                               dev_info(dev, "shutdown\n");
>> +                       dev->class->shutdown(dev);
>> +               } else if (dev->bus && dev->bus->shutdown) {
>>                         if (initcall_debug)
>>                                 dev_info(dev, "shutdown\n");
>>                         dev->bus->shutdown(dev);
>
> 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.
Why can't we be sure? Could bus->shutdown methods be registered
outside of the drivers/char/tpm/ tree?

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

This doesn't necessarily work.  The spec states that tpm2_shutdown
must be the last command issued for an orderly shutdown, so if we call
the lower-level functions after the tpm2_shutdown, the shutdown may
not be orderly. It is also a problem that register access could be
removed, though. I'm not sure what the best way to deal with this is
(I lack sufficient familiarity with the codebase). Do you have
thoughts on how to resolve this conflict?

Josh

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

Re: [PATCH v5 2/2] Add "shutdown" to "struct class".

[Jason Gunthorpe]

On Thu, Jun 01, 2017 at 09:55:20AM -0700, Josh Zimmerman wrote:
> On Thu, Jun 1, 2017 at 9:39 AM, Jason Gunthorpe
> <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> > On Thu, Jun 01, 2017 at 09:33:43AM -0700, Josh Zimmerman wrote:
> >
> >> -               if (dev->bus && dev->bus->shutdown) {
> >> +               if (dev->class && dev->class->shutdown) {
> >> +                       if (initcall_debug)
> >> +                               dev_info(dev, "shutdown\n");
> >> +                       dev->class->shutdown(dev);
> >> +               } else if (dev->bus && dev->bus->shutdown) {
> >>                         if (initcall_debug)
> >>                                 dev_info(dev, "shutdown\n");
> >>                         dev->bus->shutdown(dev);
> >
> > 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.

> Why can't we be sure? Could bus->shutdown methods be registered
> outside of the drivers/char/tpm/ tree?

Yes.

> > I would say this should be done after the tpm2_shutdown completes as
> > lower level shutdowns could remove register access.
> 
> This doesn't necessarily work.  The spec states that tpm2_shutdown
> must be the last command issued for an orderly shutdown, so if we
> call

After tpm2_shutdown is called ops must be null'd then we call down the
rest of the shutdown methods which can no longer issue TPM commands
due to ops being NULL.

Jason

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

Re: [PATCH v5 2/2] Add "shutdown" to "struct class".

[Jarkko Sakkinen]

On Thu, Jun 01, 2017 at 09:33:43AM -0700, Josh Zimmerman wrote:
> The TPM class has some common shutdown code that must be executed for
> all drivers. This adds some needed functionality for that.
> 
> Signed-off-by: Josh Zimmerman <joshz-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 
> -----
> v2: Add Signed-off-by.
> v3: Remove logically separate change.
> v4: Add "acked-by" and "cc".
> v5: Execute only one of the class/bus/driver's shutdown functions.

This breaks the patch.

> ---

The changelog should be here.

>  drivers/base/core.c    | 6 +++++-
>  include/linux/device.h | 2 ++
>  2 files changed, 7 insertions(+), 1 deletion(-)

/Jarkko

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

Re: [PATCH v5 2/2] Add "shutdown" to "struct class".

[Jarkko Sakkinen]

On Fri, Jun 16, 2017 at 10:57:47AM +0200, Jarkko Sakkinen wrote:
> On Thu, Jun 01, 2017 at 09:33:43AM -0700, Josh Zimmerman wrote:
> > The TPM class has some common shutdown code that must be executed for
> > all drivers. This adds some needed functionality for that.
> > 
> > Signed-off-by: Josh Zimmerman <joshz-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> > Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > 
> > -----
> > v2: Add Signed-off-by.
> > v3: Remove logically separate change.
> > v4: Add "acked-by" and "cc".
> > v5: Execute only one of the class/bus/driver's shutdown functions.
> 
> This breaks the patch.
> 
> > ---
> 
> The changelog should be here.
> 
> >  drivers/base/core.c    | 6 +++++-
> >  include/linux/device.h | 2 ++
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> /Jarkko


It actually doesn't probably break the patch because there are six
dashes but is incorrectly positioned. You should but right before
diffstat after three dashes. Git will ignore this part.

Still getting this:

$ git am -3 ~/Downloads/josh.patch
Applying: Add "shutdown" to "struct class".
error: patch fragment without header at line 36: @@ -407,6 +408,7 @@ struct class {
error: could not build fake ancestor
Patch failed at 0001 Add "shutdown" to "struct class".
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Look at the comment at the lie 1549:

https://github.com/git/git/blob/master/apply.c

I have no idea how you have exported and sent it but I would recommend
to use standard git format-patch and git send-email commands to ensure
the correct results.

Please send the full patch set once you know it will cleanly apply.

You could email it to yourself or some collegue and ask them to apply
it as a test measure...

/Jarkko

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

Re: [PATCH v5 2/2] Add "shutdown" to "struct class".

[Josh Zimmerman via tpmdd-devel]

Just re-sent using git send-email; hopefully the must recent version will work.
Josh


On Fri, Jun 16, 2017 at 2:14 AM, Jarkko Sakkinen
<jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> On Fri, Jun 16, 2017 at 10:57:47AM +0200, Jarkko Sakkinen wrote:
>> On Thu, Jun 01, 2017 at 09:33:43AM -0700, Josh Zimmerman wrote:
>> > The TPM class has some common shutdown code that must be executed for
>> > all drivers. This adds some needed functionality for that.
>> >
>> > Signed-off-by: Josh Zimmerman <joshz-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>> > Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
>> > Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> >
>> > -----
>> > v2: Add Signed-off-by.
>> > v3: Remove logically separate change.
>> > v4: Add "acked-by" and "cc".
>> > v5: Execute only one of the class/bus/driver's shutdown functions.
>>
>> This breaks the patch.
>>
>> > ---
>>
>> The changelog should be here.
>>
>> >  drivers/base/core.c    | 6 +++++-
>> >  include/linux/device.h | 2 ++
>> >  2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> /Jarkko
>
>
> It actually doesn't probably break the patch because there are six
> dashes but is incorrectly positioned. You should but right before
> diffstat after three dashes. Git will ignore this part.
>
> Still getting this:
>
> $ git am -3 ~/Downloads/josh.patch
> Applying: Add "shutdown" to "struct class".
> error: patch fragment without header at line 36: @@ -407,6 +408,7 @@ struct class {
> error: could not build fake ancestor
> Patch failed at 0001 Add "shutdown" to "struct class".
> The copy of the patch that failed is found in: .git/rebase-apply/patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
>
> Look at the comment at the lie 1549:
>
> https://github.com/git/git/blob/master/apply.c
>
> I have no idea how you have exported and sent it but I would recommend
> to use standard git format-patch and git send-email commands to ensure
> the correct results.
>
> Please send the full patch set once you know it will cleanly apply.
>
> You could email it to yourself or some collegue and ask them to apply
> it as a test measure...
>
> /Jarkko

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