| From a48575ae8ad76879dedacaf1a520e0d0f0942987 Mon Sep 17 00:00:00 2001 |
| From: Sasha Levin <sashal@kernel.org> |
| Date: Mon, 12 Apr 2021 18:10:09 -0600 |
| Subject: scsi: ibmvfc: Fix invalid state machine BUG_ON() |
| |
| From: Brian King <brking@linux.vnet.ibm.com> |
| |
| [ Upstream commit 15cfef8623a449d40d16541687afd58e78033be3 ] |
| |
| This fixes an issue hitting the BUG_ON() in ibmvfc_do_work(). When going |
| through a host action of IBMVFC_HOST_ACTION_RESET, we change the action to |
| IBMVFC_HOST_ACTION_TGT_DEL, then drop the host lock, and reset the CRQ, |
| which changes the host state to IBMVFC_NO_CRQ. If, prior to setting the |
| host state to IBMVFC_NO_CRQ, ibmvfc_init_host() is called, it can then end |
| up changing the host action to IBMVFC_HOST_ACTION_INIT. If we then change |
| the host state to IBMVFC_NO_CRQ, we will then hit the BUG_ON(). |
| |
| Make a couple of changes to avoid this. Leave the host action to be |
| IBMVFC_HOST_ACTION_RESET or IBMVFC_HOST_ACTION_REENABLE until after we drop |
| the host lock and reset or reenable the CRQ. Also harden the host state |
| machine to ensure we cannot leave the reset / reenable state until we've |
| finished processing the reset or reenable. |
| |
| Link: https://lore.kernel.org/r/20210413001009.902400-1-tyreld@linux.ibm.com |
| Fixes: 73ee5d867287 ("[SCSI] ibmvfc: Fix soft lockup on resume") |
| Signed-off-by: Brian King <brking@linux.vnet.ibm.com> |
| [tyreld: added fixes tag] |
| Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com> |
| [mkp: fix comment checkpatch warnings] |
| Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> |
| Signed-off-by: Sasha Levin <sashal@kernel.org> |
| --- |
| drivers/scsi/ibmvscsi/ibmvfc.c | 57 ++++++++++++++++++++++------------ |
| 1 file changed, 38 insertions(+), 19 deletions(-) |
| |
| diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c |
| index 65f168c41d23..8ac9eb962bff 100644 |
| --- a/drivers/scsi/ibmvscsi/ibmvfc.c |
| +++ b/drivers/scsi/ibmvscsi/ibmvfc.c |
| @@ -560,8 +560,17 @@ static void ibmvfc_set_host_action(struct ibmvfc_host *vhost, |
| if (vhost->action == IBMVFC_HOST_ACTION_ALLOC_TGTS) |
| vhost->action = action; |
| break; |
| + case IBMVFC_HOST_ACTION_REENABLE: |
| + case IBMVFC_HOST_ACTION_RESET: |
| + vhost->action = action; |
| + break; |
| case IBMVFC_HOST_ACTION_INIT: |
| case IBMVFC_HOST_ACTION_TGT_DEL: |
| + case IBMVFC_HOST_ACTION_LOGO: |
| + case IBMVFC_HOST_ACTION_QUERY_TGTS: |
| + case IBMVFC_HOST_ACTION_TGT_DEL_FAILED: |
| + case IBMVFC_HOST_ACTION_NONE: |
| + default: |
| switch (vhost->action) { |
| case IBMVFC_HOST_ACTION_RESET: |
| case IBMVFC_HOST_ACTION_REENABLE: |
| @@ -571,15 +580,6 @@ static void ibmvfc_set_host_action(struct ibmvfc_host *vhost, |
| break; |
| } |
| break; |
| - case IBMVFC_HOST_ACTION_LOGO: |
| - case IBMVFC_HOST_ACTION_QUERY_TGTS: |
| - case IBMVFC_HOST_ACTION_TGT_DEL_FAILED: |
| - case IBMVFC_HOST_ACTION_NONE: |
| - case IBMVFC_HOST_ACTION_RESET: |
| - case IBMVFC_HOST_ACTION_REENABLE: |
| - default: |
| - vhost->action = action; |
| - break; |
| } |
| } |
| |
| @@ -4723,26 +4723,45 @@ static void ibmvfc_do_work(struct ibmvfc_host *vhost) |
| case IBMVFC_HOST_ACTION_INIT_WAIT: |
| break; |
| case IBMVFC_HOST_ACTION_RESET: |
| - vhost->action = IBMVFC_HOST_ACTION_TGT_DEL; |
| spin_unlock_irqrestore(vhost->host->host_lock, flags); |
| rc = ibmvfc_reset_crq(vhost); |
| + |
| spin_lock_irqsave(vhost->host->host_lock, flags); |
| - if (rc == H_CLOSED) |
| + if (!rc || rc == H_CLOSED) |
| vio_enable_interrupts(to_vio_dev(vhost->dev)); |
| - if (rc || (rc = ibmvfc_send_crq_init(vhost)) || |
| - (rc = vio_enable_interrupts(to_vio_dev(vhost->dev)))) { |
| - ibmvfc_link_down(vhost, IBMVFC_LINK_DEAD); |
| - dev_err(vhost->dev, "Error after reset (rc=%d)\n", rc); |
| + if (vhost->action == IBMVFC_HOST_ACTION_RESET) { |
| + /* |
| + * The only action we could have changed to would have |
| + * been reenable, in which case, we skip the rest of |
| + * this path and wait until we've done the re-enable |
| + * before sending the crq init. |
| + */ |
| + vhost->action = IBMVFC_HOST_ACTION_TGT_DEL; |
| + |
| + if (rc || (rc = ibmvfc_send_crq_init(vhost)) || |
| + (rc = vio_enable_interrupts(to_vio_dev(vhost->dev)))) { |
| + ibmvfc_link_down(vhost, IBMVFC_LINK_DEAD); |
| + dev_err(vhost->dev, "Error after reset (rc=%d)\n", rc); |
| + } |
| } |
| break; |
| case IBMVFC_HOST_ACTION_REENABLE: |
| - vhost->action = IBMVFC_HOST_ACTION_TGT_DEL; |
| spin_unlock_irqrestore(vhost->host->host_lock, flags); |
| rc = ibmvfc_reenable_crq_queue(vhost); |
| + |
| spin_lock_irqsave(vhost->host->host_lock, flags); |
| - if (rc || (rc = ibmvfc_send_crq_init(vhost))) { |
| - ibmvfc_link_down(vhost, IBMVFC_LINK_DEAD); |
| - dev_err(vhost->dev, "Error after enable (rc=%d)\n", rc); |
| + if (vhost->action == IBMVFC_HOST_ACTION_REENABLE) { |
| + /* |
| + * The only action we could have changed to would have |
| + * been reset, in which case, we skip the rest of this |
| + * path and wait until we've done the reset before |
| + * sending the crq init. |
| + */ |
| + vhost->action = IBMVFC_HOST_ACTION_TGT_DEL; |
| + if (rc || (rc = ibmvfc_send_crq_init(vhost))) { |
| + ibmvfc_link_down(vhost, IBMVFC_LINK_DEAD); |
| + dev_err(vhost->dev, "Error after enable (rc=%d)\n", rc); |
| + } |
| } |
| break; |
| case IBMVFC_HOST_ACTION_LOGO: |
| -- |
| 2.30.2 |
| |