tpmdd-devel.lists.sourceforge.net archive mirror
 help / color / mirror / Atom feed
* [PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set.
@ 2017-08-09 21:34 Michal Suchanek
  2017-08-09 21:52 ` Jason Gunthorpe
  0 siblings, 1 reply; 8+ messages in thread
From: Michal Suchanek @ 2017-08-09 21:34 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Peter Huewe, Marcel Selhorst,
	Jarkko Sakkinen, Jason Gunthorpe, linux-kernel, tpmdd-devel
  Cc: Michal Suchanek

Disabling the driver hook by setting class hook is totally sound design
not prone to error as evidenced by the single implementation of the
class hook.

Fixes: d1bd4a792d39 ("tpm: Issue a TPM2_Shutdown for TPM2 devices.")
Fixes: f77af1516584 ("Add "shutdown" to "struct class".")

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 drivers/base/core.c         | 3 ++-
 drivers/char/tpm/tpm-chip.c | 9 +--------
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 755451f684bc..2cf752dc1421 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2668,7 +2668,8 @@ void device_shutdown(void)
 			if (initcall_debug)
 				dev_info(dev, "shutdown\n");
 			dev->class->shutdown(dev);
-		} else if (dev->bus && dev->bus->shutdown) {
+		}
+		if (dev->bus && dev->bus->shutdown) {
 			if (initcall_debug)
 				dev_info(dev, "shutdown\n");
 			dev->bus->shutdown(dev);
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 67ec9d3d04f5..edf8fa553f5f 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -164,14 +164,7 @@ static int tpm_class_shutdown(struct device *dev)
 		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 && dev->bus->shutdown)
-		dev->bus->shutdown(dev);
-	else if (dev->driver && dev->driver->shutdown)
-		dev->driver->shutdown(dev);
+
 	return 0;
 }
 
-- 
2.10.2

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set.
  2017-08-09 21:34 [PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set Michal Suchanek
@ 2017-08-09 21:52 ` Jason Gunthorpe
  2017-08-10 10:18   ` Michal Suchánek
  2017-08-11 11:50   ` Jarkko Sakkinen
  0 siblings, 2 replies; 8+ messages in thread
From: Jason Gunthorpe @ 2017-08-09 21:52 UTC (permalink / raw)
  To: Michal Suchanek
  Cc: Greg Kroah-Hartman, Peter Huewe, Marcel Selhorst,
	Jarkko Sakkinen, linux-kernel, tpmdd-devel

On Wed, Aug 09, 2017 at 11:34:20PM +0200, Michal Suchanek wrote:
> Disabling the driver hook by setting class hook is totally sound design
> not prone to error as evidenced by the single implementation of the
> class hook.

It was done this was for consistency, if you look at the full code:

                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);
                } else if (dev->driver && dev->driver->shutdown) {
                        if (initcall_debug)
                                dev_info(dev, "shutdown\n");
                        dev->driver->shutdown(dev);
                }

The bus disables the driver callback, on the expectation that the bus
implementation will do it.

Existing bus implementations do properly chain to driver shutdown (eg
look at mmc_bus_shutdown) and it appears to have been written like
this so that the bus can insert code before and after calling the
driver shutdown.

Making class act differently from bus seems very confusing, IHMO,
which why the TPM patch was written to follow the existing pattern.

Jason

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set.
  2017-08-09 21:52 ` Jason Gunthorpe
@ 2017-08-10 10:18   ` Michal Suchánek
  2017-08-10 16:30     ` Jason Gunthorpe
  2017-08-11 11:50   ` Jarkko Sakkinen
  1 sibling, 1 reply; 8+ messages in thread
From: Michal Suchánek @ 2017-08-10 10:18 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Greg Kroah-Hartman, Peter Huewe, Marcel Selhorst,
	Jarkko Sakkinen, linux-kernel, tpmdd-devel

On Wed, 9 Aug 2017 15:52:02 -0600
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:

> On Wed, Aug 09, 2017 at 11:34:20PM +0200, Michal Suchanek wrote:
> > Disabling the driver hook by setting class hook is totally sound
> > design not prone to error as evidenced by the single implementation
> > of the class hook.  
> 
> It was done this was for consistency, if you look at the full code:
> 
>                 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);
>                 } else if (dev->driver && dev->driver->shutdown) {
>                         if (initcall_debug)
>                                 dev_info(dev, "shutdown\n");
>                         dev->driver->shutdown(dev);
>                 }
> 
> The bus disables the driver callback, on the expectation that the bus
> implementation will do it.

