hpsa: minor fixes and cleanups
Subsystems (2)
cover hpsa: minor fixes and cleanups
The following series implements some minor fixes and cleanups for hpsa
---
Stephen M. Cameron (11):
hpsa: use workqueue instead of kernel thread for lockup detection
hpsa: do not attempt to flush the cache on locked up controllers
hpsa: add 5 second delay after doorbell reset
hpsa: do not discard scsi status on aborted commands
hpsa: remove unneeded include of seq_file.h
hpsa: fix memory leak in CCISS_BIG_PASSTHRU ioctl
hpsa: add MSA 2040 to list of external target devices
hpsa: cap CCISS_PASSTHRU at 20 concurrent commands.
hpsa: prevent stalled i/o
hpsa: rename scsi prefetch field
hpsa: enable unit attention reporting
drivers/scsi/hpsa.c | 252 +++++++++++++++++++++++++++-------------------- drivers/scsi/hpsa.h | 9 +- drivers/scsi/hpsa_cmd.h | 4 + 3 files changed, 157 insertions(+), 108 deletions(-)
Signature · 1 line
-- steve
PATCH 1/11 hpsa: use workqueue instead of kernel thread for lockup detection
From: Stephen M. Cameron <[email protected]>
Much simpler and avoids races starting/stopping the thread.
--- drivers/scsi/hpsa.c | 103 +++++++++++++-------------------------------------- drivers/scsi/hpsa.h | 3 + 2 files changed, 28 insertions(+), 78 deletions(-)
drivers/scsi/hpsa.c +26 -77
@@ -172,10 +172,6 @@ static struct board_type products[] = {static int number_of_controllers; -static struct list_head hpsa_ctlr_list =LIST_HEAD_INIT (hpsa_ctlr_list); -static spinlock_t lockup_detector_lock; -static struct task_struct *hpsa_lockup_detector; -static irqreturn_t do_hpsa_intr_intx (int irq,void *dev_id);static irqreturn_t do_hpsa_intr_msi (int irq,void *dev_id);static int hpsa_ioctl (struct scsi_device *dev,int cmd,void *arg);
@@ -4638,16 +4634,6 @@ static void hpsa_undo_allocations_after_kdump_soft_reset(struct ctlr_info *h)kfree (h); } -static void remove_ctlr_from_lockup_detector_list (struct ctlr_info *h) -{ -assert_spin_locked (&lockup_detector_lock); -if (!hpsa_lockup_detector) -return ; -if (h->lockup_detected) -return ;/* already stopped the lockup detector */ -list_del (&h->lockup_list); -} -/* Called when controller lockup detected. */ static void fail_all_cmds_on_list (struct ctlr_info *h,struct list_head *list) {
@@ -4666,8 +4652,6 @@ static void controller_lockup_detected(struct ctlr_info *h) {unsigned long flags; -assert_spin_locked (&lockup_detector_lock); -remove_ctlr_from_lockup_detector_list (h); h->access.set_intr_mask (h,HPSA_INTR_OFF );spin_lock_irqsave (&h->lock, flags); h->lockup_detected =readl (h->vaddr +SA5_SCRATCHPAD_OFFSET );
@@ -4687,7 +4671,6 @@ static void detect_controller_lockup(struct ctlr_info *h)u32 heartbeat;unsigned long flags; -assert_spin_locked (&lockup_detector_lock); now =get_jiffies_64 ();/* If we've received an interrupt recently, we're ok. */ if (time_after64(h->last_intr_timestamp +
@@ -4717,68 +4700,22 @@ static void detect_controller_lockup(struct ctlr_info *h) h->last_heartbeat_timestamp = now; } -static int detect_controller_lockup_thread (void *notused) -{ -struct ctlr_info *h; -unsigned long flags; - -while (1 ) { -struct list_head *this, *tmp; - -schedule_timeout_interruptible (HEARTBEAT_SAMPLE_INTERVAL ); -if (kthread_should_stop ()) -break ; -spin_lock_irqsave (&lockup_detector_lock, flags); -list_for_each_safe (this, tmp, &hpsa_ctlr_list) { - h = list_entry(this, struct ctlr_info, lockup_list); -detect_controller_lockup (h); - } -spin_unlock_irqrestore (&lockup_detector_lock, flags); - } -return 0 ; -} - -static void add_ctlr_to_lockup_detector_list (struct ctlr_info *h) +static void hpsa_monitor_ctlr_worker (struct work_struct *work) {unsigned long flags; - - h->heartbeat_sample_interval =HEARTBEAT_SAMPLE_INTERVAL ; -spin_lock_irqsave (&lockup_detector_lock, flags); -list_add_tail (&h->lockup_list, &hpsa_ctlr_list); -spin_unlock_irqrestore (&lockup_detector_lock, flags); -} - -static void start_controller_lockup_detector (struct ctlr_info *h) -{ -/* Start the lockup detector thread if not already started */ -if (!hpsa_lockup_detector) { -spin_lock_init (&lockup_detector_lock); - hpsa_lockup_detector = -kthread_run (detect_controller_lockup_thread, -NULL ,HPSA ); - } -if (!hpsa_lockup_detector) { -dev_warn (&h->pdev->dev, -"Could not start lockup detector thread\n" ); +struct ctlr_info *h =container_of (to_delayed_work (work), + struct ctlr_info, monitor_ctlr_work); +detect_controller_lockup (h); +if (h->lockup_detected) +return ; +spin_lock_irqsave (&h->lock, flags); +if (h->remove_in_progress) { +spin_unlock_irqrestore (&h->lock, flags);return ; } -add_ctlr_to_lockup_detector_list (h); -} - -static void stop_controller_lockup_detector (struct ctlr_info *h) -{ -unsigned long flags; - -spin_lock_irqsave (&lockup_detector_lock, flags); -remove_ctlr_from_lockup_detector_list (h); -/* If the list of ctlr's to monitor is empty, stop the thread */ -if (list_empty (&hpsa_ctlr_list)) { -spin_unlock_irqrestore (&lockup_detector_lock, flags); -kthread_stop (hpsa_lockup_detector); -spin_lock_irqsave (&lockup_detector_lock, flags); - hpsa_lockup_detector =NULL ; - } -spin_unlock_irqrestore (&lockup_detector_lock, flags); +schedule_delayed_work (&h->monitor_ctlr_work, + h->heartbeat_sample_interval); +spin_unlock_irqrestore (&h->lock, flags); }static int hpsa_init_one (struct pci_dev *pdev,const struct pci_device_id *ent)
@@ -4925,7 +4862,12 @@ reinit_after_soft_reset:hpsa_hba_inquiry (h);hpsa_register_scsi (h);/* hook ourselves into SCSI subsystem */ -start_controller_lockup_detector (h); + +/* Monitor the controller for firmware lockups */ + h->heartbeat_sample_interval =HEARTBEAT_SAMPLE_INTERVAL ; +INIT_DELAYED_WORK (&h->monitor_ctlr_work, hpsa_monitor_ctlr_worker); +schedule_delayed_work (&h->monitor_ctlr_work, + h->heartbeat_sample_interval);return 0 ; clean4:
@@ -4991,13 +4933,20 @@ static void hpsa_free_device_info(struct ctlr_info *h)static void hpsa_remove_one (struct pci_dev *pdev) {struct ctlr_info *h; +unsigned long flags;if (pci_get_drvdata (pdev) ==NULL ) {dev_err (&pdev->dev,"unable to remove device\n" );return ; } h =pci_get_drvdata (pdev); -stop_controller_lockup_detector (h); + +/* Get rid of any controller monitoring work items */ +spin_lock_irqsave (&h->lock, flags); + h->remove_in_progress =1 ; +cancel_delayed_work (&h->monitor_ctlr_work); +spin_unlock_irqrestore (&h->lock, flags); +hpsa_unregister_scsi (h);/* unhook from SCSI subsystem */ hpsa_shutdown (pdev);iounmap (h->vaddr);
drivers/scsi/hpsa.h +2 -1
@@ -130,7 +130,8 @@ struct ctlr_info {u32 heartbeat_sample_interval;atomic_t firmware_flash_in_progress;u32 lockup_detected; -struct list_head lockup_list; +struct delayed_work monitor_ctlr_work; +int remove_in_progress;/* Address of h->q[x] is passed to intr handler to know which queue */ u8 q[MAX_REPLY_QUEUES ];u32 TMFSupportFlags;/* cache what task mgmt funcs are supported. */
James Bottomley
maintainerRe: [PATCH 01/11] hpsa: use workqueue instead of kernel thread for lockup detection
Quoted 5 lines · Stephen M. Cameron
From: Stephen M. Cameron <[email protected]>
Much simpler and avoids races starting/stopping the thread.
Signed-off-by Stephen M. Cameron <[email protected]>
Actually, perhaps you do need to resubmit the series: this is rejecting against scsi-misc because of your prior hpsa patches:
patching file drivers/scsi/hpsa.c
Hunk #1 succeeded at 170 (offset -2 lines).
Hunk #2 succeeded at 4881 (offset 247 lines).
Hunk #3 succeeded at 4899 (offset 247 lines).
Hunk #4 succeeded at 4918 (offset 247 lines).
Hunk #5 succeeded at 4947 (offset 247 lines).
Hunk #6 FAILED at 4862.
Hunk #7 FAILED at 4928.
2 out of 7 hunks FAILED -- saving rejects to file drivers/scsi/hpsa.c.rej
patching file drivers/scsi/hpsa.h
Hunk #1 FAILED at 130.
1 out of 1 hunk FAILED -- saving rejects to file drivers/scsi/hpsa.h.rej
James
Re: [PATCH 01/11] hpsa: use workqueue instead of kernel thread for lockup detection
Quoted 9 lines · James Bottomley
Quoted 5 lines · Stephen M. Cameron
From: Stephen M. Cameron <[email protected]>
Much simpler and avoids races starting/stopping the thread.
Signed-off-by Stephen M. Cameron <[email protected]>Actually, perhaps you do need to resubmit the series: this is rejecting against scsi-misc because of your prior hpsa patches:
Ah, I did not realize you had taken these into your tree:
2f2ec0b [SCSI] hpsa: bring logical drives online when format completes c99298f [SCSI] hpsa: hide logical drives with format in progress from linux
Since you had complained about the races, and suggested using a workqueue rather than a kernel thread, I thought that meant you had dropped them, so I had gone back to the drawing board. I hope to have a new version of the series for you this afternoon.
-- steve
Quoted 17 lines
patching file
drivers/scsi/hpsa.c
Hunk #1 succeeded at 170 (offset -2 lines).
Hunk #2 succeeded at 4881 (offset 247 lines).
Hunk #3 succeeded at 4899 (offset 247 lines).
Hunk #4 succeeded at 4918 (offset 247 lines).
Hunk #5 succeeded at 4947 (offset 247 lines).
Hunk #6 FAILED at 4862.
Hunk #7 FAILED at 4928.
2 out of 7 hunks FAILED -- saving rejects to filedrivers/scsi/hpsa.c.rej
patching filedrivers/scsi/hpsa.h
Hunk #1 FAILED at 130.
1 out of 1 hunk FAILED -- saving rejects to filedrivers/scsi/hpsa.h.rejJames
James Bottomley
maintainerRe: [PATCH 01/11] hpsa: use workqueue instead of kernel thread for lockup detection
Quoted 8 lines · On Wed, 2013-12-04 at 10:31 -0600, [email protected] wrote:
Quoted 7 lines · James Bottomley
Quoted 5 lines · Stephen M. Cameron
From: Stephen M. Cameron <[email protected]>
Much simpler and avoids races starting/stopping the thread.
Signed-off-by Stephen M. Cameron <[email protected]>
Quoted 7 lines
Quoted 2 lines
Actually, perhaps you do need to resubmit the series: this is rejecting against scsi-misc because of your prior hpsa patches:
Ah, I did not realize you had taken these into your tree:
2f2ec0b [SCSI] hpsa: bring logical drives online when format completes c99298f [SCSI] hpsa: hide logical drives with format in progress from linux
That's OK, I can drop them again ... I've been having a lot of trouble with unsetting the imap labels I use to mark patches.
James
PATCH 2/11 hpsa: do not attempt to flush the cache on locked up controllers
In reply to Stephen M. CameronFrom: Stephen M. Cameron <[email protected]>
There's no point in trying since it can't work, and if you do try, it will just hang the system on shutdown.
--- drivers/scsi/hpsa.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-)
drivers/scsi/hpsa.c +9 -0
@@ -4884,6 +4884,15 @@ static void hpsa_flush_cache(struct ctlr_info *h) {char *flush_buf;struct CommandList *c; +unsigned long flags; + +/* Don't bother trying to flush the cache if locked up */ +spin_lock_irqsave (&h->lock, flags); +if (unlikely (h->lockup_detected)) { +spin_unlock_irqrestore (&h->lock, flags); +return ; + } +spin_unlock_irqrestore (&h->lock, flags); flush_buf =kzalloc (4 ,GFP_KERNEL );if (!flush_buf)
PATCH 3/11 hpsa: add 5 second delay after doorbell reset
In reply to Stephen M. CameronFrom: Stephen M. Cameron <[email protected]>
The hardware guys tell us that after initiating a software reset via the doorbell register we need to wait 5 seconds before attempting to talk to the board *at all*. This means that we cannot watch the board to verify it transitions from "ready" to to "not ready" then back "ready", since this transition will most likely happen during those 5 seconds (though we can still verify the reset happens by watching the "driver version" field get cleared.)
--- drivers/scsi/hpsa.c | 32 +++++++++++++++++++++++--------- 1 files changed, 23 insertions(+), 9 deletions(-)
drivers/scsi/hpsa.c +23 -9
@@ -3781,6 +3781,13 @@ static int hpsa_controller_hard_reset(struct pci_dev *pdev,*/ dev_info (&pdev->dev,"using doorbell to reset controller\n" );writel (use_doorbell, vaddr +SA5_DOORBELL ); + +/* PMC hardware guys tell us we need a 5 second delay after +* doorbell reset and before any attempt to talk to the board +* at all to ensure that this actually works and doesn't fall +* over in some weird corner cases. +*/ +msleep (5000 ); }else {/* Try to do it the PCI power state way */ /* Quoting from the Open CISS Specification: "The Power
@@ -3977,15 +3984,22 @@ static int hpsa_kdump_hard_reset_controller(struct pci_dev *pdev)need a little pause here */ msleep (HPSA_POST_RESET_PAUSE_MSECS ); -/* Wait for board to become not ready, then ready. */ -dev_info (&pdev->dev,"Waiting for board to reset.\n" ); - rc =hpsa_wait_for_board_state (pdev, vaddr,BOARD_NOT_READY ); -if (rc) { -dev_warn (&pdev->dev, -"failed waiting for board to reset." -" Will try soft reset.\n" ); - rc = -ENOTSUPP ;/* Not expected, but try soft reset later */ - goto unmap_cfgtable; +if (!use_doorbell) { +/* Wait for board to become not ready, then ready. +* (if we used the doorbell, then we already waited 5 secs +* so the "not ready" state is already gone by so we +* won't catch it.) +*/ +dev_info (&pdev->dev,"Waiting for board to reset.\n" ); + rc =hpsa_wait_for_board_state (pdev, vaddr,BOARD_NOT_READY ); +if (rc) { +dev_warn (&pdev->dev, +"failed waiting for board to reset." +" Will try soft reset.\n" ); +/* Not expected, but try soft reset later */ + rc = -ENOTSUPP ; + goto unmap_cfgtable; + } } rc =hpsa_wait_for_board_state (pdev, vaddr,BOARD_READY );if (rc) {
Re: [PATCH 03/11] hpsa: add 5 second delay after doorbell reset
Quoted 35 lines · Stephen M. Cameron
From: Stephen M. Cameron <[email protected]>
The hardware guys tell us that after initiating a software reset via the doorbell register we need to wait 5 seconds before attempting to talk to the board *at all*. This means that we cannot watch the board to verify it transitions from "ready" to to "not ready" then back "ready", since this transition will most likely happen during those 5 seconds (though we can still verify the reset happens by watching the "driver version" field get cleared.)
Signed-off-by Stephen M. Cameron <[email protected]>--- drivers/scsi/hpsa.c | 32 +++++++++++++++++++++++--------- 1 files changed, 23 insertions(+), 9 deletions(-)drivers/scsi/hpsa.c +7
-0@@ -3781,6 +3781,13 @@ static int hpsa_controller_hard_reset(struct pci_dev *pdev,*/ dev_info (&pdev->dev,"using doorbell to reset controller\n" );writel (use_doorbell, vaddr +SA5_DOORBELL ); + +/* PMC hardware guys tell us we need a 5 second delay after +* doorbell reset and before any attempt to talk to the board +* at all to ensure that this actually works and doesn't fall +* over in some weird corner cases. +*/ +msleep (5000 ); }else {/* Try to do it the PCI power state way */ /* Quoting from the Open CISS Specification: "The Power
drivers/scsi/hpsa.c static int hpsa_kdump_hard_reset_controller(struct pci_dev *pdev) · line 3985
Quoted 2 lines
drivers/scsi/hpsa.c +0
-0@@ -3977,15 +3984,22 @@ static int hpsa_kdump_hard_reset_controller(struct pci_dev *pdev)need a little pause here */ msleep (HPSA_POST_RESET_PAUSE_MSECS );
I know it's complicated with a lot of different devices and fw versions, but here^ we wait for 3sec - isn't the method - wait for 3s then wait for board not ready a bit fragile, what if a board comes up faster?
When the method "watching the "driver version"" works why don't you want to use it regardless of the reset method used?
Quoted 34 lines
drivers/scsi/hpsa.c +16
-10-/* Wait for board to become not ready, then ready. */ -dev_info (&pdev->dev,"Waiting for board to reset.\n" ); - rc =hpsa_wait_for_board_state (pdev, vaddr,BOARD_NOT_READY ); -if (rc) { -dev_warn (&pdev->dev, -"failed waiting for board to reset." -" Will try soft reset.\n" ); - rc = -ENOTSUPP ;/* Not expected, but try soft reset later */ - goto unmap_cfgtable; +if (!use_doorbell) { +/* Wait for board to become not ready, then ready. +* (if we used the doorbell, then we already waited 5 secs +* so the "not ready" state is already gone by so we +* won't catch it.) +*/ +dev_info (&pdev->dev,"Waiting for board to reset.\n" ); + rc =hpsa_wait_for_board_state (pdev, vaddr,BOARD_NOT_READY ); +if (rc) { +dev_warn (&pdev->dev, +"failed waiting for board to reset." +" Will try soft reset.\n" ); +/* Not expected, but try soft reset later */ + rc = -ENOTSUPP ; + goto unmap_cfgtable; + } } rc =hpsa_wait_for_board_state (pdev, vaddr,BOARD_READY );if (rc) { --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
Re: [PATCH 03/11] hpsa: add 5 second delay after doorbell reset
Quoted 34 lines · Tomas Henzl
Quoted 33 lines · Stephen M. Cameron
From: Stephen M. Cameron <[email protected]>
The hardware guys tell us that after initiating a software reset via the doorbell register we need to wait 5 seconds before attempting to talk to the board *at all*. This means that we cannot watch the board to verify it transitions from "ready" to to "not ready" then back "ready", since this transition will most likely happen during those 5 seconds (though we can still verify the reset happens by watching the "driver version" field get cleared.)
Signed-off-by Stephen M. Cameron <[email protected]>--- drivers/scsi/hpsa.c | 32 +++++++++++++++++++++++--------- 1 files changed, 23 insertions(+), 9 deletions(-)drivers/scsi/hpsa.c +7
-0@@ -3781,6 +3781,13 @@ static int hpsa_controller_hard_reset(struct pci_dev *pdev,*/ dev_info (&pdev->dev,"using doorbell to reset controller\n" );writel (use_doorbell, vaddr +SA5_DOORBELL ); + +/* PMC hardware guys tell us we need a 5 second delay after +* doorbell reset and before any attempt to talk to the board +* at all to ensure that this actually works and doesn't fall +* over in some weird corner cases. +*/ +msleep (5000 ); }else {/* Try to do it the PCI power state way */
Quoted 10 lines
drivers/scsi/hpsa.c static int hpsa_kdump_hard_reset_controller(struct pci_dev *pdev) · line 3985
Quoted 4 lines
drivers/scsi/hpsa.c +0
-0/* Quoting from the Open CISS Specification: "The Power @@ -3977,15 +3984,22 @@ static int hpsa_kdump_hard_reset_controller(struct pci_dev *pdev)need a little pause here */ msleep (HPSA_POST_RESET_PAUSE_MSECS );I know it's complicated with a lot of different devices and fw versions, but here^ we wait for 3sec - isn't the method - wait for 3s then wait for board not ready a bit fragile, what if a board comes up faster?
When the method "watching the "driver version"" works why don't you want to use it regardless of the reset method used?
The "watching the driver version" thing is only there to catch if the firmware guys break things and turn the reset into a no-op (which happened with the PCI power manaegment based reset and we didn't catch it for a year or so because we didn't have that check)
We aren't supposed to look at the driver version field (or anything) until we first verify the scratch pad register says the firmware is ready. In the case of those boards that use the "doorbell" reset, we aren't supposed to look at *anything* for the first five seconds.
I have been bugging the firmware/hardware guys for a sane reset procedure that actually works reliably for years with no luck.
For the SCSI over PCIe driver, being tired of this crap, I simply unconditionally reset the device on driver load every single time, and did this from the beginning. This kind of forced the firmware and hardware guys to make the reset on that thing work reliably and quickly, and since I did that from the earliest days, they didn't have a chance to screw it up without it being caught immediately. For Smart Array, obviously it's too late for that approach.
-- steve
Quoted 35 lines
Quoted 35 lines
drivers/scsi/hpsa.c +16
-10-/* Wait for board to become not ready, then ready. */ -dev_info (&pdev->dev,"Waiting for board to reset.\n" ); - rc =hpsa_wait_for_board_state (pdev, vaddr,BOARD_NOT_READY ); -if (rc) { -dev_warn (&pdev->dev, -"failed waiting for board to reset." -" Will try soft reset.\n" ); - rc = -ENOTSUPP ;/* Not expected, but try soft reset later */ - goto unmap_cfgtable; +if (!use_doorbell) { +/* Wait for board to become not ready, then ready. +* (if we used the doorbell, then we already waited 5 secs +* so the "not ready" state is already gone by so we +* won't catch it.) +*/ +dev_info (&pdev->dev,"Waiting for board to reset.\n" ); + rc =hpsa_wait_for_board_state (pdev, vaddr,BOARD_NOT_READY ); +if (rc) { +dev_warn (&pdev->dev, +"failed waiting for board to reset." +" Will try soft reset.\n" ); +/* Not expected, but try soft reset later */ + rc = -ENOTSUPP ; + goto unmap_cfgtable; + } } rc =hpsa_wait_for_board_state (pdev, vaddr,BOARD_READY );if (rc) { --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
Re: [PATCH 03/11] hpsa: add 5 second delay after doorbell reset
Quoted 57 lines · On 11/08/2013 03:44 PM, [email protected] wrote:
Quoted 34 lines · Tomas Henzl
Quoted 33 lines · Stephen M. Cameron
From: Stephen M. Cameron <[email protected]>
The hardware guys tell us that after initiating a software reset via the doorbell register we need to wait 5 seconds before attempting to talk to the board *at all*. This means that we cannot watch the board to verify it transitions from "ready" to to "not ready" then back "ready", since this transition will most likely happen during those 5 seconds (though we can still verify the reset happens by watching the "driver version" field get cleared.)
Signed-off-by Stephen M. Cameron <[email protected]>--- drivers/scsi/hpsa.c | 32 +++++++++++++++++++++++--------- 1 files changed, 23 insertions(+), 9 deletions(-)drivers/scsi/hpsa.c +7
-0@@ -3781,6 +3781,13 @@ static int hpsa_controller_hard_reset(struct pci_dev *pdev,*/ dev_info (&pdev->dev,"using doorbell to reset controller\n" );writel (use_doorbell, vaddr +SA5_DOORBELL ); + +/* PMC hardware guys tell us we need a 5 second delay after +* doorbell reset and before any attempt to talk to the board +* at all to ensure that this actually works and doesn't fall +* over in some weird corner cases. +*/ +msleep (5000 ); }else {/* Try to do it the PCI power state way */ Quoted 9 lines
drivers/scsi/hpsa.c static int hpsa_kdump_hard_reset_controller(struct pci_dev *pdev) · line 3985
Quoted 4 lines
drivers/scsi/hpsa.c +0
-0/* Quoting from the Open CISS Specification: "The Power @@ -3977,15 +3984,22 @@ static int hpsa_kdump_hard_reset_controller(struct pci_dev *pdev)need a little pause here */ msleep (HPSA_POST_RESET_PAUSE_MSECS );I know it's complicated with a lot of different devices and fw versions, but here^ we wait for 3sec - isn't the method - wait for 3s then wait for board not ready a bit fragile, what if a board comes up faster?
When the method "watching the "driver version"" works why don't you want to use it regardless of the reset method used?The "watching the driver version" thing is only there to catch if the firmware guys break things and turn the reset into a no-op (which happened with the PCI power manaegment based reset and we didn't catch it for a year or so because we didn't have that check)
We aren't supposed to look at the driver version field (or anything) until we first verify the scratch pad register says the firmware is ready. In the case of those boards that use the "doorbell" reset, we aren't supposed to look at *anything* for the first five seconds.
I have been bugging the
firmware/hardwareguys for a sane reset procedure that actually works reliably for years with no luck.
Quoted 7 lines
For the SCSI over PCIe driver, being tired of this crap, I simply unconditionally reset the device on driver load every single time, and did this from the beginning. This kind of forced the firmware and hardware guys to make the reset on that thing work reliably and quickly, and since I did that from the earliest days, they didn't have a chance to screw it up without it being caught immediately. For Smart Array, obviously it's too late for that approach.
OK, my question was more or less if this:msleep(HPSA_POST_RESET_PAUSE_MSECS);
just before waiting for the board to enter BOARD_NOT_READY state isn't dangerous - when the board enters a ready state in the first 3sec it will wait indefinitely for the not_ready state
thus whether the test for not ready state shouldn't be removed. The mechanism now works somehow and maybe it's better
not to touch it, I just wanted to draw your attention to that potential problem.
Quoted 37 lines
-- steve
Quoted 34 lines
Quoted 34 lines
drivers/scsi/hpsa.c +16
-10-/* Wait for board to become not ready, then ready. */ -dev_info (&pdev->dev,"Waiting for board to reset.\n" ); - rc =hpsa_wait_for_board_state (pdev, vaddr,BOARD_NOT_READY ); -if (rc) { -dev_warn (&pdev->dev, -"failed waiting for board to reset." -" Will try soft reset.\n" ); - rc = -ENOTSUPP ;/* Not expected, but try soft reset later */ - goto unmap_cfgtable; +if (!use_doorbell) { +/* Wait for board to become not ready, then ready. +* (if we used the doorbell, then we already waited 5 secs +* so the "not ready" state is already gone by so we +* won't catch it.) +*/ +dev_info (&pdev->dev,"Waiting for board to reset.\n" ); + rc =hpsa_wait_for_board_state (pdev, vaddr,BOARD_NOT_READY ); +if (rc) { +dev_warn (&pdev->dev, +"failed waiting for board to reset." +" Will try soft reset.\n" ); +/* Not expected, but try soft reset later */ + rc = -ENOTSUPP ; + goto unmap_cfgtable; + } } rc =hpsa_wait_for_board_state (pdev, vaddr,BOARD_READY );if (rc) { --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
Re: [PATCH 03/11] hpsa: add 5 second delay after doorbell reset
Quoted 66 lines · Tomas Henzl
Quoted 57 lines · On 11/08/2013 03:44 PM, [email protected] wrote:
Quoted 34 lines · Tomas Henzl
Quoted 33 lines · Stephen M. Cameron
From: Stephen M. Cameron <[email protected]>
The hardware guys tell us that after initiating a software reset via the doorbell register we need to wait 5 seconds before attempting to talk to the board *at all*. This means that we cannot watch the board to verify it transitions from "ready" to to "not ready" then back "ready", since this transition will most likely happen during those 5 seconds (though we can still verify the reset happens by watching the "driver version" field get cleared.)
Signed-off-by Stephen M. Cameron <[email protected]>--- drivers/scsi/hpsa.c | 32 +++++++++++++++++++++++--------- 1 files changed, 23 insertions(+), 9 deletions(-)drivers/scsi/hpsa.c +7
-0@@ -3781,6 +3781,13 @@ static int hpsa_controller_hard_reset(struct pci_dev *pdev,*/ dev_info (&pdev->dev,"using doorbell to reset controller\n" );writel (use_doorbell, vaddr +SA5_DOORBELL ); + +/* PMC hardware guys tell us we need a 5 second delay after +* doorbell reset and before any attempt to talk to the board +* at all to ensure that this actually works and doesn't fall +* over in some weird corner cases. +*/ +msleep (5000 ); }else {/* Try to do it the PCI power state way */ Quoted 9 lines
drivers/scsi/hpsa.c static int hpsa_kdump_hard_reset_controller(struct pci_dev *pdev) · line 3985
Quoted 4 lines
drivers/scsi/hpsa.c +0
-0/* Quoting from the Open CISS Specification: "The Power @@ -3977,15 +3984,22 @@ static int hpsa_kdump_hard_reset_controller(struct pci_dev *pdev)need a little pause here */ msleep (HPSA_POST_RESET_PAUSE_MSECS );I know it's complicated with a lot of different devices and fw versions, but here^ we wait for 3sec - isn't the method - wait for 3s then wait for board not ready a bit fragile, what if a board comes up faster?
When the method "watching the "driver version"" works why don't you want to use it regardless of the reset method used?The "watching the driver version" thing is only there to catch if the firmware guys break things and turn the reset into a no-op (which happened with the PCI power manaegment based reset and we didn't catch it for a year or so because we didn't have that check)
We aren't supposed to look at the driver version field (or anything) until we first verify the scratch pad register says the firmware is ready. In the case of those boards that use the "doorbell" reset, we aren't supposed to look at *anything* for the first five seconds.
I have been bugging the
firmware/hardwareguys for a sane reset procedure that actually works reliably for years with no luck.Quoted 7 lines
For the SCSI over PCIe driver, being tired of this crap, I simply unconditionally reset the device on driver load every single time, and did this from the beginning. This kind of forced the firmware and hardware guys to make the reset on that thing work reliably and quickly, and since I did that from the earliest days, they didn't have a chance to screw it up without it being caught immediately. For Smart Array, obviously it's too late for that approach.
Quoted 9 lines
OK, my question was more or less if this:
msleep(HPSA_POST_RESET_PAUSE_MSECS);
just before waiting for the board to enterBOARD_NOT_READYstate isn't dangerous - when the board enters a ready state in the first 3sec it will wait indefinitely for thenot_readystate
thus whether the test for not ready state shouldn't be removed. The mechanism now works somehow and maybe it's better
not to touch it, I just wanted to draw your attention to that potential problem.
Oh ok, I see. Thanks, yes that does look questionable. So you are suggesting to skip the check for transition from NOT READY to READY in the scratch pad register in all cases, since we have all these ridiculous delay requirements preventing us from watching the board closely enough and so that may mean that we would miss such a transition.
Let me talk it over with Mike Miller, but it seems reasonable.
-- steve
Quoted 39 lines
Quoted 39 lines
-- steve
Quoted 34 lines
Quoted 34 lines
drivers/scsi/hpsa.c +16
-10-/* Wait for board to become not ready, then ready. */ -dev_info (&pdev->dev,"Waiting for board to reset.\n" ); - rc =hpsa_wait_for_board_state (pdev, vaddr,BOARD_NOT_READY ); -if (rc) { -dev_warn (&pdev->dev, -"failed waiting for board to reset." -" Will try soft reset.\n" ); - rc = -ENOTSUPP ;/* Not expected, but try soft reset later */ - goto unmap_cfgtable; +if (!use_doorbell) { +/* Wait for board to become not ready, then ready. +* (if we used the doorbell, then we already waited 5 secs +* so the "not ready" state is already gone by so we +* won't catch it.) +*/ +dev_info (&pdev->dev,"Waiting for board to reset.\n" ); + rc =hpsa_wait_for_board_state (pdev, vaddr,BOARD_NOT_READY ); +if (rc) { +dev_warn (&pdev->dev, +"failed waiting for board to reset." +" Will try soft reset.\n" ); +/* Not expected, but try soft reset later */ + rc = -ENOTSUPP ; + goto unmap_cfgtable; + } } rc =hpsa_wait_for_board_state (pdev, vaddr,BOARD_READY );if (rc) { --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
James Bottomley
maintainerRe: [PATCH 03/11] hpsa: add 5 second delay after doorbell reset
Quoted 77 lines · On Fri, 2013-11-08 at 09:31 -0600, [email protected] wrote:
Quoted 66 lines · Tomas Henzl
Quoted 57 lines · On 11/08/2013 03:44 PM, [email protected] wrote:
Quoted 34 lines · Tomas Henzl
Quoted 33 lines · Stephen M. Cameron
From: Stephen M. Cameron <[email protected]>
The hardware guys tell us that after initiating a software reset via the doorbell register we need to wait 5 seconds before attempting to talk to the board *at all*. This means that we cannot watch the board to verify it transitions from "ready" to to "not ready" then back "ready", since this transition will most likely happen during those 5 seconds (though we can still verify the reset happens by watching the "driver version" field get cleared.)
Signed-off-by Stephen M. Cameron <[email protected]>--- drivers/scsi/hpsa.c | 32 +++++++++++++++++++++++--------- 1 files changed, 23 insertions(+), 9 deletions(-)drivers/scsi/hpsa.c +7
-0@@ -3781,6 +3781,13 @@ static int hpsa_controller_hard_reset(struct pci_dev *pdev,*/ dev_info (&pdev->dev,"using doorbell to reset controller\n" );writel (use_doorbell, vaddr +SA5_DOORBELL ); + +/* PMC hardware guys tell us we need a 5 second delay after +* doorbell reset and before any attempt to talk to the board +* at all to ensure that this actually works and doesn't fall +* over in some weird corner cases. +*/ +msleep (5000 ); }else {/* Try to do it the PCI power state way */ Quoted 9 lines
drivers/scsi/hpsa.c static int hpsa_kdump_hard_reset_controller(struct pci_dev *pdev) · line 3985
Quoted 4 lines
drivers/scsi/hpsa.c +0
-0/* Quoting from the Open CISS Specification: "The Power @@ -3977,15 +3984,22 @@ static int hpsa_kdump_hard_reset_controller(struct pci_dev *pdev)need a little pause here */ msleep (HPSA_POST_RESET_PAUSE_MSECS );I know it's complicated with a lot of different devices and fw versions, but here^ we wait for 3sec - isn't the method - wait for 3s then wait for board not ready a bit fragile, what if a board comes up faster?
When the method "watching the "driver version"" works why don't you want to use it regardless of the reset method used?The "watching the driver version" thing is only there to catch if the firmware guys break things and turn the reset into a no-op (which happened with the PCI power manaegment based reset and we didn't catch it for a year or so because we didn't have that check)
We aren't supposed to look at the driver version field (or anything) until we first verify the scratch pad register says the firmware is ready. In the case of those boards that use the "doorbell" reset, we aren't supposed to look at *anything* for the first five seconds.
I have been bugging the
firmware/hardwareguys for a sane reset procedure that actually works reliably for years with no luck.Quoted 7 lines
For the SCSI over PCIe driver, being tired of this crap, I simply unconditionally reset the device on driver load every single time, and did this from the beginning. This kind of forced the firmware and hardware guys to make the reset on that thing work reliably and quickly, and since I did that from the earliest days, they didn't have a chance to screw it up without it being caught immediately. For Smart Array, obviously it's too late for that approach.
Quoted 9 lines
OK, my question was more or less if this:
msleep(HPSA_POST_RESET_PAUSE_MSECS);
just before waiting for the board to enterBOARD_NOT_READYstate isn't dangerous - when the board enters a ready state in the first 3sec it will wait indefinitely for thenot_readystate
thus whether the test for not ready state shouldn't be removed. The mechanism now works somehow and maybe it's better
not to touch it, I just wanted to draw your attention to that potential problem.
Quoted 8 lines
Oh ok, I see. Thanks, yes that does look questionable. So you are suggesting to skip the check for transition from NOT READY to READY in the scratch pad register in all cases, since we have all these ridiculous delay requirements preventing us from watching the board closely enough and so that may mean that we would miss such a transition.
Let me talk it over with Mike Miller, but it seems reasonable.
Is there a resolution on this? It's holding up the patch series.
James
Re: [PATCH 03/11] hpsa: add 5 second delay after doorbell reset
Quoted 85 lines · James Bottomley
Quoted 84 lines · On Fri, 2013-11-08 at 09:31 -0600, [email protected] wrote:
Quoted 66 lines · Tomas Henzl
Quoted 57 lines · On 11/08/2013 03:44 PM, [email protected] wrote:
Quoted 34 lines · Tomas Henzl
Quoted 33 lines · Stephen M. Cameron
From: Stephen M. Cameron <[email protected]>
The hardware guys tell us that after initiating a software reset via the doorbell register we need to wait 5 seconds before attempting to talk to the board *at all*. This means that we cannot watch the board to verify it transitions from "ready" to to "not ready" then back "ready", since this transition will most likely happen during those 5 seconds (though we can still verify the reset happens by watching the "driver version" field get cleared.)
Signed-off-by Stephen M. Cameron <[email protected]>--- drivers/scsi/hpsa.c | 32 +++++++++++++++++++++++--------- 1 files changed, 23 insertions(+), 9 deletions(-)drivers/scsi/hpsa.c +7
-0@@ -3781,6 +3781,13 @@ static int hpsa_controller_hard_reset(struct pci_dev *pdev,*/ dev_info (&pdev->dev,"using doorbell to reset controller\n" );writel (use_doorbell, vaddr +SA5_DOORBELL ); + +/* PMC hardware guys tell us we need a 5 second delay after +* doorbell reset and before any attempt to talk to the board +* at all to ensure that this actually works and doesn't fall +* over in some weird corner cases. +*/ +msleep (5000 ); }else {/* Try to do it the PCI power state way */ Quoted 9 lines
drivers/scsi/hpsa.c static int hpsa_kdump_hard_reset_controller(struct pci_dev *pdev) · line 3985
Quoted 4 lines
drivers/scsi/hpsa.c +0
-0/* Quoting from the Open CISS Specification: "The Power @@ -3977,15 +3984,22 @@ static int hpsa_kdump_hard_reset_controller(struct pci_dev *pdev)need a little pause here */ msleep (HPSA_POST_RESET_PAUSE_MSECS );I know it's complicated with a lot of different devices and fw versions, but here^ we wait for 3sec - isn't the method - wait for 3s then wait for board not ready a bit fragile, what if a board comes up faster?
When the method "watching the "driver version"" works why don't you want to use it regardless of the reset method used?The "watching the driver version" thing is only there to catch if the firmware guys break things and turn the reset into a no-op (which happened with the PCI power manaegment based reset and we didn't catch it for a year or so because we didn't have that check)
We aren't supposed to look at the driver version field (or anything) until we first verify the scratch pad register says the firmware is ready. In the case of those boards that use the "doorbell" reset, we aren't supposed to look at *anything* for the first five seconds.
I have been bugging the
firmware/hardwareguys for a sane reset procedure that actually works reliably for years with no luck.Quoted 7 lines
For the SCSI over PCIe driver, being tired of this crap, I simply unconditionally reset the device on driver load every single time, and did this from the beginning. This kind of forced the firmware and hardware guys to make the reset on that thing work reliably and quickly, and since I did that from the earliest days, they didn't have a chance to screw it up without it being caught immediately. For Smart Array, obviously it's too late for that approach.
Quoted 9 lines
OK, my question was more or less if this:
msleep(HPSA_POST_RESET_PAUSE_MSECS);
just before waiting for the board to enterBOARD_NOT_READYstate isn't dangerous - when the board enters a ready state in the first 3sec it will wait indefinitely for thenot_readystate
thus whether the test for not ready state shouldn't be removed. The mechanism now works somehow and maybe it's better
not to touch it, I just wanted to draw your attention to that potential problem.Oh ok, I see. Thanks, yes that does look questionable. So you are suggesting to skip the check for transition from NOT READY to READY in the scratch pad register in all cases, since we have all these ridiculous delay requirements preventing us from watching the board closely enough and so that may mean that we would miss such a transition.
Quoted 6 lines
Quoted 1 line
Let me talk it over with Mike Miller, but it seems reasonable.
Is there a resolution on this? It's holding up the patch series.
James
James,
Let's omit the check for board ready/not ready. With these arbitrary delays already in place we're likely to miss the transition.
Do you need me to resubmit the patch set?
-- mikem
James Bottomley
maintainerRe: [PATCH 03/11] hpsa: add 5 second delay after doorbell reset
Quoted 90 lines · Mike Miller
Quoted 89 lines · James Bottomley
Quoted 77 lines · On Fri, 2013-11-08 at 09:31 -0600, [email protected] wrote:
Quoted 66 lines · Tomas Henzl
Quoted 57 lines · On 11/08/2013 03:44 PM, [email protected] wrote:
Quoted 34 lines · Tomas Henzl
Quoted 33 lines · Stephen M. Cameron
From: Stephen M. Cameron <[email protected]>
The hardware guys tell us that after initiating a software reset via the doorbell register we need to wait 5 seconds before attempting to talk to the board *at all*. This means that we cannot watch the board to verify it transitions from "ready" to to "not ready" then back "ready", since this transition will most likely happen during those 5 seconds (though we can still verify the reset happens by watching the "driver version" field get cleared.)
Signed-off-by Stephen M. Cameron <[email protected]>--- drivers/scsi/hpsa.c | 32 +++++++++++++++++++++++--------- 1 files changed, 23 insertions(+), 9 deletions(-)drivers/scsi/hpsa.c +7
-0@@ -3781,6 +3781,13 @@ static int hpsa_controller_hard_reset(struct pci_dev *pdev,*/ dev_info (&pdev->dev,"using doorbell to reset controller\n" );writel (use_doorbell, vaddr +SA5_DOORBELL ); + +/* PMC hardware guys tell us we need a 5 second delay after +* doorbell reset and before any attempt to talk to the board +* at all to ensure that this actually works and doesn't fall +* over in some weird corner cases. +*/ +msleep (5000 ); }else {/* Try to do it the PCI power state way */ Quoted 9 lines
drivers/scsi/hpsa.c static int hpsa_kdump_hard_reset_controller(struct pci_dev *pdev) · line 3985
Quoted 4 lines
drivers/scsi/hpsa.c +0
-0/* Quoting from the Open CISS Specification: "The Power @@ -3977,15 +3984,22 @@ static int hpsa_kdump_hard_reset_controller(struct pci_dev *pdev)need a little pause here */ msleep (HPSA_POST_RESET_PAUSE_MSECS );I know it's complicated with a lot of different devices and fw versions, but here^ we wait for 3sec - isn't the method - wait for 3s then wait for board not ready a bit fragile, what if a board comes up faster?
When the method "watching the "driver version"" works why don't you want to use it regardless of the reset method used?The "watching the driver version" thing is only there to catch if the firmware guys break things and turn the reset into a no-op (which happened with the PCI power manaegment based reset and we didn't catch it for a year or so because we didn't have that check)
We aren't supposed to look at the driver version field (or anything) until we first verify the scratch pad register says the firmware is ready. In the case of those boards that use the "doorbell" reset, we aren't supposed to look at *anything* for the first five seconds.
I have been bugging the
firmware/hardwareguys for a sane reset procedure that actually works reliably for years with no luck.Quoted 7 lines
For the SCSI over PCIe driver, being tired of this crap, I simply unconditionally reset the device on driver load every single time, and did this from the beginning. This kind of forced the firmware and hardware guys to make the reset on that thing work reliably and quickly, and since I did that from the earliest days, they didn't have a chance to screw it up without it being caught immediately. For Smart Array, obviously it's too late for that approach.
Quoted 9 lines
OK, my question was more or less if this:
msleep(HPSA_POST_RESET_PAUSE_MSECS);
just before waiting for the board to enterBOARD_NOT_READYstate isn't dangerous - when the board enters a ready state in the first 3sec it will wait indefinitely for thenot_readystate
thus whether the test for not ready state shouldn't be removed. The mechanism now works somehow and maybe it's better
not to touch it, I just wanted to draw your attention to that potential problem.Quoted 8 lines
Oh ok, I see. Thanks, yes that does look questionable. So you are suggesting to skip the check for transition from NOT READY to READY in the scratch pad register in all cases, since we have all these ridiculous delay requirements preventing us from watching the board closely enough and so that may mean that we would miss such a transition.
Let me talk it over with Mike Miller, but it seems reasonable.
Is there a resolution on this? It's holding up the patch series.
Quoted 7 lines
Quoted 2 lines
James
James,
Let's omit the check for board ready/not ready. With these arbitrary delays already in place we're likely to miss the transition.Do you need me to resubmit the patch set?
No; you want 1-2;4-11 of the current one ... I can do that.
James
RE: [PATCH 03/11] hpsa: add 5 second delay after doorbell reset
Quoted 119 lines
-----Original Message-----
From: James Bottomley [mailto:[email protected]]
Sent: Monday, December 02, 2013 11:23 AM
To: Miller, Mike (OS Dev)
Cc: [email protected]; Tomas Henzl; [email protected]; [email protected]; Teel, Scott Stacy
Subject: Re: [PATCH 03/11] hpsa: add 5 second delay after doorbell resetQuoted 23 lines · Mike Miller
Quoted 22 lines · James Bottomley
Quoted 21 lines · On Fri, 2013-11-08 at 09:31 -0600, [email protected] wrote:
Quoted 20 lines · Tomas Henzl
Quoted 19 lines · On 11/08/2013 03:44 PM, [email protected] wrote:
Quoted 18 lines · Tomas Henzl
Quoted 17 lines · Stephen M. Cameron
From: Stephen M. Cameron <[email protected]>
The hardware guys tell us that after initiating a software reset via the doorbell register we need to wait 5 seconds before attempting to talk to the board *at all*. This means that we cannot watch the board to verify it transitions from "ready" to to "not ready" then back "ready", since this transition will most likely happen during those 5 seconds (though we can still verify the reset happens by watching the "driver version" field get cleared.)
Signed-off-by: Stephen M. Cameron
<[email protected]>--- drivers/scsi/hpsa.c | 32 +++++++++++++++++++++++--------- 1 files changed, 23 insertions(+), 9 deletions(-)Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/scsi/hpsa.c static int
Quoted 5 lines
drivers/scsi/hpsa.c index renamed from drivers/scsi/hpsa.c +0
-020fc598..fff5fd3 100644
drivers/scsi/hpsa.c +0
-0@@ -3781,6 +3781,13 @@ static inthpsa_controller_hard_reset(struct
pci_dev*pdev,Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/scsi/hpsa.c static int · line 3782
Quoted 2 lines
drivers/scsi/hpsa.c +0
-0*/ dev_info(&pdev->dev, "using doorbell to resetcontroller\n");
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/scsi/hpsa.c static int · line 3785
Quoted 3 lines
drivers/scsi/hpsa.c +2
-0writel (use_doorbell, vaddr +SA5_DOORBELL ); + +/* PMC hardware guys tell us we need a 5 second delay after
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/scsi/hpsa.c static int · line 3786
Quoted 1 line
drivers/scsi/hpsa.c +1
-0+* doorbell reset and before any attempt to talk to the board
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/scsi/hpsa.c static int · line 3787
Quoted 1 line
drivers/scsi/hpsa.c +1
-0+* at all to ensure that this actually works and doesn't fall
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/scsi/hpsa.c static int · line 3793
Quoted 6 lines
drivers/scsi/hpsa.c +3
-0+* over in some weird corner cases. +*/ +msleep (5000 ); }else {/* Try to do it the PCI power state way */ /* Quoting from the Open CISS Specification: "The Power
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/scsi/hpsa.c static int
Quoted 1 line
drivers/scsi/hpsa.c +0
-0@@ -3977,15 +3984,22 @@ static int
hpsa_kdump_hard_reset_controller(struct pci_dev *pdev)Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/scsi/hpsa.c static int · line 3985
Quoted 2 lines
drivers/scsi/hpsa.c +0
-0need a little pause here */ msleep (HPSA_POST_RESET_PAUSE_MSECS );I know it's complicated with a lot of different devices and fw versions, but here^ we wait for 3sec - isn't the method - wait for 3s then wait for board not ready a bit fragile, what if a
board comes up faster?
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
When the method "watching the "driver version"" works why don't you want to use it regardless of the reset method used?
The "watching the driver version" thing is only there to catch if the firmware guys break things and turn the reset into a no-op (which happened with the PCI power manaegment based reset and we didn't catch it for a year or so because we didn't have that check)
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
We aren't supposed to look at the driver version field (or anything) until we first verify the scratch pad register says the firmware is ready. In the case of those boards that use the "doorbell" reset, we aren't supposed to look at *anything* for
the first five seconds.
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
I have been bugging the
firmware/hardwareguys for a sane reset procedure that actually works reliably for years with no luck.For the SCSI over PCIe driver, being tired of this crap, I simply unconditionally reset the device on driver load every single time, and did this from the beginning. This kind of forced the firmware and hardware guys to make the reset on that thing work reliably and quickly, and since I did that from the earliest days, they didn't have a chance to screw it up
without it being caught immediately.
Quoted 29 lines
Quoted 19 lines
Quoted 19 lines
Quoted 11 lines
Quoted 1 line
For Smart Array, obviously it's too late for that approach.
OK, my question was more or less if this:
msleep(HPSA_POST_RESET_PAUSE_MSECS);
just before waiting for the board to enterBOARD_NOT_READYstate isn't dangerous - when the board enters a ready state in the first 3sec it will wait indefinitely for thenot_readystate thus whether the test for not ready state shouldn't be removed. The mechanism now works somehow and maybe it's better not to touch it, I just wanted to draw your attention to that potential problem.Oh ok, I see. Thanks, yes that does look questionable. So you are suggesting to skip the check for transition from NOT READY to READY in the scratch pad register in all cases, since we have all these ridiculous delay requirements preventing us from watching the board closely enough and so that may mean that we would miss such a transition.
Quoted 6 lines
Quoted 1 line
Let me talk it over with Mike Miller, but it seems reasonable.
Is there a resolution on this? It's holding up the patch series.
James
James,
Let's omit the check for board ready/not ready. With these arbitrary delays already in place we're likely to miss the transition.
Quoted 6 lines
Quoted 1 line
Do you need me to resubmit the patch set?
No; you want 1-2;4-11 of the current one ... I can do that.
James
Yes. Thank you.
-- mikem
Quoted 1 line
PATCH 4/11 hpsa: do not discard scsi status on aborted commands
In reply to Stephen M. CameronFrom: Stephen M. Cameron <[email protected]>
We inadvertantly discarded the scsi status for aborted commands. For some commands (e.g. reads from tape drives) these can't be retried, and if we discarded the scsi status, the scsi mid layer couldn't notice anything was wrong and the error was not reported.
--- drivers/scsi/hpsa.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/scsi/hpsa.c +1 -1
@@ -1285,7 +1285,7 @@ static void complete_scsi_command(struct CommandList *cp)"has check condition: aborted command: " "ASC: 0x%x, ASCQ: 0x%x\n" , cp, asc, ascq); - cmd->result =DID_SOFT_ERROR <<16 ; + cmd->result |=DID_SOFT_ERROR <<16 ;break ; }/* Must be some other type of check condition */
PATCH 5/11 hpsa: remove unneeded include of seq_file.h
In reply to Stephen M. CameronFrom: Stephen M. Cameron <[email protected]>
--- drivers/scsi/hpsa.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
drivers/scsi/hpsa.c +0 -1
@@ -29,7 +29,6 @@#include <linux/delay.h> #include <linux/fs.h> #include <linux/timer.h> -#include <linux/seq_file.h> #include <linux/init.h> #include <linux/spinlock.h> #include <linux/compat.h>
PATCH 6/11 hpsa: fix memory leak in CCISS_BIG_PASSTHRU ioctl
In reply to Stephen M. CameronFrom: Stephen M. Cameron <[email protected]>
We were leaking a command buffer if a DMA mapping error was encountered in the CCISS_BIG_PASSTHRU ioctl.
--- drivers/scsi/hpsa.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)
drivers/scsi/hpsa.c +5 -6
@@ -3166,7 +3166,7 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info *h, void __user *argp)hpsa_pci_unmap (h->pdev, c, i,PCI_DMA_BIDIRECTIONAL ); status = -ENOMEM ; - goto cleanup1; + goto cleanup0; } c->SG[i].Addr.lower = temp64.val32.lower; c->SG[i].Addr.upper = temp64.val32.upper;
@@ -3182,24 +3182,23 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info *h, void __user *argp)/* Copy the error information out */ memcpy (&ioc->error_info, c->err_info,sizeof (ioc->error_info));if (copy_to_user (argp, ioc,sizeof (*ioc))) { -cmd_special_free (h, c); status = -EFAULT ; - goto cleanup1; + goto cleanup0; }if (ioc->Request.Type.Direction ==XFER_READ && ioc->buf_size >0 ) {/* Copy the data out of the buffer we created */ BYTE __user *ptr = ioc->buf;for (i =0 ; i < sg_used; i++) {if (copy_to_user (ptr, buff[i], buff_size[i])) { -cmd_special_free (h, c); status = -EFAULT ; - goto cleanup1; + goto cleanup0; } ptr += buff_size[i]; } } -cmd_special_free (h, c); status =0 ; +cleanup0: +cmd_special_free (h, c); cleanup1:if (buff) {for (i =0 ; i < sg_used; i++)
PATCH 7/11 hpsa: add MSA 2040 to list of external target devices
In reply to Stephen M. CameronFrom: Stephen M. Cameron <[email protected]>
--- drivers/scsi/hpsa.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/scsi/hpsa.c +1 -0
@@ -1778,6 +1778,7 @@ static unsigned char *ext_target_model[] = {"MSA2312" ,"MSA2324" ,"P2000 G3 SAS" , +"MSA 2040 SAS" , NULL, };
PATCH 8/11 hpsa: cap CCISS_PASSTHRU at 20 concurrent commands.
In reply to Stephen M. CameronFrom: Stephen M. Cameron <[email protected]>
Cap CCISS_BIG_PASSTHRU as well. If an attempt is made to exceed this, ioctl() will return -1 with errno == EAGAIN.
This is to prevent a userland program from exhausting all of pci_alloc_consistent memory. I've only seen this problem when running a special test program designed to provoke it. 20 concurrent commands via the passthru ioctls (not counting SG_IO) should be more than enough.
--- drivers/scsi/hpsa.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- drivers/scsi/hpsa.h | 5 +++++ 2 files changed, 47 insertions(+), 2 deletions(-)
drivers/scsi/hpsa.c +42 -2
@@ -3218,6 +3218,36 @@ static void check_ioctl_unit_attention(struct ctlr_info *h, c->err_info->ScsiStatus != SAM_STAT_CHECK_CONDITION) (void) check_for_unit_attention(h, c); } + +static int increment_passthru_count (struct ctlr_info *h) +{ +unsigned long flags; + +spin_lock_irqsave (&h->passthru_count_lock, flags); +if (h->passthru_count >=HPSA_MAX_CONCURRENT_PASSTHRUS ) { +spin_unlock_irqrestore (&h->passthru_count_lock, flags); +return -1 ; + } + h->passthru_count++; +spin_unlock_irqrestore (&h->passthru_count_lock, flags); +return 0 ; +} + +static void decrement_passthru_count (struct ctlr_info *h) +{ +unsigned long flags; + +spin_lock_irqsave (&h->passthru_count_lock, flags); +if (h->passthru_count <=0 ) { +spin_unlock_irqrestore (&h->passthru_count_lock, flags); +/* not expecting to get here. */ +dev_warn (&h->pdev->dev,"Bug detected, passthru_count seems to be incorrect.\n" ); +return ; + } + h->passthru_count--; +spin_unlock_irqrestore (&h->passthru_count_lock, flags); +} +/* * ioctl */
@@ -3225,6 +3255,7 @@ static int hpsa_ioctl(struct scsi_device *dev, int cmd, void *arg) {struct ctlr_info *h;void __user *argp = (void __user *)arg; +int rc; h =sdev_to_hba (dev);
@@ -3239,9 +3270,17 @@ static int hpsa_ioctl(struct scsi_device *dev, int cmd, void *arg)case CCISS_GETDRIVVER :return hpsa_getdrivver_ioctl (h, argp);case CCISS_PASSTHRU : -return hpsa_passthru_ioctl (h, argp); +if (increment_passthru_count (h)) +return -EAGAIN ; + rc =hpsa_passthru_ioctl (h, argp); +decrement_passthru_count (h); +return rc;case CCISS_BIG_PASSTHRU : -return hpsa_big_passthru_ioctl (h, argp); +if (increment_passthru_count (h)) +return -EAGAIN ; + rc =hpsa_big_passthru_ioctl (h, argp); +decrement_passthru_count (h); +return rc;default :return -ENOTTY ; }
@@ -4772,6 +4811,7 @@ reinit_after_soft_reset:INIT_LIST_HEAD (&h->reqQ);spin_lock_init (&h->lock);spin_lock_init (&h->scan_lock); +spin_lock_init (&h->passthru_count_lock); rc =hpsa_pci_init (h);if (rc !=0 ) goto clean1;
drivers/scsi/hpsa.h +5 -0
@@ -114,6 +114,11 @@ struct ctlr_info {struct TransTable_struct *transtable;unsigned long transMethod; +/* cap concurrent passthrus at some reasonable maximum */ +#define HPSA_MAX_CONCURRENT_PASSTHRUS (20) +spinlock_t passthru_count_lock;/* protects passthru_count */ +int passthru_count; +/* * Performant mode completion buffers */
PATCH 9/11 hpsa: prevent stalled i/o
In reply to Stephen M. CameronFrom: Stephen M. Cameron <[email protected]>
If a fifo full condition is encountered, i/o requests will stack up in the h->reqQ queue. The only thing which empties this queue is start_io, which only gets called when new i/o requests come in. If none are forthcoming, i/o in h->reqQ will be stalled.
To fix this, whenever fifo full condition is encountered, this is recorded, and the interrupt handler examines this to see if a fifo full condition was recently encountered when a command completes and will call start_io to prevent i/o's in h->reqQ from getting stuck.
I've only ever seen this problem occur when running specialized test programs that pound on the the CCISS_PASSTHRU ioctl.
--- drivers/scsi/hpsa.c | 32 ++++++++++++++++++++++++++++++-- drivers/scsi/hpsa.h | 1 + 2 files changed, 31 insertions(+), 2 deletions(-)
drivers/scsi/hpsa.c +30 -2
@@ -3479,9 +3479,11 @@ static void start_io(struct ctlr_info *h) c = list_entry(h->reqQ.next, struct CommandList, list);/* can't do anything if fifo is full */ if ((h->access.fifo_full (h))) { + h->fifo_recently_full =1 ;dev_warn (&h->pdev->dev,"fifo full\n" );break ; } + h->fifo_recently_full =0 ;/* Get the first entry from the Request Q */ removeQ (c);
@@ -3535,15 +3537,41 @@ static inline int bad_tag(struct ctlr_info *h, u32 tag_index,static inline void finish_cmd (struct CommandList *c) {unsigned long flags; +int io_may_be_stalled =0 ; +struct ctlr_info *h = c->h; -spin_lock_irqsave (&c->h->lock, flags); +spin_lock_irqsave (&h->lock, flags);removeQ (c); -spin_unlock_irqrestore (&c->h->lock, flags); + +/* +* Check for possibly stalled i/o. +* +* If a fifo_full condition is encountered, requests will back up +* in h->reqQ. This queue is only emptied out by start_io which is +* only called when a new i/o request comes in. If no i/o's are +* forthcoming, the i/o's in h->reqQ can get stuck. So we call +* start_io from here if we detect such a danger. +* +* Normally, we shouldn't hit this case, but pounding on the +* CCISS_PASSTHRU ioctl can provoke it. Only call start_io if +* commands_outstanding is low. We want to avoid calling +* start_io from in here as much as possible, and esp. don't +* want to get in a cycle where we call start_io every time +* through here. +*/ +if (unlikely (h->fifo_recently_full) && + h->commands_outstanding <5 ) + io_may_be_stalled =1 ; + +spin_unlock_irqrestore (&h->lock, flags); +dial_up_lockup_detection_on_fw_flash_complete (c->h, c);if (likely (c->cmd_type ==CMD_SCSI ))complete_scsi_command (c);else if (c->cmd_type ==CMD_IOCTL_PEND )complete (c->waiting); +if (unlikely (io_may_be_stalled)) +start_io (h); }static inline u32 hpsa_tag_contains_index (u32 tag)
drivers/scsi/hpsa.h +1 -0
@@ -137,6 +137,7 @@ struct ctlr_info {u32 lockup_detected;struct delayed_work monitor_ctlr_work;int remove_in_progress; +u32 fifo_recently_full;/* Address of h->q[x] is passed to intr handler to know which queue */ u8 q[MAX_REPLY_QUEUES ];u32 TMFSupportFlags;/* cache what task mgmt funcs are supported. */
PATCH 10/11 hpsa: rename scsi prefetch field
In reply to Stephen M. CameronFrom: Stephen M. Cameron <[email protected]>
The field contains more bits than just the one to indicate whether scsi prefetch should be turned on.
--- drivers/scsi/hpsa.c | 14 +++++++------- drivers/scsi/hpsa_cmd.h | 3 ++- 2 files changed, 9 insertions(+), 8 deletions(-)
drivers/scsi/hpsa.c +7 -7
@@ -4384,15 +4384,15 @@ static inline bool hpsa_CISS_signature_present(struct ctlr_info *h)return true; } -/* Need to enable prefetch in the SCSI core for 6400 in x86 */ -static inline void hpsa_enable_scsi_prefetch (struct ctlr_info *h) +static inline void hpsa_set_driver_support_bits (struct ctlr_info *h) {#ifdef CONFIG_X86 -u32 prefetch; +/* Need to enable prefetch in the SCSI core for 6400 in x86 */ +u32 driver_support; - prefetch =readl (&(h->cfgtable->SCSI_Prefetch)); - prefetch |=0x100 ; -writel (prefetch, &(h->cfgtable->SCSI_Prefetch)); + driver_support =readl (&(h->cfgtable->driver_support)); + driver_support |=ENABLE_SCSI_PREFETCH ; +writel (driver_support, &(h->cfgtable->driver_support));#endif }
@@ -4503,7 +4503,7 @@ static int hpsa_pci_init(struct ctlr_info *h) err = -ENODEV ; goto err_out_free_res; } -hpsa_enable_scsi_prefetch (h); +hpsa_set_driver_support_bits (h);hpsa_p600_dma_prefetch_quirk (h); err =hpsa_enter_simple_mode (h);if (err)
drivers/scsi/hpsa_cmd.h +2 -1
@@ -356,7 +356,8 @@ struct CfgTable {u32 TransMethodOffset;u8 ServerName[16 ];u32 HeartBeat; -u32 SCSI_Prefetch; +u32 driver_support; +#define ENABLE_SCSI_PREFETCH 0x100u32 MaxScatterGatherElements;u32 MaxLogicalUnits;u32 MaxPhysicalDevices;
PATCH 11/11 hpsa: enable unit attention reporting
In reply to Stephen M. CameronFrom: Stephen M. Cameron <[email protected]>
This used to be the default, but at some point the firmware guys changed the default and I failed to notice. Now to get unit attention notifications, you must twiddle a bit indicating you want them.
--- drivers/scsi/hpsa.c | 7 ++++--- drivers/scsi/hpsa_cmd.h | 1 + 2 files changed, 5 insertions(+), 3 deletions(-)
drivers/scsi/hpsa.c +4 -3
@@ -4386,14 +4386,15 @@ static inline bool hpsa_CISS_signature_present(struct ctlr_info *h)static inline void hpsa_set_driver_support_bits (struct ctlr_info *h) { -#ifdef CONFIG_X86 -/* Need to enable prefetch in the SCSI core for 6400 in x86 */ u32 driver_support; +#ifdef CONFIG_X86 +/* Need to enable prefetch in the SCSI core for 6400 in x86 */ driver_support =readl (&(h->cfgtable->driver_support)); driver_support |=ENABLE_SCSI_PREFETCH ; -writel (driver_support, &(h->cfgtable->driver_support));#endif + driver_support |=ENABLE_UNIT_ATTN ; +writel (driver_support, &(h->cfgtable->driver_support)); }/* Disable DMA prefetch for the P600. Otherwise an ASIC bug may result
drivers/scsi/hpsa_cmd.h +1 -0
@@ -358,6 +358,7 @@ struct CfgTable {u32 HeartBeat;u32 driver_support;#define ENABLE_SCSI_PREFETCH 0x100 +#define ENABLE_UNIT_ATTN 0x01u32 MaxScatterGatherElements;u32 MaxLogicalUnits;u32 MaxPhysicalDevices;