[Patch 2/2] cciss: supercedes add shutdown support (replaces reboot notifier)

[Patch 2/2] cciss: supercedes add shutdown support (replaces reboot notifier)

[Mike Miller (OS Dev)]

Patch 2/2

This patch supercedes yesterdays cciss-shutdown patch. The primary difference is
removing __devexit from cciss_remove_one. Instead of create another function I'd
rather use the code that was intended to perform the cleanup and cache flush. I've
tested as a loadable module and statically linked without error.
Please consider this for inclusion.

Signed-off-by: Mike Miller <mike.miller@hp.com>
------------------------------------------------------------------------------------------
diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 9d84ab3..b16f48c 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -3404,7 +3404,7 @@ #endif
 	return -1;
 }
 
-static void __devexit cciss_remove_one(struct pci_dev *pdev)
+static void cciss_remove_one(struct pci_dev *pdev)
 {
 	ctlr_info_t *tmp_ptr;
 	int i, j;
@@ -3428,9 +3428,10 @@ static void __devexit cciss_remove_one(s
 	memset(flush_buf, 0, 4);
 	return_code = sendcmd(CCISS_CACHE_FLUSH, i, flush_buf, 4, 0, 0, 0, NULL,
 			      TYPE_CMD);
-	if (return_code != IO_OK) {
-		printk(KERN_WARNING "Error Flushing cache on controller %d\n",
-		       i);
+	if (return_code == IO_OK) {
+		printk(KERN_WARNING "Completed flushing cache on controller %d\n", i);
+	} else {
+		printk(KERN_WARNING "Error flushing cache on controller %d\n", i);
 	}
 	free_irq(hba[i]->intr[2], hba[i]);
 
@@ -3481,6 +3482,7 @@ static struct pci_driver cciss_pci_drive
 	.probe = cciss_init_one,
 	.remove = __devexit_p(cciss_remove_one),
 	.id_table = cciss_pci_device_id,	/* id_table */
+	.shutdown = cciss_remove_one,
 };
 
 /*

Re: [Patch 2/2] cciss: supercedes add shutdown support (replaces reboot notifier)

[Greg KH]

On Fri, Feb 23, 2007 at 02:42:39PM -0600, Mike Miller (OS Dev) wrote:
> Patch 2/2
> 
> This patch supercedes yesterdays cciss-shutdown patch. The primary difference is
> removing __devexit from cciss_remove_one. Instead of create another function I'd
> rather use the code that was intended to perform the cleanup and cache flush. I've
> tested as a loadable module and statically linked without error.
> Please consider this for inclusion.
> 
> Signed-off-by: Mike Miller <mike.miller@hp.com>
> ------------------------------------------------------------------------------------------
> diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
> index 9d84ab3..b16f48c 100644
> --- a/drivers/block/cciss.c
> +++ b/drivers/block/cciss.c
> @@ -3404,7 +3404,7 @@ #endif
>  	return -1;
>  }
>  
> -static void __devexit cciss_remove_one(struct pci_dev *pdev)
> +static void cciss_remove_one(struct pci_dev *pdev)
>  {
>  	ctlr_info_t *tmp_ptr;
>  	int i, j;
> @@ -3428,9 +3428,10 @@ static void __devexit cciss_remove_one(s
>  	memset(flush_buf, 0, 4);
>  	return_code = sendcmd(CCISS_CACHE_FLUSH, i, flush_buf, 4, 0, 0, 0, NULL,
>  			      TYPE_CMD);
> -	if (return_code != IO_OK) {
> -		printk(KERN_WARNING "Error Flushing cache on controller %d\n",
> -		       i);
> +	if (return_code == IO_OK) {
> +		printk(KERN_WARNING "Completed flushing cache on controller %d\n", i);

Why do you want the world to know this on every shutdown?

At the least, use dev_warn() to show the proper device and driver that
this is happening to, but does it really need to go to the warning log?
It sounds like a normal operation to me.  How about dev_dbg() instead?

thanks,

greg k-h

Re: [Patch 2/2] cciss: supercedes add shutdown support (replaces reboot notifier)

[Mike Miller (OS Dev)]

On Fri, Feb 23, 2007 at 01:32:36PM -0800, Greg KH wrote:
> On Fri, Feb 23, 2007 at 02:42:39PM -0600, Mike Miller (OS Dev) wrote:
> > Patch 2/2
> > 
> > This patch supercedes yesterdays cciss-shutdown patch. The primary difference is
> > removing __devexit from cciss_remove_one. Instead of create another function I'd
> > rather use the code that was intended to perform the cleanup and cache flush. I've
> > tested as a loadable module and statically linked without error.
> > Please consider this for inclusion.
> > 
> > Signed-off-by: Mike Miller <mike.miller@hp.com>
> > ------------------------------------------------------------------------------------------
> > diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
> > index 9d84ab3..b16f48c 100644
> > --- a/drivers/block/cciss.c
> > +++ b/drivers/block/cciss.c
> > @@ -3404,7 +3404,7 @@ #endif
> >  	return -1;
> >  }
> >  
> > -static void __devexit cciss_remove_one(struct pci_dev *pdev)
> > +static void cciss_remove_one(struct pci_dev *pdev)
> >  {
> >  	ctlr_info_t *tmp_ptr;
> >  	int i, j;
> > @@ -3428,9 +3428,10 @@ static void __devexit cciss_remove_one(s
> >  	memset(flush_buf, 0, 4);
> >  	return_code = sendcmd(CCISS_CACHE_FLUSH, i, flush_buf, 4, 0, 0, 0, NULL,
> >  			      TYPE_CMD);
> > -	if (return_code != IO_OK) {
> > -		printk(KERN_WARNING "Error Flushing cache on controller %d\n",
> > -		       i);
> > +	if (return_code == IO_OK) {
> > +		printk(KERN_WARNING "Completed flushing cache on controller %d\n", i);
> 
> Why do you want the world to know this on every shutdown?
> 
> At the least, use dev_warn() to show the proper device and driver that
> this is happening to, but does it really need to go to the warning log?
> It sounds like a normal operation to me.  How about dev_dbg() instead?
> 
> thanks,
> 
> greg k-h

OK, here's a version that only prints if a problem occurred. That's the way the code
was originally written.

-- mikem
------------------------------------------------------------------------------------------
diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 9d84ab3..c50c09b 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -3404,7 +3404,7 @@ #endif
 	return -1;
 }
 
-static void __devexit cciss_remove_one(struct pci_dev *pdev)
+static void cciss_remove_one(struct pci_dev *pdev)
 {
 	ctlr_info_t *tmp_ptr;
 	int i, j;
@@ -3429,8 +3429,7 @@ static void __devexit cciss_remove_one(s
 	return_code = sendcmd(CCISS_CACHE_FLUSH, i, flush_buf, 4, 0, 0, 0, NULL,
 			      TYPE_CMD);
 	if (return_code != IO_OK) {
-		printk(KERN_WARNING "Error Flushing cache on controller %d\n",
-		       i);
+		printk(KERN_WARNING "Error flushing cache on controller %d\n", i);
 	}
 	free_irq(hba[i]->intr[2], hba[i]);
 
@@ -3481,6 +3480,7 @@ static struct pci_driver cciss_pci_drive
 	.probe = cciss_init_one,
 	.remove = __devexit_p(cciss_remove_one),
 	.id_table = cciss_pci_device_id,	/* id_table */
+	.shutdown = cciss_remove_one,
 };
 
 /*

Re: [Patch 2/2] cciss: supercedes add shutdown support (replaces reboot notifier)

[Andrew Morton]

> On Fri, 23 Feb 2007 14:42:39 -0600 "Mike Miller (OS Dev)" <mikem@beardog.cca.cpqcorp.net> wrote:
> This patch supercedes yesterdays cciss-shutdown patch. The primary difference is
> removing __devexit from cciss_remove_one. Instead of create another function I'd
> rather use the code that was intended to perform the cleanup and cache flush. I've
> tested as a loadable module and statically linked without error.
> Please consider this for inclusion.

Please don't document patches like this.

The entirety of your changelog and the Subject: are relative to a patch
which will never hit the mainline git tree.  Put yourself in the position
of someone reading the git changelogs in a year's time.  They're going to
be left scratching their heads at the above, aren't they?

Always include a complete and standalone, not-referential-to-an-old-patch
changelog in each iteration of a patch.

Always choose a suitable Subject:

Yes, it's good to tell us things about how this patch differs from the
previous one.  That info can be placed after the ^--- which comes after
your signed-off-by:, or can be placed at the top of the email, as long as
the full permanent changelog is there too.

Bottom line: you are submitting code and its documentation into the
permanent kernel record, not just the mailing list.  Try to make it
appropriate, thanks.


Please send a new Subject: and changlog for this patch.

Re: [Patch 2/2] cciss: add struct pci_driver shutdown support (replaces reboot notifier)

[Mike Miller (OS Dev)]

On Tue, Feb 27, 2007 at 12:26:34PM -0800, Andrew Morton wrote:
> > On Fri, 23 Feb 2007 14:42:39 -0600 "Mike Miller (OS Dev)" <mikem@beardog.cca.cpqcorp.net> wrote:
> > This patch supercedes yesterdays cciss-shutdown patch. The primary difference is
> > removing __devexit from cciss_remove_one. Instead of create another function I'd
> > rather use the code that was intended to perform the cleanup and cache flush. I've
> > tested as a loadable module and statically linked without error.
> > Please consider this for inclusion.
> 
> Please don't document patches like this.
> 
> The entirety of your changelog and the Subject: are relative to a patch
> which will never hit the mainline git tree.  Put yourself in the position
> of someone reading the git changelogs in a year's time.  They're going to
> be left scratching their heads at the above, aren't they?
> 
> Always include a complete and standalone, not-referential-to-an-old-patch
> changelog in each iteration of a patch.
> 
> Always choose a suitable Subject:
> 
> Yes, it's good to tell us things about how this patch differs from the
> previous one.  That info can be placed after the ^--- which comes after
> your signed-off-by:, or can be placed at the top of the email, as long as
> the full permanent changelog is there too.
> 
> Bottom line: you are submitting code and its documentation into the
> permanent kernel record, not just the mailing list.  Try to make it
> appropriate, thanks.
> 
> 
> Please send a new Subject: and changlog for this patch.
> 
Patch 2/2

This patch adds support for the struct pci_driver shutdown method to cciss. We require
notification of an impending reboot or shutdown so that we can flush the battery
backed write cache (BBWC) on the Smart Array controller.
Please consider this for inclusion.

Signed-off-by: Mike Miller <mike.miller@hp.com>
------------------------------------------------------------------------------------------
diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 9d84ab3..b16f48c 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -3404,7 +3404,7 @@ #endif
 	return -1;
 }
 
-static void __devexit cciss_remove_one(struct pci_dev *pdev)
+static void cciss_remove_one(struct pci_dev *pdev)
 {
 	ctlr_info_t *tmp_ptr;
 	int i, j;
@@ -3428,9 +3428,10 @@ static void __devexit cciss_remove_one(s
 	memset(flush_buf, 0, 4);
 	return_code = sendcmd(CCISS_CACHE_FLUSH, i, flush_buf, 4, 0, 0, 0, NULL,
 			      TYPE_CMD);
-	if (return_code != IO_OK) {
-		printk(KERN_WARNING "Error Flushing cache on controller %d\n",
-		       i);
+	if (return_code == IO_OK) {
+		printk(KERN_INFO "Completed flushing cache on controller %d\n", i);
+	} else {
+		printk(KERN_WARNING "Error flushing cache on controller %d\n", i);
 	}
 	free_irq(hba[i]->intr[2], hba[i]);
 
@@ -3481,6 +3482,7 @@ static struct pci_driver cciss_pci_drive
 	.probe = cciss_init_one,
 	.remove = __devexit_p(cciss_remove_one),
 	.id_table = cciss_pci_device_id,	/* id_table */
+	.shutdown = cciss_remove_one,
 };
 
 /*