[PATCH v4 0/2] Ensure FCoE target interrupts work correctly

[PATCH v4 0/2] Ensure FCoE target interrupts work correctly

[Lee Duncan]

From: Lee Duncan <lduncan@suse.com>

Commit 1a1975551943 "scsi: fcoe: Fix potential deadlock on &fip->ctlr_lock"
changed locking for fnic/FCoE, but it did so by disabling interrupts
where they weren't disabled before, and this caused FCoE targets
to go offline. Reverting that patch fixed the issue.

But to handle the problem originally addressed by the commit,
instead of modifying the locking, move the work to be done
into a work queue.

Differences in v4:
  - Corrected "Fixes" attributes in both patches
  - Added identifier name in fnic_flush_tx() prototype, for checkpatch

Differences in v3:
  - Added "fixes" clause to the fnic patch, as requested by Hannes

Differences in V2:
  - Fix kerneldoc comments in fnic_flush_tx()

Lee Duncan (1):
  Revert "scsi: fcoe: Fix potential deadlock on &fip->ctlr_lock"

Hannes Reinecke (1):
  fnic: move fnic_fnic_flush_tx() to a work queue

 drivers/scsi/fcoe/fcoe_ctlr.c | 20 ++++++++------------
 drivers/scsi/fnic/fnic.h      |  3 ++-
 drivers/scsi/fnic/fnic_fcs.c  |  5 +++--
 drivers/scsi/fnic/fnic_main.c |  1 +
 drivers/scsi/fnic/fnic_scsi.c |  4 ++--
 5 files changed, 16 insertions(+), 17 deletions(-)

-- 
2.35.3

[PATCH v4 1/2] Revert "scsi: fcoe: Fix potential deadlock on &fip->ctlr_lock"

[Lee Duncan]

From: Lee Duncan <lduncan@suse.com>

This reverts commit 1a1975551943f681772720f639ff42fbaa746212

This commit causes interrupts to be lost for FCoE devices,
since it changed sping locks from "bh" to "irqsave".

Instead, a work queue should be used, and will be addressed
in a separate patch.

