fnic: fixup INTx and MSI interrupt modes
Subsystems (2)
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
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
@@ -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
Karan Tilak Kumar (kartilak)
maintainerRE: [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.
Signed-off-by Hannes Reinecke <[email protected]>---
drivers/scsi/fnic/fnic_main.c| 2 +--1 file changed, 1 insertion(+-), 1 deletion(-)drivers/scsi/fnic/fnic_main.c +1
-1--- 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)); structpci_dev*l_pdev=fnic->pdev;
- intintr_mode=fnic->config.intr_mode;
+- intintr_mode=vnic_dev_get_intr_mode(fnic->vdev);
structblk_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
RE: [PATCH 1/4] fnic: use resulting interrupt mode during configuration
In reply to Hannes ReineckeCisco 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.
Signed-off-by Hannes Reinecke <[email protected]>---
drivers/scsi/fnic/fnic_main.c| 2 +--1 file changed, 1 insertion(+-), 1 deletion(-)drivers/scsi/fnic/fnic_main.c +1
-1--- 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)); structpci_dev*l_pdev=fnic->pdev;
- intintr_mode=fnic->config.intr_mode;
+- intintr_mode=vnic_dev_get_intr_mode(fnic->vdev);
structblk_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
PATCH 2/4 fnic: correctly initialize 'fnic->name'
In reply to Hannes ReineckeThe 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
@@ -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
Karan Tilak Kumar (kartilak)
maintainerRE: [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.Signed-off-by Hannes Reinecke <[email protected]>---
drivers/scsi/fnic/fnic_main.c| 5 +-+----1 file changed, 2 insertions(+-), 3 deletions(-)drivers/scsi/fnic/fnic_main.c +1
-1--- 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){
structScsi_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
RE: [PATCH 2/4] fnic: correctly initialize 'fnic->name'
In reply to Hannes ReineckeCisco 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.Signed-off-by Hannes Reinecke <[email protected]>---
drivers/scsi/fnic/fnic_main.c| 5 +-+----1 file changed, 2 insertions(+-), 3 deletions(-)drivers/scsi/fnic/fnic_main.c +1
-1--- 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){
structScsi_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
PATCH 3/4 fnic: missing initialisation for wq_copy_base
In reply to Hannes ReineckeWith 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.
--- drivers/scsi/fnic/fnic_isr.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/scsi/fnic/fnic_isr.c +2 -0
@@ -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
Karan Tilak Kumar (kartilak)
maintainerRE: [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_SUCCESSicmnd 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)")Signed-off-by Hannes Reinecke <[email protected]>---
drivers/scsi/fnic/fnic_isr.c| 2 +-+-1 file changed, 2 insertions(+-)drivers/scsi/fnic/fnic_isr.c +1
-1--- 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 @@ intfnic_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
Karan Tilak Kumar (kartilak)
maintainerRE: [PATCH 3/4] fnic: missing initialisation for wq_copy_base
In reply to Hannes ReineckeCisco 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_SUCCESSicmnd 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)")Signed-off-by Hannes Reinecke <[email protected]>---
drivers/scsi/fnic/fnic_isr.c| 2 +-+-1 file changed, 2 insertions(+-)drivers/scsi/fnic/fnic_isr.c +1
-1--- 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 @@ intfnic_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
PATCH 4/4 fnic: make interrupt mode configurable
In reply to Hannes ReineckeIn 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
@@ -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
@@ -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
@@ -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
Karan Tilak Kumar (kartilak)
maintainerRE: [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.Signed-off-by Hannes Reinecke <[email protected]>---
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--- 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 structattribute_group*fnic_host_groups[];void
fnic_clear_intr_mode(struct fnic *fnic);
-intfnic_set_intr_mode(struct fnic *fnic);
+-intfnic_set_intr_mode(struct fnic *fnic, unsigned int mode); intfnic_set_intr_mode_msix(struct fnic *fnic);
voidfnic_free_intr(struct fnic *fnic);
intfnic_request_intr(struct fnic *fnic);drivers/scsi/fnic/fnic_isr.c +1
-1--- 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)
+-intfnic_set_intr_mode(struct fnic *fnic, unsigned int intr_mode){
intret_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)
returnret_status;
-+-try_msi: +- if (intr_mode != VNIC_DEV_INTR_MODE_MSI) +- goto try_intx;/*
* Next try MSI
* We need 1 RQ, 1 WQ, 1WQ_COPY, 3 CQs, and 1 INTR @@ -358,7 +-363,7 @@ intfnic_set_intr_mode(struct fnic *fnic)return 0;
}
-
+-try_intx:
/*
* Next try INTx
* We need 1 RQ, 1 WQ, 1WQ_COPY, 3 CQs, and 3 INTRsdrivers/scsi/fnic/fnic_main.c +1
-1--- 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;
structworkqueue_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
Karan Tilak Kumar (kartilak)
maintainerRE: [PATCH 4/4] fnic: make interrupt mode configurable
In reply to Hannes ReineckeCisco 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.Signed-off-by Hannes Reinecke <[email protected]>---
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--- 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 structattribute_group*fnic_host_groups[];void
fnic_clear_intr_mode(struct fnic *fnic);
-intfnic_set_intr_mode(struct fnic *fnic);
+-intfnic_set_intr_mode(struct fnic *fnic, unsigned int mode); intfnic_set_intr_mode_msix(struct fnic *fnic);
voidfnic_free_intr(struct fnic *fnic);
intfnic_request_intr(struct fnic *fnic);drivers/scsi/fnic/fnic_isr.c +1
-1--- 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)
+-intfnic_set_intr_mode(struct fnic *fnic, unsigned int intr_mode){
intret_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)
returnret_status;
-+-try_msi: +- if (intr_mode != VNIC_DEV_INTR_MODE_MSI) +- goto try_intx;/*
* Next try MSI
* We need 1 RQ, 1 WQ, 1WQ_COPY, 3 CQs, and 1 INTR @@ -358,7 +-363,7 @@ intfnic_set_intr_mode(struct fnic *fnic)return 0;
}
-
+-try_intx:
/*
* Next try INTx
* We need 1 RQ, 1 WQ, 1WQ_COPY, 3 CQs, and 3 INTRsdrivers/scsi/fnic/fnic_main.c +1
-1--- 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;
structworkqueue_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
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.Signed-off-by Hannes Reinecke <[email protected]>---
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@@ -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@@ -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@@ -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
-0struct 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.0Thanks 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
Karan Tilak Kumar (kartilak)
maintainerRE: [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.Signed-off-by Hannes Reinecke <[email protected]>---
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--- 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 structattribute_group*fnic_host_groups[];void
fnic_clear_intr_mode(struct fnic *fnic);
-intfnic_set_intr_mode(struct fnic *fnic);
+-intfnic_set_intr_mode(struct fnic *fnic, unsigned int mode); intfnic_set_intr_mode_msix(struct fnic *fnic);
voidfnic_free_intr(struct fnic *fnic);
intfnic_request_intr(struct fnic *fnic);drivers/scsi/fnic/fnic_isr.c +1
-1--- 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)
+-intfnic_set_intr_mode(struct fnic *fnic, unsigned int intr_mode){
intret_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)
returnret_status;
-+-try_msi: +- if (intr_mode != VNIC_DEV_INTR_MODE_MSI) +- goto try_intx;/*
* Next try MSI
* We need 1 RQ, 1 WQ, 1WQ_COPY, 3 CQs, and 1 INTR @@ -358,7 +-363,7 @@ intfnic_set_intr_mode(struct fnic *fnic)return 0;
}
-
+-try_intx:
/*
* Next try INTx
* We need 1 RQ, 1 WQ, 1WQ_COPY, 3 CQs, and 3 INTRsdrivers/scsi/fnic/fnic_main.c +1
-1--- 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;
structworkqueue_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.0Thanks 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
Karan Tilak Kumar (kartilak)
maintainerRE: [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.Signed-off-by Hannes Reinecke <[email protected]>---
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--- 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 structattribute_group*fnic_host_groups[];void
fnic_clear_intr_mode(struct fnic *fnic);
-intfnic_set_intr_mode(struct fnic *fnic);
+-intfnic_set_intr_mode(struct fnic *fnic, unsigned int mode); intfnic_set_intr_mode_msix(struct fnic *fnic);
voidfnic_free_intr(struct fnic *fnic);
intfnic_request_intr(struct fnic *fnic);drivers/scsi/fnic/fnic_isr.c +1
-1--- 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)
+-intfnic_set_intr_mode(struct fnic *fnic, unsigned int intr_mode){
intret_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)
returnret_status;
-+-try_msi: +- if (intr_mode != VNIC_DEV_INTR_MODE_MSI) +- goto try_intx;/*
* Next try MSI
* We need 1 RQ, 1 WQ, 1WQ_COPY, 3 CQs, and 1 INTR @@ -358,7 +-363,7 @@ intfnic_set_intr_mode(struct fnic *fnic)return 0;
}
-
+-try_intx:
/*
* Next try INTx
* We need 1 RQ, 1 WQ, 1WQ_COPY, 3 CQs, and 3 INTRsdrivers/scsi/fnic/fnic_main.c +1
-1--- 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;
structworkqueue_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.0Thanks 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
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.Signed-off-by Hannes Reinecke <[email protected]>---
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@@ -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@@ -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@@ -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
-0struct 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.0Thanks 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. KnoblichQuoted 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.
Karan Tilak Kumar (kartilak)
maintainerRE: [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.Signed-off-by Hannes Reinecke <[email protected]>---
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@@ -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@@ -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@@ -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
-0struct 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.0Thanks 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. KnoblichQuoted 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,
KaranQuoted 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,
KaranHi 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 INTXI 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