Skip to contentSkip to conversation

fnic: fixup INTx and MSI interrupt modes

PATCH In review Hannes Reinecke 4 participants 18 messages Nov 4, 2025 – Jan 23

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover fnic: fixup INTx and MSI interrupt modes

Hi all,

with the rework to enable multiqueue the non-multiqueue interrupt modes (ie MSI and INTx) got broken and resulted in a non-functional drivers. This patchset fixes up the driver to work in all interrupt modes and adds a new module parameter 'fnic_intr_mode' to switch between them.

As usual, comments and reviews are welcome.

Hannes Reinecke (4):
fnic: use resulting interrupt mode during configuration
fnic: correctly initialize 'fnic->name'
fnic: missing initialisation for wq_copy_base
fnic: make interrupt mode configurable

 drivers/scsi/fnic/fnic.h      |  2 +-
 drivers/scsi/fnic/fnic_isr.c  | 15 +++++++++++----
 drivers/scsi/fnic/fnic_main.c | 17 ++++++++++++-----
 3 files changed, 24 insertions(+), 10 deletions(-)
Signature · 1 line
2.43.0
#1 lore raw

PATCH 1/4 fnic: use resulting interrupt mode during configuration

The configured interrupt mode might be different than the resulting interrupts mode after all configuration limitations have been applied.

---
 drivers/scsi/fnic/fnic_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/fnic/fnic_main.c +1 -1
