[PATCH 1/2] fusion - removed unnecessary code in mptscsih_resume()

[PATCH 1/2] fusion - removed unnecessary code in mptscsih_resume()

[Horms]

It seems that most of the code in mptscsih_resume() doesn't
do anything. This patch removes that code.

Signed-off-by: Simon Horman <horms@verge.net.au>

Index: linux-2.6/drivers/message/fusion/mptscsih.c
===================================================================
--- linux-2.6.orig/drivers/message/fusion/mptscsih.c	2007-03-16 14:47:18.000000000 +0900
+++ linux-2.6/drivers/message/fusion/mptscsih.c	2007-03-16 14:48:53.000000000 +0900
@@ -1188,19 +1188,7 @@
 int
 mptscsih_resume(struct pci_dev *pdev)
 {
-	MPT_ADAPTER 		*ioc = pci_get_drvdata(pdev);
-	struct Scsi_Host 	*host = ioc->sh;
-	MPT_SCSI_HOST		*hd;
-
 	mpt_resume(pdev);
-
-	if(!host)
-		return 0;
-
-	hd = (MPT_SCSI_HOST *)host->hostdata;
-	if(!hd)
-		return 0;
-
 	return 0;
 }
 

[PATCH 2/2] fusion - honour return value of pci_enable_device() in mpt_resume()

[Horms]

Honour the return value of pci_enable_device(), which
seems to be a desirable thing to do:

  2.6.20-rc4
  gcc (GCC) 4.1.2 20061115 (prerelease) (Debian 4.1.1-21)

    CC [M]  drivers/message/fusion/mptbase.o
    drivers/message/fusion/mptbase.c: In function `mpt_resume':
    drivers/message/fusion/mptbase.c:1541: warning: ignoring return value
    of `pci_enable_device', declared with attribute warn_unused_result

It also in turn has mptscsih_resume() honour the return value of
mpt_resume()

I'm not sure about the handling of the other potential error cases
in mpt_resume(), of which there appear to be many. But this does
seem to be a good start.

Signed-off-by: Simon Horman <horms@verge.net.au>

Index: linux-2.6/drivers/message/fusion/mptbase.c
===================================================================
--- linux-2.6.orig/drivers/message/fusion/mptbase.c	2007-03-16 16:00:01.000000000 +0900
+++ linux-2.6/drivers/message/fusion/mptbase.c	2007-03-16 16:02:23.000000000 +0900
@@ -1531,6 +1531,7 @@
 	MPT_ADAPTER *ioc = pci_get_drvdata(pdev);
 	u32 device_state = pdev->current_state;
 	int recovery_state;
+	int err;
 
 	printk(MYIOC_s_INFO_FMT
 	"pci-resume: pdev=0x%p, slot=%s, Previous operating state [D%d]\n",
@@ -1538,7 +1539,9 @@
 
 	pci_set_power_state(pdev, 0);
 	pci_restore_state(pdev);
-	pci_enable_device(pdev);
+	err = pci_enable_device(pdev);
+	if (err < 0)
+		return err;
 
 	/* enable interrupts */
 	CHIPREG_WRITE32(&ioc->chip->IntMask, MPI_HIM_DIM);
Index: linux-2.6/drivers/message/fusion/mptscsih.c
===================================================================
--- linux-2.6.orig/drivers/message/fusion/mptscsih.c	2007-03-16 16:00:01.000000000 +0900
+++ linux-2.6/drivers/message/fusion/mptscsih.c	2007-03-16 16:02:23.000000000 +0900
@@ -1188,8 +1188,7 @@
 int
 mptscsih_resume(struct pci_dev *pdev)
 {
-	mpt_resume(pdev);
-	return 0;
+	return mpt_resume(pdev);
 }
 
 #endif

Re: [PATCH 2/2] fusion - honour return value of pci_enable_device() in mpt_resume()

[James Bottomley]

On Fri, 2007-03-16 at 16:05 +0900, Horms wrote:
> +       err = pci_enable_device(pdev);
> +       if (err < 0)
> +               return err;

Traditionally, this should be 

if (err)
	return err;

The reason is that <0 is a signed comparison which can be slightly more
expensive on some architectures and it's unnecessary if zero is the only
successful return.

James

Re: [PATCH 2/2] fusion - honour return value of pci_enable_device() in mpt_resume()

[Randy Dunlap]

On Fri, 16 Mar 2007 09:27:26 -0500 James Bottomley wrote:

> On Fri, 2007-03-16 at 16:05 +0900, Horms wrote:
> > +       err = pci_enable_device(pdev);
> > +       if (err < 0)
> > +               return err;
> 
> Traditionally, this should be 
> 
> if (err)
> 	return err;
> 
> The reason is that <0 is a signed comparison which can be slightly more
> expensive on some architectures and it's unnecessary if zero is the only
> successful return.

Tradition vs. Linus, eh?  Linus wrote (2007-Mar-06, on lkml,
Message-ID: <Pine.LNX.4.64.0703060817060.5963@woody.linux-foundation.org>):

 - "negative error values" should preferably always be tested as such

	if (tick_init_highres() < 0) {
		printk("Aieee! Couldn't init!\n");
		return 0;
	}


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 2/2] fusion - honour return value of pci_enable_device() in mpt_resume()

