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 <[email protected]>
------------------------------------------------------------------------------------------
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,
};
/*
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 <[email protected]>
> ------------------------------------------------------------------------------------------
> 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
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 <[email protected]>
> > ------------------------------------------------------------------------------------------
> > 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,
};
/*
> On Fri, 23 Feb 2007 14:42:39 -0600 "Mike Miller (OS Dev)" <[email protected]> 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.