Fixes: 1a1975551943 ("scsi: fcoe: Fix potential deadlock on &fip->ctlr_lock")
Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 drivers/scsi/fcoe/fcoe_ctlr.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe_ctlr.c b/drivers/scsi/fcoe/fcoe_ctlr.c
index 19eee108db02..5c8d1ba3f8f3 100644
--- a/drivers/scsi/fcoe/fcoe_ctlr.c
+++ b/drivers/scsi/fcoe/fcoe_ctlr.c
@@ -319,17 +319,16 @@ static void fcoe_ctlr_announce(struct fcoe_ctlr *fip)
 {
 	struct fcoe_fcf *sel;
 	struct fcoe_fcf *fcf;
-	unsigned long flags;
 
 	mutex_lock(&fip->ctlr_mutex);
-	spin_lock_irqsave(&fip->ctlr_lock, flags);
+	spin_lock_bh(&fip->ctlr_lock);
 
 	kfree_skb(fip->flogi_req);
 	fip->flogi_req = NULL;
 	list_for_each_entry(fcf, &fip->fcfs, list)
 		fcf->flogi_sent = 0;
 
-	spin_unlock_irqrestore(&fip->ctlr_lock, flags);
+	spin_unlock_bh(&fip->ctlr_lock);
 	sel = fip->sel_fcf;
 
 	if (sel && ether_addr_equal(sel->fcf_mac, fip->dest_addr))
@@ -700,7 +699,6 @@ int fcoe_ctlr_els_send(struct fcoe_ctlr *fip, struct fc_lport *lport,
 {
 	struct fc_frame *fp;
 	struct fc_frame_header *fh;
-	unsigned long flags;
 	u16 old_xid;
 	u8 op;
 	u8 mac[ETH_ALEN];
@@ -734,11 +732,11 @@ int fcoe_ctlr_els_send(struct fcoe_ctlr *fip, struct fc_lport *lport,
 		op = FIP_DT_FLOGI;
 		if (fip->mode == FIP_MODE_VN2VN)
 			break;
-		spin_lock_irqsave(&fip->ctlr_lock, flags);
+		spin_lock_bh(&fip->ctlr_lock);
 		kfree_skb(fip->flogi_req);
 		fip->flogi_req = skb;
 		fip->flogi_req_send = 1;
-		spin_unlock_irqrestore(&fip->ctlr_lock, flags);
+		spin_unlock_bh(&fip->ctlr_lock);
 		schedule_work(&fip->timer_work);
 		return -EINPROGRESS;
 	case ELS_FDISC:
@@ -1707,11 +1705,10 @@ static int fcoe_ctlr_flogi_send_locked(struct fcoe_ctlr *fip)
 static int fcoe_ctlr_flogi_retry(struct fcoe_ctlr *fip)
 {
 	struct fcoe_fcf *fcf;
-	unsigned long flags;
 	int error;
 
 	mutex_lock(&fip->ctlr_mutex);
-	spin_lock_irqsave(&fip->ctlr_lock, flags);
+	spin_lock_bh(&fip->ctlr_lock);
 	LIBFCOE_FIP_DBG(fip, "re-sending FLOGI - reselect\n");
 	fcf = fcoe_ctlr_select(fip);
 	if (!fcf || fcf->flogi_sent) {
@@ -1722,7 +1719,7 @@ static int fcoe_ctlr_flogi_retry(struct fcoe_ctlr *fip)
 		fcoe_ctlr_solicit(fip, NULL);
 		error = fcoe_ctlr_flogi_send_locked(fip);
 	}
-	spin_unlock_irqrestore(&fip->ctlr_lock, flags);
+	spin_unlock_bh(&fip->ctlr_lock);
 	mutex_unlock(&fip->ctlr_mutex);
 	return error;
 }
@@ -1739,9 +1736,8 @@ static int fcoe_ctlr_flogi_retry(struct fcoe_ctlr *fip)
 static void fcoe_ctlr_flogi_send(struct fcoe_ctlr *fip)
 {
 	struct fcoe_fcf *fcf;
-	unsigned long flags;
 
-	spin_lock_irqsave(&fip->ctlr_lock, flags);
+	spin_lock_bh(&fip->ctlr_lock);
 	fcf = fip->sel_fcf;
 	if (!fcf || !fip->flogi_req_send)
 		goto unlock;
@@ -1768,7 +1764,7 @@ static void fcoe_ctlr_flogi_send(struct fcoe_ctlr *fip)
 	} else /* XXX */
 		LIBFCOE_FIP_DBG(fip, "No FCF selected - defer send\n");
 unlock:
-	spin_unlock_irqrestore(&fip->ctlr_lock, flags);
+	spin_unlock_bh(&fip->ctlr_lock);
 }
 
 /**
-- 
2.35.3

Re: [PATCH v4 1/2] Revert "scsi: fcoe: Fix potential deadlock on &fip->ctlr_lock"

[Hannes Reinecke]

On 2/10/24 02:07, Lee Duncan wrote:
> From: Lee Duncan <lduncan@suse.com>
> 
> This reverts commit 1a1975551943f681772720f639ff42fbaa746212
> 
> This commit causes interrupts to be lost for FCoE devices,
> since it changed sping locks from "bh" to "irqsave".
> 
> Instead, a work queue should be used, and will be addressed
> in a separate patch.
> 
> Fixes: 1a1975551943 ("scsi: fcoe: Fix potential deadlock on &fip->ctlr_lock")
> Signed-off-by: Lee Duncan <lduncan@suse.com>
> ---
>   drivers/scsi/fcoe/fcoe_ctlr.c | 20 ++++++++------------
>   1 file changed, 8 insertions(+), 12 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Ivo Totev, Andrew McDonald,
Werner Knoblich

[PATCH v4 2/2] fnic: move fnic_fnic_flush_tx() to a work queue

[Lee Duncan]

From: Lee Duncan <lduncan@suse.com>

Rather than call 'fnic_flush_tx()' from interrupt context we should
be moving it onto a work queue to avoid any locking issues.

Fixes: 1a1975551943 ("scsi: fcoe: Fix potential deadlock on &fip->ctlr_lock")
Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 drivers/scsi/fnic/fnic.h      | 3 ++-
 drivers/scsi/fnic/fnic_fcs.c  | 5 +++--
 drivers/scsi/fnic/fnic_main.c | 1 +
 drivers/scsi/fnic/fnic_scsi.c | 4 ++--
 4 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
index 2074937c05bc..3b8eb7dee500 100644
--- a/drivers/scsi/fnic/fnic.h
+++ b/drivers/scsi/fnic/fnic.h
@@ -305,6 +305,7 @@ struct fnic {
 	unsigned int copy_wq_base;
 	struct work_struct link_work;
 	struct work_struct frame_work;
+	struct work_struct flush_work;
 	struct sk_buff_head frame_queue;
 	struct sk_buff_head tx_queue;
 
@@ -363,7 +364,7 @@ void fnic_handle_event(struct work_struct *work);
 int fnic_rq_cmpl_handler(struct fnic *fnic, int);
 int fnic_alloc_rq_frame(struct vnic_rq *rq);
 void fnic_free_rq_buf(struct vnic_rq *rq, struct vnic_rq_buf *buf);
-void fnic_flush_tx(struct fnic *);
+void fnic_flush_tx(struct work_struct *work);
 void fnic_eth_send(struct fcoe_ctlr *, struct sk_buff *skb);
 void fnic_set_port_id(struct fc_lport *, u32, struct fc_frame *);
 void fnic_update_mac(struct fc_lport *, u8 *new);
diff --git a/drivers/scsi/fnic/fnic_fcs.c b/drivers/scsi/fnic/fnic_fcs.c
index 5e312a55cc7d..a08293b2ad9f 100644
--- a/drivers/scsi/fnic/fnic_fcs.c
+++ b/drivers/scsi/fnic/fnic_fcs.c
@@ -1182,7 +1182,7 @@ int fnic_send(struct fc_lport *lp, struct fc_frame *fp)
 
 /**
  * fnic_flush_tx() - send queued frames.
- * @fnic: fnic device
+ * @work: pointer to work element
  *
  * Send frames that were waiting to go out in FC or Ethernet mode.
  * Whenever changing modes we purge queued frames, so these frames should
@@ -1190,8 +1190,9 @@ int fnic_send(struct fc_lport *lp, struct fc_frame *fp)
  *
  * Called without fnic_lock held.
  */
-void fnic_flush_tx(struct fnic *fnic)
+void fnic_flush_tx(struct work_struct *work)
 {
+	struct fnic *fnic = container_of(work, struct fnic, flush_work);
 	struct sk_buff *skb;
 	struct fc_frame *fp;
 
diff --git a/drivers/scsi/fnic/fnic_main.c b/drivers/scsi/fnic/fnic_main.c
index 5ed1d897311a..29eead383eb9 100644
--- a/drivers/scsi/fnic/fnic_main.c
+++ b/drivers/scsi/fnic/fnic_main.c
@@ -830,6 +830,7 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		spin_lock_init(&fnic->vlans_lock);
 		INIT_WORK(&fnic->fip_frame_work, fnic_handle_fip_frame);
 		INIT_WORK(&fnic->event_work, fnic_handle_event);
+		INIT_WORK(&fnic->flush_work, fnic_flush_tx);
 		skb_queue_head_init(&fnic->fip_frame_queue);
 		INIT_LIST_HEAD(&fnic->evlist);
 		INIT_LIST_HEAD(&fnic->vlans);
diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
index 8d7fc5284293..fc4cee91b175 100644
--- a/drivers/scsi/fnic/fnic_scsi.c
+++ b/drivers/scsi/fnic/fnic_scsi.c
@@ -680,7 +680,7 @@ static int fnic_fcpio_fw_reset_cmpl_handler(struct fnic *fnic,
 
 	spin_unlock_irqrestore(&fnic->fnic_lock, flags);
 
-	fnic_flush_tx(fnic);
+	queue_work(fnic_event_queue, &fnic->flush_work);
 
  reset_cmpl_handler_end:
 	fnic_clear_state_flags(fnic, FNIC_FLAGS_FWRESET);
@@ -736,7 +736,7 @@ static int fnic_fcpio_flogi_reg_cmpl_handler(struct fnic *fnic,
 		}
 		spin_unlock_irqrestore(&fnic->fnic_lock, flags);
 
-		fnic_flush_tx(fnic);
+		queue_work(fnic_event_queue, &fnic->flush_work);
 		queue_work(fnic_event_queue, &fnic->frame_work);
 	} else {
 		spin_unlock_irqrestore(&fnic->fnic_lock, flags);
-- 
2.35.3

Re: [PATCH v4 0/2] Ensure FCoE target interrupts work correctly

[Martin K. Petersen]

On Fri, 09 Feb 2024 10:07:33 -0800, Lee Duncan wrote:

> Commit 1a1975551943 "scsi: fcoe: Fix potential deadlock on &fip->ctlr_lock"
> changed locking for fnic/FCoE, but it did so by disabling interrupts
> where they weren't disabled before, and this caused FCoE targets
> to go offline. Reverting that patch fixed the issue.
> 
> But to handle the problem originally addressed by the commit,
> instead of modifying the locking, move the work to be done
> into a work queue.
> 
> [...]

Applied to 6.8/scsi-fixes, thanks!

[1/2] Revert "scsi: fcoe: Fix potential deadlock on &fip->ctlr_lock"
      https://git.kernel.org/mkp/scsi/c/977fe773dcc7
[2/2] fnic: move fnic_fnic_flush_tx() to a work queue
      https://git.kernel.org/mkp/scsi/c/379a58caa199

-- 
Martin K. Petersen	Oracle Linux Engineering