[PATCH 3/3] ipr: Wait for aborted command responses

[PATCH 3/3] ipr: Wait for aborted command responses

[Brian King]

Fixes a race condition in abort handling that was injected
when multiple interrupt support was added. When only a single
interrupt is present, the adapter guarantees it will send
responses for aborted commands prior to the response for the
abort command itself. With multiple interrupts, these responses
generally come back on different interrupts, so we need to
ensure the abort thread waits until the aborted command is
complete so we don't perform a double completion. This race
condition was being hit frequently in environments which
were triggering command timeouts, which was resulting in
a double completion causing a kernel oops.

Cc: <stable@vger.kernel.org>
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---

 drivers/scsi/ipr.c |   92 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ipr.h |    1 
 2 files changed, 93 insertions(+)

diff -puN drivers/scsi/ipr.c~ipr_eh_wait drivers/scsi/ipr.c
--- scsi-queue/drivers/scsi/ipr.c~ipr_eh_wait	2014-10-30 17:15:37.302753120 -0500
+++ scsi-queue-bjking1/drivers/scsi/ipr.c	2014-10-30 17:15:37.311753039 -0500
@@ -683,6 +683,7 @@ static void ipr_init_ipr_cmnd(struct ipr
 	ipr_reinit_ipr_cmnd(ipr_cmd);
 	ipr_cmd->u.scratch = 0;
 	ipr_cmd->sibling = NULL;
+	ipr_cmd->eh_comp = NULL;
 	ipr_cmd->fast_done = fast_done;
 	init_timer(&ipr_cmd->timer);
 }
@@ -848,6 +849,8 @@ static void ipr_scsi_eh_done(struct ipr_
 
 	scsi_dma_unmap(ipr_cmd->scsi_cmd);
 	scsi_cmd->scsi_done(scsi_cmd);
+	if (ipr_cmd->eh_comp)
+		complete(ipr_cmd->eh_comp);
 	list_add_tail(&ipr_cmd->queue, &ipr_cmd->hrrq->hrrq_free_q);
 }
 
@@ -4854,6 +4857,84 @@ static int ipr_slave_alloc(struct scsi_d
 	return rc;
 }
 
