[PATCH] pci: don't override drv->shutdown unconditionally

[PATCH] pci: don't override drv->shutdown unconditionally

[Christoph Hellwig]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 856 bytes --]

There are many drivers that have been setting the generic driver
model­level shutdown callback, and pci thus must not override it.

Without this patch we can have really bad data loss on various
raid controllers.


Index: linux-2.6/drivers/pci/pci-driver.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci-driver.c	2005-05-05 11:01:03.000000000 +0200
+++ linux-2.6/drivers/pci/pci-driver.c	2005-06-17 18:44:52.000000000 +0200
@@ -393,7 +393,8 @@
 	drv->driver.bus = &pci_bus_type;
 	drv->driver.probe = pci_device_probe;
 	drv->driver.remove = pci_device_remove;
-	drv->driver.shutdown = pci_device_shutdown,
+	if (!drv->driver.shutdown)
+		drv->driver.shutdown = pci_device_shutdown,
 	drv->driver.owner = drv->owner;
 	drv->driver.kobj.ktype = &pci_driver_kobj_type;
 	pci_init_dynids(&drv->dynids);

Re: [PATCH] pci: don't override drv->shutdown unconditionally

[Greg KH]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 891 bytes --]

On Fri, Jun 17, 2005 at 08:30:57PM +0200, Christoph Hellwig wrote:
> There are many drivers that have been setting the generic driver
> model­level shutdown callback, and pci thus must not override it.
> 
> Without this patch we can have really bad data loss on various
> raid controllers.

Without the kexec patch?

So, why are these drivers setting the shutdown function in the first
place if they don't want it to be called?  My change finally enabled
this call, which is what the driver authors expected in the first place.

Without the change I made, the same data loss would be had as drivers
never know that the box is going down.

The fact that a few drivers never tested their shutdown calls is no
reason to penalize the whole kernel.

So, no, I do not want this change in, the drivers should be fixed
properly.

Care to point me to any drivers that need fixing?

thanks,

greg k-h

Re: [PATCH] pci: don't override drv->shutdown unconditionally

[Christoph Hellwig]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 701 bytes --]

On Fri, Jun 17, 2005 at 11:49:14AM -0700, Greg KH wrote:
> On Fri, Jun 17, 2005 at 08:30:57PM +0200, Christoph Hellwig wrote:
> > There are many drivers that have been setting the generic driver
> > model­level shutdown callback, and pci thus must not override it.
> > 
> > Without this patch we can have really bad data loss on various
> > raid controllers.
> 
> Without the kexec patch?

On shutdown.  I don't know why you're talking about kexec here.

> So, why are these drivers setting the shutdown function in the first
> place if they don't want it to be called? 

They _do_ want it called.  They set the driver-model level one because
there hasn't been a pci-level one until a few years ago.

Re: [PATCH] pci: don't override drv->shutdown unconditionally

[Greg KH]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 964 bytes --]

On Fri, Jun 17, 2005 at 08:51:04PM +0200, Christoph Hellwig wrote:
> On Fri, Jun 17, 2005 at 11:49:14AM -0700, Greg KH wrote:
> > On Fri, Jun 17, 2005 at 08:30:57PM +0200, Christoph Hellwig wrote:
> > > There are many drivers that have been setting the generic driver
> > > model­level shutdown callback, and pci thus must not override it.
> > > 
> > > Without this patch we can have really bad data loss on various
> > > raid controllers.
> > 
> > Without the kexec patch?
> 
> On shutdown.  I don't know why you're talking about kexec here.

Because of the previous kexec comments on the linux-scsi list.

> > So, why are these drivers setting the shutdown function in the first
> > place if they don't want it to be called? 
> 
> They _do_ want it called.  They set the driver-model level one because
> there hasn't been a pci-level one until a few years ago.

So they are setting two callbacks?  Have a pointer to any driver that
does this?

thanks,

greg k-h

Re: [PATCH] pci: don't override drv->shutdown unconditionally

[Greg KH]

On Fri, Jun 17, 2005 at 11:53:11AM -0700, Greg KH wrote:
> On Fri, Jun 17, 2005 at 08:51:04PM +0200, Christoph Hellwig wrote:
> > They _do_ want it called.  They set the driver-model level one because
> > there hasn't been a pci-level one until a few years ago.
> 
> So they are setting two callbacks?  Have a pointer to any driver that
> does this?

Ok, I see the drivers that do this now, they should be fixed.  I'll make
up a patch now to do that.

thanks,

greg k-h

Re: [PATCH] pci: don't override drv->shutdown unconditionally

[Greg KH]

On Fri, Jun 17, 2005 at 12:01:33PM -0700, Greg KH wrote:
> On Fri, Jun 17, 2005 at 11:53:11AM -0700, Greg KH wrote:
> > On Fri, Jun 17, 2005 at 08:51:04PM +0200, Christoph Hellwig wrote:
> > > They _do_ want it called.  They set the driver-model level one because
> > > there hasn't been a pci-level one until a few years ago.
> > 
> > So they are setting two callbacks?  Have a pointer to any driver that
> > does this?
> 
> Ok, I see the drivers that do this now, they should be fixed.  I'll make
> up a patch now to do that.

Hm, that's too big of a patch for 2.6.12 this late in the game, I've
forwarded your patch on to Linus with an added comment.  I'll fix up the
drivers for 2.6.13 and then change the pci core back.

Sorry for the confusion, you were right.

thanks,

greg k-h

[PATCH] PCI: don't override drv->shutdown unconditionally

[Greg KH]

There are many drivers that have been setting the generic driver
model level shutdown callback, and pci thus must not override it.

Without this patch we can have really bad data loss on various
raid controllers.


From: Christoph Hellwig <hch@lst.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>


---
 drivers/pci/pci-driver.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletion(-)

--- gregkh-2.6.orig/drivers/pci/pci-driver.c	2005-06-16 15:19:44.000000000 -0700
+++ gregkh-2.6/drivers/pci/pci-driver.c	2005-06-17 12:17:34.000000000 -0700
@@ -395,7 +395,10 @@
 	drv->driver.bus = &pci_bus_type;
 	drv->driver.probe = pci_device_probe;
 	drv->driver.remove = pci_device_remove;
-	drv->driver.shutdown = pci_device_shutdown,
+	/* FIXME, once all of the existing PCI drivers have been fixed to set
+	 * the pci shutdown function, this test can go away. */
+	if (!drv->driver.shutdown)
+		drv->driver.shutdown = pci_device_shutdown,
 	drv->driver.owner = drv->owner;
 	drv->driver.kobj.ktype = &pci_driver_kobj_type;
 	pci_init_dynids(&drv->dynids);