[James Bottomley]

On Fri, 2007-03-16 at 08:06 -0700, Randy Dunlap wrote:
> On Fri, 16 Mar 2007 09:27:26 -0500 James Bottomley wrote:
> 
> > On Fri, 2007-03-16 at 16:05 +0900, Horms wrote:
> > > +       err = pci_enable_device(pdev);
> > > +       if (err < 0)
> > > +               return err;
> > 
> > Traditionally, this should be 
> > 
> > if (err)
> > 	return err;
> > 
> > The reason is that <0 is a signed comparison which can be slightly more
> > expensive on some architectures and it's unnecessary if zero is the only
> > successful return.
> 
> Tradition vs. Linus, eh?  Linus wrote (2007-Mar-06, on lkml,
> Message-ID: <Pine.LNX.4.64.0703060817060.5963@woody.linux-foundation.org>):

Sure ... we can all maintain our own traditions .. what was the subject
of this email?

James

Re: [PATCH 2/2] fusion - honour return value of pci_enable_device() in mpt_resume()

[Randy Dunlap]

On Fri, 16 Mar 2007 11:14:51 -0500 James Bottomley wrote:

> On Fri, 2007-03-16 at 08:06 -0700, Randy Dunlap wrote:
> > On Fri, 16 Mar 2007 09:27:26 -0500 James Bottomley wrote:
> > 
> > > On Fri, 2007-03-16 at 16:05 +0900, Horms wrote:
> > > > +       err = pci_enable_device(pdev);
> > > > +       if (err < 0)
> > > > +               return err;
> > > 
> > > Traditionally, this should be 
> > > 
> > > if (err)
> > > 	return err;
> > > 
> > > The reason is that <0 is a signed comparison which can be slightly more
> > > expensive on some architectures and it's unnecessary if zero is the only
> > > successful return.
> > 
> > Tradition vs. Linus, eh?  Linus wrote (2007-Mar-06, on lkml,
> > Message-ID: <Pine.LNX.4.64.0703060817060.5963@woody.linux-foundation.org>):
> 
> Sure ... we can all maintain our own traditions .. what was the subject
> of this email?

The subject was coding style and return/error codes.
The Subject: line was: Re: [5/6] 2.6.21-rc2: known regressions


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 2/2] fusion - honour return value of pci_enable_device() in mpt_resume()

[Douglas Gilbert]

Randy Dunlap wrote:
> On Fri, 16 Mar 2007 11:14:51 -0500 James Bottomley wrote:
> 
>> On Fri, 2007-03-16 at 08:06 -0700, Randy Dunlap wrote:
>>> On Fri, 16 Mar 2007 09:27:26 -0500 James Bottomley wrote:
>>>
>>>> On Fri, 2007-03-16 at 16:05 +0900, Horms wrote:
>>>>> +       err = pci_enable_device(pdev);
>>>>> +       if (err < 0)
>>>>> +               return err;
>>>> Traditionally, this should be 
>>>>
>>>> if (err)
>>>> 	return err;
>>>>
>>>> The reason is that <0 is a signed comparison which can be slightly more
>>>> expensive on some architectures and it's unnecessary if zero is the only
>>>> successful return.
>>> Tradition vs. Linus, eh?  Linus wrote (2007-Mar-06, on lkml,
>>> Message-ID: <Pine.LNX.4.64.0703060817060.5963@woody.linux-foundation.org>):
>> Sure ... we can all maintain our own traditions .. what was the subject
>> of this email?
> 
> The subject was coding style and return/error codes.
> The Subject: line was: Re: [5/6] 2.6.21-rc2: known regressions

