[PATCH 0/5] megaraid_sas: scsi-mq support

[PATCH 0/5] megaraid_sas: scsi-mq support

[Hannes Reinecke]

Hi all,

here's the patcheset to enable scsi-mq support for megaraid_sas.
It's based on hch's irq rework currently pending in tip.
As the overall results have been less then stellar (see the thread
'reduced latency is killing performance') I've also added a module
parameter 'use_blk_mq' to allowing to switch off scsi-mq support
on a per-driver basis.

Hannes Reinecke (5):
  megaraid_sas: switch to pci_alloc_irq_vectors
  megaraid_sas: avoid calling megasas_lookup_instance()
  megaraid_sas: do not crash on invalid completion
  megaraid_sas: scsi-mq support
  megaraid_sas: add mmio barrier after register writes

 drivers/scsi/megaraid/megaraid_sas.h        |   4 +-
 drivers/scsi/megaraid/megaraid_sas_base.c   | 109 ++++++++++++++++------------
 drivers/scsi/megaraid/megaraid_sas_fusion.c |  63 +++++++++++-----
 3 files changed, 109 insertions(+), 67 deletions(-)

-- 
1.8.5.6

[PATCH 1/5] megaraid_sas: switch to pci_alloc_irq_vectors

[Hannes Reinecke]

Cleanup the MSI-X handling allowing us to use
the PCI-layer provided vector allocation.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/megaraid/megaraid_sas.h      |  1 -
 drivers/scsi/megaraid/megaraid_sas_base.c | 63 ++++++++++++++-----------------
 2 files changed, 29 insertions(+), 35 deletions(-)