Which is totally sound design not prone to errors.

> 
> Existing bus implementations do properly chain to driver shutdown (eg
> look at mmc_bus_shutdown) and it appears to have been written like

Neither isa nor ibmebus does. These are two random buses I tried to
look at.

> this so that the bus can insert code before and after calling the
> driver shutdown.

So basically there is bus pre-shutdown and post-shutdown hook jumbled
together in one function. While I can understand the concept of
post-shutdown hook I wonder what gross hack would require a
pre-shutdown hook.

> 
> Making class act differently from bus seems very confusing, IHMO,
> which why the TPM patch was written to follow the existing pattern.

The Linux development process at its best. There is poor design
implemented so when touching the code it is extended to worse because
it is smaller patch more likely to get past maintainers than fixing the
mess.

If you argue the existing way is the best could you please give an
actual technical argument for implementing the hooks this way?

Thanks

Michal

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set.
  2017-08-10 10:18   ` Michal Suchánek
@ 2017-08-10 16:30     ` Jason Gunthorpe
  2017-08-11  5:04       ` Michal Suchánek
  0 siblings, 1 reply; 8+ messages in thread
From: Jason Gunthorpe @ 2017-08-10 16:30 UTC (permalink / raw)
  To: Michal Suchánek
  Cc: Greg Kroah-Hartman, Peter Huewe, Marcel Selhorst,
	Jarkko Sakkinen, linux-kernel, tpmdd-devel

On Thu, Aug 10, 2017 at 12:18:11PM +0200, Michal Suchánek wrote:
> > The bus disables the driver callback, on the expectation that the bus
> > implementation will do it.
> 
> Which is totally sound design not prone to errors.

Well, I agree it isn't the easiest...

> > Existing bus implementations do properly chain to driver shutdown (eg
> > look at mmc_bus_shutdown) and it appears to have been written like
> 
> Neither isa nor ibmebus does. These are two random buses I tried to
> look at.

I'm not following, I see this:

static void ibmebus_bus_device_shutdown(struct device *dev)
{
        struct platform_device *of_dev = to_platform_device(dev);
        struct platform_driver *drv = to_platform_driver(dev->driver);

        if (dev->driver && drv->shutdown)
                drv->shutdown(of_dev);
}

It looks to me like in this case the struct device_driver shutdown is
not used, and instead the struct platform_driver shutdown is called.

> > this so that the bus can insert code before and after calling the
> > driver shutdown.
> 
> So basically there is bus pre-shutdown and post-shutdown hook jumbled
> together in one function.

and a redirect, apparently.

> While I can understand the concept of post-shutdown hook I wonder
> what gross hack would require a pre-shutdown hook.

TPM requires pre-shutdown. It fences off access to the TPM so the TPM
can have a clean shutdown. We cannot do a clean TPM shutdown if there
is a possibility of another transaction being send to the TPM. TPM's
have non-volatile state and record if they were not shut down
properly, so doing this is actually quite important.

> The Linux development process at its best. There is poor design
> implemented so when touching the code it is extended to worse because

I'm not sure I completely agree, there is obviously a lot going on with
bus->shutdown.

If you want to go ahead with your patch then please also rename the
class shutdown to shutdown_pre to make it clear it is doing something
different.

> it is smaller patch more likely to get past maintainers than fixing the
> mess.

Yes, this is probably true, the TPM fix needed to be back ported to -stable.

Jason

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set.
  2017-08-10 16:30     ` Jason Gunthorpe
@ 2017-08-11  5:04       ` Michal Suchánek
  2017-08-11 15:28         ` Henrique de Moraes Holschuh
  0 siblings, 1 reply; 8+ messages in thread
From: Michal Suchánek @ 2017-08-11  5:04 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Greg Kroah-Hartman, Peter Huewe, Marcel Selhorst,
	Jarkko Sakkinen, linux-kernel, tpmdd-devel

On 2017-08-10 18:30, Jason Gunthorpe wrote:
> On Thu, Aug 10, 2017 at 12:18:11PM +0200, Michal Suchánek wrote:

>> > Existing bus implementations do properly chain to driver shutdown (eg
>> > look at mmc_bus_shutdown) and it appears to have been written like
>> 
>> Neither isa nor ibmebus does. These are two random buses I tried to
>> look at.
> 
> I'm not following, I see this:
> 
> static void ibmebus_bus_device_shutdown(struct device *dev)
> {
>         struct platform_device *of_dev = to_platform_device(dev);
>         struct platform_driver *drv = to_platform_driver(dev->driver);
> 
>         if (dev->driver && drv->shutdown)
>                 drv->shutdown(of_dev);
> }
> 
> It looks to me like in this case the struct device_driver shutdown is
> not used, and instead the struct platform_driver shutdown is called.

And it is not used even if a device driver sets it and expects it to 
run.

> 
>> > this so that the bus can insert code before and after calling the
>> > driver shutdown.
>> 
>> So basically there is bus pre-shutdown and post-shutdown hook jumbled
>> together in one function.
> 
> and a redirect, apparently.
> 
>> While I can understand the concept of post-shutdown hook I wonder
>> what gross hack would require a pre-shutdown hook.
> 
> TPM requires pre-shutdown.

Yes, a class pre-shutdown. Not a bus pre-shutdown, however.

I have no idea what business has a bus driver before a device shuts 
down.

> 
>> The Linux development process at its best. There is poor design
>> implemented so when touching the code it is extended to worse because
> 
> I'm not sure I completely agree, there is obviously a lot going on with
> bus->shutdown.
> 
> If you want to go ahead with your patch then please also rename the
> class shutdown to shutdown_pre to make it clear it is doing something
> different.

That makes sense.

Thanks

Michal

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set.
  2017-08-09 21:52 ` Jason Gunthorpe
  2017-08-10 10:18   ` Michal Suchánek
@ 2017-08-11 11:50   ` Jarkko Sakkinen
  1 sibling, 0 replies; 8+ messages in thread
From: Jarkko Sakkinen @ 2017-08-11 11:50 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Michal Suchanek, Greg Kroah-Hartman, Peter Huewe,
	Marcel Selhorst, linux-kernel, tpmdd-devel

On Wed, Aug 09, 2017 at 03:52:02PM -0600, Jason Gunthorpe wrote:
> On Wed, Aug 09, 2017 at 11:34:20PM +0200, Michal Suchanek wrote:
> > Disabling the driver hook by setting class hook is totally sound design
> > not prone to error as evidenced by the single implementation of the
> > class hook.
> 
> It was done this was for consistency, if you look at the full code:
> 
>                 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);
>                 } else if (dev->driver && dev->driver->shutdown) {
>                         if (initcall_debug)
>                                 dev_info(dev, "shutdown\n");
>                         dev->driver->shutdown(dev);
>                 }
> 
> The bus disables the driver callback, on the expectation that the bus
> implementation will do it.
> 
> Existing bus implementations do properly chain to driver shutdown (eg
> look at mmc_bus_shutdown) and it appears to have been written like
> this so that the bus can insert code before and after calling the
> driver shutdown.
> 
> Making class act differently from bus seems very confusing, IHMO,
> which why the TPM patch was written to follow the existing pattern.
> 
> Jason

There's also more fundamental problem. There are Fixes tags but no
real regression. Even if this patch made sense I would not consider
it as a bug fix.

/Jarkko

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set.
  2017-08-11  5:04       ` Michal Suchánek
@ 2017-08-11 15:28         ` Henrique de Moraes Holschuh
  2017-08-11 17:01           ` Michal Suchánek
  0 siblings, 1 reply; 8+ messages in thread
From: Henrique de Moraes Holschuh @ 2017-08-11 15:28 UTC (permalink / raw)
  To: Michal Suchánek
  Cc: Jason Gunthorpe, Greg Kroah-Hartman, Peter Huewe,
	Marcel Selhorst, Jarkko Sakkinen, linux-kernel, tpmdd-devel

