[PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set.

[PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set.

[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

Re: [PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set.

[Jason Gunthorpe]

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

Re: [PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set.

[Michal Suchánek]

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

Re: [PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set.

[Jason Gunthorpe]

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

Re: [PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set.

[Michal Suchánek]

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

Re: [PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set.

[Jarkko Sakkinen]

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

Re: [PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set.

[Henrique de Moraes Holschuh]

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

Re: [PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set.

[Michal Suchánek]

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