diff --git a/drivers/scsi/megaraid/megaraid_sas.h b/drivers/scsi/megaraid/megaraid_sas.h
index ca86c88..8f1d2b4 100644
--- a/drivers/scsi/megaraid/megaraid_sas.h
+++ b/drivers/scsi/megaraid/megaraid_sas.h
@@ -2118,7 +2118,6 @@ struct megasas_instance {
 	u32 ctrl_context_pages;
 	struct megasas_ctrl_info *ctrl_info;
 	unsigned int msix_vectors;
-	struct msix_entry msixentry[MEGASAS_MAX_MSIX_QUEUES];
 	struct megasas_irq_context irq_context[MEGASAS_MAX_MSIX_QUEUES];
 	u64 map_id;
 	u64 pd_seq_map_id;
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index 3236632..e7e3efd 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -4843,7 +4843,7 @@ int megasas_set_crash_dump_params(struct megasas_instance *instance,
 }
 
 /*
- * megasas_setup_irqs_msix -		register legacy interrupts.
+ * megasas_setup_irqs_ioapic -		register legacy interrupts.
  * @instance:				Adapter soft state
  *
  * Do not enable interrupt, only setup ISRs.
@@ -4858,8 +4858,9 @@ int megasas_set_crash_dump_params(struct megasas_instance *instance,
 	pdev = instance->pdev;
 	instance->irq_context[0].instance = instance;
 	instance->irq_context[0].MSIxIndex = 0;
-	if (request_irq(pdev->irq, instance->instancet->service_isr,
-		IRQF_SHARED, "megasas", &instance->irq_context[0])) {
+	if (request_irq(pci_irq_vector(pdev, 0),
+			instance->instancet->service_isr, IRQF_SHARED,
+			"megasas", &instance->irq_context[0])) {
 		dev_err(&instance->pdev->dev,
 				"Failed to register IRQ from %s %d\n",
 				__func__, __LINE__);
@@ -4880,28 +4881,23 @@ int megasas_set_crash_dump_params(struct megasas_instance *instance,
 static int
 megasas_setup_irqs_msix(struct megasas_instance *instance, u8 is_probe)
 {
-	int i, j, cpu;
+	int i, j;
 	struct pci_dev *pdev;
 
 	pdev = instance->pdev;
 
 	/* Try MSI-x */
-	cpu = cpumask_first(cpu_online_mask);
 	for (i = 0; i < instance->msix_vectors; i++) {
 		instance->irq_context[i].instance = instance;
 		instance->irq_context[i].MSIxIndex = i;
-		if (request_irq(instance->msixentry[i].vector,
+		if (request_irq(pci_irq_vector(pdev, i),
 			instance->instancet->service_isr, 0, "megasas",
 			&instance->irq_context[i])) {
 			dev_err(&instance->pdev->dev,
 				"Failed to register IRQ for vector %d.\n", i);
-			for (j = 0; j < i; j++) {
-				if (smp_affinity_enable)
-					irq_set_affinity_hint(
-						instance->msixentry[j].vector, NULL);
-				free_irq(instance->msixentry[j].vector,
-					&instance->irq_context[j]);
-			}
+			for (j = 0; j < i; j++)
+				free_irq(pci_irq_vector(pdev, j),
+					 &instance->irq_context[j]);
 			/* Retry irq register for IO_APIC*/
 			instance->msix_vectors = 0;
 			if (is_probe)
@@ -4909,14 +4905,6 @@ int megasas_set_crash_dump_params(struct megasas_instance *instance,
 			else
 				return -1;
 		}
-		if (smp_affinity_enable) {
-			if (irq_set_affinity_hint(instance->msixentry[i].vector,
-				get_cpu_mask(cpu)))
-				dev_err(&instance->pdev->dev,
-					"Failed to set affinity hint"
-					" for cpu %d\n", cpu);
-			cpu = cpumask_next(cpu, cpu_online_mask);
-		}
 	}
 	return 0;
 }
@@ -4933,14 +4921,12 @@ int megasas_set_crash_dump_params(struct megasas_instance *instance,
 
 	if (instance->msix_vectors)
 		for (i = 0; i < instance->msix_vectors; i++) {
-			if (smp_affinity_enable)
-				irq_set_affinity_hint(
-					instance->msixentry[i].vector, NULL);
-			free_irq(instance->msixentry[i].vector,
+			free_irq(pci_irq_vector(instance->pdev, i),
 				 &instance->irq_context[i]);
 		}
 	else
-		free_irq(instance->pdev->irq, &instance->irq_context[0]);
+		free_irq(pci_irq_vector(instance->pdev, 0),
+			 &instance->irq_context[0]);
 }
 
 /**
@@ -5018,6 +5004,7 @@ static int megasas_init_fw(struct megasas_instance *instance)
 	int i, loop, fw_msix_count = 0;
 	struct IOV_111 *iovPtr;
 	struct fusion_context *fusion;
+	int irq_flags = PCI_IRQ_MSIX;
 
 	fusion = instance->ctrl_context;
 
@@ -5134,15 +5121,18 @@ static int megasas_init_fw(struct megasas_instance *instance)
 		/* Don't bother allocating more MSI-X vectors than cpus */
 		instance->msix_vectors = min(instance->msix_vectors,
 					     (unsigned int)num_online_cpus());
-		for (i = 0; i < instance->msix_vectors; i++)
-			instance->msixentry[i].entry = i;
-		i = pci_enable_msix_range(instance->pdev, instance->msixentry,
-					  1, instance->msix_vectors);
+		if (smp_affinity_enable)
+			irq_flags |= PCI_IRQ_AFFINITY;
+		i = pci_alloc_irq_vectors(instance->pdev, 1,
+					  instance->msix_vectors, irq_flags);
 		if (i > 0)
 			instance->msix_vectors = i;
 		else
 			instance->msix_vectors = 0;
 	}
+	i = pci_alloc_irq_vectors(instance->pdev, 1, 1, PCI_IRQ_LEGACY);
+	if (i < 0)
+		goto fail_setup_irqs;
 
 	dev_info(&instance->pdev->dev,
 		"firmware supports msix\t: (%d)", fw_msix_count);
@@ -5587,7 +5577,6 @@ static int megasas_io_attach(struct megasas_instance *instance)
 	/*
 	 * Export parameters required by SCSI mid-layer
 	 */
-	host->irq = instance->pdev->irq;
 	host->unique_id = instance->unique_id;
 	host->can_queue = instance->max_scsi_cmds;
 	host->this_id = instance->init_id;
@@ -6128,6 +6117,7 @@ static void megasas_shutdown_controller(struct megasas_instance *instance,
 	int rval;
 	struct Scsi_Host *host;
 	struct megasas_instance *instance;
+	int irq_flags = PCI_IRQ_LEGACY;
 
 	instance = pci_get_drvdata(pdev);
 	host = instance->host;
@@ -6163,9 +6153,14 @@ static void megasas_shutdown_controller(struct megasas_instance *instance,
 		goto fail_ready_state;
 
 	/* Now re-enable MSI-X */
-	if (instance->msix_vectors &&
-	    pci_enable_msix_exact(instance->pdev, instance->msixentry,
-				  instance->msix_vectors))
+	if (instance->msix_vectors)
+		irq_flags = PCI_IRQ_MSIX;
+	if (smp_affinity_enable)
+		irq_flags |= PCI_IRQ_AFFINITY;
+	rval = pci_alloc_irq_vectors(instance->pdev, 1,
+				     instance->msix_vectors ?
+				     instance->msix_vectors : 1, irq_flags);
+	if (rval < 0)
 		goto fail_reenable_msix;
 
 	if (instance->ctrl_context) {
-- 
1.8.5.6

RE: [PATCH 1/5] megaraid_sas: switch to pci_alloc_irq_vectors

[Sumit Saxena]

>-----Original Message-----
>From: Hannes Reinecke [mailto:hare@suse.de]
>Sent: Friday, November 11, 2016 3:15 PM
>To: Martin K. Petersen
>Cc: Christoph Hellwig; James Bottomley; Sumit Saxena; linux-
>scsi@vger.kernel.org; Hannes Reinecke; Hannes Reinecke
>Subject: [PATCH 1/5] megaraid_sas: switch to pci_alloc_irq_vectors
>
>Cleanup the MSI-X handling allowing us to use the PCI-layer provided
vector
>allocation.
>
>Signed-off-by: Hannes Reinecke <hare@suse.com>
>---
> drivers/scsi/megaraid/megaraid_sas.h      |  1 -
> drivers/scsi/megaraid/megaraid_sas_base.c | 63
++++++++++++++-----------------
> 2 files changed, 29 insertions(+), 35 deletions(-)
>
>diff --git a/drivers/scsi/megaraid/megaraid_sas.h
>b/drivers/scsi/megaraid/megaraid_sas.h
>index ca86c88..8f1d2b4 100644
>--- a/drivers/scsi/megaraid/megaraid_sas.h
>+++ b/drivers/scsi/megaraid/megaraid_sas.h
>@@ -2118,7 +2118,6 @@ struct megasas_instance {
> 	u32 ctrl_context_pages;
> 	struct megasas_ctrl_info *ctrl_info;
> 	unsigned int msix_vectors;
>-	struct msix_entry msixentry[MEGASAS_MAX_MSIX_QUEUES];
> 	struct megasas_irq_context
>irq_context[MEGASAS_MAX_MSIX_QUEUES];
> 	u64 map_id;
> 	u64 pd_seq_map_id;
>diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
>b/drivers/scsi/megaraid/megaraid_sas_base.c
>index 3236632..e7e3efd 100644
>--- a/drivers/scsi/megaraid/megaraid_sas_base.c
>+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
>@@ -4843,7 +4843,7 @@ int megasas_set_crash_dump_params(struct
>megasas_instance *instance,  }
>
> /*
>- * megasas_setup_irqs_msix -		register legacy interrupts.
>+ * megasas_setup_irqs_ioapic -		register legacy
interrupts.
>  * @instance:				Adapter soft state
>  *
>  * Do not enable interrupt, only setup ISRs.
>@@ -4858,8 +4858,9 @@ int megasas_set_crash_dump_params(struct
>megasas_instance *instance,
> 	pdev = instance->pdev;
> 	instance->irq_context[0].instance = instance;
> 	instance->irq_context[0].MSIxIndex = 0;
>-	if (request_irq(pdev->irq, instance->instancet->service_isr,
>-		IRQF_SHARED, "megasas", &instance->irq_context[0])) {
>+	if (request_irq(pci_irq_vector(pdev, 0),
>+			instance->instancet->service_isr, IRQF_SHARED,
>+			"megasas", &instance->irq_context[0])) {
> 		dev_err(&instance->pdev->dev,
> 				"Failed to register IRQ from %s %d\n",
> 				__func__, __LINE__);
>@@ -4880,28 +4881,23 @@ int megasas_set_crash_dump_params(struct
>megasas_instance *instance,  static int  megasas_setup_irqs_msix(struct
>megasas_instance *instance, u8 is_probe)  {
>-	int i, j, cpu;
>+	int i, j;
> 	struct pci_dev *pdev;
>
> 	pdev = instance->pdev;
>
> 	/* Try MSI-x */
>-	cpu = cpumask_first(cpu_online_mask);
> 	for (i = 0; i < instance->msix_vectors; i++) {
> 		instance->irq_context[i].instance = instance;
> 		instance->irq_context[i].MSIxIndex = i;
>-		if (request_irq(instance->msixentry[i].vector,
>+		if (request_irq(pci_irq_vector(pdev, i),
> 			instance->instancet->service_isr, 0, "megasas",
> 			&instance->irq_context[i])) {
> 			dev_err(&instance->pdev->dev,
> 				"Failed to register IRQ for vector %d.\n",
i);
>-			for (j = 0; j < i; j++) {
>-				if (smp_affinity_enable)
>-					irq_set_affinity_hint(
>-
instance->msixentry[j].vector,
>NULL);
>-				free_irq(instance->msixentry[j].vector,
>-					&instance->irq_context[j]);
>-			}
>+			for (j = 0; j < i; j++)
>+				free_irq(pci_irq_vector(pdev, j),
>+					 &instance->irq_context[j]);
> 			/* Retry irq register for IO_APIC*/
> 			instance->msix_vectors = 0;
> 			if (is_probe)
>@@ -4909,14 +4905,6 @@ int megasas_set_crash_dump_params(struct
>megasas_instance *instance,
> 			else
> 				return -1;
> 		}
>-		if (smp_affinity_enable) {
>-			if
(irq_set_affinity_hint(instance->msixentry[i].vector,
>-				get_cpu_mask(cpu)))
>-				dev_err(&instance->pdev->dev,
>-					"Failed to set affinity hint"
>-					" for cpu %d\n", cpu);
>-			cpu = cpumask_next(cpu, cpu_online_mask);
>-		}
> 	}
> 	return 0;
> }
>@@ -4933,14 +4921,12 @@ int megasas_set_crash_dump_params(struct
>megasas_instance *instance,
>
> 	if (instance->msix_vectors)
> 		for (i = 0; i < instance->msix_vectors; i++) {
>-			if (smp_affinity_enable)
>-				irq_set_affinity_hint(
>-					instance->msixentry[i].vector,
NULL);
>-			free_irq(instance->msixentry[i].vector,
>+			free_irq(pci_irq_vector(instance->pdev, i),
> 				 &instance->irq_context[i]);
> 		}
> 	else
>-		free_irq(instance->pdev->irq, &instance->irq_context[0]);
>+		free_irq(pci_irq_vector(instance->pdev, 0),
>+			 &instance->irq_context[0]);
> }
>
> /**
>@@ -5018,6 +5004,7 @@ static int megasas_init_fw(struct megasas_instance
>*instance)
> 	int i, loop, fw_msix_count = 0;
> 	struct IOV_111 *iovPtr;
> 	struct fusion_context *fusion;
>+	int irq_flags = PCI_IRQ_MSIX;
>
> 	fusion = instance->ctrl_context;
>
>@@ -5134,15 +5121,18 @@ static int megasas_init_fw(struct
megasas_instance
>*instance)
> 		/* Don't bother allocating more MSI-X vectors than cpus */
> 		instance->msix_vectors = min(instance->msix_vectors,
> 					     (unsigned
int)num_online_cpus());
>-		for (i = 0; i < instance->msix_vectors; i++)
>-			instance->msixentry[i].entry = i;
>-		i = pci_enable_msix_range(instance->pdev,
instance->msixentry,
>-					  1, instance->msix_vectors);
>+		if (smp_affinity_enable)
>+			irq_flags |= PCI_IRQ_AFFINITY;
>+		i = pci_alloc_irq_vectors(instance->pdev, 1,
>+					  instance->msix_vectors,
irq_flags);
> 		if (i > 0)
> 			instance->msix_vectors = i;
> 		else
> 			instance->msix_vectors = 0;
> 	}
>+	i = pci_alloc_irq_vectors(instance->pdev, 1, 1, PCI_IRQ_LEGACY);
>+	if (i < 0)
>+		goto fail_setup_irqs;
>
> 	dev_info(&instance->pdev->dev,
> 		"firmware supports msix\t: (%d)", fw_msix_count); @@
-5587,7
>+5577,6 @@ static int megasas_io_attach(struct megasas_instance
*instance)
> 	/*
> 	 * Export parameters required by SCSI mid-layer
> 	 */
>-	host->irq = instance->pdev->irq;
> 	host->unique_id = instance->unique_id;
> 	host->can_queue = instance->max_scsi_cmds;
> 	host->this_id = instance->init_id;
>@@ -6128,6 +6117,7 @@ static void megasas_shutdown_controller(struct
>megasas_instance *instance,
> 	int rval;
> 	struct Scsi_Host *host;
> 	struct megasas_instance *instance;
>+	int irq_flags = PCI_IRQ_LEGACY;
>
> 	instance = pci_get_drvdata(pdev);
> 	host = instance->host;
>@@ -6163,9 +6153,14 @@ static void megasas_shutdown_controller(struct
>megasas_instance *instance,
> 		goto fail_ready_state;
>
> 	/* Now re-enable MSI-X */
>-	if (instance->msix_vectors &&
>-	    pci_enable_msix_exact(instance->pdev, instance->msixentry,
>-				  instance->msix_vectors))
>+	if (instance->msix_vectors)
>+		irq_flags = PCI_IRQ_MSIX;
>+	if (smp_affinity_enable)
>+		irq_flags |= PCI_IRQ_AFFINITY;
>+	rval = pci_alloc_irq_vectors(instance->pdev, 1,
>+				     instance->msix_vectors ?
>+				     instance->msix_vectors : 1,
irq_flags);
>+	if (rval < 0)
If smp_affinity_enable parameter is set to 1(which is default) and legacy
interrupts are setup, then here driver will pass irq_flags set to
(PCI_IRQ_LEGACY | PCI_IRQ_AFFINITY) while calling function
pci_alloc_irq_vectors().
Should not driver set PCI_IRQ_AFFINITY only for MSI-x interrupts? Setting
PCI_IRQ_AFFINITY for legacy interrupts may restrict all interrupts
completion to single CPU(because IRQ affinity will be set) as there will
be single IRQ for legacy interrupts ?

> 		goto fail_reenable_msix;
>
> 	if (instance->ctrl_context) {
>--
>1.8.5.6

Re: [PATCH 1/5] megaraid_sas: switch to pci_alloc_irq_vectors

[Hannes Reinecke]

On 11/11/2016 12:32 PM, Sumit Saxena wrote:
>> -----Original Message-----
>> From: Hannes Reinecke [mailto:hare@suse.de]
>> Sent: Friday, November 11, 2016 3:15 PM
>> To: Martin K. Petersen
>> Cc: Christoph Hellwig; James Bottomley; Sumit Saxena; linux-
>> scsi@vger.kernel.org; Hannes Reinecke; Hannes Reinecke
>> Subject: [PATCH 1/5] megaraid_sas: switch to pci_alloc_irq_vectors
>>
>> Cleanup the MSI-X handling allowing us to use the PCI-layer provided
> vector
>> allocation.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>> ---
>> drivers/scsi/megaraid/megaraid_sas.h      |  1 -
>> drivers/scsi/megaraid/megaraid_sas_base.c | 63
> ++++++++++++++-----------------
>> 2 files changed, 29 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/scsi/megaraid/megaraid_sas.h
>> b/drivers/scsi/megaraid/megaraid_sas.h
>> index ca86c88..8f1d2b4 100644
>> --- a/drivers/scsi/megaraid/megaraid_sas.h
>> +++ b/drivers/scsi/megaraid/megaraid_sas.h
[ .. ]
>> @@ -6163,9 +6153,14 @@ static void megasas_shutdown_controller(struct
>> megasas_instance *instance,
>> 		goto fail_ready_state;
>>
>> 	/* Now re-enable MSI-X */
>> -	if (instance->msix_vectors &&
>> -	    pci_enable_msix_exact(instance->pdev, instance->msixentry,
>> -				  instance->msix_vectors))
>> +	if (instance->msix_vectors)
>> +		irq_flags = PCI_IRQ_MSIX;
>> +	if (smp_affinity_enable)
>> +		irq_flags |= PCI_IRQ_AFFINITY;
>> +	rval = pci_alloc_irq_vectors(instance->pdev, 1,
>> +				     instance->msix_vectors ?
>> +				     instance->msix_vectors : 1,
> irq_flags);
>> +	if (rval < 0)
> If smp_affinity_enable parameter is set to 1(which is default) and legacy
> interrupts are setup, then here driver will pass irq_flags set to
> (PCI_IRQ_LEGACY | PCI_IRQ_AFFINITY) while calling function
> pci_alloc_irq_vectors().
> Should not driver set PCI_IRQ_AFFINITY only for MSI-x interrupts? Setting
> PCI_IRQ_AFFINITY for legacy interrupts may restrict all interrupts
> completion to single CPU(because IRQ affinity will be set) as there will
> be single IRQ for legacy interrupts ?
> 
You are right; irq affinity only makes sense for MSI-X.
I'll be fixing it up.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH 1/5] megaraid_sas: switch to pci_alloc_irq_vectors

[Christoph Hellwig]

On Fri, Nov 11, 2016 at 03:59:05PM +0100, Hannes Reinecke wrote:
> You are right; irq affinity only makes sense for MSI-X.
> I'll be fixing it up.

It works for multi-MSI and MSI-X.  And even for single-MSI or
INTx it's harmless as it will be ignored if only a single
vector is present.

Re: [PATCH 1/5] megaraid_sas: switch to pci_alloc_irq_vectors

[Christoph Hellwig]

>  	if (instance->msix_vectors)
>  		for (i = 0; i < instance->msix_vectors; i++) {
> +			free_irq(pci_irq_vector(instance->pdev, i),
>  				 &instance->irq_context[i]);
>  		}
>  	else
> +		free_irq(pci_irq_vector(instance->pdev, 0),
> +			 &instance->irq_context[0]);
>  }

Don't forget to replace the call to pci_disable_msix with one to pci_free_irq_vectors.

>  
>  /**
> @@ -5018,6 +5004,7 @@ static int megasas_init_fw(struct megasas_instance *instance)
>  	int i, loop, fw_msix_count = 0;
>  	struct IOV_111 *iovPtr;
>  	struct fusion_context *fusion;
> +	int irq_flags = PCI_IRQ_MSIX;
>  
>  	fusion = instance->ctrl_context;
>  
> @@ -5134,15 +5121,18 @@ static int megasas_init_fw(struct megasas_instance *instance)
>  		/* Don't bother allocating more MSI-X vectors than cpus */
>  		instance->msix_vectors = min(instance->msix_vectors,
>  					     (unsigned int)num_online_cpus());
> -		for (i = 0; i < instance->msix_vectors; i++)
> -			instance->msixentry[i].entry = i;
> -		i = pci_enable_msix_range(instance->pdev, instance->msixentry,
> -					  1, instance->msix_vectors);
> +		if (smp_affinity_enable)
> +			irq_flags |= PCI_IRQ_AFFINITY;
> +		i = pci_alloc_irq_vectors(instance->pdev, 1,
> +					  instance->msix_vectors, irq_flags);
>  		if (i > 0)
>  			instance->msix_vectors = i;
>  		else
>  			instance->msix_vectors = 0;
>  	}
> +	i = pci_alloc_irq_vectors(instance->pdev, 1, 1, PCI_IRQ_LEGACY);
> +	if (i < 0)
> +		goto fail_setup_irqs;

This looks wrong - you have to call to pci_alloc_irq_vectors right next
to each other here, so for the MSI-X case you'll still end up calling
the second one as well, which will then fail.  I think the better way
to structure the driver would be to do the following here.

 - Rename the msix_vectors field to irq_vectors, and make sure it's
   initialized to 1 for non-MSI-X capable adapters.
 - Have a single call to pci_alloc_irq_vectors here.
 - Have all the request_irq/free_irq code iterate over ->irq_vectors
   instead of duplicating it
 - use pdev->msix_enabled for the few cases that need to check for
   MSI-X mode specificly.

>  	/* Now re-enable MSI-X */
> -	if (instance->msix_vectors &&
> -	    pci_enable_msix_exact(instance->pdev, instance->msixentry,
> -				  instance->msix_vectors))
> +	if (instance->msix_vectors)
> +		irq_flags = PCI_IRQ_MSIX;
> +	if (smp_affinity_enable)
> +		irq_flags |= PCI_IRQ_AFFINITY;
> +	rval = pci_alloc_irq_vectors(instance->pdev, 1,
> +				     instance->msix_vectors ?
> +				     instance->msix_vectors : 1, irq_flags);
> +	if (rval < 0)

The old code is doing a pci_enable_msix_exact so you also need to pass
the number of vectors as the first argument here.

[PATCH 2/5] megaraid_sas: avoid calling megasas_lookup_instance()

[Hannes Reinecke]

It's quite pointless to call megasas_lookup_instance() if we can
derive a pointer to the structure directly.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/megaraid/megaraid_sas.h        |  3 ++-
 drivers/scsi/megaraid/megaraid_sas_base.c   | 24 +++++++++++-------------
 drivers/scsi/megaraid/megaraid_sas_fusion.c |  2 +-
 3 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/megaraid/megaraid_sas.h b/drivers/scsi/megaraid/megaraid_sas.h
index 8f1d2b4..296e692 100644
--- a/drivers/scsi/megaraid/megaraid_sas.h
+++ b/drivers/scsi/megaraid/megaraid_sas.h
@@ -2375,7 +2375,8 @@ void megasas_return_mfi_mpt_pthr(struct megasas_instance *instance,
 int megasas_cmd_type(struct scsi_cmnd *cmd);
 void megasas_setup_jbod_map(struct megasas_instance *instance);
 
-void megasas_update_sdev_properties(struct scsi_device *sdev);
+void megasas_update_sdev_properties(struct megasas_instance *instance,
+				    struct scsi_device *sdev);
 int megasas_reset_fusion(struct Scsi_Host *shost, int reason);
 int megasas_task_abort_fusion(struct scsi_cmnd *scmd);
 int megasas_reset_target_fusion(struct scsi_cmnd *scmd);
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index e7e3efd..d580406 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -1736,22 +1736,22 @@ static struct megasas_instance *megasas_lookup_instance(u16 host_no)
 /*
 * megasas_update_sdev_properties - Update sdev structure based on controller's FW capabilities
 *
+* @instance: Megasas instance
 * @sdev: OS provided scsi device
 *
 * Returns void
 */
-void megasas_update_sdev_properties(struct scsi_device *sdev)
+void megasas_update_sdev_properties(struct megasas_instance *instance,
+				    struct scsi_device *sdev)
 {
 	u16 pd_index = 0;
 	u32 device_id, ld;
-	struct megasas_instance *instance;
 	struct fusion_context *fusion;
 	struct MR_PRIV_DEVICE *mr_device_priv_data;
 	struct MR_PD_CFG_SEQ_NUM_SYNC *pd_sync;
 	struct MR_LD_RAID *raid;
 	struct MR_DRV_RAID_MAP_ALL *local_map_ptr;
 
-	instance = megasas_lookup_instance(sdev->host->host_no);
 	fusion = instance->ctrl_context;
 	mr_device_priv_data = sdev->hostdata;
 
@@ -1780,13 +1780,11 @@ void megasas_update_sdev_properties(struct scsi_device *sdev)
 	}
 }
 
-static void megasas_set_device_queue_depth(struct scsi_device *sdev)
+static void megasas_set_device_queue_depth(struct megasas_instance *instance,
+					   struct scsi_device *sdev)
 {
 	u16				pd_index = 0;
 	int		ret = DCMD_FAILED;
-	struct megasas_instance *instance;
-
-	instance = megasas_lookup_instance(sdev->host->host_no);
 
 	if (sdev->channel < MEGASAS_MAX_PD_CHANNELS) {
 		pd_index = (sdev->channel * MEGASAS_MAX_DEV_PER_CHANNEL) + sdev->id;
@@ -1822,9 +1820,9 @@ static void megasas_set_device_queue_depth(struct scsi_device *sdev)
 static int megasas_slave_configure(struct scsi_device *sdev)
 {
 	u16 pd_index = 0;
-	struct megasas_instance *instance;
+	struct megasas_instance *instance = (struct megasas_instance *)
+		sdev->host->hostdata;
 
-	instance = megasas_lookup_instance(sdev->host->host_no);
 	if (instance->pd_list_not_supported) {
 		if (sdev->channel < MEGASAS_MAX_PD_CHANNELS &&
 			sdev->type == TYPE_DISK) {
@@ -1835,8 +1833,8 @@ static int megasas_slave_configure(struct scsi_device *sdev)
 				return -ENXIO;
 		}
 	}
-	megasas_set_device_queue_depth(sdev);
-	megasas_update_sdev_properties(sdev);
+	megasas_set_device_queue_depth(instance, sdev);
+	megasas_update_sdev_properties(instance, sdev);
 
 	/*
 	 * The RAID firmware may require extended timeouts.
@@ -1850,10 +1848,10 @@ static int megasas_slave_configure(struct scsi_device *sdev)
 static int megasas_slave_alloc(struct scsi_device *sdev)
 {
 	u16 pd_index = 0;
-	struct megasas_instance *instance ;
+	struct megasas_instance *instance = (struct megasas_instance *)
+		sdev->host->hostdata;
 	struct MR_PRIV_DEVICE *mr_device_priv_data;
 
-	instance = megasas_lookup_instance(sdev->host->host_no);
 	if (sdev->channel < MEGASAS_MAX_PD_CHANNELS) {
 		/*
 		 * Open the OS scan to the SYSTEM PD
diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index 2159f6a..38137de 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -3535,7 +3535,7 @@ int megasas_reset_fusion(struct Scsi_Host *shost, int reason)
 			megasas_setup_jbod_map(instance);
 
 			shost_for_each_device(sdev, shost)
-				megasas_update_sdev_properties(sdev);
+				megasas_update_sdev_properties(instance, sdev);
 
 			clear_bit(MEGASAS_FUSION_IN_RESET,
 				  &instance->reset_flags);
-- 
1.8.5.6

RE: [PATCH 2/5] megaraid_sas: avoid calling megasas_lookup_instance()

[Sumit Saxena]

>-----Original Message-----
>From: Hannes Reinecke [mailto:hare@suse.de]
>Sent: Friday, November 11, 2016 3:15 PM
>To: Martin K. Petersen
>Cc: Christoph Hellwig; James Bottomley; Sumit Saxena; linux-
>scsi@vger.kernel.org; Hannes Reinecke; Hannes Reinecke
>Subject: [PATCH 2/5] megaraid_sas: avoid calling
megasas_lookup_instance()
>
>It's quite pointless to call megasas_lookup_instance() if we can derive a
pointer
>to the structure directly.
>
>Signed-off-by: Hannes Reinecke <hare@suse.com>
Acked-by: Sumit Saxena <sumit.saxena@broadcom.com>

>---
> drivers/scsi/megaraid/megaraid_sas.h        |  3 ++-
> drivers/scsi/megaraid/megaraid_sas_base.c   | 24
+++++++++++-------------
> drivers/scsi/megaraid/megaraid_sas_fusion.c |  2 +-
> 3 files changed, 14 insertions(+), 15 deletions(-)
>
>diff --git a/drivers/scsi/megaraid/megaraid_sas.h
>b/drivers/scsi/megaraid/megaraid_sas.h
>index 8f1d2b4..296e692 100644
>--- a/drivers/scsi/megaraid/megaraid_sas.h
>+++ b/drivers/scsi/megaraid/megaraid_sas.h
>@@ -2375,7 +2375,8 @@ void megasas_return_mfi_mpt_pthr(struct
>megasas_instance *instance,  int megasas_cmd_type(struct scsi_cmnd *cmd);
>void megasas_setup_jbod_map(struct megasas_instance *instance);
>
>-void megasas_update_sdev_properties(struct scsi_device *sdev);
>+void megasas_update_sdev_properties(struct megasas_instance *instance,
>+				    struct scsi_device *sdev);
> int megasas_reset_fusion(struct Scsi_Host *shost, int reason);  int
>megasas_task_abort_fusion(struct scsi_cmnd *scmd);  int
>megasas_reset_target_fusion(struct scsi_cmnd *scmd); diff --git
>a/drivers/scsi/megaraid/megaraid_sas_base.c
>b/drivers/scsi/megaraid/megaraid_sas_base.c
>index e7e3efd..d580406 100644
>--- a/drivers/scsi/megaraid/megaraid_sas_base.c
>+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
>@@ -1736,22 +1736,22 @@ static struct megasas_instance
>*megasas_lookup_instance(u16 host_no)
> /*
> * megasas_update_sdev_properties - Update sdev structure based on
>controller's FW capabilities
> *
>+* @instance: Megasas instance
> * @sdev: OS provided scsi device
> *
> * Returns void
> */
>-void megasas_update_sdev_properties(struct scsi_device *sdev)
>+void megasas_update_sdev_properties(struct megasas_instance *instance,
>+				    struct scsi_device *sdev)
> {
> 	u16 pd_index = 0;
> 	u32 device_id, ld;
>-	struct megasas_instance *instance;
> 	struct fusion_context *fusion;
> 	struct MR_PRIV_DEVICE *mr_device_priv_data;
> 	struct MR_PD_CFG_SEQ_NUM_SYNC *pd_sync;
> 	struct MR_LD_RAID *raid;
> 	struct MR_DRV_RAID_MAP_ALL *local_map_ptr;
>
>-	instance = megasas_lookup_instance(sdev->host->host_no);
> 	fusion = instance->ctrl_context;
> 	mr_device_priv_data = sdev->hostdata;
>
>@@ -1780,13 +1780,11 @@ void megasas_update_sdev_properties(struct
>scsi_device *sdev)
> 	}
> }
>
>-static void megasas_set_device_queue_depth(struct scsi_device *sdev)
>+static void megasas_set_device_queue_depth(struct megasas_instance
>*instance,
>+					   struct scsi_device *sdev)
> {
> 	u16				pd_index = 0;
> 	int		ret = DCMD_FAILED;
>-	struct megasas_instance *instance;
>-
>-	instance = megasas_lookup_instance(sdev->host->host_no);
>
> 	if (sdev->channel < MEGASAS_MAX_PD_CHANNELS) {
> 		pd_index = (sdev->channel *
>MEGASAS_MAX_DEV_PER_CHANNEL) + sdev->id; @@ -1822,9 +1820,9 @@
>static void megasas_set_device_queue_depth(struct scsi_device *sdev)
static int
>megasas_slave_configure(struct scsi_device *sdev)  {
> 	u16 pd_index = 0;
>-	struct megasas_instance *instance;
>+	struct megasas_instance *instance = (struct megasas_instance *)
>+		sdev->host->hostdata;
>
>-	instance = megasas_lookup_instance(sdev->host->host_no);
> 	if (instance->pd_list_not_supported) {
> 		if (sdev->channel < MEGASAS_MAX_PD_CHANNELS &&
> 			sdev->type == TYPE_DISK) {
>@@ -1835,8 +1833,8 @@ static int megasas_slave_configure(struct
scsi_device
>*sdev)
> 				return -ENXIO;
> 		}
> 	}
>-	megasas_set_device_queue_depth(sdev);
>-	megasas_update_sdev_properties(sdev);
>+	megasas_set_device_queue_depth(instance, sdev);
>+	megasas_update_sdev_properties(instance, sdev);
>
> 	/*
> 	 * The RAID firmware may require extended timeouts.
>@@ -1850,10 +1848,10 @@ static int megasas_slave_configure(struct
>scsi_device *sdev)  static int megasas_slave_alloc(struct scsi_device
*sdev)  {
> 	u16 pd_index = 0;
>-	struct megasas_instance *instance ;
>+	struct megasas_instance *instance = (struct megasas_instance *)
>+		sdev->host->hostdata;
> 	struct MR_PRIV_DEVICE *mr_device_priv_data;
>
>-	instance = megasas_lookup_instance(sdev->host->host_no);
> 	if (sdev->channel < MEGASAS_MAX_PD_CHANNELS) {
> 		/*
> 		 * Open the OS scan to the SYSTEM PD
>diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>index 2159f6a..38137de 100644
>--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>@@ -3535,7 +3535,7 @@ int megasas_reset_fusion(struct Scsi_Host *shost,
int
>reason)
> 			megasas_setup_jbod_map(instance);
>
> 			shost_for_each_device(sdev, shost)
>-				megasas_update_sdev_properties(sdev);
>+				megasas_update_sdev_properties(instance,
>sdev);
>
> 			clear_bit(MEGASAS_FUSION_IN_RESET,
> 				  &instance->reset_flags);
>--
>1.8.5.6

[PATCH 3/5] megaraid_sas: do not crash on invalid completion

[Hannes Reinecke]

Avoid a kernel oops when receiving an invalid command completion.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/megaraid/megaraid_sas_fusion.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index 38137de..eb3cb0f 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -2298,13 +2298,15 @@ static void megasas_build_ld_nonrw_fusion(struct megasas_instance *instance,
 			break;
 		case MPI2_FUNCTION_SCSI_IO_REQUEST:  /*Fast Path IO.*/
 			/* Update load balancing info */
-			device_id = MEGASAS_DEV_INDEX(scmd_local);
-			lbinfo = &fusion->load_balance_info[device_id];
-			if (cmd_fusion->scmd->SCp.Status &
-			    MEGASAS_LOAD_BALANCE_FLAG) {
-				atomic_dec(&lbinfo->scsi_pending_cmds[cmd_fusion->pd_r1_lb]);
-				cmd_fusion->scmd->SCp.Status &=
-					~MEGASAS_LOAD_BALANCE_FLAG;
+			if (scmd_local) {
+				device_id = MEGASAS_DEV_INDEX(scmd_local);
+				lbinfo = &fusion->load_balance_info[device_id];
+				if (cmd_fusion->scmd->SCp.Status &
+				    MEGASAS_LOAD_BALANCE_FLAG) {
+					atomic_dec(&lbinfo->scsi_pending_cmds[cmd_fusion->pd_r1_lb]);
+					cmd_fusion->scmd->SCp.Status &=
+						~MEGASAS_LOAD_BALANCE_FLAG;
+				}
 			}
 			if (reply_descript_type ==
 			    MPI2_RPY_DESCRIPT_FLAGS_SCSI_IO_SUCCESS) {
@@ -2315,6 +2317,12 @@ static void megasas_build_ld_nonrw_fusion(struct megasas_instance *instance,
 			/* Fall thru and complete IO */
 		case MEGASAS_MPI2_FUNCTION_LD_IO_REQUEST: /* LD-IO Path */
 			/* Map the FW Cmd Status */
+			if (!scmd_local) {
+				dev_err(&instance->pdev->dev,
+					"cmd[%d:%d] already completed\n",
+					MSIxIndex, smid);
+				break;
+			}
 			map_cmd_status(cmd_fusion, status, extStatus);
 			scsi_io_req->RaidContext.status = 0;
 			scsi_io_req->RaidContext.exStatus = 0;
-- 
1.8.5.6

RE: [PATCH 3/5] megaraid_sas: do not crash on invalid completion

[Sumit Saxena]

>-----Original Message-----
>From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
>owner@vger.kernel.org] On Behalf Of Hannes Reinecke
>Sent: Friday, November 11, 2016 3:15 PM
>To: Martin K. Petersen
>Cc: Christoph Hellwig; James Bottomley; Sumit Saxena; linux-
>scsi@vger.kernel.org; Hannes Reinecke; Hannes Reinecke
>Subject: [PATCH 3/5] megaraid_sas: do not crash on invalid completion
>
>Avoid a kernel oops when receiving an invalid command completion.
scmd_local set to NULL(for cases MPI2_FUNCTION_SCSI_IO_REQUEST and
MEGASAS_MPI2_FUNCTION_LD_IO_REQUEST) will be serious bug(either in driver
or firmware) which should be debugged
and driver should not really continue beyond that. This indicates that
driver internal frames are corrupted. If needed, whenever driver detects
it, it can mark the adapter as dead(stopping further activities).
If OS is installed behind megasas controller then after declaring adapter
dead, system reboot will be required. Kernel panic may give here more
information whenever this condition hits so we kept it like this.
If you are facing this issue, please share the details. I will work on
this.

Thanks,
Sumit

>
>Signed-off-by: Hannes Reinecke <hare@suse.com>
>---
> drivers/scsi/megaraid/megaraid_sas_fusion.c | 22 +++++++++++++++-------
> 1 file changed, 15 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>index 38137de..eb3cb0f 100644
>--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>@@ -2298,13 +2298,15 @@ static void megasas_build_ld_nonrw_fusion(struct
>megasas_instance *instance,
> 			break;
> 		case MPI2_FUNCTION_SCSI_IO_REQUEST:  /*Fast Path IO.*/
> 			/* Update load balancing info */
>-			device_id = MEGASAS_DEV_INDEX(scmd_local);
>-			lbinfo = &fusion->load_balance_info[device_id];
>-			if (cmd_fusion->scmd->SCp.Status &
>-			    MEGASAS_LOAD_BALANCE_FLAG) {
>-
>atomic_dec(&lbinfo->scsi_pending_cmds[cmd_fusion->pd_r1_lb]);
>-				cmd_fusion->scmd->SCp.Status &=
>-					~MEGASAS_LOAD_BALANCE_FLAG;
>+			if (scmd_local) {
>+				device_id = MEGASAS_DEV_INDEX(scmd_local);
>+				lbinfo =
>&fusion->load_balance_info[device_id];
>+				if (cmd_fusion->scmd->SCp.Status &
>+				    MEGASAS_LOAD_BALANCE_FLAG) {
>+
>atomic_dec(&lbinfo->scsi_pending_cmds[cmd_fusion->pd_r1_lb]);
>+					cmd_fusion->scmd->SCp.Status &=
>+
>~MEGASAS_LOAD_BALANCE_FLAG;
>+				}
> 			}
> 			if (reply_descript_type ==
> 			    MPI2_RPY_DESCRIPT_FLAGS_SCSI_IO_SUCCESS) { @@
>-2315,6 +2317,12 @@ static void megasas_build_ld_nonrw_fusion(struct
>megasas_instance *instance,
> 			/* Fall thru and complete IO */
> 		case MEGASAS_MPI2_FUNCTION_LD_IO_REQUEST: /* LD-IO
>Path */
> 			/* Map the FW Cmd Status */
>+			if (!scmd_local) {
>+				dev_err(&instance->pdev->dev,
>+					"cmd[%d:%d] already completed\n",
>+					MSIxIndex, smid);
>+				break;
>+			}
> 			map_cmd_status(cmd_fusion, status, extStatus);
> 			scsi_io_req->RaidContext.status = 0;
> 			scsi_io_req->RaidContext.exStatus = 0;
>--
>1.8.5.6
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of
>a message to majordomo@vger.kernel.org More majordomo info at
>http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] megaraid_sas: do not crash on invalid completion

[Hannes Reinecke]

On 11/11/2016 12:51 PM, Sumit Saxena wrote:
>> -----Original Message-----
>> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
>> owner@vger.kernel.org] On Behalf Of Hannes Reinecke
>> Sent: Friday, November 11, 2016 3:15 PM
>> To: Martin K. Petersen
>> Cc: Christoph Hellwig; James Bottomley; Sumit Saxena; linux-
>> scsi@vger.kernel.org; Hannes Reinecke; Hannes Reinecke
>> Subject: [PATCH 3/5] megaraid_sas: do not crash on invalid completion
>>
>> Avoid a kernel oops when receiving an invalid command completion.
> scmd_local set to NULL(for cases MPI2_FUNCTION_SCSI_IO_REQUEST and
> MEGASAS_MPI2_FUNCTION_LD_IO_REQUEST) will be serious bug(either in driver
> or firmware) which should be debugged
> and driver should not really continue beyond that. This indicates that
> driver internal frames are corrupted. If needed, whenever driver detects
> it, it can mark the adapter as dead(stopping further activities).
> If OS is installed behind megasas controller then after declaring adapter
> dead, system reboot will be required. Kernel panic may give here more
> information whenever this condition hits so we kept it like this.
> If you are facing this issue, please share the details. I will work on
> this.
>

I have come across this problem when developing scsi-mq support. Due to 
the missing mmio barrier when writing to the inbound queue port the I/O 
submission became confused, resulting in already completed frames on the 
completion queue.
While I do agree this is a pretty serious problem, the driver should 
_not_ crash; after all, it just received a completion for an unknown 
command. No reason to take the kernel down.
I'd be in favour of resetting the HBA and taking it offline if required, 
but we really should not crash here.

Incidentally, we _will_ take the HBA offline even now, as these invalid 
command completions causes a scsi timeout for the original command, and 
as the HBA couldn't send completions for them the driver would 
eventually offline the card. So it worked as expected from my POV.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

[PATCH 4/5] megaraid_sas: scsi-mq support

[Hannes Reinecke]

This patch enables full scsi multiqueue support. But as I have
been seeing performance regressions I've also added a module
parameter 'use_blk_mq', allowing multiqueue to be switched
off if required.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/megaraid/megaraid_sas_base.c   | 22 +++++++++++++++++
 drivers/scsi/megaraid/megaraid_sas_fusion.c | 38 +++++++++++++++++++++--------
 2 files changed, 50 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index d580406..1af47e6 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -48,6 +48,7 @@
 #include <linux/blkdev.h>
 #include <linux/mutex.h>
 #include <linux/poll.h>
+#include <linux/blk-mq-pci.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
@@ -104,6 +105,9 @@
 module_param(scmd_timeout, int, S_IRUGO);
 MODULE_PARM_DESC(scmd_timeout, "scsi command timeout (10-90s), default 90s. See megasas_reset_timer.");
 
+bool use_blk_mq = true;
+module_param_named(use_blk_mq, use_blk_mq, bool, 0);
+
 MODULE_LICENSE("GPL");
 MODULE_VERSION(MEGASAS_VERSION);
 MODULE_AUTHOR("megaraidlinux.pdl@avagotech.com");
@@ -1882,6 +1886,17 @@ static void megasas_slave_destroy(struct scsi_device *sdev)
 	sdev->hostdata = NULL;
 }
 
+static int megasas_map_queues(struct Scsi_Host *shost)
+{
+	struct megasas_instance *instance = (struct megasas_instance *)
+		shost->hostdata;
+
+	if (!instance->ctrl_context)
+		return 0;
+
+	return blk_mq_pci_map_queues(&shost->tag_set, instance->pdev, 0);
+}
+
 /*
 * megasas_complete_outstanding_ioctls - Complete outstanding ioctls after a
 *                                       kill adapter
@@ -2986,6 +3001,7 @@ struct device_attribute *megaraid_host_attrs[] = {
 	.slave_configure = megasas_slave_configure,
 	.slave_alloc = megasas_slave_alloc,
 	.slave_destroy = megasas_slave_destroy,
+	.map_queues = megasas_map_queues,
 	.queuecommand = megasas_queue_command,
 	.eh_target_reset_handler = megasas_reset_target,
 	.eh_abort_handler = megasas_task_abort,
@@ -5610,6 +5626,12 @@ static int megasas_io_attach(struct megasas_instance *instance)
 	host->max_id = MEGASAS_MAX_DEV_PER_CHANNEL;
 	host->max_lun = MEGASAS_MAX_LUN;
 	host->max_cmd_len = 16;
+	host->nr_hw_queues = instance->msix_vectors ?
+		instance->msix_vectors : 1;
+	if (use_blk_mq) {
+		host->can_queue = instance->max_scsi_cmds / host->nr_hw_queues;
+		host->use_blk_mq = 1;
+	}
 
 	/*
 	 * Notify the mid-layer about the new controller
diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index eb3cb0f..aba53c0 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -1703,6 +1703,7 @@ static int megasas_create_sg_sense_fusion(struct megasas_instance *instance)
 	struct IO_REQUEST_INFO io_info;
 	struct fusion_context *fusion;
 	struct MR_DRV_RAID_MAP_ALL *local_map_ptr;
+	u16 msix_index;
 	u8 *raidLUN;
 
 	device_id = MEGASAS_DEV_INDEX(scp);
@@ -1792,11 +1793,13 @@ static int megasas_create_sg_sense_fusion(struct megasas_instance *instance)
 			fp_possible = io_info.fpOkForIo;
 	}
 
-	/* Use raw_smp_processor_id() for now until cmd->request->cpu is CPU
-	   id by default, not CPU group id, otherwise all MSI-X queues won't
-	   be utilized */
-	cmd->request_desc->SCSIIO.MSIxIndex = instance->msix_vectors ?
-		raw_smp_processor_id() % instance->msix_vectors : 0;
+	if (shost_use_blk_mq(instance->host)) {
+		u32 blk_tag = blk_mq_unique_tag(scp->request);
+		msix_index = blk_mq_unique_tag_to_hwq(blk_tag);
+	} else
+		msix_index = instance->msix_vectors ?
+			raw_smp_processor_id() % instance->msix_vectors : 0;
+	cmd->request_desc->SCSIIO.MSIxIndex = msix_index;
 
 	if (fp_possible) {
 		megasas_set_pd_lba(io_request, scp->cmd_len, &io_info, scp,
@@ -1971,6 +1974,7 @@ static void megasas_build_ld_nonrw_fusion(struct megasas_instance *instance,
 	struct RAID_CONTEXT	*pRAID_Context;
 	struct MR_PD_CFG_SEQ_NUM_SYNC *pd_sync;
 	struct fusion_context *fusion = instance->ctrl_context;
+	u16 msix_index;
 	pd_sync = (void *)fusion->pd_seq_sync[(instance->pd_seq_map_id - 1) & 1];
 
 	device_id = MEGASAS_DEV_INDEX(scmd);
@@ -2013,11 +2017,14 @@ static void megasas_build_ld_nonrw_fusion(struct megasas_instance *instance,
 		io_request->DevHandle = cpu_to_le16(0xFFFF);
 	}
 
+	if (shost_use_blk_mq(instance->host)) {
+		u32 blk_tag = blk_mq_unique_tag(scmd->request);
+		msix_index = blk_mq_unique_tag_to_hwq(blk_tag);
+	} else
+		msix_index = instance->msix_vectors ?
+			(raw_smp_processor_id() % instance->msix_vectors) : 0;
+	cmd->request_desc->SCSIIO.MSIxIndex = msix_index;
 	cmd->request_desc->SCSIIO.DevHandle = io_request->DevHandle;
-	cmd->request_desc->SCSIIO.MSIxIndex =
-		instance->msix_vectors ?
-		(raw_smp_processor_id() % instance->msix_vectors) : 0;
-
 
 	if (!fp_possible) {
 		/* system pd firmware path */
@@ -2188,7 +2195,18 @@ static void megasas_build_ld_nonrw_fusion(struct megasas_instance *instance,
 		return SCSI_MLQUEUE_DEVICE_BUSY;
 	}
 
-	cmd = megasas_get_cmd_fusion(instance, scmd->request->tag);
+	if (shost_use_blk_mq(instance->host)) {
+		int msix_vectors, hwq_size;
+		u32 blk_tag = blk_mq_unique_tag(scmd->request);
+		u16 hwq = blk_mq_unique_tag_to_hwq(blk_tag);
+		u16 tag = blk_mq_unique_tag_to_tag(blk_tag);
+
+		msix_vectors = instance->msix_vectors ?
+			instance->msix_vectors : 1;
+		hwq_size = instance->max_scsi_cmds / msix_vectors;
+		cmd = megasas_get_cmd_fusion(instance, (hwq * hwq_size) + tag);
+	} else
+		cmd = megasas_get_cmd_fusion(instance, scmd->request->tag);
 
 	index = cmd->index;
 
-- 
1.8.5.6

Re: [PATCH 4/5] megaraid_sas: scsi-mq support

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1665 bytes --]

Hi Hannes,

[auto build test ERROR on scsi/for-next]
[also build test ERROR on v4.9-rc4 next-20161111]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hannes-Reinecke/megaraid_sas-scsi-mq-support/20161111-180101
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: x86_64-kexec (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/scsi/megaraid/megaraid_sas_base.c: In function 'megasas_map_queues':
>> drivers/scsi/megaraid/megaraid_sas_base.c:1891:9: error: too many arguments to function 'blk_mq_pci_map_queues'
     return blk_mq_pci_map_queues(&shost->tag_set, instance->pdev, 0);
            ^~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/scsi/megaraid/megaraid_sas_base.c:51:0:
   include/linux/blk-mq-pci.h:7:5: note: declared here
    int blk_mq_pci_map_queues(struct blk_mq_tag_set *set, struct pci_dev *pdev);
        ^~~~~~~~~~~~~~~~~~~~~

vim +/blk_mq_pci_map_queues +1891 drivers/scsi/megaraid/megaraid_sas_base.c

  1885		struct megasas_instance *instance = (struct megasas_instance *)
  1886			shost->hostdata;
  1887	
  1888		if (!instance->ctrl_context)
  1889			return 0;
  1890	
> 1891		return blk_mq_pci_map_queues(&shost->tag_set, instance->pdev, 0);
  1892	}
  1893	
  1894	/*

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24225 bytes --]

RE: [PATCH 4/5] megaraid_sas: scsi-mq support

[Sumit Saxena]

>-----Original Message-----
>From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
>owner@vger.kernel.org] On Behalf Of Hannes Reinecke
>Sent: Friday, November 11, 2016 3:15 PM
>To: Martin K. Petersen
>Cc: Christoph Hellwig; James Bottomley; Sumit Saxena; linux-
>scsi@vger.kernel.org; Hannes Reinecke; Hannes Reinecke
>Subject: [PATCH 4/5] megaraid_sas: scsi-mq support
>
>This patch enables full scsi multiqueue support. But as I have been
seeing
>performance regressions I've also added a module parameter 'use_blk_mq',
>allowing multiqueue to be switched off if required.

I may need sometime to comment on this patch. I will have some performance
runs with this patch. Can you share your test configuration details?

Thanks,
Sumit
>
>Signed-off-by: Hannes Reinecke <hare@suse.com>
>---
> drivers/scsi/megaraid/megaraid_sas_base.c   | 22 +++++++++++++++++
> drivers/scsi/megaraid/megaraid_sas_fusion.c | 38
>+++++++++++++++++++++--------
> 2 files changed, 50 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
>b/drivers/scsi/megaraid/megaraid_sas_base.c
>index d580406..1af47e6 100644
>--- a/drivers/scsi/megaraid/megaraid_sas_base.c
>+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
>@@ -48,6 +48,7 @@
> #include <linux/blkdev.h>
> #include <linux/mutex.h>
> #include <linux/poll.h>
>+#include <linux/blk-mq-pci.h>
>
> #include <scsi/scsi.h>
> #include <scsi/scsi_cmnd.h>
>@@ -104,6 +105,9 @@
> module_param(scmd_timeout, int, S_IRUGO);
>MODULE_PARM_DESC(scmd_timeout, "scsi command timeout (10-90s), default
>90s. See megasas_reset_timer.");
>
>+bool use_blk_mq = true;
>+module_param_named(use_blk_mq, use_blk_mq, bool, 0);
>+
> MODULE_LICENSE("GPL");
> MODULE_VERSION(MEGASAS_VERSION);
> MODULE_AUTHOR("megaraidlinux.pdl@avagotech.com");
>@@ -1882,6 +1886,17 @@ static void megasas_slave_destroy(struct
scsi_device
>*sdev)
> 	sdev->hostdata = NULL;
> }
>
>+static int megasas_map_queues(struct Scsi_Host *shost) {
>+	struct megasas_instance *instance = (struct megasas_instance *)
>+		shost->hostdata;
>+
>+	if (!instance->ctrl_context)
>+		return 0;
>+
>+	return blk_mq_pci_map_queues(&shost->tag_set, instance->pdev, 0);
}
>+
> /*
> * megasas_complete_outstanding_ioctls - Complete outstanding ioctls
after a
> *                                       kill adapter
>@@ -2986,6 +3001,7 @@ struct device_attribute *megaraid_host_attrs[] = {
> 	.slave_configure = megasas_slave_configure,
> 	.slave_alloc = megasas_slave_alloc,
> 	.slave_destroy = megasas_slave_destroy,
>+	.map_queues = megasas_map_queues,
> 	.queuecommand = megasas_queue_command,
> 	.eh_target_reset_handler = megasas_reset_target,
> 	.eh_abort_handler = megasas_task_abort, @@ -5610,6 +5626,12 @@
>static int megasas_io_attach(struct megasas_instance *instance)
> 	host->max_id = MEGASAS_MAX_DEV_PER_CHANNEL;
> 	host->max_lun = MEGASAS_MAX_LUN;
> 	host->max_cmd_len = 16;
>+	host->nr_hw_queues = instance->msix_vectors ?
>+		instance->msix_vectors : 1;
>+	if (use_blk_mq) {
>+		host->can_queue = instance->max_scsi_cmds /
>host->nr_hw_queues;
>+		host->use_blk_mq = 1;
>+	}
>
> 	/*
> 	 * Notify the mid-layer about the new controller diff --git
>a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>index eb3cb0f..aba53c0 100644
>--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>@@ -1703,6 +1703,7 @@ static int megasas_create_sg_sense_fusion(struct
>megasas_instance *instance)
> 	struct IO_REQUEST_INFO io_info;
> 	struct fusion_context *fusion;
> 	struct MR_DRV_RAID_MAP_ALL *local_map_ptr;
>+	u16 msix_index;
> 	u8 *raidLUN;
>
> 	device_id = MEGASAS_DEV_INDEX(scp);
>@@ -1792,11 +1793,13 @@ static int megasas_create_sg_sense_fusion(struct
>megasas_instance *instance)
> 			fp_possible = io_info.fpOkForIo;
> 	}
>
>-	/* Use raw_smp_processor_id() for now until cmd->request->cpu is
>CPU
>-	   id by default, not CPU group id, otherwise all MSI-X queues
>won't
>-	   be utilized */
>-	cmd->request_desc->SCSIIO.MSIxIndex = instance->msix_vectors ?
>-		raw_smp_processor_id() % instance->msix_vectors : 0;
>+	if (shost_use_blk_mq(instance->host)) {
>+		u32 blk_tag = blk_mq_unique_tag(scp->request);
>+		msix_index = blk_mq_unique_tag_to_hwq(blk_tag);
>+	} else
>+		msix_index = instance->msix_vectors ?
>+			raw_smp_processor_id() % instance->msix_vectors :
>0;
>+	cmd->request_desc->SCSIIO.MSIxIndex = msix_index;
>
> 	if (fp_possible) {
> 		megasas_set_pd_lba(io_request, scp->cmd_len, &io_info,
scp,
>@@ -1971,6 +1974,7 @@ static void megasas_build_ld_nonrw_fusion(struct
>megasas_instance *instance,
> 	struct RAID_CONTEXT	*pRAID_Context;
> 	struct MR_PD_CFG_SEQ_NUM_SYNC *pd_sync;
> 	struct fusion_context *fusion = instance->ctrl_context;
>+	u16 msix_index;
> 	pd_sync = (void *)fusion->pd_seq_sync[(instance->pd_seq_map_id -
>1) & 1];
>
> 	device_id = MEGASAS_DEV_INDEX(scmd);
>@@ -2013,11 +2017,14 @@ static void megasas_build_ld_nonrw_fusion(struct
>megasas_instance *instance,
> 		io_request->DevHandle = cpu_to_le16(0xFFFF);
> 	}
>
>+	if (shost_use_blk_mq(instance->host)) {
>+		u32 blk_tag = blk_mq_unique_tag(scmd->request);
>+		msix_index = blk_mq_unique_tag_to_hwq(blk_tag);
>+	} else
>+		msix_index = instance->msix_vectors ?
>+			(raw_smp_processor_id() % instance->msix_vectors)
>: 0;
>+	cmd->request_desc->SCSIIO.MSIxIndex = msix_index;
> 	cmd->request_desc->SCSIIO.DevHandle = io_request->DevHandle;
>-	cmd->request_desc->SCSIIO.MSIxIndex =
>-		instance->msix_vectors ?
>-		(raw_smp_processor_id() % instance->msix_vectors) : 0;
>-
>
> 	if (!fp_possible) {
> 		/* system pd firmware path */
>@@ -2188,7 +2195,18 @@ static void megasas_build_ld_nonrw_fusion(struct
>megasas_instance *instance,
> 		return SCSI_MLQUEUE_DEVICE_BUSY;
> 	}
>
>-	cmd = megasas_get_cmd_fusion(instance, scmd->request->tag);
>+	if (shost_use_blk_mq(instance->host)) {
>+		int msix_vectors, hwq_size;
>+		u32 blk_tag = blk_mq_unique_tag(scmd->request);
>+		u16 hwq = blk_mq_unique_tag_to_hwq(blk_tag);
>+		u16 tag = blk_mq_unique_tag_to_tag(blk_tag);
>+
>+		msix_vectors = instance->msix_vectors ?
>+			instance->msix_vectors : 1;
>+		hwq_size = instance->max_scsi_cmds / msix_vectors;
>+		cmd = megasas_get_cmd_fusion(instance, (hwq * hwq_size) +
>tag);
>+	} else
>+		cmd = megasas_get_cmd_fusion(instance,
>scmd->request->tag);
>
> 	index = cmd->index;
>
>--
>1.8.5.6
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of
>a message to majordomo@vger.kernel.org More majordomo info at
>http://vger.kernel.org/majordomo-info.html

RE: [PATCH 4/5] megaraid_sas: scsi-mq support

[Kashyap Desai]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Hannes Reinecke
> Sent: Friday, November 11, 2016 3:15 PM
> To: Martin K. Petersen
> Cc: Christoph Hellwig; James Bottomley; Sumit Saxena; linux-
> scsi@vger.kernel.org; Hannes Reinecke; Hannes Reinecke
> Subject: [PATCH 4/5] megaraid_sas: scsi-mq support
>
> This patch enables full scsi multiqueue support. But as I have been
seeing
> performance regressions I've also added a module parameter 'use_blk_mq',
> allowing multiqueue to be switched off if required.

Hannes - As discussed for hpsa driver related thread for similar support,
I don't think this code changes are helping MR as well.

>
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/megaraid/megaraid_sas_base.c   | 22 +++++++++++++++++
>  drivers/scsi/megaraid/megaraid_sas_fusion.c | 38
+++++++++++++++++++++----
> ----
>  2 files changed, 50 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
> b/drivers/scsi/megaraid/megaraid_sas_base.c
> index d580406..1af47e6 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -48,6 +48,7 @@
>  #include <linux/blkdev.h>
>  #include <linux/mutex.h>
>  #include <linux/poll.h>
> +#include <linux/blk-mq-pci.h>
>
>  #include <scsi/scsi.h>
>  #include <scsi/scsi_cmnd.h>
> @@ -104,6 +105,9 @@
>  module_param(scmd_timeout, int, S_IRUGO);
> MODULE_PARM_DESC(scmd_timeout, "scsi command timeout (10-90s), default
> 90s. See megasas_reset_timer.");
>
> +bool use_blk_mq = true;
> +module_param_named(use_blk_mq, use_blk_mq, bool, 0);
> +
>  MODULE_LICENSE("GPL");
>  MODULE_VERSION(MEGASAS_VERSION);
>  MODULE_AUTHOR("megaraidlinux.pdl@avagotech.com");
> @@ -1882,6 +1886,17 @@ static void megasas_slave_destroy(struct
> scsi_device *sdev)
>  	sdev->hostdata = NULL;
>  }
>
> +static int megasas_map_queues(struct Scsi_Host *shost) {
> +	struct megasas_instance *instance = (struct megasas_instance *)
> +		shost->hostdata;
> +
> +	if (!instance->ctrl_context)
> +		return 0;
> +
> +	return blk_mq_pci_map_queues(&shost->tag_set, instance->pdev, 0);
}
> +
>  /*
>  * megasas_complete_outstanding_ioctls - Complete outstanding ioctls
after a
>  *                                       kill adapter
> @@ -2986,6 +3001,7 @@ struct device_attribute *megaraid_host_attrs[] = {
>  	.slave_configure = megasas_slave_configure,
>  	.slave_alloc = megasas_slave_alloc,
>  	.slave_destroy = megasas_slave_destroy,
> +	.map_queues = megasas_map_queues,
>  	.queuecommand = megasas_queue_command,
>  	.eh_target_reset_handler = megasas_reset_target,
>  	.eh_abort_handler = megasas_task_abort, @@ -5610,6 +5626,12 @@
> static int megasas_io_attach(struct megasas_instance *instance)
>  	host->max_id = MEGASAS_MAX_DEV_PER_CHANNEL;
>  	host->max_lun = MEGASAS_MAX_LUN;
>  	host->max_cmd_len = 16;
> +	host->nr_hw_queues = instance->msix_vectors ?
> +		instance->msix_vectors : 1;
> +	if (use_blk_mq) {
> +		host->can_queue = instance->max_scsi_cmds / host-
> >nr_hw_queues;
> +		host->use_blk_mq = 1;
> +	}
>
>  	/*
>  	 * Notify the mid-layer about the new controller diff --git
> a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> index eb3cb0f..aba53c0 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> @@ -1703,6 +1703,7 @@ static int megasas_create_sg_sense_fusion(struct
> megasas_instance *instance)
>  	struct IO_REQUEST_INFO io_info;
>  	struct fusion_context *fusion;
>  	struct MR_DRV_RAID_MAP_ALL *local_map_ptr;
> +	u16 msix_index;
>  	u8 *raidLUN;
>
>  	device_id = MEGASAS_DEV_INDEX(scp);
> @@ -1792,11 +1793,13 @@ static int megasas_create_sg_sense_fusion(struct
> megasas_instance *instance)
>  			fp_possible = io_info.fpOkForIo;
>  	}
>
> -	/* Use raw_smp_processor_id() for now until cmd->request->cpu is
CPU
> -	   id by default, not CPU group id, otherwise all MSI-X queues
won't
> -	   be utilized */
> -	cmd->request_desc->SCSIIO.MSIxIndex = instance->msix_vectors ?
> -		raw_smp_processor_id() % instance->msix_vectors : 0;
> +	if (shost_use_blk_mq(instance->host)) {
> +		u32 blk_tag = blk_mq_unique_tag(scp->request);
> +		msix_index = blk_mq_unique_tag_to_hwq(blk_tag);
> +	} else
> +		msix_index = instance->msix_vectors ?
> +			raw_smp_processor_id() % instance->msix_vectors :
0;
> +	cmd->request_desc->SCSIIO.MSIxIndex = msix_index;
>
>  	if (fp_possible) {
>  		megasas_set_pd_lba(io_request, scp->cmd_len, &io_info,
scp,
> @@ -1971,6 +1974,7 @@ static void megasas_build_ld_nonrw_fusion(struct
> megasas_instance *instance,
>  	struct RAID_CONTEXT	*pRAID_Context;
>  	struct MR_PD_CFG_SEQ_NUM_SYNC *pd_sync;
>  	struct fusion_context *fusion = instance->ctrl_context;
> +	u16 msix_index;
>  	pd_sync = (void *)fusion->pd_seq_sync[(instance->pd_seq_map_id -
1)
> & 1];
>
>  	device_id = MEGASAS_DEV_INDEX(scmd);
> @@ -2013,11 +2017,14 @@ static void megasas_build_ld_nonrw_fusion(struct
> megasas_instance *instance,
>  		io_request->DevHandle = cpu_to_le16(0xFFFF);
>  	}
>
> +	if (shost_use_blk_mq(instance->host)) {
> +		u32 blk_tag = blk_mq_unique_tag(scmd->request);
> +		msix_index = blk_mq_unique_tag_to_hwq(blk_tag);
> +	} else
> +		msix_index = instance->msix_vectors ?
> +			(raw_smp_processor_id() % instance->msix_vectors)
:
> 0;
> +	cmd->request_desc->SCSIIO.MSIxIndex = msix_index;
>  	cmd->request_desc->SCSIIO.DevHandle = io_request->DevHandle;
> -	cmd->request_desc->SCSIIO.MSIxIndex =
> -		instance->msix_vectors ?
> -		(raw_smp_processor_id() % instance->msix_vectors) : 0;
> -
>
>  	if (!fp_possible) {
>  		/* system pd firmware path */
> @@ -2188,7 +2195,18 @@ static void megasas_build_ld_nonrw_fusion(struct
> megasas_instance *instance,
>  		return SCSI_MLQUEUE_DEVICE_BUSY;
>  	}
>
> -	cmd = megasas_get_cmd_fusion(instance, scmd->request->tag);
> +	if (shost_use_blk_mq(instance->host)) {
> +		int msix_vectors, hwq_size;
> +		u32 blk_tag = blk_mq_unique_tag(scmd->request);
> +		u16 hwq = blk_mq_unique_tag_to_hwq(blk_tag);
> +		u16 tag = blk_mq_unique_tag_to_tag(blk_tag);
> +
> +		msix_vectors = instance->msix_vectors ?
> +			instance->msix_vectors : 1;
> +		hwq_size = instance->max_scsi_cmds / msix_vectors;
> +		cmd = megasas_get_cmd_fusion(instance, (hwq * hwq_size) +
> tag);
> +	} else
> +		cmd = megasas_get_cmd_fusion(instance, scmd->request-
> >tag);
>
>  	index = cmd->index;
>
> --
> 1.8.5.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of
> a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

[PATCH 5/5] megaraid_sas: add mmio barrier after register writes

[Hannes Reinecke]

The megaraid_sas HBA only has a single register for I/O submission,
which will be hit pretty hard with scsi-mq. To ensure that the
PCI writes have made it across we need to add a mmio barrier
after each write; otherwise I've been seeing spurious command
completions and I/O stalls.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/megaraid/megaraid_sas_fusion.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index aba53c0..729a654 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -196,6 +196,7 @@ inline void megasas_return_cmd_fusion(struct megasas_instance *instance,
 			le32_to_cpu(req_desc->u.low));
 
 	writeq(req_data, &instance->reg_set->inbound_low_queue_port);
+	mmiowb();
 #else
 	unsigned long flags;
 
-- 
1.8.5.6

RE: [PATCH 5/5] megaraid_sas: add mmio barrier after register writes

[Sumit Saxena]

>-----Original Message-----
>From: Hannes Reinecke [mailto:hare@suse.de]
>Sent: Friday, November 11, 2016 3:15 PM
>To: Martin K. Petersen
>Cc: Christoph Hellwig; James Bottomley; Sumit Saxena; linux-
>scsi@vger.kernel.org; Hannes Reinecke; Hannes Reinecke
>Subject: [PATCH 5/5] megaraid_sas: add mmio barrier after register writes
>
>The megaraid_sas HBA only has a single register for I/O submission, which
will be
>hit pretty hard with scsi-mq. To ensure that the PCI writes have made it
across we
>need to add a mmio barrier after each write; otherwise I've been seeing
spurious
>command completions and I/O stalls.
>
>Signed-off-by: Hannes Reinecke <hare@suse.com>
Acked-by: Sumit Saxena <sumit.saxena@broadcom.com>

>---
> drivers/scsi/megaraid/megaraid_sas_fusion.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>index aba53c0..729a654 100644
>--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>@@ -196,6 +196,7 @@ inline void megasas_return_cmd_fusion(struct
>megasas_instance *instance,
> 			le32_to_cpu(req_desc->u.low));
>
> 	writeq(req_data, &instance->reg_set->inbound_low_queue_port);
>+	mmiowb();
> #else
> 	unsigned long flags;
>
>--
>1.8.5.6

Re: [PATCH 5/5] megaraid_sas: add mmio barrier after register writes

[Tomas Henzl]

On 11.11.2016 10:44, Hannes Reinecke wrote:
> The megaraid_sas HBA only has a single register for I/O submission,
> which will be hit pretty hard with scsi-mq. To ensure that the
> PCI writes have made it across we need to add a mmio barrier
> after each write; otherwise I've been seeing spurious command
> completions and I/O stalls.

Why is it needed that the PCI write reaches the hw exactly at this point?
Is it possible that this is a hw deficiency like that the hw can't handle
communication without tiny pauses, and so possible to remove
in next generation?
Thanks,
Tomas

>
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/megaraid/megaraid_sas_fusion.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> index aba53c0..729a654 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> @@ -196,6 +196,7 @@ inline void megasas_return_cmd_fusion(struct megasas_instance *instance,
>  			le32_to_cpu(req_desc->u.low));
>  
>  	writeq(req_data, &instance->reg_set->inbound_low_queue_port);
> +	mmiowb();
>  #else
>  	unsigned long flags;
>  

RE: [PATCH 5/5] megaraid_sas: add mmio barrier after register writes

[Kashyap Desai]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Tomas Henzl
> Sent: Friday, November 18, 2016 9:23 PM
> To: Hannes Reinecke; Martin K. Petersen
> Cc: Christoph Hellwig; James Bottomley; Sumit Saxena; linux-
> scsi@vger.kernel.org; Hannes Reinecke
> Subject: Re: [PATCH 5/5] megaraid_sas: add mmio barrier after register
writes
>
> On 11.11.2016 10:44, Hannes Reinecke wrote:
> > The megaraid_sas HBA only has a single register for I/O submission,
> > which will be hit pretty hard with scsi-mq. To ensure that the PCI
> > writes have made it across we need to add a mmio barrier after each
> > write; otherwise I've been seeing spurious command completions and I/O
> > stalls.
>
> Why is it needed that the PCI write reaches the hw exactly at this
point?
> Is it possible that this is a hw deficiency like that the hw can't
handle
> communication without tiny pauses, and so possible to remove in next
> generation?
> Thanks,
> Tomas

I think this is good to have mmiowb as we are already doing  for writel()
version of megasas_return_cmd_fusion.
May be not for x86, but for some other CPU arch it is useful. I think it
become more evident while scs-mq support for more than one submission
queue patch of Hannes expose this issue.

Probably this patch is good. Intermediate PCI device (PCI bridge ?)  may
cache PCI packet and eventually out of order PCI packet to MegaRAID HBA
can cause this type of spurious completion.

>
> >
> > Signed-off-by: Hannes Reinecke <hare@suse.com>
> > ---
> >  drivers/scsi/megaraid/megaraid_sas_fusion.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > index aba53c0..729a654 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > @@ -196,6 +196,7 @@ inline void megasas_return_cmd_fusion(struct
> megasas_instance *instance,
> >  			le32_to_cpu(req_desc->u.low));
> >
> >  	writeq(req_data, &instance->reg_set->inbound_low_queue_port);
> > +	mmiowb();
> >  #else
> >  	unsigned long flags;
> >
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of
> a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] megaraid_sas: add mmio barrier after register writes

[Tomas Henzl]

On 18.11.2016 17:48, Kashyap Desai wrote:
>> -----Original Message-----
>> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
>> owner@vger.kernel.org] On Behalf Of Tomas Henzl
>> Sent: Friday, November 18, 2016 9:23 PM
>> To: Hannes Reinecke; Martin K. Petersen
>> Cc: Christoph Hellwig; James Bottomley; Sumit Saxena; linux-
>> scsi@vger.kernel.org; Hannes Reinecke
>> Subject: Re: [PATCH 5/5] megaraid_sas: add mmio barrier after register
> writes
>> On 11.11.2016 10:44, Hannes Reinecke wrote:
>>> The megaraid_sas HBA only has a single register for I/O submission,
>>> which will be hit pretty hard with scsi-mq. To ensure that the PCI
>>> writes have made it across we need to add a mmio barrier after each
>>> write; otherwise I've been seeing spurious command completions and I/O
>>> stalls.
>> Why is it needed that the PCI write reaches the hw exactly at this
> point?
>> Is it possible that this is a hw deficiency like that the hw can't
> handle
>> communication without tiny pauses, and so possible to remove in next
>> generation?
>> Thanks,
>> Tomas
> I think this is good to have mmiowb as we are already doing  for writel()
> version of megasas_return_cmd_fusion.
> May be not for x86, but for some other CPU arch it is useful. I think it
> become more evident while scs-mq support for more than one submission
> queue patch of Hannes expose this issue.
>
> Probably this patch is good. Intermediate PCI device (PCI bridge ?)  may
> cache PCI packet and eventually out of order PCI packet to MegaRAID HBA
> can cause this type of spurious completion.

Usually drivers do not add a write barrier after every pci write, unless
like here in megasas_fire_cmd_fusion in the 32bit part where are two paired writes
and it must be ensured that the pair arrives without any other write in between.

Why is it wrong when a pci write is overtaken by another write or when happens
a bit later and if it is wrong - don't we need an additional locking too ?
The execution of  megasas_fire_cmd_fusion might be interrupted and a delay 
can happen at any time.

tomash

>
>>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>>> ---
>>>  drivers/scsi/megaraid/megaraid_sas_fusion.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>> b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>> index aba53c0..729a654 100644
>>> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>> @@ -196,6 +196,7 @@ inline void megasas_return_cmd_fusion(struct
>> megasas_instance *instance,
>>>  			le32_to_cpu(req_desc->u.low));
>>>
>>>  	writeq(req_data, &instance->reg_set->inbound_low_queue_port);
>>> +	mmiowb();
>>>  #else
>>>  	unsigned long flags;
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of
>> a message to majordomo@vger.kernel.org More majordomo info at
>> http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 5/5] megaraid_sas: add mmio barrier after register writes

[Kashyap Desai]

> -----Original Message-----
> From: Tomas Henzl [mailto:thenzl@redhat.com]
> Sent: Monday, November 21, 2016 9:27 PM
> To: Kashyap Desai; Hannes Reinecke; Martin K. Petersen
> Cc: Christoph Hellwig; James Bottomley; Sumit Saxena; linux-
> scsi@vger.kernel.org; Hannes Reinecke
> Subject: Re: [PATCH 5/5] megaraid_sas: add mmio barrier after register
> writes
>
> On 18.11.2016 17:48, Kashyap Desai wrote:
> >> -----Original Message-----
> >> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> >> owner@vger.kernel.org] On Behalf Of Tomas Henzl
> >> Sent: Friday, November 18, 2016 9:23 PM
> >> To: Hannes Reinecke; Martin K. Petersen
> >> Cc: Christoph Hellwig; James Bottomley; Sumit Saxena; linux-
> >> scsi@vger.kernel.org; Hannes Reinecke
> >> Subject: Re: [PATCH 5/5] megaraid_sas: add mmio barrier after
> >> register
> > writes
> >> On 11.11.2016 10:44, Hannes Reinecke wrote:
> >>> The megaraid_sas HBA only has a single register for I/O submission,
> >>> which will be hit pretty hard with scsi-mq. To ensure that the PCI
> >>> writes have made it across we need to add a mmio barrier after each
> >>> write; otherwise I've been seeing spurious command completions and
> >>> I/O stalls.
> >> Why is it needed that the PCI write reaches the hw exactly at this
> > point?
> >> Is it possible that this is a hw deficiency like that the hw can't
> > handle
> >> communication without tiny pauses, and so possible to remove in next
> >> generation?
> >> Thanks,
> >> Tomas
> > I think this is good to have mmiowb as we are already doing  for
> > writel() version of megasas_return_cmd_fusion.
> > May be not for x86, but for some other CPU arch it is useful. I think
> > it become more evident while scs-mq support for more than one
> > submission queue patch of Hannes expose this issue.
> >
> > Probably this patch is good. Intermediate PCI device (PCI bridge ?)
> > may cache PCI packet and eventually out of order PCI packet to
> > MegaRAID HBA can cause this type of spurious completion.
>
> Usually drivers do not add a write barrier after every pci write, unless
> like here in
> megasas_fire_cmd_fusion in the 32bit part where are two paired writes and
> it
> must be ensured that the pair arrives without any other write in between.
>
> Why is it wrong when a pci write is overtaken by another write or when
> happens
> a bit later and if it is wrong - don't we need an additional locking too ?
> The execution of  megasas_fire_cmd_fusion might be interrupted and a delay
> can happen at any time.

Since Hannes mentioned that with his experiment of mq megaraid_sas patch and
creating more Submission queue to SML cause invalid/suprious completion in
his code, I am trying to understand if " mmiowb" after writeq() call is safe
?  My understanding is "writeq" is atomic and it will have two 32bit PCI
WRITE in same sequence reaching to PCI end device. Assuming there will not
be PCI level caching on Intel x86 platform.  E.g if we have two CPU
executing writeq(), PCI write will always reach to end device in same
sequence. Assume ...Tag-1, Tag-2, Tag-3 and Tag-4 is expected sequence.  In
case of any optimization at system level, if device see Tag-1, Tag-3, Tag-2,
Tag-4 arrives,  then we may see the issue as Hannes experience.  We have
experience very rare instance of dual/spurious SMID completion  on released
megaraid_sas driver and not easy to be reproduced...so thinking on those
line, is this easy to reproduce such issue opening more submission queue to
SML (just to reproduce spurious completion effectively). We will apply all
the patches posted by Hannes *just* to understand this particular spurious
completion issue and understand in what condition it will impact.

We will post if this mmiowb() call after writeq() is good to have.

~ Kashyap

>
> tomash
>
> >
> >>> Signed-off-by: Hannes Reinecke <hare@suse.com>
> >>> ---
> >>>  drivers/scsi/megaraid/megaraid_sas_fusion.c | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> >>> b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> >>> index aba53c0..729a654 100644
> >>> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> >>> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> >>> @@ -196,6 +196,7 @@ inline void megasas_return_cmd_fusion(struct
> >> megasas_instance *instance,
> >>>  			le32_to_cpu(req_desc->u.low));
> >>>
> >>>  	writeq(req_data, &instance->reg_set->inbound_low_queue_port);
> >>> +	mmiowb();
> >>>  #else
> >>>  	unsigned long flags;
> >>>
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-scsi"
> >> in
> > the body of
> >> a message to majordomo@vger.kernel.org More majordomo info at
> >> http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-scsi"
> > in the body of a message to majordomo@vger.kernel.org More majordomo
> > info at  http://vger.kernel.org/majordomo-info.html
>