Skip to contentSkip to conversation

cciss: add 30 second initial timeout wait on controller reset

PATCH In review Tomas Henzl 2 participants 5 messages Mar 15, 2010 – May 19, 2010

Subsystems (2)
SCSI Block layer
lore
Share & tools
lore

Tomas Henzl

author
#0 lore raw

PATCH 1/1 cciss: add 30 second initial timeout wait on controller reset

It looks like the patch - cciss: remove 30 second initial timeout on controller reset
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=5e18cfd04feca78cc08a6b8b71a60a610de81eaa
has caused a regression.
During kdump a box with an 5i controller freezes.
The HP Smart Array 5i Controller probably needs some more time
of inactivity after reset.
To get rid of it we can revert the above mentioned patch or use
the patch below which adds an additional timeout only for this
one controller (HP Smart Array 5i). I haven't seen this with other cciss controllers.

cciss: add 30 second initial timeout wait on controller reset

Signed-off-by Tomas Henzl <[email protected]>
drivers/block/cciss.c +7 -3
index 9e3af30..34ec2c7 100644
@@ -4172,9 +4172,13 @@ static int __devinit cciss_init_one(struct pci_dev *pdev,
 		if (cciss_hard_reset_controller(pdev) || cciss_reset_msi(pdev))
 			return -ENODEV;
 
-		/* Now try to get the controller to respond to a no-op. Some
-		   devices (notably the HP Smart Array 5i Controller) need
-		   up to 30 seconds to respond. */
+		/* The HP Smart Array 5i Controller needs
+		 * at least 20 seconds before first status checking
+		 * set it to 30 seconds for this controller to be sure */
+		if (0x4080 == pdev->subsystem_device)
+			schedule_timeout_uninterruptible(30*HZ);
+
+		/* Now try to get the controller to respond to a no-op. */
 		for (i=0; i<30; i++) {
 			if (cciss_noop(pdev) == 0)
 				break;
#1 lore raw
Quoted 21 lines · Tomas Henzl

It looks like the patch - cciss: remove 30 second initial timeout on controller reset
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=5e18cfd04feca78cc08a6b8b71a60a610de81eaa
has caused a regression.
During kdump a box with an 5i controller freezes.
The HP Smart Array 5i Controller probably needs some more time
of inactivity after reset.
To get rid of it we can revert the above mentioned patch or use
the patch below which adds an additional timeout only for this
one controller (HP Smart Array 5i). I haven't seen this with other cciss controllers.

cciss: add 30 second initial timeout wait on controller reset

Signed-off-by Tomas Henzl <[email protected]>
drivers/block/cciss.c +0 -0
index 9e3af30..34ec2c7 100644
@@ -4172,9 +4172,13 @@ static int __devinit cciss_init_one(struct pci_dev *pdev,
 		if (cciss_hard_reset_controller(pdev) || cciss_reset_msi(pdev))

drivers/block/cciss.c static int __devinit cciss_init_one(struct pci_dev *pdev, · line 4179

Quoted 10 lines
drivers/block/cciss.c +5 -3
 			return -ENODEV;
 
-		/* Now try to get the controller to respond to a no-op. Some
-		   devices (notably the HP Smart Array 5i Controller) need
-		   up to 30 seconds to respond. */
+		/* The HP Smart Array 5i Controller needs
+		 * at least 20 seconds before first status checking
+		 * set it to 30 seconds for this controller to be sure */
+		if (0x4080 == pdev->subsystem_device)
+			schedule_timeout_uninterruptible(30*HZ);

The HZ thing is deprecated, plus if you really want an interruptible sleep, you need to check for signals going in.

It's far better to use msleep_interruptible, which does all of this for you (of course, it might be even better if we had ssleep_interruptible).

James

Quoted 11 lines
drivers/block/cciss.c +2 -1
+
+		/* Now try to get the controller to respond to a no-op. */
 		for (i=0; i<30; i++) {
 			if (cciss_noop(pdev) == 0)
 				break;
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#2 lore raw
Quoted 37 lines · James Bottomley
Quoted 22 lines · Tomas Henzl

It looks like the patch - cciss: remove 30 second initial timeout on controller reset
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=5e18cfd04feca78cc08a6b8b71a60a610de81eaa
has caused a regression.
During kdump a box with an 5i controller freezes.
The HP Smart Array 5i Controller probably needs some more time
of inactivity after reset.
To get rid of it we can revert the above mentioned patch or use
the patch below which adds an additional timeout only for this
one controller (HP Smart Array 5i). I haven't seen this with other cciss controllers.

cciss: add 30 second initial timeout wait on controller reset

Signed-off-by Tomas Henzl <[email protected]>
drivers/block/cciss.c +0 -0
index 9e3af30..34ec2c7 100644
@@ -4172,9 +4172,13 @@ static int __devinit cciss_init_one(struct pci_dev *pdev,
 		if (cciss_hard_reset_controller(pdev) || cciss_reset_msi(pdev))
 			return -ENODEV;

drivers/block/cciss.c static int __devinit cciss_init_one(struct pci_dev *pdev, · line 4180

Quoted 10 lines
drivers/block/cciss.c +5 -3
 
-		/* Now try to get the controller to respond to a no-op. Some
-		   devices (notably the HP Smart Array 5i Controller) need
-		   up to 30 seconds to respond. */
+		/* The HP Smart Array 5i Controller needs
+		 * at least 20 seconds before first status checking
+		 * set it to 30 seconds for this controller to be sure */
+		if (0x4080 == pdev->subsystem_device)
+			schedule_timeout_uninterruptible(30*HZ);

The HZ thing is deprecated, plus if you really want an interruptible sleep, you need to check for signals going in.

Quoted 5 lines

It's far better to use msleep_interruptible, which does all of this for you (of course, it might be even better if we had ssleep_interruptible).

James

schedule_timeout_uninterruptible is used in this module and this function so it would keep the style.
I don't want to care about signals so ssleep is fine?

Tomas

drivers/block/cciss.c +7 -3
index 9e3af30..6696eb6 100644
@@ -4172,9 +4172,13 @@ static int __devinit cciss_init_one(struct pci_dev *pdev,
 		if (cciss_hard_reset_controller(pdev) || cciss_reset_msi(pdev))
 			return -ENODEV;
 
-		/* Now try to get the controller to respond to a no-op. Some
-		   devices (notably the HP Smart Array 5i Controller) need
-		   up to 30 seconds to respond. */
+		/* The HP Smart Array 5i Controller needs
+		 * at least 20 seconds before first status checking
+		 * set it to 30 seconds for this controller to be sure */
+		if (0x4080 == pdev->subsystem_device)
+			ssleep(30);
+
+		/* Now try to get the controller to respond to a no-op. */
 		for (i=0; i<30; i++) {
 			if (cciss_noop(pdev) == 0)
 				break;
#3 lore raw
Quoted 41 lines · Tomas Henzl
Quoted 40 lines · James Bottomley
Quoted 22 lines · Tomas Henzl

It looks like the patch - cciss: remove 30 second initial timeout on controller reset
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=5e18cfd04feca78cc08a6b8b71a60a610de81eaa
has caused a regression.
During kdump a box with an 5i controller freezes.
The HP Smart Array 5i Controller probably needs some more time
of inactivity after reset.
To get rid of it we can revert the above mentioned patch or use
the patch below which adds an additional timeout only for this
one controller (HP Smart Array 5i). I haven't seen this with other cciss controllers.

cciss: add 30 second initial timeout wait on controller reset

Signed-off-by Tomas Henzl <[email protected]>
drivers/block/cciss.c +0 -0
index 9e3af30..34ec2c7 100644
@@ -4172,9 +4172,13 @@ static int __devinit cciss_init_one(struct pci_dev *pdev,
 		if (cciss_hard_reset_controller(pdev) || cciss_reset_msi(pdev))
 			return -ENODEV;

drivers/block/cciss.c static int __devinit cciss_init_one(struct pci_dev *pdev, · line 4180

Quoted 10 lines
drivers/block/cciss.c +5 -3
 
-		/* Now try to get the controller to respond to a no-op. Some
-		   devices (notably the HP Smart Array 5i Controller) need
-		   up to 30 seconds to respond. */
+		/* The HP Smart Array 5i Controller needs
+		 * at least 20 seconds before first status checking
+		 * set it to 30 seconds for this controller to be sure */
+		if (0x4080 == pdev->subsystem_device)
+			schedule_timeout_uninterruptible(30*HZ);

The HZ thing is deprecated, plus if you really want an interruptible sleep, you need to check for signals going in.

It's far better to use msleep_interruptible, which does all of this for you (of course, it might be even better if we had ssleep_interruptible).

Quoted 6 lines
Quoted 2 lines

James

schedule_timeout_uninterruptible is used in this module and this function so it would keep the style.
I don't want to care about signals so ssleep is fine?

Yes, sorry, we've so many different variants of functions that all do approximately the same thing that I sometimes get confused ... I misread the above for schedule_timeout_interruptible (missing the un). So ssleep is perfect.

James

author
#4 lore raw
Quoted 53 lines · James Bottomley
Quoted 44 lines · Tomas Henzl
Quoted 42 lines · James Bottomley
Quoted 23 lines · Tomas Henzl

It looks like the patch - cciss: remove 30 second initial timeout on controller reset
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=5e18cfd04feca78cc08a6b8b71a60a610de81eaa
has caused a regression.
During kdump a box with an 5i controller freezes.
The HP Smart Array 5i Controller probably needs some more time
of inactivity after reset.
To get rid of it we can revert the above mentioned patch or use
the patch below which adds an additional timeout only for this
one controller (HP Smart Array 5i). I haven't seen this with other cciss controllers.

cciss: add 30 second initial timeout wait on controller reset

Signed-off-by Tomas Henzl <[email protected]>
drivers/block/cciss.c +0 -0
index 9e3af30..34ec2c7 100644
@@ -4172,9 +4172,13 @@ static int __devinit cciss_init_one(struct pci_dev *pdev,
 		if (cciss_hard_reset_controller(pdev) || cciss_reset_msi(pdev))
 			return -ENODEV;

drivers/block/cciss.c static int __devinit cciss_init_one(struct pci_dev *pdev, · line 4181

Quoted 10 lines
drivers/block/cciss.c +5 -3
-		/* Now try to get the controller to respond to a no-op. Some
-		   devices (notably the HP Smart Array 5i Controller) need
-		   up to 30 seconds to respond. */
+		/* The HP Smart Array 5i Controller needs
+		 * at least 20 seconds before first status checking
+		 * set it to 30 seconds for this controller to be sure */
+		if (0x4080 == pdev->subsystem_device)
+			schedule_timeout_uninterruptible(30*HZ);

The HZ thing is deprecated, plus if you really want an interruptible sleep, you need to check for signals going in.

It's far better to use msleep_interruptible, which does all of this for you (of course, it might be even better if we had ssleep_interruptible).

Quoted 7 lines
Quoted 3 lines

James

schedule_timeout_uninterruptible is used in this module and this function so it would keep the style.
I don't want to care about signals so ssleep is fine?

Quoted 7 lines

Yes, sorry, we've so many different variants of functions that all do approximately the same thing that I sometimes get confused ... I misread the above for schedule_timeout_interruptible (missing the un). So ssleep is perfect.

James

Is there something more I could do for this patch?

Tomas