Randy,
While on the subject of traditions, how about the
C90 and C99 ones?

C identifiers starting with "__" are reserved!
Reference: ISO/IEC 9899:1999 (C99) section 7.1.3 "All
identifiers that start with an underscore and either
an upper case letter or another underscore are always
reserved for any use". It was the same in C90.

Now we might start getting rid of __u32 and its
friends first :-)

Doug Gilbert

Re: [PATCH 2/2] fusion - honour return value of pci_enable_device() in mpt_resume()

[Horms]

On Fri, Mar 16, 2007 at 09:27:26AM -0500, James Bottomley wrote:
> On Fri, 2007-03-16 at 16:05 +0900, Horms wrote:
> > +       err = pci_enable_device(pdev);
> > +       if (err < 0)
> > +               return err;
> 
> Traditionally, this should be 
> 
> if (err)
> 	return err;
> 
> The reason is that <0 is a signed comparison which can be slightly more
> expensive on some architectures and it's unnecessary if zero is the only
> successful return.

That isn't a tradition that I am familiar with, but it seems reasonable
to me.

Updated patch is below.

-- 
Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/

Honour the return value of pci_enable_device(), which
seems to be a desirable thing to do:

  2.6.20-rc4
  gcc (GCC) 4.1.2 20061115 (prerelease) (Debian 4.1.1-21)

    CC [M]  drivers/message/fusion/mptbase.o
    drivers/message/fusion/mptbase.c: In function `mpt_resume':
    drivers/message/fusion/mptbase.c:1541: warning: ignoring return value
    of `pci_enable_device', declared with attribute warn_unused_result

It also in turn has mptscsih_resume() honour the return value of
mpt_resume()

I'm not sure about the handling of the other potential error cases
in mpt_resume(), of which there appear to be many. But this does
seem to be a good start.

Signed-off-by: Simon Horman <horms@verge.net.au>

Index: linux-2.6/drivers/message/fusion/mptbase.c
===================================================================
--- linux-2.6.orig/drivers/message/fusion/mptbase.c	2007-03-19 10:59:18.000000000 +0900
+++ linux-2.6/drivers/message/fusion/mptbase.c	2007-03-19 15:04:24.000000000 +0900
@@ -1531,6 +1531,7 @@
 	MPT_ADAPTER *ioc = pci_get_drvdata(pdev);
 	u32 device_state = pdev->current_state;
 	int recovery_state;
+	int err;
 
 	printk(MYIOC_s_INFO_FMT
 	"pci-resume: pdev=0x%p, slot=%s, Previous operating state [D%d]\n",
@@ -1538,7 +1539,9 @@
 
 	pci_set_power_state(pdev, 0);
 	pci_restore_state(pdev);
-	pci_enable_device(pdev);
+	err = pci_enable_device(pdev);
+	if (err)
+		return err;
 
 	/* enable interrupts */
 	CHIPREG_WRITE32(&ioc->chip->IntMask, MPI_HIM_DIM);
Index: linux-2.6/drivers/message/fusion/mptscsih.c
===================================================================
--- linux-2.6.orig/drivers/message/fusion/mptscsih.c	2007-03-19 15:03:22.000000000 +0900
+++ linux-2.6/drivers/message/fusion/mptscsih.c	2007-03-19 15:03:23.000000000 +0900
@@ -1188,8 +1188,7 @@
 int
 mptscsih_resume(struct pci_dev *pdev)
 {
-	mpt_resume(pdev);
-	return 0;
+	return mpt_resume(pdev);
 }
 
 #endif

RE: [PATCH 2/2] fusion - honour return value of pci_enable_device() in mpt_resume()

[Moore, Eric]

On Friday, March 16, 2007 1:06 AM,  Horms wrote:

> Honour the return value of pci_enable_device(), which
> seems to be a desirable thing to do:

Both patch's look good.

Thanks,
Eric