| Message ID | CAHSjozA+xfUM-NmbP7Kb5B=T+u6qaEFzMWeF6OxX2N7-xHLAVQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
On Thu, Jun 1, 2017 at 9:39 AM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> 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
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@obsidianresearch.com> 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
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@google.com> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: stable@vger.kernel.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
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@google.com> > > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: stable@vger.kernel.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
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@linux.intel.com> 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@google.com> >> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> > Cc: stable@vger.kernel.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
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