index 4cc4077ea53c..7075a23d9229 100644
@@ -678,7 +678,7 @@ void fnic_mq_map_queues_cpus(struct Scsi_Host *host)
 {
 	struct fnic *fnic = *((struct fnic **) shost_priv(host));
 	struct pci_dev *l_pdev = fnic->pdev;
-	int intr_mode = fnic->config.intr_mode;
+	int intr_mode = vnic_dev_get_intr_mode(fnic->vdev);
 	struct blk_mq_queue_map *qmap = &host->tag_set.map[HCTX_TYPE_DEFAULT];
 
 	if (intr_mode == VNIC_DEV_INTR_MODE_MSI || intr_mode == VNIC_DEV_INTR_MODE_INTX) {
Signature · 1 line
2.43.0
#2 lore raw

RE: [PATCH 1/4] fnic: use resulting interrupt mode during configuration

Cisco Confidential

Quoted 21 lines · Hannes Reinecke

The configured interrupt mode might be different than the resulting interrupts mode after all configuration limitations have been applied.

---
drivers/scsi/fnic/fnic_main.c | 2 +--

1 file changed, 1 insertion(+-), 1 deletion(-)
drivers/scsi/fnic/fnic_main.c +1 -1
index 4cc4077ea53c..7075a23d9229 100644
--- a/drivers/scsi/fnic/fnic_main.c
+-+-+- b/drivers/scsi/fnic/fnic_main.c

@@ -678,7 +-678,7 @@ void fnic_mq_map_queues_cpus(struct Scsi_Host *host) {
struct fnic *fnic = *((struct fnic **) shost_priv(host)); struct pci_dev *l_pdev = fnic->pdev;
- int intr_mode = fnic->config.intr_mode;
+- int intr_mode = vnic_dev_get_intr_mode(fnic->vdev);
struct blk_mq_queue_map *qmap = &host->tag_set.map[HCTX_TYPE_DEFAULT];

Quoted 5 lines

if (intr_mode == VNIC_DEV_INTR_MODE_MSI || intr_mode == VNIC_DEV_INTR_MODE_INTX) { --
2.43.0

Thanks for this change, Hannes.
The fnic team will review this and get back to you.

Regards,
Karan

maintainer Reviewed-by on v1 1/4
#3 lore raw

RE: [PATCH 1/4] fnic: use resulting interrupt mode during configuration

In reply to Hannes Reinecke

Cisco Confidential

Quoted 21 lines · Hannes Reinecke

The configured interrupt mode might be different than the resulting interrupts mode after all configuration limitations have been applied.

---
drivers/scsi/fnic/fnic_main.c | 2 +--

1 file changed, 1 insertion(+-), 1 deletion(-)
drivers/scsi/fnic/fnic_main.c +1 -1
index 4cc4077ea53c..7075a23d9229 100644
--- a/drivers/scsi/fnic/fnic_main.c
+-+-+- b/drivers/scsi/fnic/fnic_main.c

@@ -678,7 +-678,7 @@ void fnic_mq_map_queues_cpus(struct Scsi_Host *host) {
struct fnic *fnic = *((struct fnic **) shost_priv(host)); struct pci_dev *l_pdev = fnic->pdev;
- int intr_mode = fnic->config.intr_mode;
+- int intr_mode = vnic_dev_get_intr_mode(fnic->vdev);
struct blk_mq_queue_map *qmap = &host->tag_set.map[HCTX_TYPE_DEFAULT];

Quoted 5 lines

if (intr_mode == VNIC_DEV_INTR_MODE_MSI || intr_mode == VNIC_DEV_INTR_MODE_INTX) { --
2.43.0

Thanks for this change, Hannes. It looks good.

Regards,
Karan

#4 lore raw

PATCH 2/4 fnic: correctly initialize 'fnic->name'

In reply to Hannes Reinecke

The instance name is required for setting up interrupt handlers, so set it early enough to avoid the request_irq() routine crashing on duplicate or empty names.

---
 drivers/scsi/fnic/fnic_main.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/scsi/fnic/fnic_main.c +2 -3
index 7075a23d9229..870b265be41a 100644
@@ -575,9 +575,6 @@ static void fnic_scsi_init(struct fnic *fnic)
 {
 	struct Scsi_Host *host = fnic->host;
 
-	snprintf(fnic->name, sizeof(fnic->name) - 1, "%s%d", DRV_NAME,
-			 host->host_no);
-
 	host->transportt = fnic_fc_transport;
 }
 
@@ -732,6 +729,8 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	fnic->pdev = pdev;
 	fnic->fnic_num = fnic_id;
+	snprintf(fnic->name, sizeof(fnic->name) - 1, "%s%d", DRV_NAME,
+		 fnic->fnic_num);
 
 	/* Find model name from PCIe subsys ID */
 	if (fnic_get_desc_by_devid(pdev, &desc, &subsys_desc) == 0) {
Signature · 1 line
2.43.0
#5 lore raw

RE: [PATCH 2/4] fnic: correctly initialize 'fnic->name'

Cisco Confidential

Quoted 28 lines · Hannes Reinecke

The instance name is required for setting up interrupt handlers, so set it early enough to avoid the request_irq() routine crashing on duplicate or empty names.

---
drivers/scsi/fnic/fnic_main.c | 5 +-+----

1 file changed, 2 insertions(+-), 3 deletions(-)
drivers/scsi/fnic/fnic_main.c +1 -1
index 7075a23d9229..870b265be41a 100644
--- a/drivers/scsi/fnic/fnic_main.c
+-+-+- b/drivers/scsi/fnic/fnic_main.c

@@ -575,9 +-575,6 @@ static void fnic_scsi_init(struct fnic *fnic) {
struct Scsi_Host *host = fnic->host;

drivers/scsi/fnic/fnic_main.c +0 -3
-     snprintf(fnic->name, sizeof(fnic->name) - 1, "%s%d", DRV_NAME,
-                      host->host_no);
-

host->transportt = fnic_fc_transport;
}

@@ -732,6 +-729,8 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)

fnic->pdev = pdev;
fnic->fnic_num = fnic_id;

Quoted 9 lines
+-     snprintf(fnic->name, sizeof(fnic->name) - 1, "%s%d", DRV_NAME,
+-              fnic->fnic_num);

/* Find model name from PCIe subsys ID */
if (fnic_get_desc_by_devid(pdev, &desc, &subsys_desc) == 0) { --
2.43.0

Thanks for this change, Hannes.
The fnic team will review this and get back to you.

Regards,
Karan

maintainer Reviewed-by on v1 2/4
#6 lore raw

RE: [PATCH 2/4] fnic: correctly initialize 'fnic->name'

In reply to Hannes Reinecke

Cisco Confidential

Quoted 28 lines · Hannes Reinecke

The instance name is required for setting up interrupt handlers, so set it early enough to avoid the request_irq() routine crashing on duplicate or empty names.

---
drivers/scsi/fnic/fnic_main.c | 5 +-+----

1 file changed, 2 insertions(+-), 3 deletions(-)
drivers/scsi/fnic/fnic_main.c +1 -1
index 7075a23d9229..870b265be41a 100644
--- a/drivers/scsi/fnic/fnic_main.c
+-+-+- b/drivers/scsi/fnic/fnic_main.c

@@ -575,9 +-575,6 @@ static void fnic_scsi_init(struct fnic *fnic) {
struct Scsi_Host *host = fnic->host;

drivers/scsi/fnic/fnic_main.c +0 -3
-     snprintf(fnic->name, sizeof(fnic->name) - 1, "%s%d", DRV_NAME,
-                      host->host_no);
-

host->transportt = fnic_fc_transport;
}

@@ -732,6 +-729,8 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)

fnic->pdev = pdev;
fnic->fnic_num = fnic_id;

Quoted 9 lines
+-     snprintf(fnic->name, sizeof(fnic->name) - 1, "%s%d", DRV_NAME,
+-              fnic->fnic_num);

/* Find model name from PCIe subsys ID */
if (fnic_get_desc_by_devid(pdev, &desc, &subsys_desc) == 0) { --
2.43.0

Thanks for this change, Hannes. It looks good.

Regards,
Karan

#7 lore raw

PATCH 3/4 fnic: missing initialisation for wq_copy_base

In reply to Hannes Reinecke

With the conversion to multiqueue the 'wq_copy_base' value were left uninitialized for MSI and INTx interrupts, causing the driver to issue a message 'FCPIO_SUCCESS icmnd completion on the wrong queue' and finally running out of command tags as the completions would be accounted on the wrong queue.

Fixes 8a8449ca5e33 ("scsi: fnic: Modify ISRs to support multiqueue (MQ)")
---
 drivers/scsi/fnic/fnic_isr.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/scsi/fnic/fnic_isr.c +2 -0
index 7ed50b11afa6..e16b76d537e8 100644
@@ -350,6 +350,7 @@ int fnic_set_intr_mode(struct fnic *fnic)
 		fnic->cq_count = 3;
 		fnic->intr_count = 1;
 		fnic->err_intr_offset = 0;
+		fnic->copy_wq_base = fnic->rq_count + fnic->raw_wq_count;
 
 		FNIC_ISR_DBG(KERN_DEBUG, fnic->host, fnic->fnic_num,
 			     "Using MSI Interrupts\n");
@@ -376,6 +377,7 @@ int fnic_set_intr_mode(struct fnic *fnic)
 		fnic->wq_copy_count = 1;
 		fnic->cq_count = 3;
 		fnic->intr_count = 3;
+		fnic->copy_wq_base = fnic->rq_count + fnic->raw_wq_count;
 
 		FNIC_ISR_DBG(KERN_DEBUG, fnic->host, fnic->fnic_num,
 			     "Using Legacy Interrupts\n");
Signature · 1 line
2.43.0
#8 lore raw

RE: [PATCH 3/4] fnic: missing initialisation for wq_copy_base

Cisco Confidential

Quoted 32 lines · Hannes Reinecke

With the conversion to multiqueue the 'wq_copy_base' value were left uninitialized for MSI and INTx interrupts, causing the driver to issue a message 'FCPIO_SUCCESS icmnd completion on the wrong queue' and finally running out of command tags as the completions would be accounted on the wrong queue.

Fixes 8a8449ca5e33 ("scsi: fnic: Modify ISRs to support multiqueue (MQ)")

---
drivers/scsi/fnic/fnic_isr.c | 2 +-+-

1 file changed, 2 insertions(+-)
drivers/scsi/fnic/fnic_isr.c +1 -1
index 7ed50b11afa6..e16b76d537e8 100644
--- a/drivers/scsi/fnic/fnic_isr.c
+-+-+- b/drivers/scsi/fnic/fnic_isr.c

@@ -350,6 +-350,7 @@ int fnic_set_intr_mode(struct fnic *fnic) fnic->cq_count = 3;
fnic->intr_count = 1;
fnic->err_intr_offset = 0;
+- fnic->copy_wq_base = fnic->rq_count +- fnic->raw_wq_count;

FNIC_ISR_DBG(KERN_DEBUG, fnic->host, fnic->fnic_num, "Using MSI Interrupts\n");
@@ -376,6 +-377,7 @@ int fnic_set_intr_mode(struct fnic *fnic) fnic->wq_copy_count = 1;
fnic->cq_count = 3;
fnic->intr_count = 3;
+- fnic->copy_wq_base = fnic->rq_count +- fnic->raw_wq_count;

Quoted 6 lines

FNIC_ISR_DBG(KERN_DEBUG, fnic->host, fnic->fnic_num, "Using Legacy Interrupts\n");
--
2.43.0

Thanks for this change, Hannes.
The fnic team will review this and get back to you.

Regards,
Karan

#9 lore raw

RE: [PATCH 3/4] fnic: missing initialisation for wq_copy_base

In reply to Hannes Reinecke

Cisco Confidential

Quoted 32 lines · Hannes Reinecke

With the conversion to multiqueue the 'wq_copy_base' value were left uninitialized for MSI and INTx interrupts, causing the driver to issue a message 'FCPIO_SUCCESS icmnd completion on the wrong queue' and finally running out of command tags as the completions would be accounted on the wrong queue.

Fixes 8a8449ca5e33 ("scsi: fnic: Modify ISRs to support multiqueue (MQ)")

---
drivers/scsi/fnic/fnic_isr.c | 2 +-+-

1 file changed, 2 insertions(+-)
drivers/scsi/fnic/fnic_isr.c +1 -1
index 7ed50b11afa6..e16b76d537e8 100644
--- a/drivers/scsi/fnic/fnic_isr.c
+-+-+- b/drivers/scsi/fnic/fnic_isr.c

@@ -350,6 +-350,7 @@ int fnic_set_intr_mode(struct fnic *fnic) fnic->cq_count = 3;
fnic->intr_count = 1;
fnic->err_intr_offset = 0;
+- fnic->copy_wq_base = fnic->rq_count +- fnic->raw_wq_count;

FNIC_ISR_DBG(KERN_DEBUG, fnic->host, fnic->fnic_num, "Using MSI Interrupts\n");
@@ -376,6 +-377,7 @@ int fnic_set_intr_mode(struct fnic *fnic) fnic->wq_copy_count = 1;
fnic->cq_count = 3;
fnic->intr_count = 3;
+- fnic->copy_wq_base = fnic->rq_count +- fnic->raw_wq_count;

Quoted 6 lines

FNIC_ISR_DBG(KERN_DEBUG, fnic->host, fnic->fnic_num, "Using Legacy Interrupts\n");
--
2.43.0

Thanks for these changes, Hannes.

I will need to test these changes and get back to you.

Regards,
Karan

#10 lore raw

PATCH 4/4 fnic: make interrupt mode configurable

In reply to Hannes Reinecke

In some environments (eg kdump) not all CPUs are online, so the MQ mapping might be resulting in an invalid layout. So make the interrupt mode settable via an 'fnic_intr_mode' module parameter and switch to INTx if the 'reset_devices' kernel parameter is specified.

---
 drivers/scsi/fnic/fnic.h      |  2 +-
 drivers/scsi/fnic/fnic_isr.c  | 13 +++++++++----
 drivers/scsi/fnic/fnic_main.c | 10 +++++++++-
 3 files changed, 19 insertions(+), 6 deletions(-)
drivers/scsi/fnic/fnic.h +1 -1
index 1199d701c3f5..c679283955e9 100644
@@ -484,7 +484,7 @@ extern struct workqueue_struct *fnic_fip_queue;
 extern const struct attribute_group *fnic_host_groups[];
 
 void fnic_clear_intr_mode(struct fnic *fnic);
-int fnic_set_intr_mode(struct fnic *fnic);
+int fnic_set_intr_mode(struct fnic *fnic, unsigned int mode);
 int fnic_set_intr_mode_msix(struct fnic *fnic);
 void fnic_free_intr(struct fnic *fnic);
 int fnic_request_intr(struct fnic *fnic);
drivers/scsi/fnic/fnic_isr.c +9 -4
index e16b76d537e8..b6594ad064ca 100644
@@ -319,20 +319,25 @@ int fnic_set_intr_mode_msix(struct fnic *fnic)
 	return 1;
 }
 
-int fnic_set_intr_mode(struct fnic *fnic)
+int fnic_set_intr_mode(struct fnic *fnic, unsigned int intr_mode)
 {
 	int ret_status = 0;
 
 	/*
 	 * Set interrupt mode (INTx, MSI, MSI-X) depending
 	 * system capabilities.
-	 *
+	 */
+	if (intr_mode != VNIC_DEV_INTR_MODE_MSIX)
+		goto try_msi;
+	/*
 	 * Try MSI-X first
 	 */
 	ret_status = fnic_set_intr_mode_msix(fnic);
 	if (ret_status == 0)
 		return ret_status;
-
+try_msi:
+	if (intr_mode != VNIC_DEV_INTR_MODE_MSI)
+		goto try_intx;
 	/*
 	 * Next try MSI
 	 * We need 1 RQ, 1 WQ, 1 WQ_COPY, 3 CQs, and 1 INTR
@@ -358,7 +363,7 @@ int fnic_set_intr_mode(struct fnic *fnic)
 
 		return 0;
 	}
-
+try_intx:
 	/*
 	 * Next try INTx
 	 * We need 1 RQ, 1 WQ, 1 WQ_COPY, 3 CQs, and 3 INTRs
drivers/scsi/fnic/fnic_main.c +9 -1
index 870b265be41a..4bdd55958f59 100644
@@ -97,6 +97,10 @@ module_param(pc_rscn_handling_feature_flag, uint, 0644);
 MODULE_PARM_DESC(pc_rscn_handling_feature_flag,
 		 "PCRSCN handling (0 for none. 1 to handle PCRSCN (default))");
 
+static unsigned int fnic_intr_mode = VNIC_DEV_INTR_MODE_MSIX;
+module_param(fnic_intr_mode, uint, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(fnic_intr_mode, "Interrupt mode, 1 = INTx, 2 = MSI, 3 = MSIx (default: 3)");
+
 struct workqueue_struct *reset_fnic_work_queue;
 struct workqueue_struct *fnic_fip_queue;
 
@@ -869,7 +873,11 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	fnic_get_res_counts(fnic);
 
-	err = fnic_set_intr_mode(fnic);
+	/* Override interrupt selection during kdump */
+	if (reset_devices)
+		fnic_intr_mode = VNIC_DEV_INTR_MODE_INTX;
+
+	err = fnic_set_intr_mode(fnic, fnic_intr_mode);
 	if (err) {
 		dev_err(&fnic->pdev->dev, "Failed to set intr mode, "
 			     "aborting.\n");
Signature · 1 line
2.43.0
#11 lore raw

RE: [PATCH 4/4] fnic: make interrupt mode configurable

Cisco Confidential

Quoted 93 lines · Hannes Reinecke

In some environments (eg kdump) not all CPUs are online, so the MQ mapping might be resulting in an invalid layout. So make the interrupt mode settable via an 'fnic_intr_mode' module parameter and switch to INTx if the 'reset_devices' kernel parameter is specified.

---

drivers/scsi/fnic/fnic.h      |  2 +--
drivers/scsi/fnic/fnic_isr.c  | 13 +-+-+-+-+-+-+-+-+-----
drivers/scsi/fnic/fnic_main.c | 10 +-+-+-+-+-+-+-+-+--
3 files changed, 19 insertions(+-), 6 deletions(-)
drivers/scsi/fnic/fnic.h +1 -1
index 1199d701c3f5..c679283955e9 100644
--- a/drivers/scsi/fnic/fnic.h
+-+-+- b/drivers/scsi/fnic/fnic.h

@@ -484,7 +-484,7 @@ extern struct workqueue_struct *fnic_fip_queue; extern const struct attribute_group *fnic_host_groups[];

void fnic_clear_intr_mode(struct fnic *fnic);
-int fnic_set_intr_mode(struct fnic *fnic);
+-int fnic_set_intr_mode(struct fnic *fnic, unsigned int mode); int fnic_set_intr_mode_msix(struct fnic *fnic);
void fnic_free_intr(struct fnic *fnic);
int fnic_request_intr(struct fnic *fnic);

drivers/scsi/fnic/fnic_isr.c +1 -1
index e16b76d537e8..b6594ad064ca 100644
--- a/drivers/scsi/fnic/fnic_isr.c
+-+-+- b/drivers/scsi/fnic/fnic_isr.c

@@ -319,20 +-319,25 @@ int fnic_set_intr_mode_msix(struct fnic *fnic) return 1;
}

-int fnic_set_intr_mode(struct fnic *fnic)
+-int fnic_set_intr_mode(struct fnic *fnic, unsigned int intr_mode) {
int ret_status = 0;

/*
* Set interrupt mode (INTx, MSI, MSI-X) depending
* system capabilities.
- *

+-      */
+-     if (intr_mode != VNIC_DEV_INTR_MODE_MSIX)
+-             goto try_msi;
+-     /*

* Try MSI-X first
*/
ret_status = fnic_set_intr_mode_msix(fnic);
if (ret_status == 0)
return ret_status;
-

+-try_msi:
+-     if (intr_mode != VNIC_DEV_INTR_MODE_MSI)
+-             goto try_intx;

/*
* Next try MSI
* We need 1 RQ, 1 WQ, 1 WQ_COPY, 3 CQs, and 1 INTR @@ -358,7 +-363,7 @@ int fnic_set_intr_mode(struct fnic *fnic)

return 0;
}
-
+-try_intx:
/*
* Next try INTx
* We need 1 RQ, 1 WQ, 1 WQ_COPY, 3 CQs, and 3 INTRs

drivers/scsi/fnic/fnic_main.c +1 -1
index 870b265be41a..4bdd55958f59 100644
--- a/drivers/scsi/fnic/fnic_main.c
+-+-+- b/drivers/scsi/fnic/fnic_main.c

@@ -97,6 +-97,10 @@ module_param(pc_rscn_handling_feature_flag, uint, 0644); MODULE_PARM_DESC(pc_rscn_handling_feature_flag,
"PCRSCN handling (0 for none. 1 to handle PCRSCN (default))");

+-static unsigned int fnic_intr_mode = VNIC_DEV_INTR_MODE_MSIX;
+-module_param(fnic_intr_mode, uint, S_IRUGO | S_IWUSR);
+-MODULE_PARM_DESC(fnic_intr_mode, "Interrupt mode, 1 = INTx, 2 = MSI, 3 = MSIx (default: 3)");
+-

struct workqueue_struct *reset_fnic_work_queue;
struct workqueue_struct *fnic_fip_queue;

@@ -869,7 +-873,11 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)

fnic_get_res_counts(fnic);

- err = fnic_set_intr_mode(fnic);

+-     /* Override interrupt selection during kdump */
+-     if (reset_devices)
+-             fnic_intr_mode = VNIC_DEV_INTR_MODE_INTX;
+-
+-     err = fnic_set_intr_mode(fnic, fnic_intr_mode);
Quoted 7 lines

if (err) {
dev_err(&fnic->pdev->dev, "Failed to set intr mode, " "aborting.\n");
--
2.43.0

Thanks for this change, Hannes.
The fnic team will review this and get back to you.

Regards,
Karan

#12 lore raw

RE: [PATCH 4/4] fnic: make interrupt mode configurable

In reply to Hannes Reinecke

Cisco Confidential

Quoted 76 lines · Hannes Reinecke

In some environments (eg kdump) not all CPUs are online, so the MQ mapping might be resulting in an invalid layout. So make the interrupt mode settable via an 'fnic_intr_mode' module parameter and switch to INTx if the 'reset_devices' kernel parameter is specified.

---

drivers/scsi/fnic/fnic.h      |  2 +--
drivers/scsi/fnic/fnic_isr.c  | 13 +-+-+-+-+-+-+-+-+-----
drivers/scsi/fnic/fnic_main.c | 10 +-+-+-+-+-+-+-+-+--
3 files changed, 19 insertions(+-), 6 deletions(-)
drivers/scsi/fnic/fnic.h +1 -1
index 1199d701c3f5..c679283955e9 100644
--- a/drivers/scsi/fnic/fnic.h
+-+-+- b/drivers/scsi/fnic/fnic.h

@@ -484,7 +-484,7 @@ extern struct workqueue_struct *fnic_fip_queue; extern const struct attribute_group *fnic_host_groups[];

void fnic_clear_intr_mode(struct fnic *fnic);
-int fnic_set_intr_mode(struct fnic *fnic);
+-int fnic_set_intr_mode(struct fnic *fnic, unsigned int mode); int fnic_set_intr_mode_msix(struct fnic *fnic);
void fnic_free_intr(struct fnic *fnic);
int fnic_request_intr(struct fnic *fnic);

drivers/scsi/fnic/fnic_isr.c +1 -1
index e16b76d537e8..b6594ad064ca 100644
--- a/drivers/scsi/fnic/fnic_isr.c
+-+-+- b/drivers/scsi/fnic/fnic_isr.c

@@ -319,20 +-319,25 @@ int fnic_set_intr_mode_msix(struct fnic *fnic) return 1;
}

-int fnic_set_intr_mode(struct fnic *fnic)
+-int fnic_set_intr_mode(struct fnic *fnic, unsigned int intr_mode) {
int ret_status = 0;

/*
* Set interrupt mode (INTx, MSI, MSI-X) depending
* system capabilities.
- *

+-      */
+-     if (intr_mode != VNIC_DEV_INTR_MODE_MSIX)
+-             goto try_msi;
+-     /*

* Try MSI-X first
*/
ret_status = fnic_set_intr_mode_msix(fnic);
if (ret_status == 0)
return ret_status;
-

+-try_msi:
+-     if (intr_mode != VNIC_DEV_INTR_MODE_MSI)
+-             goto try_intx;

/*
* Next try MSI
* We need 1 RQ, 1 WQ, 1 WQ_COPY, 3 CQs, and 1 INTR @@ -358,7 +-363,7 @@ int fnic_set_intr_mode(struct fnic *fnic)

return 0;
}
-
+-try_intx:
/*
* Next try INTx
* We need 1 RQ, 1 WQ, 1 WQ_COPY, 3 CQs, and 3 INTRs

drivers/scsi/fnic/fnic_main.c +1 -1
index 870b265be41a..4bdd55958f59 100644
--- a/drivers/scsi/fnic/fnic_main.c
+-+-+- b/drivers/scsi/fnic/fnic_main.c

@@ -97,6 +-97,10 @@ module_param(pc_rscn_handling_feature_flag, uint, 0644); MODULE_PARM_DESC(pc_rscn_handling_feature_flag,
"PCRSCN handling (0 for none. 1 to handle PCRSCN (default))");

Quoted 3 lines
+-static unsigned int fnic_intr_mode = VNIC_DEV_INTR_MODE_MSIX;
+-module_param(fnic_intr_mode, uint, S_IRUGO | S_IWUSR);
+-MODULE_PARM_DESC(fnic_intr_mode, "Interrupt mode, 1 = INTx, 2 = MSI, 3 = MSIx (default: 3)");

Based on fnic team's review: there is a way to choose the interrupt mode using the UCS management platform. We do not want to expose this as a module parameter.

Quoted 13 lines

struct workqueue_struct *reset_fnic_work_queue;
struct workqueue_struct *fnic_fip_queue;

@@ -869,7 +-873,11 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)

fnic_get_res_counts(fnic);

- err = fnic_set_intr_mode(fnic);

+-     /* Override interrupt selection during kdump */
+-     if (reset_devices)
+-             fnic_intr_mode = VNIC_DEV_INTR_MODE_INTX;
+-
+-     err = fnic_set_intr_mode(fnic, fnic_intr_mode);
Quoted 7 lines

if (err) {
dev_err(&fnic->pdev->dev, "Failed to set intr mode, " "aborting.\n");
--
2.43.0

Thanks for these changes, Hannes.
For the other changes in this patch, I will need to test and get back to you.

Regards,
Karan

#13 lore raw

Re: [PATCH 4/4] fnic: make interrupt mode configurable

Quoted 79 lines · Karan Tilak Kumar (kartilak)

Cisco Confidential

Quoted 76 lines · Hannes Reinecke

In some environments (eg kdump) not all CPUs are online, so the MQ mapping might be resulting in an invalid layout. So make the interrupt mode settable via an 'fnic_intr_mode' module parameter and switch to INTx if the 'reset_devices' kernel parameter is specified.

---

drivers/scsi/fnic/fnic.h      |  2 +-
drivers/scsi/fnic/fnic_isr.c  | 13 +++++++++----
drivers/scsi/fnic/fnic_main.c | 10 +++++++++-
3 files changed, 19 insertions(+), 6 deletions(-)
drivers/scsi/fnic/fnic.h +1 -1
index 1199d701c3f5..c679283955e9 100644
@@ -484,7 +484,7 @@ extern struct workqueue_struct *fnic_fip_queue;
 extern const struct attribute_group *fnic_host_groups[];
 
 void fnic_clear_intr_mode(struct fnic *fnic);
-int fnic_set_intr_mode(struct fnic *fnic);
+int fnic_set_intr_mode(struct fnic *fnic, unsigned int mode);
 int fnic_set_intr_mode_msix(struct fnic *fnic);
 void fnic_free_intr(struct fnic *fnic);
 int fnic_request_intr(struct fnic *fnic);
drivers/scsi/fnic/fnic_isr.c +9 -4
index e16b76d537e8..b6594ad064ca 100644
@@ -319,20 +319,25 @@ int fnic_set_intr_mode_msix(struct fnic *fnic)
 return 1;
 }
 
-int fnic_set_intr_mode(struct fnic *fnic)
+int fnic_set_intr_mode(struct fnic *fnic, unsigned int intr_mode)
 {
 int ret_status = 0;
 
 /*
 * Set interrupt mode (INTx, MSI, MSI-X) depending
 * system capabilities.
-      *
+      */
+     if (intr_mode != VNIC_DEV_INTR_MODE_MSIX)
+             goto try_msi;
+     /*
 * Try MSI-X first
 */
 ret_status = fnic_set_intr_mode_msix(fnic);
 if (ret_status == 0)
 return ret_status;
-
+try_msi:
+     if (intr_mode != VNIC_DEV_INTR_MODE_MSI)
+             goto try_intx;
 /*
 * Next try MSI
 * We need 1 RQ, 1 WQ, 1 WQ_COPY, 3 CQs, and 1 INTR
@@ -358,7 +363,7 @@ int fnic_set_intr_mode(struct fnic *fnic)
 
 return 0;
 }
-
+try_intx:
 /*
 * Next try INTx
 * We need 1 RQ, 1 WQ, 1 WQ_COPY, 3 CQs, and 3 INTRs
drivers/scsi/fnic/fnic_main.c +0 -0
index 870b265be41a..4bdd55958f59 100644
@@ -97,6 +97,10 @@ module_param(pc_rscn_handling_feature_flag, uint, 0644);
 MODULE_PARM_DESC(pc_rscn_handling_feature_flag,
 "PCRSCN handling (0 for none. 1 to handle PCRSCN (default))");
 
Quoted 7 lines

drivers/scsi/fnic/fnic_main.c module_param(pc_rscn_handling_feature_flag, uint, 0644); · line 102

Quoted 3 lines
drivers/scsi/fnic/fnic_main.c +3 -0
+static unsigned int fnic_intr_mode = VNIC_DEV_INTR_MODE_MSIX;
+module_param(fnic_intr_mode, uint, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(fnic_intr_mode, "Interrupt mode, 1 = INTx, 2 = MSI, 3 = MSIx (default: 3)");

Based on fnic team's review: there is a way to choose the interrupt mode using the UCS management platform. We do not want to expose this as a module parameter.

Yeah, I know. It was primarily used during testing to easily change between the various modes. I can drop it for the next round.

Quoted 7 lines
Quoted 7 lines
drivers/scsi/fnic/fnic_main.c +0 -0
 struct workqueue_struct *reset_fnic_work_queue;
 struct workqueue_struct *fnic_fip_queue;
 
@@ -869,7 +873,11 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 fnic_get_res_counts(fnic);
 
Quoted 17 lines
Quoted 7 lines
drivers/scsi/fnic/fnic_main.c +5 -1
-     err = fnic_set_intr_mode(fnic);
+     /* Override interrupt selection during kdump */
+     if (reset_devices)
+             fnic_intr_mode = VNIC_DEV_INTR_MODE_INTX;
+
+     err = fnic_set_intr_mode(fnic, fnic_intr_mode);
 if (err) {
Quoted 6 lines

dev_err(&fnic->pdev->dev, "Failed to set intr mode, " "aborting.\n");
--
2.43.0

Thanks for these changes, Hannes.
For the other changes in this patch, I will need to test and get back to you.

Thanks.
The 'reset_devices' thing is primarily there for kdump; might be an idea to make is explicit by using 'is_kdump_kernel()' directly.

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
#14 lore raw

RE: [PATCH 4/4] fnic: make interrupt mode configurable

Cisco Confidential

Quoted 88 lines · Hannes Reinecke
Quoted 79 lines · Karan Tilak Kumar (kartilak)

Cisco Confidential

Quoted 76 lines · Hannes Reinecke

In some environments (eg kdump) not all CPUs are online, so the MQ mapping might be resulting in an invalid layout. So make the interrupt mode settable via an 'fnic_intr_mode' module parameter and switch to INTx if the 'reset_devices' kernel parameter is specified.

---

drivers/scsi/fnic/fnic.h      |  2 +--
drivers/scsi/fnic/fnic_isr.c  | 13 +-+-+-+-+-+-+-+-+-----
drivers/scsi/fnic/fnic_main.c | 10 +-+-+-+-+-+-+-+-+--
3 files changed, 19 insertions(+-), 6 deletions(-)
drivers/scsi/fnic/fnic.h +1 -1
index 1199d701c3f5..c679283955e9 100644
--- a/drivers/scsi/fnic/fnic.h
+-+-+- b/drivers/scsi/fnic/fnic.h

@@ -484,7 +-484,7 @@ extern struct workqueue_struct *fnic_fip_queue; extern const struct attribute_group *fnic_host_groups[];

void fnic_clear_intr_mode(struct fnic *fnic);
-int fnic_set_intr_mode(struct fnic *fnic);
+-int fnic_set_intr_mode(struct fnic *fnic, unsigned int mode); int fnic_set_intr_mode_msix(struct fnic *fnic);
void fnic_free_intr(struct fnic *fnic);
int fnic_request_intr(struct fnic *fnic);

drivers/scsi/fnic/fnic_isr.c +1 -1
index e16b76d537e8..b6594ad064ca 100644
--- a/drivers/scsi/fnic/fnic_isr.c
+-+-+- b/drivers/scsi/fnic/fnic_isr.c

@@ -319,20 +-319,25 @@ int fnic_set_intr_mode_msix(struct fnic *fnic) return 1;
}

-int fnic_set_intr_mode(struct fnic *fnic)
+-int fnic_set_intr_mode(struct fnic *fnic, unsigned int intr_mode) {
int ret_status = 0;

/*
* Set interrupt mode (INTx, MSI, MSI-X) depending
* system capabilities.
- *

+-      */
+-     if (intr_mode != VNIC_DEV_INTR_MODE_MSIX)
+-             goto try_msi;
+-     /*

* Try MSI-X first
*/
ret_status = fnic_set_intr_mode_msix(fnic);
if (ret_status == 0)
return ret_status;
-

+-try_msi:
+-     if (intr_mode != VNIC_DEV_INTR_MODE_MSI)
+-             goto try_intx;

/*
* Next try MSI
* We need 1 RQ, 1 WQ, 1 WQ_COPY, 3 CQs, and 1 INTR @@ -358,7 +-363,7 @@ int fnic_set_intr_mode(struct fnic *fnic)

return 0;
}
-
+-try_intx:
/*
* Next try INTx
* We need 1 RQ, 1 WQ, 1 WQ_COPY, 3 CQs, and 3 INTRs

drivers/scsi/fnic/fnic_main.c +1 -1
index 870b265be41a..4bdd55958f59 100644
--- a/drivers/scsi/fnic/fnic_main.c
+-+-+- b/drivers/scsi/fnic/fnic_main.c

@@ -97,6 +-97,10 @@ module_param(pc_rscn_handling_feature_flag, uint, 0644); MODULE_PARM_DESC(pc_rscn_handling_feature_flag,
"PCRSCN handling (0 for none. 1 to handle PCRSCN (default))");

Quoted 7 lines
Quoted 3 lines
+-static unsigned int fnic_intr_mode = VNIC_DEV_INTR_MODE_MSIX;
+-module_param(fnic_intr_mode, uint, S_IRUGO | S_IWUSR);
+-MODULE_PARM_DESC(fnic_intr_mode, "Interrupt mode, 1 = INTx, 2 = MSI, 3 = MSIx (default: 3)");

Based on fnic team's review: there is a way to choose the interrupt mode using the UCS management platform. We do not want to expose this as a module parameter.

Quoted 3 lines

Yeah, I know. It was primarily used during testing to easily change between the various modes.
I can drop it for the next round.

Thanks Hannes. Sounds good.

Quoted 25 lines
Quoted 7 lines
Quoted 7 lines

struct workqueue_struct *reset_fnic_work_queue;
struct workqueue_struct *fnic_fip_queue;

@@ -869,7 +-873,11 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)

fnic_get_res_counts(fnic);

Quoted 17 lines
Quoted 6 lines

- err = fnic_set_intr_mode(fnic);

+-     /* Override interrupt selection during kdump */
+-     if (reset_devices)
+-             fnic_intr_mode = VNIC_DEV_INTR_MODE_INTX;
+-
+-     err = fnic_set_intr_mode(fnic, fnic_intr_mode);
Quoted 7 lines

if (err) {
dev_err(&fnic->pdev->dev, "Failed to set intr mode, " "aborting.\n");
--
2.43.0

Thanks for these changes, Hannes.
For the other changes in this patch, I will need to test and get back to you.

Quoted 3 lines

Thanks.
The 'reset_devices' thing is primarily there for kdump; might be an idea to make is explicit by using 'is_kdump_kernel()' directly.

Yes Hannes. That would be better. Thanks.

Quoted 10 lines

Cheers,

Hannes

--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +-49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

I'm bringing up a setup to test. I'll wait for your next revision.

My plan is to induce a kdump to test out these changes. If you have any other tests in mind, please let me know.

Thanks,
Karan

#15 lore raw

RE: [PATCH 4/4] fnic: make interrupt mode configurable

Cisco Confidential

Quoted 138 lines · Karan Tilak Kumar (kartilak)
Quoted 88 lines · Hannes Reinecke
Quoted 79 lines · Karan Tilak Kumar (kartilak)

Cisco Confidential

Quoted 76 lines · Hannes Reinecke

In some environments (eg kdump) not all CPUs are online, so the MQ mapping might be resulting in an invalid layout. So make the interrupt mode settable via an 'fnic_intr_mode' module parameter and switch to INTx if the 'reset_devices' kernel parameter is specified.

---

drivers/scsi/fnic/fnic.h      |  2 +--
drivers/scsi/fnic/fnic_isr.c  | 13 +-+-+-+-+-+-+-+-+-----
drivers/scsi/fnic/fnic_main.c | 10 +-+-+-+-+-+-+-+-+--
3 files changed, 19 insertions(+-), 6 deletions(-)
drivers/scsi/fnic/fnic.h +1 -1
index 1199d701c3f5..c679283955e9 100644
--- a/drivers/scsi/fnic/fnic.h
+-+-+- b/drivers/scsi/fnic/fnic.h

@@ -484,7 +-484,7 @@ extern struct workqueue_struct *fnic_fip_queue; extern const struct attribute_group *fnic_host_groups[];

void fnic_clear_intr_mode(struct fnic *fnic);
-int fnic_set_intr_mode(struct fnic *fnic);
+-int fnic_set_intr_mode(struct fnic *fnic, unsigned int mode); int fnic_set_intr_mode_msix(struct fnic *fnic);
void fnic_free_intr(struct fnic *fnic);
int fnic_request_intr(struct fnic *fnic);

drivers/scsi/fnic/fnic_isr.c +1 -1
index e16b76d537e8..b6594ad064ca 100644
--- a/drivers/scsi/fnic/fnic_isr.c
+-+-+- b/drivers/scsi/fnic/fnic_isr.c

@@ -319,20 +-319,25 @@ int fnic_set_intr_mode_msix(struct fnic *fnic) return 1;
}

-int fnic_set_intr_mode(struct fnic *fnic)
+-int fnic_set_intr_mode(struct fnic *fnic, unsigned int intr_mode) {
int ret_status = 0;

/*
* Set interrupt mode (INTx, MSI, MSI-X) depending
* system capabilities.
- *

+-      */
+-     if (intr_mode != VNIC_DEV_INTR_MODE_MSIX)
+-             goto try_msi;
+-     /*

* Try MSI-X first
*/
ret_status = fnic_set_intr_mode_msix(fnic);
if (ret_status == 0)
return ret_status;
-

+-try_msi:
+-     if (intr_mode != VNIC_DEV_INTR_MODE_MSI)
+-             goto try_intx;

/*
* Next try MSI
* We need 1 RQ, 1 WQ, 1 WQ_COPY, 3 CQs, and 1 INTR @@ -358,7 +-363,7 @@ int fnic_set_intr_mode(struct fnic *fnic)

return 0;
}
-
+-try_intx:
/*
* Next try INTx
* We need 1 RQ, 1 WQ, 1 WQ_COPY, 3 CQs, and 3 INTRs

drivers/scsi/fnic/fnic_main.c +1 -1
index 870b265be41a..4bdd55958f59 100644
--- a/drivers/scsi/fnic/fnic_main.c
+-+-+- b/drivers/scsi/fnic/fnic_main.c

@@ -97,6 +-97,10 @@ module_param(pc_rscn_handling_feature_flag, uint, 0644); MODULE_PARM_DESC(pc_rscn_handling_feature_flag,
"PCRSCN handling (0 for none. 1 to handle PCRSCN (default))");

Quoted 7 lines
Quoted 3 lines
+-static unsigned int fnic_intr_mode = VNIC_DEV_INTR_MODE_MSIX;
+-module_param(fnic_intr_mode, uint, S_IRUGO | S_IWUSR);
+-MODULE_PARM_DESC(fnic_intr_mode, "Interrupt mode, 1 = INTx, 2 = MSI, 3 = MSIx (default: 3)");

Based on fnic team's review: there is a way to choose the interrupt mode using the UCS management platform. We do not want to expose this as a module parameter.

Quoted 3 lines

Yeah, I know. It was primarily used during testing to easily change between the various modes.
I can drop it for the next round.

Thanks Hannes. Sounds good.

Quoted 25 lines
Quoted 7 lines
Quoted 7 lines

struct workqueue_struct *reset_fnic_work_queue;
struct workqueue_struct *fnic_fip_queue;

@@ -869,7 +-873,11 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)

fnic_get_res_counts(fnic);

Quoted 17 lines
Quoted 6 lines

- err = fnic_set_intr_mode(fnic);

+-     /* Override interrupt selection during kdump */
+-     if (reset_devices)
+-             fnic_intr_mode = VNIC_DEV_INTR_MODE_INTX;
+-
+-     err = fnic_set_intr_mode(fnic, fnic_intr_mode);
Quoted 7 lines

if (err) {
dev_err(&fnic->pdev->dev, "Failed to set intr mode, " "aborting.\n");
--
2.43.0

Thanks for these changes, Hannes.
For the other changes in this patch, I will need to test and get back to you.

Quoted 3 lines

Thanks.
The 'reset_devices' thing is primarily there for kdump; might be an idea to make is explicit by using 'is_kdump_kernel()' directly.

Yes Hannes. That would be better. Thanks.

Quoted 10 lines

Cheers,

Hannes

--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +-49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Quoted 8 lines

I'm bringing up a setup to test. I'll wait for your next revision.

My plan is to induce a kdump to test out these changes. If you have any other tests in mind, please let me know.

Thanks,
Karan

Hi Hannes,

Please consider sending out a new revision of these changes. I'd like to test all the changes and add a tested-by tag to them.

Thanks,
Karan

#16 lore raw

Re: [PATCH 4/4] fnic: make interrupt mode configurable

Quoted 151 lines · Karan Tilak Kumar (kartilak)

Cisco Confidential

Quoted 138 lines · Karan Tilak Kumar (kartilak)
Quoted 88 lines · Hannes Reinecke
Quoted 79 lines · Karan Tilak Kumar (kartilak)

Cisco Confidential

Quoted 76 lines · Hannes Reinecke

In some environments (eg kdump) not all CPUs are online, so the MQ mapping might be resulting in an invalid layout. So make the interrupt mode settable via an 'fnic_intr_mode' module parameter and switch to INTx if the 'reset_devices' kernel parameter is specified.

---

drivers/scsi/fnic/fnic.h      |  2 +-
drivers/scsi/fnic/fnic_isr.c  | 13 +++++++++----
drivers/scsi/fnic/fnic_main.c | 10 +++++++++-
3 files changed, 19 insertions(+), 6 deletions(-)
drivers/scsi/fnic/fnic.h +1 -1
index 1199d701c3f5..c679283955e9 100644
@@ -484,7 +484,7 @@ extern struct workqueue_struct *fnic_fip_queue;
 extern const struct attribute_group *fnic_host_groups[];
 
 void fnic_clear_intr_mode(struct fnic *fnic);
-int fnic_set_intr_mode(struct fnic *fnic);
+int fnic_set_intr_mode(struct fnic *fnic, unsigned int mode);
 int fnic_set_intr_mode_msix(struct fnic *fnic);
 void fnic_free_intr(struct fnic *fnic);
 int fnic_request_intr(struct fnic *fnic);
drivers/scsi/fnic/fnic_isr.c +9 -4
index e16b76d537e8..b6594ad064ca 100644
@@ -319,20 +319,25 @@ int fnic_set_intr_mode_msix(struct fnic *fnic)
 return 1;
 }
 
-int fnic_set_intr_mode(struct fnic *fnic)
+int fnic_set_intr_mode(struct fnic *fnic, unsigned int intr_mode)
 {
 int ret_status = 0;
 
 /*
 * Set interrupt mode (INTx, MSI, MSI-X) depending
 * system capabilities.
-      *
+      */
+     if (intr_mode != VNIC_DEV_INTR_MODE_MSIX)
+             goto try_msi;
+     /*
 * Try MSI-X first
 */
 ret_status = fnic_set_intr_mode_msix(fnic);
 if (ret_status == 0)
 return ret_status;
-
+try_msi:
+     if (intr_mode != VNIC_DEV_INTR_MODE_MSI)
+             goto try_intx;
 /*
 * Next try MSI
 * We need 1 RQ, 1 WQ, 1 WQ_COPY, 3 CQs, and 1 INTR
@@ -358,7 +363,7 @@ int fnic_set_intr_mode(struct fnic *fnic)
 
 return 0;
 }
-
+try_intx:
 /*
 * Next try INTx
 * We need 1 RQ, 1 WQ, 1 WQ_COPY, 3 CQs, and 3 INTRs
drivers/scsi/fnic/fnic_main.c +0 -0
index 870b265be41a..4bdd55958f59 100644
@@ -97,6 +97,10 @@ module_param(pc_rscn_handling_feature_flag, uint, 0644);
 MODULE_PARM_DESC(pc_rscn_handling_feature_flag,
 "PCRSCN handling (0 for none. 1 to handle PCRSCN (default))");
 
Quoted 7 lines

drivers/scsi/fnic/fnic_main.c module_param(pc_rscn_handling_feature_flag, uint, 0644); · line 102

Quoted 3 lines
drivers/scsi/fnic/fnic_main.c +3 -0
+static unsigned int fnic_intr_mode = VNIC_DEV_INTR_MODE_MSIX;
+module_param(fnic_intr_mode, uint, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(fnic_intr_mode, "Interrupt mode, 1 = INTx, 2 = MSI, 3 = MSIx (default: 3)");

Based on fnic team's review: there is a way to choose the interrupt mode using the UCS management platform. We do not want to expose this as a module parameter.

Quoted 3 lines

Yeah, I know. It was primarily used during testing to easily change between the various modes.
I can drop it for the next round.

Thanks Hannes. Sounds good.

Quoted 25 lines
Quoted 7 lines
Quoted 7 lines
drivers/scsi/fnic/fnic_main.c +0 -0
 struct workqueue_struct *reset_fnic_work_queue;
 struct workqueue_struct *fnic_fip_queue;
 
@@ -869,7 +873,11 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 fnic_get_res_counts(fnic);
 
Quoted 17 lines
Quoted 7 lines
drivers/scsi/fnic/fnic_main.c +5 -1
-     err = fnic_set_intr_mode(fnic);
+     /* Override interrupt selection during kdump */
+     if (reset_devices)
+             fnic_intr_mode = VNIC_DEV_INTR_MODE_INTX;
+
+     err = fnic_set_intr_mode(fnic, fnic_intr_mode);
 if (err) {
Quoted 6 lines

dev_err(&fnic->pdev->dev, "Failed to set intr mode, " "aborting.\n");
--
2.43.0

Thanks for these changes, Hannes.
For the other changes in this patch, I will need to test and get back to you.

Quoted 3 lines

Thanks.
The 'reset_devices' thing is primarily there for kdump; might be an idea to make is explicit by using 'is_kdump_kernel()' directly.

Yes Hannes. That would be better. Thanks.

Quoted 10 lines

Cheers,

Hannes

--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Quoted 8 lines

I'm bringing up a setup to test. I'll wait for your next revision.

My plan is to induce a kdump to test out these changes. If you have any other tests in mind, please let me know.

Thanks,
Karan

Quoted 8 lines

Hi Hannes,

Please consider sending out a new revision of these changes. I'd like to test all the changes and add a tested-by tag to them.

Thanks,
Karan

Hi Karan:

My name is Lee Duncan, and I work with Hannes. He is otherwise occupied right now, so I'll do my best to take it from here.

It seems like you wanted two changes to patch#4:
1. Remove the newly-added module parameter, and
2. Use "is_kdump_kernel()" to test for setting the interrupt mode to INTX

I have made those changes to patch#4, and internal testing shows that kdump works.

I will resubmit the patch sequence after one more internal test. I appreciate your patience.

P.S, I believe the patches will end up getting to you through other channels, since a bug was filed, but if you'd like to test them and want a copy before I repost the sequence, please contact me.

#17 lore raw

RE: [PATCH 4/4] fnic: make interrupt mode configurable

Quoted 168 lines · Lee Duncan
Quoted 151 lines · Karan Tilak Kumar (kartilak)

Cisco Confidential

Quoted 138 lines · Karan Tilak Kumar (kartilak)
Quoted 88 lines · Hannes Reinecke
Quoted 79 lines · Karan Tilak Kumar (kartilak)

Cisco Confidential

Quoted 76 lines · Hannes Reinecke

In some environments (eg kdump) not all CPUs are online, so the MQ mapping might be resulting in an invalid layout. So make the interrupt mode settable via an 'fnic_intr_mode' module parameter and switch to INTx if the 'reset_devices' kernel parameter is specified.

---

drivers/scsi/fnic/fnic.h      |  2 +-
drivers/scsi/fnic/fnic_isr.c  | 13 +++++++++----
drivers/scsi/fnic/fnic_main.c | 10 +++++++++-
3 files changed, 19 insertions(+), 6 deletions(-)
drivers/scsi/fnic/fnic.h +1 -1
index 1199d701c3f5..c679283955e9 100644
@@ -484,7 +484,7 @@ extern struct workqueue_struct *fnic_fip_queue;
 extern const struct attribute_group *fnic_host_groups[];
 
 void fnic_clear_intr_mode(struct fnic *fnic);
-int fnic_set_intr_mode(struct fnic *fnic);
+int fnic_set_intr_mode(struct fnic *fnic, unsigned int mode);
 int fnic_set_intr_mode_msix(struct fnic *fnic);
 void fnic_free_intr(struct fnic *fnic);
 int fnic_request_intr(struct fnic *fnic);
drivers/scsi/fnic/fnic_isr.c +9 -4
index e16b76d537e8..b6594ad064ca 100644
@@ -319,20 +319,25 @@ int fnic_set_intr_mode_msix(struct fnic *fnic)
 return 1;
 }
 
-int fnic_set_intr_mode(struct fnic *fnic)
+int fnic_set_intr_mode(struct fnic *fnic, unsigned int intr_mode)
 {
 int ret_status = 0;
 
 /*
 * Set interrupt mode (INTx, MSI, MSI-X) depending
 * system capabilities.
-      *
+      */
+     if (intr_mode != VNIC_DEV_INTR_MODE_MSIX)
+             goto try_msi;
+     /*
 * Try MSI-X first
 */
 ret_status = fnic_set_intr_mode_msix(fnic);
 if (ret_status == 0)
 return ret_status;
-
+try_msi:
+     if (intr_mode != VNIC_DEV_INTR_MODE_MSI)
+             goto try_intx;
 /*
 * Next try MSI
 * We need 1 RQ, 1 WQ, 1 WQ_COPY, 3 CQs, and 1 INTR
@@ -358,7 +363,7 @@ int fnic_set_intr_mode(struct fnic *fnic)
 
 return 0;
 }
-
+try_intx:
 /*
 * Next try INTx
 * We need 1 RQ, 1 WQ, 1 WQ_COPY, 3 CQs, and 3 INTRs
drivers/scsi/fnic/fnic_main.c +0 -0
index 870b265be41a..4bdd55958f59 100644
@@ -97,6 +97,10 @@ module_param(pc_rscn_handling_feature_flag, uint, 0644);
 MODULE_PARM_DESC(pc_rscn_handling_feature_flag,
 "PCRSCN handling (0 for none. 1 to handle PCRSCN (default))");
 
Quoted 7 lines

drivers/scsi/fnic/fnic_main.c module_param(pc_rscn_handling_feature_flag, uint, 0644); · line 102

Quoted 3 lines
drivers/scsi/fnic/fnic_main.c +3 -0
+static unsigned int fnic_intr_mode = VNIC_DEV_INTR_MODE_MSIX;
+module_param(fnic_intr_mode, uint, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(fnic_intr_mode, "Interrupt mode, 1 = INTx, 2 = MSI, 3 = MSIx (default: 3)");

Based on fnic team's review: there is a way to choose the interrupt mode using the UCS management platform. We do not want to expose this as a module parameter.

Quoted 3 lines

Yeah, I know. It was primarily used during testing to easily change between the various modes.
I can drop it for the next round.

Thanks Hannes. Sounds good.

Quoted 25 lines
Quoted 7 lines
Quoted 7 lines
drivers/scsi/fnic/fnic_main.c +0 -0
 struct workqueue_struct *reset_fnic_work_queue;
 struct workqueue_struct *fnic_fip_queue;
 
@@ -869,7 +873,11 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 fnic_get_res_counts(fnic);
 
Quoted 17 lines
Quoted 7 lines
drivers/scsi/fnic/fnic_main.c +5 -1
-     err = fnic_set_intr_mode(fnic);
+     /* Override interrupt selection during kdump */
+     if (reset_devices)
+             fnic_intr_mode = VNIC_DEV_INTR_MODE_INTX;
+
+     err = fnic_set_intr_mode(fnic, fnic_intr_mode);
 if (err) {
Quoted 6 lines

dev_err(&fnic->pdev->dev, "Failed to set intr mode, " "aborting.\n");
--
2.43.0

Thanks for these changes, Hannes.
For the other changes in this patch, I will need to test and get back to you.

Quoted 3 lines

Thanks.
The 'reset_devices' thing is primarily there for kdump; might be an idea to make is explicit by using 'is_kdump_kernel()' directly.

Yes Hannes. That would be better. Thanks.

Quoted 10 lines

Cheers,

Hannes

--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Quoted 8 lines

I'm bringing up a setup to test. I'll wait for your next revision.

My plan is to induce a kdump to test out these changes. If you have any other tests in mind, please let me know.

Thanks,
Karan

Quoted 8 lines

Hi Hannes,

Please consider sending out a new revision of these changes. I'd like to test all the changes and add a tested-by tag to them.

Thanks,
Karan

Hi Karan:

My name is Lee Duncan, and I work with Hannes. He is otherwise occupied right now, so I'll do my best to take it from here.

Quoted 7 lines

It seems like you wanted two changes to patch#4:
1. Remove the newly-added module parameter, and
2. Use "is_kdump_kernel()" to test for setting the interrupt mode to INTX

I have made those changes to patch#4, and internal testing shows that kdump works.

Hi Lee,

Appreciate your response and your willingness to take this forward. Sounds good. Thank you for the changes and internal tests.

Quoted 7 lines

I will resubmit the patch sequence after one more internal test. I appreciate your patience.

P.S, I believe the patches will end up getting to you through other channels, since a bug was filed, but if you'd like to test them and want a copy before I repost the sequence, please contact me.

I understand. I would like to dev test these changes on top the latest kernel just to make sure things are working correctly. I don't mind waiting until the internal tests are completed. I'll take your next revision in for testing. Thanks again for your help.

Regards,
Karan