On Fri, 11 Aug 2017, Michal Suchánek wrote:
> On 2017-08-10 18:30, Jason Gunthorpe wrote:
> > On Thu, Aug 10, 2017 at 12:18:11PM +0200, Michal Suchánek wrote:
> > > > Existing bus implementations do properly chain to driver shutdown (eg
> > > > look at mmc_bus_shutdown) and it appears to have been written like
> > > 
> > > Neither isa nor ibmebus does. These are two random buses I tried to
> > > look at.
> > 
> > I'm not following, I see this:
> > 
> > static void ibmebus_bus_device_shutdown(struct device *dev)
> > {
> >         struct platform_device *of_dev = to_platform_device(dev);
> >         struct platform_driver *drv = to_platform_driver(dev->driver);
> > 
> >         if (dev->driver && drv->shutdown)
> >                 drv->shutdown(of_dev);
> > }
> > 
> > It looks to me like in this case the struct device_driver shutdown is
> > not used, and instead the struct platform_driver shutdown is called.
> 
> And it is not used even if a device driver sets it and expects it to run.

Which is the kind of landmine it is best avoided in drivers/, so it
would be nice to get WARN_ON() during device register when
dev->shutdown() methods *that are going to be ignored* because of
class/bus handlers are non-NULL...

Either that, or the device->shutdown() methods should always be called,
and drivers that should not/need not have them should be fixed...

-- 
  Henrique Holschuh

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set.
  2017-08-11 15:28         ` Henrique de Moraes Holschuh
@ 2017-08-11 17:01           ` Michal Suchánek
  0 siblings, 0 replies; 8+ messages in thread
From: Michal Suchánek @ 2017-08-11 17:01 UTC (permalink / raw)
  To: Henrique de Moraes Holschuh
  Cc: Jason Gunthorpe, Greg Kroah-Hartman, Peter Huewe,
	Marcel Selhorst, Jarkko Sakkinen, linux-kernel, tpmdd-devel

On Fri, 11 Aug 2017 12:28:57 -0300
Henrique de Moraes Holschuh <hmh@hmh.eng.br> wrote:

> On Fri, 11 Aug 2017, Michal Suchánek wrote:
> > On 2017-08-10 18:30, Jason Gunthorpe wrote:  
> > > On Thu, Aug 10, 2017 at 12:18:11PM +0200, Michal Suchánek wrote:  
> > > > > Existing bus implementations do properly chain to driver
> > > > > shutdown (eg look at mmc_bus_shutdown) and it appears to have
> > > > > been written like  
> > > > 
> > > > Neither isa nor ibmebus does. These are two random buses I
> > > > tried to look at.  
> > > 
> > > I'm not following, I see this:
> > > 
> > > static void ibmebus_bus_device_shutdown(struct device *dev)
> > > {
> > >         struct platform_device *of_dev = to_platform_device(dev);
> > >         struct platform_driver *drv =
> > > to_platform_driver(dev->driver);
> > > 
> > >         if (dev->driver && drv->shutdown)
> > >                 drv->shutdown(of_dev);
> > > }
> > > 
> > > It looks to me like in this case the struct device_driver
> > > shutdown is not used, and instead the struct platform_driver
> > > shutdown is called.  
> > 
> > And it is not used even if a device driver sets it and expects it
> > to run.  
> 
> Which is the kind of landmine it is best avoided in drivers/, so it
> would be nice to get WARN_ON() during device register when
> dev->shutdown() methods *that are going to be ignored* because of
> class/bus handlers are non-NULL...

We actually have the warning:

int driver_register(struct device_driver *drv)
{
        int ret;
        struct device_driver *other;

        BUG_ON(!drv->bus->p);

        if ((drv->bus->probe && drv->probe) ||
            (drv->bus->remove && drv->remove) ||
            (drv->bus->shutdown && drv->shutdown))
                printk(KERN_WARNING "Driver '%s' needs updating -
        please use " "bus_type methods\n", drv->name);

So presumably setting both bus and driver shutdown is bogus while
setting both class and bus shutdown is fine and expected. So the logic
in the initial class shutdown addition that disables driver and
bus shutdown is bogus - there is no reason to disable them.

Given that nobody pointed out this shows how well understood this
interface is :/

Thanks

Michal

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-08-11 17:01 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-09 21:34 [PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set Michal Suchanek
2017-08-09 21:52 ` Jason Gunthorpe
2017-08-10 10:18   ` Michal Suchánek
2017-08-10 16:30     ` Jason Gunthorpe
2017-08-11  5:04       ` Michal Suchánek
2017-08-11 15:28         ` Henrique de Moraes Holschuh
2017-08-11 17:01           ` Michal Suchánek
2017-08-11 11:50   ` Jarkko Sakkinen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).