+/**
+ * ipr_match_lun - Match function for specified LUN
+ * @ipr_cmd:	ipr command struct
+ * @device:		device to match (sdev)
+ *
+ * Returns:
+ *	1 if command matches sdev / 0 if command does not match sdev
+ **/
+static int ipr_match_lun(struct ipr_cmnd *ipr_cmd, void *device)
+{
+	if (ipr_cmd->scsi_cmd && ipr_cmd->scsi_cmd->device == device)
+		return 1;
+	return 0;
+}
+
+/**
+ * ipr_wait_for_ops - Wait for matching commands to complete
+ * @ipr_cmd:	ipr command struct
+ * @device:		device to match (sdev)
+ * @match:		match function to use
+ *
+ * Returns:
+ *	SUCCESS / FAILED
+ **/
+static int ipr_wait_for_ops(struct ipr_ioa_cfg *ioa_cfg, void *device,
+			    int (*match)(struct ipr_cmnd *, void *))
+{
+	struct ipr_cmnd *ipr_cmd;
+	int wait;
+	unsigned long flags;
+	struct ipr_hrr_queue *hrrq;
+	signed long timeout = IPR_ABORT_TASK_TIMEOUT;
+	DECLARE_COMPLETION_ONSTACK(comp);
+
+	ENTER;
+	do {
+		wait = 0;
+
+		for_each_hrrq(hrrq, ioa_cfg) {
+			spin_lock_irqsave(hrrq->lock, flags);
+			list_for_each_entry(ipr_cmd, &hrrq->hrrq_pending_q, queue) {
+				if (match(ipr_cmd, device)) {
+					ipr_cmd->eh_comp = &comp;
+					wait++;
+				}
+			}
+			spin_unlock_irqrestore(hrrq->lock, flags);
+		}
+
+		if (wait) {
+			timeout = wait_for_completion_timeout(&comp, timeout);
+
+			if (!timeout) {
+				wait = 0;
+
+				for_each_hrrq(hrrq, ioa_cfg) {
+					spin_lock_irqsave(hrrq->lock, flags);
+					list_for_each_entry(ipr_cmd, &hrrq->hrrq_pending_q, queue) {
+						if (match(ipr_cmd, device)) {
+							ipr_cmd->eh_comp = NULL;
+							wait++;
+						}
+					}
+					spin_unlock_irqrestore(hrrq->lock, flags);
+				}
+
+				if (wait)
+					dev_err(&ioa_cfg->pdev->dev, "Timed out waiting for aborted commands\n");
+				LEAVE;
+				return wait ? FAILED : SUCCESS;
+			}
+		}
+	} while (wait);
+
+	LEAVE;
+	return SUCCESS;
+}
+
 static int ipr_eh_host_reset(struct scsi_cmnd *cmd)
 {
 	struct ipr_ioa_cfg *ioa_cfg;
@@ -5073,11 +5154,17 @@ static int __ipr_eh_dev_reset(struct scs
 static int ipr_eh_dev_reset(struct scsi_cmnd *cmd)
 {
 	int rc;
+	struct ipr_ioa_cfg *ioa_cfg;
+
+	ioa_cfg = (struct ipr_ioa_cfg *) cmd->device->host->hostdata;
 
 	spin_lock_irq(cmd->device->host->host_lock);
 	rc = __ipr_eh_dev_reset(cmd);
 	spin_unlock_irq(cmd->device->host->host_lock);
 
+	if (rc == SUCCESS)
+		rc = ipr_wait_for_ops(ioa_cfg, cmd->device, ipr_match_lun);
+
 	return rc;
 }
 
@@ -5255,13 +5342,18 @@ static int ipr_eh_abort(struct scsi_cmnd
 {
 	unsigned long flags;
 	int rc;
+	struct ipr_ioa_cfg *ioa_cfg;
 
 	ENTER;
 
+	ioa_cfg = (struct ipr_ioa_cfg *) scsi_cmd->device->host->hostdata;
+
 	spin_lock_irqsave(scsi_cmd->device->host->host_lock, flags);
 	rc = ipr_cancel_op(scsi_cmd);
 	spin_unlock_irqrestore(scsi_cmd->device->host->host_lock, flags);
 
+	if (rc == SUCCESS)
+		rc = ipr_wait_for_ops(ioa_cfg, scsi_cmd->device, ipr_match_lun);
 	LEAVE;
 	return rc;
 }
diff -puN drivers/scsi/ipr.h~ipr_eh_wait drivers/scsi/ipr.h
--- scsi-queue/drivers/scsi/ipr.h~ipr_eh_wait	2014-10-30 17:15:37.305753093 -0500
+++ scsi-queue-bjking1/drivers/scsi/ipr.h	2014-10-30 17:15:37.315753003 -0500
@@ -1608,6 +1608,7 @@ struct ipr_cmnd {
 		struct scsi_device *sdev;
 	} u;
 
+	struct completion *eh_comp;
 	struct ipr_hrr_queue *hrrq;
 	struct ipr_ioa_cfg *ioa_cfg;
 };
_

[PATCH 3/3] ipr: Wait for aborted command responses

[Brian King]

Fixes a race condition in abort handling that was injected
when multiple interrupt support was added. When only a single
interrupt is present, the adapter guarantees it will send
responses for aborted commands prior to the response for the
abort command itself. With multiple interrupts, these responses
generally come back on different interrupts, so we need to
ensure the abort thread waits until the aborted command is
complete so we don't perform a double completion. This race
condition was being hit frequently in environments which
were triggering command timeouts, which was resulting in
a double completion causing a kernel oops.

Cc: <stable@vger.kernel.org>
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---

 drivers/scsi/ipr.c |   92 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ipr.h |    1 
 2 files changed, 93 insertions(+)

diff -puN drivers/scsi/ipr.c~ipr_eh_wait drivers/scsi/ipr.c
--- scsi-queue/drivers/scsi/ipr.c~ipr_eh_wait	2014-10-30 17:15:37.302753120 -0500
+++ scsi-queue-bjking1/drivers/scsi/ipr.c	2014-10-30 17:15:37.311753039 -0500
@@ -683,6 +683,7 @@ static void ipr_init_ipr_cmnd(struct ipr
 	ipr_reinit_ipr_cmnd(ipr_cmd);
 	ipr_cmd->u.scratch = 0;
 	ipr_cmd->sibling = NULL;
+	ipr_cmd->eh_comp = NULL;
 	ipr_cmd->fast_done = fast_done;
 	init_timer(&ipr_cmd->timer);
 }
@@ -848,6 +849,8 @@ static void ipr_scsi_eh_done(struct ipr_
 
 	scsi_dma_unmap(ipr_cmd->scsi_cmd);
 	scsi_cmd->scsi_done(scsi_cmd);
+	if (ipr_cmd->eh_comp)
+		complete(ipr_cmd->eh_comp);
 	list_add_tail(&ipr_cmd->queue, &ipr_cmd->hrrq->hrrq_free_q);
 }
 
@@ -4854,6 +4857,84 @@ static int ipr_slave_alloc(struct scsi_d
 	return rc;
 }
 
+/**
+ * ipr_match_lun - Match function for specified LUN
+ * @ipr_cmd:	ipr command struct
+ * @device:		device to match (sdev)
+ *
+ * Returns:
+ *	1 if command matches sdev / 0 if command does not match sdev
+ **/
+static int ipr_match_lun(struct ipr_cmnd *ipr_cmd, void *device)
+{
+	if (ipr_cmd->scsi_cmd && ipr_cmd->scsi_cmd->device == device)
+		return 1;
+	return 0;
+}
+
+/**
+ * ipr_wait_for_ops - Wait for matching commands to complete
+ * @ipr_cmd:	ipr command struct
+ * @device:		device to match (sdev)
+ * @match:		match function to use
+ *
+ * Returns:
+ *	SUCCESS / FAILED
+ **/
+static int ipr_wait_for_ops(struct ipr_ioa_cfg *ioa_cfg, void *device,
+			    int (*match)(struct ipr_cmnd *, void *))
+{
+	struct ipr_cmnd *ipr_cmd;
+	int wait;
+	unsigned long flags;
+	struct ipr_hrr_queue *hrrq;
+	signed long timeout = IPR_ABORT_TASK_TIMEOUT;
+	DECLARE_COMPLETION_ONSTACK(comp);
+
+	ENTER;
+	do {
+		wait = 0;
+
+		for_each_hrrq(hrrq, ioa_cfg) {
+			spin_lock_irqsave(hrrq->lock, flags);
+			list_for_each_entry(ipr_cmd, &hrrq->hrrq_pending_q, queue) {
+				if (match(ipr_cmd, device)) {
+					ipr_cmd->eh_comp = &comp;
+					wait++;
+				}
+			}
+			spin_unlock_irqrestore(hrrq->lock, flags);
+		}
+
+		if (wait) {
+			timeout = wait_for_completion_timeout(&comp, timeout);
+
+			if (!timeout) {
+				wait = 0;
+
+				for_each_hrrq(hrrq, ioa_cfg) {
+					spin_lock_irqsave(hrrq->lock, flags);
+					list_for_each_entry(ipr_cmd, &hrrq->hrrq_pending_q, queue) {
+						if (match(ipr_cmd, device)) {
+							ipr_cmd->eh_comp = NULL;
+							wait++;
+						}
+					}
+					spin_unlock_irqrestore(hrrq->lock, flags);
+				}
+
+				if (wait)
+					dev_err(&ioa_cfg->pdev->dev, "Timed out waiting for aborted commands\n");
+				LEAVE;
+				return wait ? FAILED : SUCCESS;
+			}
+		}
+	} while (wait);
+
+	LEAVE;
+	return SUCCESS;
+}
+
 static int ipr_eh_host_reset(struct scsi_cmnd *cmd)
 {
 	struct ipr_ioa_cfg *ioa_cfg;
@@ -5073,11 +5154,17 @@ static int __ipr_eh_dev_reset(struct scs
 static int ipr_eh_dev_reset(struct scsi_cmnd *cmd)
 {
 	int rc;
+	struct ipr_ioa_cfg *ioa_cfg;
+
+	ioa_cfg = (struct ipr_ioa_cfg *) cmd->device->host->hostdata;
 
 	spin_lock_irq(cmd->device->host->host_lock);
 	rc = __ipr_eh_dev_reset(cmd);
 	spin_unlock_irq(cmd->device->host->host_lock);
 
+	if (rc == SUCCESS)
+		rc = ipr_wait_for_ops(ioa_cfg, cmd->device, ipr_match_lun);
+
 	return rc;
 }
 
@@ -5255,13 +5342,18 @@ static int ipr_eh_abort(struct scsi_cmnd
 {
 	unsigned long flags;
 	int rc;
+	struct ipr_ioa_cfg *ioa_cfg;
 
 	ENTER;
 
+	ioa_cfg = (struct ipr_ioa_cfg *) scsi_cmd->device->host->hostdata;
+
 	spin_lock_irqsave(scsi_cmd->device->host->host_lock, flags);
 	rc = ipr_cancel_op(scsi_cmd);
 	spin_unlock_irqrestore(scsi_cmd->device->host->host_lock, flags);
 
+	if (rc == SUCCESS)
+		rc = ipr_wait_for_ops(ioa_cfg, scsi_cmd->device, ipr_match_lun);
 	LEAVE;
 	return rc;
 }
diff -puN drivers/scsi/ipr.h~ipr_eh_wait drivers/scsi/ipr.h
--- scsi-queue/drivers/scsi/ipr.h~ipr_eh_wait	2014-10-30 17:15:37.305753093 -0500
+++ scsi-queue-bjking1/drivers/scsi/ipr.h	2014-10-30 17:15:37.315753003 -0500
@@ -1608,6 +1608,7 @@ struct ipr_cmnd {
 		struct scsi_device *sdev;
 	} u;
 
+	struct completion *eh_comp;
 	struct ipr_hrr_queue *hrrq;
 	struct ipr_ioa_cfg *ioa_cfg;
 };
_

Re: [PATCH 3/3] ipr: Wait for aborted command responses

[Brian King]

Adding Wendy...

On 10/30/2014 05:27 PM, Brian King wrote:
> Fixes a race condition in abort handling that was injected
> when multiple interrupt support was added. When only a single
> interrupt is present, the adapter guarantees it will send
> responses for aborted commands prior to the response for the
> abort command itself. With multiple interrupts, these responses
> generally come back on different interrupts, so we need to
> ensure the abort thread waits until the aborted command is
> complete so we don't perform a double completion. This race
> condition was being hit frequently in environments which
> were triggering command timeouts, which was resulting in
> a double completion causing a kernel oops.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
> ---
> 
>  drivers/scsi/ipr.c |   92 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/ipr.h |    1 
>  2 files changed, 93 insertions(+)
> 
> diff -puN drivers/scsi/ipr.c~ipr_eh_wait drivers/scsi/ipr.c
> --- scsi-queue/drivers/scsi/ipr.c~ipr_eh_wait	2014-10-30 17:15:37.302753120 -0500
> +++ scsi-queue-bjking1/drivers/scsi/ipr.c	2014-10-30 17:15:37.311753039 -0500
> @@ -683,6 +683,7 @@ static void ipr_init_ipr_cmnd(struct ipr
>  	ipr_reinit_ipr_cmnd(ipr_cmd);
>  	ipr_cmd->u.scratch = 0;
>  	ipr_cmd->sibling = NULL;
> +	ipr_cmd->eh_comp = NULL;
>  	ipr_cmd->fast_done = fast_done;
>  	init_timer(&ipr_cmd->timer);
>  }
> @@ -848,6 +849,8 @@ static void ipr_scsi_eh_done(struct ipr_
> 
>  	scsi_dma_unmap(ipr_cmd->scsi_cmd);
>  	scsi_cmd->scsi_done(scsi_cmd);
> +	if (ipr_cmd->eh_comp)
> +		complete(ipr_cmd->eh_comp);
>  	list_add_tail(&ipr_cmd->queue, &ipr_cmd->hrrq->hrrq_free_q);
>  }
> 
> @@ -4854,6 +4857,84 @@ static int ipr_slave_alloc(struct scsi_d
>  	return rc;
>  }
> 
> +/**
> + * ipr_match_lun - Match function for specified LUN
> + * @ipr_cmd:	ipr command struct
> + * @device:		device to match (sdev)
> + *
> + * Returns:
> + *	1 if command matches sdev / 0 if command does not match sdev
> + **/
> +static int ipr_match_lun(struct ipr_cmnd *ipr_cmd, void *device)
> +{
> +	if (ipr_cmd->scsi_cmd && ipr_cmd->scsi_cmd->device == device)
> +		return 1;
> +	return 0;
> +}
> +
> +/**
> + * ipr_wait_for_ops - Wait for matching commands to complete
> + * @ipr_cmd:	ipr command struct
> + * @device:		device to match (sdev)
> + * @match:		match function to use
> + *
> + * Returns:
> + *	SUCCESS / FAILED
> + **/
> +static int ipr_wait_for_ops(struct ipr_ioa_cfg *ioa_cfg, void *device,
> +			    int (*match)(struct ipr_cmnd *, void *))
> +{
> +	struct ipr_cmnd *ipr_cmd;
> +	int wait;
> +	unsigned long flags;
> +	struct ipr_hrr_queue *hrrq;
> +	signed long timeout = IPR_ABORT_TASK_TIMEOUT;
> +	DECLARE_COMPLETION_ONSTACK(comp);
> +
> +	ENTER;
> +	do {
> +		wait = 0;
> +
> +		for_each_hrrq(hrrq, ioa_cfg) {
> +			spin_lock_irqsave(hrrq->lock, flags);
> +			list_for_each_entry(ipr_cmd, &hrrq->hrrq_pending_q, queue) {
> +				if (match(ipr_cmd, device)) {
> +					ipr_cmd->eh_comp = &comp;
> +					wait++;
> +				}
> +			}
> +			spin_unlock_irqrestore(hrrq->lock, flags);
> +		}
> +
> +		if (wait) {
> +			timeout = wait_for_completion_timeout(&comp, timeout);
> +
> +			if (!timeout) {
> +				wait = 0;
> +
> +				for_each_hrrq(hrrq, ioa_cfg) {
> +					spin_lock_irqsave(hrrq->lock, flags);
> +					list_for_each_entry(ipr_cmd, &hrrq->hrrq_pending_q, queue) {
> +						if (match(ipr_cmd, device)) {
> +							ipr_cmd->eh_comp = NULL;
> +							wait++;
> +						}
> +					}
> +					spin_unlock_irqrestore(hrrq->lock, flags);
> +				}
> +
> +				if (wait)
> +					dev_err(&ioa_cfg->pdev->dev, "Timed out waiting for aborted commands\n");
> +				LEAVE;
> +				return wait ? FAILED : SUCCESS;
> +			}
> +		}
> +	} while (wait);
> +
> +	LEAVE;
> +	return SUCCESS;
> +}
> +
>  static int ipr_eh_host_reset(struct scsi_cmnd *cmd)
>  {
>  	struct ipr_ioa_cfg *ioa_cfg;
> @@ -5073,11 +5154,17 @@ static int __ipr_eh_dev_reset(struct scs
>  static int ipr_eh_dev_reset(struct scsi_cmnd *cmd)
>  {
>  	int rc;
> +	struct ipr_ioa_cfg *ioa_cfg;
> +
> +	ioa_cfg = (struct ipr_ioa_cfg *) cmd->device->host->hostdata;
> 
>  	spin_lock_irq(cmd->device->host->host_lock);
>  	rc = __ipr_eh_dev_reset(cmd);
>  	spin_unlock_irq(cmd->device->host->host_lock);
> 
> +	if (rc == SUCCESS)
> +		rc = ipr_wait_for_ops(ioa_cfg, cmd->device, ipr_match_lun);
> +
>  	return rc;
>  }
> 
> @@ -5255,13 +5342,18 @@ static int ipr_eh_abort(struct scsi_cmnd
>  {
>  	unsigned long flags;
>  	int rc;
> +	struct ipr_ioa_cfg *ioa_cfg;
> 
>  	ENTER;
> 
> +	ioa_cfg = (struct ipr_ioa_cfg *) scsi_cmd->device->host->hostdata;
> +
>  	spin_lock_irqsave(scsi_cmd->device->host->host_lock, flags);
>  	rc = ipr_cancel_op(scsi_cmd);
>  	spin_unlock_irqrestore(scsi_cmd->device->host->host_lock, flags);
> 
> +	if (rc == SUCCESS)
> +		rc = ipr_wait_for_ops(ioa_cfg, scsi_cmd->device, ipr_match_lun);
>  	LEAVE;
>  	return rc;
>  }
> diff -puN drivers/scsi/ipr.h~ipr_eh_wait drivers/scsi/ipr.h
> --- scsi-queue/drivers/scsi/ipr.h~ipr_eh_wait	2014-10-30 17:15:37.305753093 -0500
> +++ scsi-queue-bjking1/drivers/scsi/ipr.h	2014-10-30 17:15:37.315753003 -0500
> @@ -1608,6 +1608,7 @@ struct ipr_cmnd {
>  		struct scsi_device *sdev;
>  	} u;
> 
> +	struct completion *eh_comp;
>  	struct ipr_hrr_queue *hrrq;
>  	struct ipr_ioa_cfg *ioa_cfg;
>  };
> _
> 


-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: [PATCH 3/3] ipr: Wait for aborted command responses

[Brian King]

Adding Wendy...

On 10/30/2014 05:27 PM, Brian King wrote:
> Fixes a race condition in abort handling that was injected
> when multiple interrupt support was added. When only a single
> interrupt is present, the adapter guarantees it will send
> responses for aborted commands prior to the response for the
> abort command itself. With multiple interrupts, these responses
> generally come back on different interrupts, so we need to
> ensure the abort thread waits until the aborted command is
> complete so we don't perform a double completion. This race
> condition was being hit frequently in environments which
> were triggering command timeouts, which was resulting in
> a double completion causing a kernel oops.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
> ---
> 
>  drivers/scsi/ipr.c |   92 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/ipr.h |    1 
>  2 files changed, 93 insertions(+)
> 
> diff -puN drivers/scsi/ipr.c~ipr_eh_wait drivers/scsi/ipr.c
> --- scsi-queue/drivers/scsi/ipr.c~ipr_eh_wait	2014-10-30 17:15:37.302753120 -0500
> +++ scsi-queue-bjking1/drivers/scsi/ipr.c	2014-10-30 17:15:37.311753039 -0500
> @@ -683,6 +683,7 @@ static void ipr_init_ipr_cmnd(struct ipr
>  	ipr_reinit_ipr_cmnd(ipr_cmd);
>  	ipr_cmd->u.scratch = 0;
>  	ipr_cmd->sibling = NULL;
> +	ipr_cmd->eh_comp = NULL;
>  	ipr_cmd->fast_done = fast_done;
>  	init_timer(&ipr_cmd->timer);
>  }
> @@ -848,6 +849,8 @@ static void ipr_scsi_eh_done(struct ipr_
> 
>  	scsi_dma_unmap(ipr_cmd->scsi_cmd);
>  	scsi_cmd->scsi_done(scsi_cmd);
> +	if (ipr_cmd->eh_comp)
> +		complete(ipr_cmd->eh_comp);
>  	list_add_tail(&ipr_cmd->queue, &ipr_cmd->hrrq->hrrq_free_q);
>  }
> 
> @@ -4854,6 +4857,84 @@ static int ipr_slave_alloc(struct scsi_d
>  	return rc;
>  }
> 
> +/**
> + * ipr_match_lun - Match function for specified LUN
> + * @ipr_cmd:	ipr command struct
> + * @device:		device to match (sdev)
> + *
> + * Returns:
> + *	1 if command matches sdev / 0 if command does not match sdev
> + **/
> +static int ipr_match_lun(struct ipr_cmnd *ipr_cmd, void *device)
> +{
> +	if (ipr_cmd->scsi_cmd && ipr_cmd->scsi_cmd->device == device)
> +		return 1;
> +	return 0;
> +}
> +
> +/**
> + * ipr_wait_for_ops - Wait for matching commands to complete
> + * @ipr_cmd:	ipr command struct
> + * @device:		device to match (sdev)
> + * @match:		match function to use
> + *
> + * Returns:
> + *	SUCCESS / FAILED
> + **/
> +static int ipr_wait_for_ops(struct ipr_ioa_cfg *ioa_cfg, void *device,
> +			    int (*match)(struct ipr_cmnd *, void *))
> +{
> +	struct ipr_cmnd *ipr_cmd;
> +	int wait;
> +	unsigned long flags;
> +	struct ipr_hrr_queue *hrrq;
> +	signed long timeout = IPR_ABORT_TASK_TIMEOUT;
> +	DECLARE_COMPLETION_ONSTACK(comp);
> +
> +	ENTER;
> +	do {
> +		wait = 0;
> +
> +		for_each_hrrq(hrrq, ioa_cfg) {
> +			spin_lock_irqsave(hrrq->lock, flags);
> +			list_for_each_entry(ipr_cmd, &hrrq->hrrq_pending_q, queue) {
> +				if (match(ipr_cmd, device)) {
> +					ipr_cmd->eh_comp = &comp;
> +					wait++;
> +				}
> +			}
> +			spin_unlock_irqrestore(hrrq->lock, flags);
> +		}
> +
> +		if (wait) {
> +			timeout = wait_for_completion_timeout(&comp, timeout);
> +
> +			if (!timeout) {
> +				wait = 0;
> +
> +				for_each_hrrq(hrrq, ioa_cfg) {
> +					spin_lock_irqsave(hrrq->lock, flags);
> +					list_for_each_entry(ipr_cmd, &hrrq->hrrq_pending_q, queue) {
> +						if (match(ipr_cmd, device)) {
> +							ipr_cmd->eh_comp = NULL;
> +							wait++;
> +						}
> +					}
> +					spin_unlock_irqrestore(hrrq->lock, flags);
> +				}
> +
> +				if (wait)
> +					dev_err(&ioa_cfg->pdev->dev, "Timed out waiting for aborted commands\n");
> +				LEAVE;
> +				return wait ? FAILED : SUCCESS;
> +			}
> +		}
> +	} while (wait);
> +
> +	LEAVE;
> +	return SUCCESS;
> +}
> +
>  static int ipr_eh_host_reset(struct scsi_cmnd *cmd)
>  {
>  	struct ipr_ioa_cfg *ioa_cfg;
> @@ -5073,11 +5154,17 @@ static int __ipr_eh_dev_reset(struct scs
>  static int ipr_eh_dev_reset(struct scsi_cmnd *cmd)
>  {
>  	int rc;
> +	struct ipr_ioa_cfg *ioa_cfg;
> +
> +	ioa_cfg = (struct ipr_ioa_cfg *) cmd->device->host->hostdata;
> 
>  	spin_lock_irq(cmd->device->host->host_lock);
>  	rc = __ipr_eh_dev_reset(cmd);
>  	spin_unlock_irq(cmd->device->host->host_lock);
> 
> +	if (rc == SUCCESS)
> +		rc = ipr_wait_for_ops(ioa_cfg, cmd->device, ipr_match_lun);
> +
>  	return rc;
>  }
> 
> @@ -5255,13 +5342,18 @@ static int ipr_eh_abort(struct scsi_cmnd
>  {
>  	unsigned long flags;
>  	int rc;
> +	struct ipr_ioa_cfg *ioa_cfg;
> 
>  	ENTER;
> 
> +	ioa_cfg = (struct ipr_ioa_cfg *) scsi_cmd->device->host->hostdata;
> +
>  	spin_lock_irqsave(scsi_cmd->device->host->host_lock, flags);
>  	rc = ipr_cancel_op(scsi_cmd);
>  	spin_unlock_irqrestore(scsi_cmd->device->host->host_lock, flags);
> 
> +	if (rc == SUCCESS)
> +		rc = ipr_wait_for_ops(ioa_cfg, scsi_cmd->device, ipr_match_lun);
>  	LEAVE;
>  	return rc;
>  }
> diff -puN drivers/scsi/ipr.h~ipr_eh_wait drivers/scsi/ipr.h
> --- scsi-queue/drivers/scsi/ipr.h~ipr_eh_wait	2014-10-30 17:15:37.305753093 -0500
> +++ scsi-queue-bjking1/drivers/scsi/ipr.h	2014-10-30 17:15:37.315753003 -0500
> @@ -1608,6 +1608,7 @@ struct ipr_cmnd {
>  		struct scsi_device *sdev;
>  	} u;
> 
> +	struct completion *eh_comp;
>  	struct ipr_hrr_queue *hrrq;
>  	struct ipr_ioa_cfg *ioa_cfg;
>  };
> _
> 


-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: [PATCH 3/3] ipr: Wait for aborted command responses

[wenxiong]

I have reviewed the patch and it fixed the issue we saw in our  
environment recently.

Thanks,
Wendy

Quoting Brian King <brking@linux.vnet.ibm.com>:

> Adding Wendy...
>
> On 10/30/2014 05:27 PM, Brian King wrote:
>> Fixes a race condition in abort handling that was injected
>> when multiple interrupt support was added. When only a single
>> interrupt is present, the adapter guarantees it will send
>> responses for aborted commands prior to the response for the
>> abort command itself. With multiple interrupts, these responses
>> generally come back on different interrupts, so we need to
>> ensure the abort thread waits until the aborted command is
>> complete so we don't perform a double completion. This race
>> condition was being hit frequently in environments which
>> were triggering command timeouts, which was resulting in
>> a double completion causing a kernel oops.
>>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
>> ---
>>
>>  drivers/scsi/ipr.c |   92  
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/scsi/ipr.h |    1
>>  2 files changed, 93 insertions(+)
>>
>> diff -puN drivers/scsi/ipr.c~ipr_eh_wait drivers/scsi/ipr.c
>> --- scsi-queue/drivers/scsi/ipr.c~ipr_eh_wait	2014-10-30  
>> 17:15:37.302753120 -0500
>> +++ scsi-queue-bjking1/drivers/scsi/ipr.c	2014-10-30  
>> 17:15:37.311753039 -0500
>> @@ -683,6 +683,7 @@ static void ipr_init_ipr_cmnd(struct ipr
>>  	ipr_reinit_ipr_cmnd(ipr_cmd);
>>  	ipr_cmd->u.scratch = 0;
>>  	ipr_cmd->sibling = NULL;
>> +	ipr_cmd->eh_comp = NULL;
>>  	ipr_cmd->fast_done = fast_done;
>>  	init_timer(&ipr_cmd->timer);
>>  }
>> @@ -848,6 +849,8 @@ static void ipr_scsi_eh_done(struct ipr_
>>
>>  	scsi_dma_unmap(ipr_cmd->scsi_cmd);
>>  	scsi_cmd->scsi_done(scsi_cmd);
>> +	if (ipr_cmd->eh_comp)
>> +		complete(ipr_cmd->eh_comp);
>>  	list_add_tail(&ipr_cmd->queue, &ipr_cmd->hrrq->hrrq_free_q);
>>  }
>>
>> @@ -4854,6 +4857,84 @@ static int ipr_slave_alloc(struct scsi_d
>>  	return rc;
>>  }
>>
>> +/**
>> + * ipr_match_lun - Match function for specified LUN
>> + * @ipr_cmd:	ipr command struct
>> + * @device:		device to match (sdev)
>> + *
>> + * Returns:
>> + *	1 if command matches sdev / 0 if command does not match sdev
>> + **/
>> +static int ipr_match_lun(struct ipr_cmnd *ipr_cmd, void *device)
>> +{
>> +	if (ipr_cmd->scsi_cmd && ipr_cmd->scsi_cmd->device == device)
>> +		return 1;
>> +	return 0;
>> +}
>> +
>> +/**
>> + * ipr_wait_for_ops - Wait for matching commands to complete
>> + * @ipr_cmd:	ipr command struct
>> + * @device:		device to match (sdev)
>> + * @match:		match function to use
>> + *
>> + * Returns:
>> + *	SUCCESS / FAILED
>> + **/
>> +static int ipr_wait_for_ops(struct ipr_ioa_cfg *ioa_cfg, void *device,
>> +			    int (*match)(struct ipr_cmnd *, void *))
>> +{
>> +	struct ipr_cmnd *ipr_cmd;
>> +	int wait;
>> +	unsigned long flags;
>> +	struct ipr_hrr_queue *hrrq;
>> +	signed long timeout = IPR_ABORT_TASK_TIMEOUT;
>> +	DECLARE_COMPLETION_ONSTACK(comp);
>> +
>> +	ENTER;
>> +	do {
>> +		wait = 0;
>> +
>> +		for_each_hrrq(hrrq, ioa_cfg) {
>> +			spin_lock_irqsave(hrrq->lock, flags);
>> +			list_for_each_entry(ipr_cmd, &hrrq->hrrq_pending_q, queue) {
>> +				if (match(ipr_cmd, device)) {
>> +					ipr_cmd->eh_comp = &comp;
>> +					wait++;
>> +				}
>> +			}
>> +			spin_unlock_irqrestore(hrrq->lock, flags);
>> +		}
>> +
>> +		if (wait) {
>> +			timeout = wait_for_completion_timeout(&comp, timeout);
>> +
>> +			if (!timeout) {
>> +				wait = 0;
>> +
>> +				for_each_hrrq(hrrq, ioa_cfg) {
>> +					spin_lock_irqsave(hrrq->lock, flags);
>> +					list_for_each_entry(ipr_cmd, &hrrq->hrrq_pending_q, queue) {
>> +						if (match(ipr_cmd, device)) {
>> +							ipr_cmd->eh_comp = NULL;
>> +							wait++;
>> +						}
>> +					}
>> +					spin_unlock_irqrestore(hrrq->lock, flags);
>> +				}
>> +
>> +				if (wait)
>> +					dev_err(&ioa_cfg->pdev->dev, "Timed out waiting for aborted  
>> commands\n");
>> +				LEAVE;
>> +				return wait ? FAILED : SUCCESS;
>> +			}
>> +		}
>> +	} while (wait);
>> +
>> +	LEAVE;
>> +	return SUCCESS;
>> +}
>> +
>>  static int ipr_eh_host_reset(struct scsi_cmnd *cmd)
>>  {
>>  	struct ipr_ioa_cfg *ioa_cfg;
>> @@ -5073,11 +5154,17 @@ static int __ipr_eh_dev_reset(struct scs
>>  static int ipr_eh_dev_reset(struct scsi_cmnd *cmd)
>>  {
>>  	int rc;
>> +	struct ipr_ioa_cfg *ioa_cfg;
>> +
>> +	ioa_cfg = (struct ipr_ioa_cfg *) cmd->device->host->hostdata;
>>
>>  	spin_lock_irq(cmd->device->host->host_lock);
>>  	rc = __ipr_eh_dev_reset(cmd);
>>  	spin_unlock_irq(cmd->device->host->host_lock);
>>
>> +	if (rc == SUCCESS)
>> +		rc = ipr_wait_for_ops(ioa_cfg, cmd->device, ipr_match_lun);
>> +
>>  	return rc;
>>  }
>>
>> @@ -5255,13 +5342,18 @@ static int ipr_eh_abort(struct scsi_cmnd
>>  {
>>  	unsigned long flags;
>>  	int rc;
>> +	struct ipr_ioa_cfg *ioa_cfg;
>>
>>  	ENTER;
>>
>> +	ioa_cfg = (struct ipr_ioa_cfg *) scsi_cmd->device->host->hostdata;
>> +
>>  	spin_lock_irqsave(scsi_cmd->device->host->host_lock, flags);
>>  	rc = ipr_cancel_op(scsi_cmd);
>>  	spin_unlock_irqrestore(scsi_cmd->device->host->host_lock, flags);
>>
>> +	if (rc == SUCCESS)
>> +		rc = ipr_wait_for_ops(ioa_cfg, scsi_cmd->device, ipr_match_lun);
>>  	LEAVE;
>>  	return rc;
>>  }
>> diff -puN drivers/scsi/ipr.h~ipr_eh_wait drivers/scsi/ipr.h
>> --- scsi-queue/drivers/scsi/ipr.h~ipr_eh_wait	2014-10-30  
>> 17:15:37.305753093 -0500
>> +++ scsi-queue-bjking1/drivers/scsi/ipr.h	2014-10-30  
>> 17:15:37.315753003 -0500
>> @@ -1608,6 +1608,7 @@ struct ipr_cmnd {
>>  		struct scsi_device *sdev;
>>  	} u;
>>
>> +	struct completion *eh_comp;
>>  	struct ipr_hrr_queue *hrrq;
>>  	struct ipr_ioa_cfg *ioa_cfg;
>>  };
>> _
>>
>
>
> --
> Brian King
> Power Linux I/O
> IBM Linux Technology Center

Re: [PATCH 3/3] ipr: Wait for aborted command responses

[wenxiong]

I have reviewed the patch and it fixed the issue we saw in our  
environment recently.

Thanks,
Wendy

Quoting Brian King <brking@linux.vnet.ibm.com>:

> Adding Wendy...
>
> On 10/30/2014 05:27 PM, Brian King wrote:
>> Fixes a race condition in abort handling that was injected
>> when multiple interrupt support was added. When only a single
>> interrupt is present, the adapter guarantees it will send
>> responses for aborted commands prior to the response for the
>> abort command itself. With multiple interrupts, these responses
>> generally come back on different interrupts, so we need to
>> ensure the abort thread waits until the aborted command is
>> complete so we don't perform a double completion. This race
>> condition was being hit frequently in environments which
>> were triggering command timeouts, which was resulting in
>> a double completion causing a kernel oops.
>>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
>> ---
>>
>>  drivers/scsi/ipr.c |   92  
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/scsi/ipr.h |    1
>>  2 files changed, 93 insertions(+)
>>
>> diff -puN drivers/scsi/ipr.c~ipr_eh_wait drivers/scsi/ipr.c
>> --- scsi-queue/drivers/scsi/ipr.c~ipr_eh_wait	2014-10-30  
>> 17:15:37.302753120 -0500
>> +++ scsi-queue-bjking1/drivers/scsi/ipr.c	2014-10-30  
>> 17:15:37.311753039 -0500
>> @@ -683,6 +683,7 @@ static void ipr_init_ipr_cmnd(struct ipr
>>  	ipr_reinit_ipr_cmnd(ipr_cmd);
>>  	ipr_cmd->u.scratch = 0;
>>  	ipr_cmd->sibling = NULL;
>> +	ipr_cmd->eh_comp = NULL;
>>  	ipr_cmd->fast_done = fast_done;
>>  	init_timer(&ipr_cmd->timer);
>>  }
>> @@ -848,6 +849,8 @@ static void ipr_scsi_eh_done(struct ipr_
>>
>>  	scsi_dma_unmap(ipr_cmd->scsi_cmd);
>>  	scsi_cmd->scsi_done(scsi_cmd);
>> +	if (ipr_cmd->eh_comp)
>> +		complete(ipr_cmd->eh_comp);
>>  	list_add_tail(&ipr_cmd->queue, &ipr_cmd->hrrq->hrrq_free_q);
>>  }
>>
>> @@ -4854,6 +4857,84 @@ static int ipr_slave_alloc(struct scsi_d
>>  	return rc;
>>  }
>>
>> +/**
>> + * ipr_match_lun - Match function for specified LUN
>> + * @ipr_cmd:	ipr command struct
>> + * @device:		device to match (sdev)
>> + *
>> + * Returns:
>> + *	1 if command matches sdev / 0 if command does not match sdev
>> + **/
>> +static int ipr_match_lun(struct ipr_cmnd *ipr_cmd, void *device)
>> +{
>> +	if (ipr_cmd->scsi_cmd && ipr_cmd->scsi_cmd->device == device)
>> +		return 1;
>> +	return 0;
>> +}
>> +
>> +/**
>> + * ipr_wait_for_ops - Wait for matching commands to complete
>> + * @ipr_cmd:	ipr command struct
>> + * @device:		device to match (sdev)
>> + * @match:		match function to use
>> + *
>> + * Returns:
>> + *	SUCCESS / FAILED
>> + **/
>> +static int ipr_wait_for_ops(struct ipr_ioa_cfg *ioa_cfg, void *device,
>> +			    int (*match)(struct ipr_cmnd *, void *))
>> +{
>> +	struct ipr_cmnd *ipr_cmd;
>> +	int wait;
>> +	unsigned long flags;
>> +	struct ipr_hrr_queue *hrrq;
>> +	signed long timeout = IPR_ABORT_TASK_TIMEOUT;
>> +	DECLARE_COMPLETION_ONSTACK(comp);
>> +
>> +	ENTER;
>> +	do {
>> +		wait = 0;
>> +
>> +		for_each_hrrq(hrrq, ioa_cfg) {
>> +			spin_lock_irqsave(hrrq->lock, flags);
>> +			list_for_each_entry(ipr_cmd, &hrrq->hrrq_pending_q, queue) {
>> +				if (match(ipr_cmd, device)) {
>> +					ipr_cmd->eh_comp = &comp;
>> +					wait++;
>> +				}
>> +			}
>> +			spin_unlock_irqrestore(hrrq->lock, flags);
>> +		}
>> +
>> +		if (wait) {
>> +			timeout = wait_for_completion_timeout(&comp, timeout);
>> +
>> +			if (!timeout) {
>> +				wait = 0;
>> +
>> +				for_each_hrrq(hrrq, ioa_cfg) {
>> +					spin_lock_irqsave(hrrq->lock, flags);
>> +					list_for_each_entry(ipr_cmd, &hrrq->hrrq_pending_q, queue) {
>> +						if (match(ipr_cmd, device)) {
>> +							ipr_cmd->eh_comp = NULL;
>> +							wait++;
>> +						}
>> +					}
>> +					spin_unlock_irqrestore(hrrq->lock, flags);
>> +				}
>> +
>> +				if (wait)
>> +					dev_err(&ioa_cfg->pdev->dev, "Timed out waiting for aborted  
>> commands\n");
>> +				LEAVE;
>> +				return wait ? FAILED : SUCCESS;
>> +			}
>> +		}
>> +	} while (wait);
>> +
>> +	LEAVE;
>> +	return SUCCESS;
>> +}
>> +
>>  static int ipr_eh_host_reset(struct scsi_cmnd *cmd)
>>  {
>>  	struct ipr_ioa_cfg *ioa_cfg;
>> @@ -5073,11 +5154,17 @@ static int __ipr_eh_dev_reset(struct scs
>>  static int ipr_eh_dev_reset(struct scsi_cmnd *cmd)
>>  {
>>  	int rc;
>> +	struct ipr_ioa_cfg *ioa_cfg;
>> +
>> +	ioa_cfg = (struct ipr_ioa_cfg *) cmd->device->host->hostdata;
>>
>>  	spin_lock_irq(cmd->device->host->host_lock);
>>  	rc = __ipr_eh_dev_reset(cmd);
>>  	spin_unlock_irq(cmd->device->host->host_lock);
>>
>> +	if (rc == SUCCESS)
>> +		rc = ipr_wait_for_ops(ioa_cfg, cmd->device, ipr_match_lun);
>> +
>>  	return rc;
>>  }
>>
>> @@ -5255,13 +5342,18 @@ static int ipr_eh_abort(struct scsi_cmnd
>>  {
>>  	unsigned long flags;
>>  	int rc;
>> +	struct ipr_ioa_cfg *ioa_cfg;
>>
>>  	ENTER;
>>
>> +	ioa_cfg = (struct ipr_ioa_cfg *) scsi_cmd->device->host->hostdata;
>> +
>>  	spin_lock_irqsave(scsi_cmd->device->host->host_lock, flags);
>>  	rc = ipr_cancel_op(scsi_cmd);
>>  	spin_unlock_irqrestore(scsi_cmd->device->host->host_lock, flags);
>>
>> +	if (rc == SUCCESS)
>> +		rc = ipr_wait_for_ops(ioa_cfg, scsi_cmd->device, ipr_match_lun);
>>  	LEAVE;
>>  	return rc;
>>  }
>> diff -puN drivers/scsi/ipr.h~ipr_eh_wait drivers/scsi/ipr.h
>> --- scsi-queue/drivers/scsi/ipr.h~ipr_eh_wait	2014-10-30  
>> 17:15:37.305753093 -0500
>> +++ scsi-queue-bjking1/drivers/scsi/ipr.h	2014-10-30  
>> 17:15:37.315753003 -0500
>> @@ -1608,6 +1608,7 @@ struct ipr_cmnd {
>>  		struct scsi_device *sdev;
>>  	} u;
>>
>> +	struct completion *eh_comp;
>>  	struct ipr_hrr_queue *hrrq;
>>  	struct ipr_ioa_cfg *ioa_cfg;
>>  };
>> _
>>
>
>
> --
> Brian King
> Power Linux I/O
> IBM Linux Technology Center