[PATCH 0/2 v2] hpsa: workqueue + minor fix

[PATCH 0/2 v2] hpsa: workqueue + minor fix

[Tomas Henzl]

I've adapted previously posted changes on top of the latest hpsa series.
And once again.

Tomas

[PATCH 1/2] hpsa: use a driver's own workqueue instead of a shared wq

[Tomas Henzl]

hpsa driver uses a shared wq, max sleep time in function spent
hpsa_wait_for_clear_event_notify_ack may take up to 40sec
and that is too much for a shared workqueue.
This patch takes the easiest approach and just creates a
driver's own workqueue.

Signed-off-by: Tomas Henzl <thenzl@redhat.com>
---
 drivers/scsi/hpsa.c | 18 +++++++++++++-----
 drivers/scsi/hpsa.h |  1 +
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 95d581c454..af32962259 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -6822,8 +6822,8 @@ static void hpsa_monitor_ctlr_worker(struct work_struct *work)
 
 	spin_lock_irqsave(&h->lock, flags);
 	if (!h->remove_in_progress)
-		schedule_delayed_work(&h->monitor_ctlr_work,
-				h->heartbeat_sample_interval);
+		queue_delayed_work(h->monitor_wq, &h->monitor_ctlr_work,
+			h->heartbeat_sample_interval);
 	spin_unlock_irqrestore(&h->lock, flags);
 }
 
@@ -6888,12 +6888,17 @@ reinit_after_soft_reset:
 		rc = -ENOMEM;
 		goto clean1;
 	}
-
 	h->resubmit_wq = hpsa_create_controller_wq(h, "resubmit");
 	if (!h->resubmit_wq) {
 		rc = -ENOMEM;
 		goto clean1;
 	}
+	h->monitor_wq = hpsa_create_controller_wq(h, "monitor");
+	if (!h->monitor_wq) {
+		rc = -ENOMEM;
+		goto clean1;
+	}
+
 
 	/* Allocate and clear per-cpu variable lockup_detected */
 	h->lockup_detected = alloc_percpu(u32);
@@ -7016,8 +7021,8 @@ reinit_after_soft_reset:
 	/* 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);
+	queue_delayed_work(h->monitor_wq, &h->monitor_ctlr_work,
+		h->heartbeat_sample_interval);
 	INIT_DELAYED_WORK(&h->rescan_ctlr_work, hpsa_rescan_ctlr_worker);
 	queue_delayed_work(h->rescan_ctlr_wq, &h->rescan_ctlr_work,
 				h->heartbeat_sample_interval);
@@ -7030,6 +7035,8 @@ clean2_and_free_irqs:
 	hpsa_free_irqs(h);
 clean2:
 clean1:
+	if (h->monitor_wq)
+		destroy_workqueue(h->monitor_wq);
 	if (h->resubmit_wq)
 		destroy_workqueue(h->resubmit_wq);
 	if (h->rescan_ctlr_wq)
@@ -7112,6 +7119,7 @@ static void hpsa_remove_one(struct pci_dev *pdev)
 	cancel_delayed_work_sync(&h->rescan_ctlr_work);
 	destroy_workqueue(h->rescan_ctlr_wq);
 	destroy_workqueue(h->resubmit_wq);
+	destroy_workqueue(h->monitor_wq);
 	hpsa_unregister_scsi(h);	/* unhook from SCSI subsystem */
 	hpsa_shutdown(pdev);
 	iounmap(h->vaddr);
diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
index 6577130503..39c720e6f0 100644
--- a/drivers/scsi/hpsa.h
+++ b/drivers/scsi/hpsa.h
@@ -253,6 +253,7 @@ struct ctlr_info {
 	int	raid_offload_debug;
 	struct workqueue_struct *resubmit_wq;
 	struct workqueue_struct *rescan_ctlr_wq;
+	struct workqueue_struct *monitor_wq;
 };
 
 struct offline_device_entry {
-- 
1.9.3

[PATCH 2/2] hpsa: remove a needless call - pci_get_drvdata

[Tomas Henzl]

Signed-off-by: Tomas Henzl <thenzl@redhat.com>
---
 drivers/scsi/hpsa.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index af32962259..6e4abba285 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -7102,14 +7102,13 @@ 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;
+	struct ctlr_info *h = pci_get_drvdata(pdev);
 
-	if (pci_get_drvdata(pdev) == NULL) {
+	if (!h) {
 		dev_err(&pdev->dev, "unable to remove device\n");
 		return;
 	}
-	h = pci_get_drvdata(pdev);
 
 	/* Get rid of any controller monitoring work items */
 	spin_lock_irqsave(&h->lock, flags);
-- 
1.9.3

Re: [PATCH 0/2 v2] hpsa: workqueue + minor fix

[Tomas Henzl]

On 01/27/2015 05:56 PM, Tomas Henzl wrote:
> I've adapted previously posted changes on top of the latest hpsa series.
> And once again.

And because I haven't read the code in the last hpsa series properly
I haven't realised that the body of this patch is already there.

Please ignore this series.

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

[PATCH 0/2] hpsa: workqueue + minor fix

[Tomas Henzl]

I've adapted previously posted changes on top of the latest hpsa series.

Tomas

[PATCH 1/2] hpsa: use a driver's own workqueue instead of a shared wq

[Tomas Henzl]

hpsa driver uses a shared wq, max sleep time in function spent
hpsa_wait_for_clear_event_notify_ack may take up to 20sec
and that is too much for a shared workqueue.
This patch takes the easiest approach and just creates a
driver's own workqueue.

Signed-off-by: Tomas Henzl <thenzl@redhat.com>
---
 drivers/scsi/hpsa.c | 39 +++++++++++++++++++++++++--------------
 drivers/scsi/hpsa.h |  1 +
 2 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 15ef65c25d..909562c09f 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -6229,6 +6229,12 @@ static int hpsa_wait_for_mode_change_ack(struct ctlr_info *h)
 			goto done;
 		/* delay and try again */
 		msleep(MODE_CHANGE_WAIT_INTERVAL);
+
+	/* we allow a timeout under certain conditions,
+	 * so we may return here when a removal is in progress
+	 */
+		if (h->remove_in_progress)
+			return -ENODEV;
 	}
 	return -ENODEV;
 done:
@@ -6790,7 +6796,6 @@ static int hpsa_offline_devices_ready(struct ctlr_info *h)
 
 static void hpsa_monitor_ctlr_worker(struct work_struct *work)
 {
-	unsigned long flags;
 	struct ctlr_info *h = container_of(to_delayed_work(work),
 					struct ctlr_info, monitor_ctlr_work);
 	detect_controller_lockup(h);
@@ -6804,14 +6809,10 @@ static void hpsa_monitor_ctlr_worker(struct work_struct *work)
 		scsi_host_put(h->scsi_host);
 	}
 
-	spin_lock_irqsave(&h->lock, flags);
-	if (h->remove_in_progress) {
-		spin_unlock_irqrestore(&h->lock, flags);
+	if (h->remove_in_progress)
 		return;
-	}
-	schedule_delayed_work(&h->monitor_ctlr_work,
-				h->heartbeat_sample_interval);
-	spin_unlock_irqrestore(&h->lock, flags);
+	queue_delayed_work(h->monitor_ctrl_wq, &h->monitor_ctlr_work,
+		h->heartbeat_sample_interval);
 }
 
 static int hpsa_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
@@ -6820,6 +6821,7 @@ static int hpsa_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	struct ctlr_info *h;
 	int try_soft_reset = 0;
 	unsigned long flags;
+	char wq_name[20];
 
 	if (number_of_controllers == 0)
 		printk(KERN_INFO DRIVER_NAME "\n");
@@ -6983,10 +6985,20 @@ reinit_after_soft_reset:
 	/* 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);
+	snprintf(wq_name, sizeof(wq_name), "monitor_%d_hpsa", h->ctlr);
+	h->monitor_ctrl_wq = alloc_ordered_workqueue(wq_name, 0);
+	if (!h->monitor_ctrl_wq) {
+		dev_err(&pdev->dev, "failed to create monitor workqueue\n");
+		goto clean5;
+	}
+	queue_delayed_work(h->monitor_ctrl_wq, &h->monitor_ctlr_work,
+		h->heartbeat_sample_interval);
 	return 0;
 
+clean5:
+	hpsa_unregister_scsi(h);
+	kfree(h->hba_inquiry_data);
+	h->access.set_intr_mask(h, HPSA_INTR_OFF);
 clean4:
 	hpsa_free_sg_chain_blocks(h);
 	hpsa_free_cmd_pool(h);
@@ -7058,7 +7070,6 @@ 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");
@@ -7067,10 +7078,10 @@ static void hpsa_remove_one(struct pci_dev *pdev)
 	h = pci_get_drvdata(pdev);
 
 	/* 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);
+	cancel_delayed_work_sync(&h->monitor_ctlr_work);
+	destroy_workqueue(h->monitor_ctrl_wq);
+
 	hpsa_unregister_scsi(h);	/* unhook from SCSI subsystem */
 	hpsa_shutdown(pdev);
 	destroy_workqueue(h->resubmit_wq);
diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
index 62c50c3207..bff65d0a7b 100644
--- a/drivers/scsi/hpsa.h
+++ b/drivers/scsi/hpsa.h
@@ -207,6 +207,7 @@ struct ctlr_info {
 	atomic_t firmware_flash_in_progress;
 	u32 __percpu *lockup_detected;
 	struct delayed_work monitor_ctlr_work;
+	struct workqueue_struct *monitor_ctrl_wq;
 	int remove_in_progress;
 	/* Address of h->q[x] is passed to intr handler to know which queue */
 	u8 q[MAX_REPLY_QUEUES];
-- 
1.9.3