[PATCH v1 01/20] [SCSI] mpt3sas: Added Combined Reply Queue feature to extend up-to 96 MSIX vector support

[PATCH v1 01/20] [SCSI] mpt3sas: Added Combined Reply Queue feature to extend up-to 96 MSIX vector support

[Sreekanth Reddy]

In this patch, increased the number of MSIX vector support for SAS3 C0 HBAs to up-to 96.

Following are changes that are done in this patch
1. Support this feature only for SAS3 C0 cards and also only when reply post free queue count is greater than 8.
2. Instead of using single ReplyPostHostIndex system interface, here 12 ReplyPostHostIndex system interfaces are used. reply post free queues numbered from 0 to 7 use the first ReplyPostHostIndex system interface to update its corresponding ReplyPostHostIndex values, reply post free queues numbered from 8 to 15 will use the second ReplyPostHostIndex system interface and so on. These 12 ReplyPostHostIndex system interfaces address are saved in the array replyPostRegisterIndex[].
3. Update the ReplyPostHostIndex value of corresponding reply post free queue in the (its msix_index/8)th entry of replyPostRegisterIndex[] array after processing the reply post descriptor.

Changes in v1:
 Updated the description of module parameter max_msix_vectors

Signed-off-by: Sreekanth Reddy <Sreekanth.Reddy@avagotech.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 72 ++++++++++++++++++++++++++++++++-----
 drivers/scsi/mpt3sas/mpt3sas_base.h |  7 +++-
 2 files changed, 70 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 14a781b..7d0ec5c 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -83,10 +83,10 @@ static int msix_disable = -1;
 module_param(msix_disable, int, 0);
 MODULE_PARM_DESC(msix_disable, " disable msix routed interrupts (default=0)");
 
-static int max_msix_vectors = 8;
+static int max_msix_vectors = -1;
 module_param(max_msix_vectors, int, 0);
 MODULE_PARM_DESC(max_msix_vectors,
-	" max msix vectors - (default=8)");
+	" max msix vectors");
 
 static int mpt3sas_fwfault_debug;
 MODULE_PARM_DESC(mpt3sas_fwfault_debug,
@@ -1009,8 +1009,15 @@ _base_interrupt(int irq, void *bus_id)
 	}
 
 	wmb();
-	writel(reply_q->reply_post_host_index | (msix_index <<
-	    MPI2_RPHI_MSIX_INDEX_SHIFT), &ioc->chip->ReplyPostHostIndex);
+	if (ioc->msix96_vector) {
+		writel(reply_q->reply_post_host_index | ((msix_index  & 7) <<
+			MPI2_RPHI_MSIX_INDEX_SHIFT),
+				ioc->replyPostRegisterIndex[msix_index/8]);
+	} else {
+		writel(reply_q->reply_post_host_index | (msix_index <<
+			MPI2_RPHI_MSIX_INDEX_SHIFT),
+				&ioc->chip->ReplyPostHostIndex);
+	}
 	atomic_dec(&reply_q->busy);
 	return IRQ_HANDLED;
 }
@@ -1560,8 +1567,6 @@ _base_check_enable_msix(struct MPT3SAS_ADAPTER *ioc)
 
 	pci_read_config_word(ioc->pdev, base + 2, &message_control);
 	ioc->msix_vector_count = (message_control & 0x3FF) + 1;
-	if (ioc->msix_vector_count > 8)
-		ioc->msix_vector_count = 8;
 	dinitprintk(ioc, pr_info(MPT3SAS_FMT
 		"msix is supported, vector_count(%d)\n",
 		ioc->name, ioc->msix_vector_count));
@@ -1880,6 +1885,31 @@ mpt3sas_base_map_resources(struct MPT3SAS_ADAPTER *ioc)
 	if (r)
 		goto out_fail;
 
+	/* Use the Combined reply queue feature only for SAS3 C0 HBAs and
+	 * also only when reply queue count is greater than 8
+	 */
+	if (ioc->msix96_vector && ioc->reply_queue_count > 8) {
+		/* If this is an 96 vector supported device,
+		set up ReplyPostIndex addresses */
+		ioc->replyPostRegisterIndex = kcalloc(12,
+			sizeof(resource_size_t *), GFP_KERNEL);
+		if (!ioc->replyPostRegisterIndex) {
+			dfailprintk(ioc, printk(MPT3SAS_FMT
+			"allocation for reply Post Register Index failed!!!\n",
+								   ioc->name));
+			r = -ENOMEM;
+			goto out_fail;
+		}
+
+		for (i = 0; i < 12; i++) {
+			ioc->replyPostRegisterIndex[i] = (resource_size_t *)
+				((u8 *)&ioc->chip->Doorbell +
+				MPI25_SUP_REPLY_POST_HOST_INDEX_OFFSET +
+				(i * 0x10));
+		}
+	} else
+		ioc->msix96_vector = 0;
+
 	list_for_each_entry(reply_q, &ioc->reply_queue_list, list)
 		pr_info(MPT3SAS_FMT "%s: IRQ %d\n",
 		    reply_q->name,  ((ioc->msix_enable) ? "PCI-MSI-X enabled" :
@@ -1901,6 +1931,8 @@ mpt3sas_base_map_resources(struct MPT3SAS_ADAPTER *ioc)
 	pci_release_selected_regions(ioc->pdev, ioc->bars);
 	pci_disable_pcie_error_reporting(pdev);
 	pci_disable_device(pdev);
+	if (ioc->msix96_vector)
+		kfree(ioc->replyPostRegisterIndex);
 	return r;
 }
 
@@ -4522,8 +4554,16 @@ _base_make_ioc_operational(struct MPT3SAS_ADAPTER *ioc, int sleep_flag)
 
 	/* initialize reply post host index */
 	list_for_each_entry(reply_q, &ioc->reply_queue_list, list) {
-		writel(reply_q->msix_index << MPI2_RPHI_MSIX_INDEX_SHIFT,
-		    &ioc->chip->ReplyPostHostIndex);
+		if (ioc->msix96_vector) {
+			writel((reply_q->msix_index & 7)<<
+			   MPI2_RPHI_MSIX_INDEX_SHIFT,
+			   ioc->replyPostRegisterIndex[reply_q->msix_index/8]);
+		} else {
+			writel(reply_q->msix_index <<
+				MPI2_RPHI_MSIX_INDEX_SHIFT,
+					&ioc->chip->ReplyPostHostIndex);
+		}
+
 		if (!_base_is_controller_msix_enabled(ioc))
 			goto skip_init_reply_post_host_index;
 	}
@@ -4577,6 +4617,9 @@ mpt3sas_base_free_resources(struct MPT3SAS_ADAPTER *ioc)
 	_base_free_irq(ioc);
 	_base_disable_msix(ioc);
 
+	if (ioc->msix96_vector)
+		kfree(ioc->replyPostRegisterIndex);
+
 	if (ioc->chip_phys && ioc->chip)
 		iounmap(ioc->chip);
 	ioc->chip_phys = 0;
@@ -4600,6 +4643,7 @@ mpt3sas_base_attach(struct MPT3SAS_ADAPTER *ioc)
 {
 	int r, i;
 	int cpu_id, last_cpu_id = 0;
+	u8 revision;
 
 	dinitprintk(ioc, pr_info(MPT3SAS_FMT "%s\n", ioc->name,
 	    __func__));
@@ -4618,6 +4662,18 @@ mpt3sas_base_attach(struct MPT3SAS_ADAPTER *ioc)
 		r = -ENOMEM;
 		goto out_free_resources;
 	}
+	/* Check whether the controller revision is C0 or above.
+	    C0 and above revision controllers support 96 vectors */
+	revision = ioc->pdev->revision;
+
+	if ((ioc->pdev->device == MPI25_MFGPAGE_DEVID_SAS3004 ||
+	     ioc->pdev->device == MPI25_MFGPAGE_DEVID_SAS3008 ||
+	     ioc->pdev->device == MPI25_MFGPAGE_DEVID_SAS3108_1 ||
+	     ioc->pdev->device == MPI25_MFGPAGE_DEVID_SAS3108_2 ||
+	     ioc->pdev->device == MPI25_MFGPAGE_DEVID_SAS3108_5 ||
+	     ioc->pdev->device == MPI25_MFGPAGE_DEVID_SAS3108_6) &&
+	     (revision >= 0x02))
+		ioc->msix96_vector = 1;
 
 	ioc->rdpq_array_enable_assigned = 0;
 	ioc->dma_mask = 0;
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h
index afa8816..6b8d8f1 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -728,7 +728,8 @@ typedef void (*MPT3SAS_FLUSH_RUNNING_CMDS)(struct MPT3SAS_ADAPTER *ioc);
  *				is assigned only ones
  * @reply_queue_count: number of reply queue's
  * @reply_queue_list: link list contaning the reply queue info
- * @reply_post_host_index: head index in the pool where FW completes IO
+ * @msix96_vector: 96 MSI-X vector support
+ * @replyPostRegisterIndex: index of next position in Reply Desc Post Queue
  * @delayed_tr_list: target reset link list
  * @delayed_tr_volume_list: volume target reset link list
  * @@temp_sensors_count: flag to carry the number of temperature sensors
@@ -937,6 +938,10 @@ struct MPT3SAS_ADAPTER {
 	u8		reply_queue_count;
 	struct list_head reply_queue_list;
 
+	u8		msix96_vector;
+	/* reply post register index */
+	resource_size_t	**replyPostRegisterIndex;
+
 	struct list_head delayed_tr_list;
 	struct list_head delayed_tr_volume_list;
 	u8		temp_sensors_count;
-- 
2.0.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 01/20] [SCSI] mpt3sas: Added Combined Reply Queue feature to extend up-to 96 MSIX vector support

[Johannes Thumshirn]

On Fri, Jun 19, 2015 at 04:25:46PM +0530, Sreekanth Reddy wrote:
> In this patch, increased the number of MSIX vector support for SAS3 C0 HBAs to up-to 96.
> 
> Following are changes that are done in this patch
> 1. Support this feature only for SAS3 C0 cards and also only when reply post free queue count is greater than 8.
> 2. Instead of using single ReplyPostHostIndex system interface, here 12 ReplyPostHostIndex system interfaces are used. reply post free queues numbered from 0 to 7 use the first ReplyPostHostIndex system interface to update its corresponding ReplyPostHostIndex values, reply post free queues numbered from 8 to 15 will use the second ReplyPostHostIndex system interface and so on. These 12 ReplyPostHostIndex system interfaces address are saved in the array replyPostRegisterIndex[].
> 3. Update the ReplyPostHostIndex value of corresponding reply post free queue in the (its msix_index/8)th entry of replyPostRegisterIndex[] array after processing the reply post descriptor.
> 
> Changes in v1:
>  Updated the description of module parameter max_msix_vectors
> 
> Signed-off-by: Sreekanth Reddy <Sreekanth.Reddy@avagotech.com>
> ---
>  drivers/scsi/mpt3sas/mpt3sas_base.c | 72 ++++++++++++++++++++++++++++++++-----
>  drivers/scsi/mpt3sas/mpt3sas_base.h |  7 +++-
>  2 files changed, 70 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 14a781b..7d0ec5c 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -83,10 +83,10 @@ static int msix_disable = -1;
>  module_param(msix_disable, int, 0);
>  MODULE_PARM_DESC(msix_disable, " disable msix routed interrupts (default=0)");
>  
> -static int max_msix_vectors = 8;
> +static int max_msix_vectors = -1;
>  module_param(max_msix_vectors, int, 0);
>  MODULE_PARM_DESC(max_msix_vectors,
> -	" max msix vectors - (default=8)");
> +	" max msix vectors");
>  
>  static int mpt3sas_fwfault_debug;
>  MODULE_PARM_DESC(mpt3sas_fwfault_debug,
> @@ -1009,8 +1009,15 @@ _base_interrupt(int irq, void *bus_id)
>  	}
>  
>  	wmb();
> -	writel(reply_q->reply_post_host_index | (msix_index <<
> -	    MPI2_RPHI_MSIX_INDEX_SHIFT), &ioc->chip->ReplyPostHostIndex);
> +	if (ioc->msix96_vector) {
> +		writel(reply_q->reply_post_host_index | ((msix_index  & 7) <<
> +			MPI2_RPHI_MSIX_INDEX_SHIFT),
> +				ioc->replyPostRegisterIndex[msix_index/8]);
> +	} else {
> +		writel(reply_q->reply_post_host_index | (msix_index <<
> +			MPI2_RPHI_MSIX_INDEX_SHIFT),
> +				&ioc->chip->ReplyPostHostIndex);
> +	}
>  	atomic_dec(&reply_q->busy);
>  	return IRQ_HANDLED;
>  }
> @@ -1560,8 +1567,6 @@ _base_check_enable_msix(struct MPT3SAS_ADAPTER *ioc)
>  
>  	pci_read_config_word(ioc->pdev, base + 2, &message_control);
>  	ioc->msix_vector_count = (message_control & 0x3FF) + 1;
> -	if (ioc->msix_vector_count > 8)
> -		ioc->msix_vector_count = 8;
>  	dinitprintk(ioc, pr_info(MPT3SAS_FMT
>  		"msix is supported, vector_count(%d)\n",
>  		ioc->name, ioc->msix_vector_count));
> @@ -1880,6 +1885,31 @@ mpt3sas_base_map_resources(struct MPT3SAS_ADAPTER *ioc)
>  	if (r)
>  		goto out_fail;
>  
> +	/* Use the Combined reply queue feature only for SAS3 C0 HBAs and
> +	 * also only when reply queue count is greater than 8
> +	 */
> +	if (ioc->msix96_vector && ioc->reply_queue_count > 8) {
> +		/* If this is an 96 vector supported device,
> +		set up ReplyPostIndex addresses */
> +		ioc->replyPostRegisterIndex = kcalloc(12,
> +			sizeof(resource_size_t *), GFP_KERNEL);
> +		if (!ioc->replyPostRegisterIndex) {
> +			dfailprintk(ioc, printk(MPT3SAS_FMT
> +			"allocation for reply Post Register Index failed!!!\n",
> +								   ioc->name));
> +			r = -ENOMEM;
> +			goto out_fail;
> +		}
> +
> +		for (i = 0; i < 12; i++) {
> +			ioc->replyPostRegisterIndex[i] = (resource_size_t *)
> +				((u8 *)&ioc->chip->Doorbell +
> +				MPI25_SUP_REPLY_POST_HOST_INDEX_OFFSET +
> +				(i * 0x10));
> +		}
> +	} else
> +		ioc->msix96_vector = 0;
> +
>  	list_for_each_entry(reply_q, &ioc->reply_queue_list, list)
>  		pr_info(MPT3SAS_FMT "%s: IRQ %d\n",
>  		    reply_q->name,  ((ioc->msix_enable) ? "PCI-MSI-X enabled" :
> @@ -1901,6 +1931,8 @@ mpt3sas_base_map_resources(struct MPT3SAS_ADAPTER *ioc)
>  	pci_release_selected_regions(ioc->pdev, ioc->bars);
>  	pci_disable_pcie_error_reporting(pdev);
>  	pci_disable_device(pdev);
> +	if (ioc->msix96_vector)
> +		kfree(ioc->replyPostRegisterIndex);
>  	return r;
>  }
>  
> @@ -4522,8 +4554,16 @@ _base_make_ioc_operational(struct MPT3SAS_ADAPTER *ioc, int sleep_flag)
>  
>  	/* initialize reply post host index */
>  	list_for_each_entry(reply_q, &ioc->reply_queue_list, list) {
> -		writel(reply_q->msix_index << MPI2_RPHI_MSIX_INDEX_SHIFT,
> -		    &ioc->chip->ReplyPostHostIndex);
> +		if (ioc->msix96_vector) {
> +			writel((reply_q->msix_index & 7)<<
> +			   MPI2_RPHI_MSIX_INDEX_SHIFT,
> +			   ioc->replyPostRegisterIndex[reply_q->msix_index/8]);
> +		} else {
> +			writel(reply_q->msix_index <<
> +				MPI2_RPHI_MSIX_INDEX_SHIFT,
> +					&ioc->chip->ReplyPostHostIndex);
> +		}
> +
>  		if (!_base_is_controller_msix_enabled(ioc))
>  			goto skip_init_reply_post_host_index;
>  	}
> @@ -4577,6 +4617,9 @@ mpt3sas_base_free_resources(struct MPT3SAS_ADAPTER *ioc)
>  	_base_free_irq(ioc);
>  	_base_disable_msix(ioc);
>  
> +	if (ioc->msix96_vector)
> +		kfree(ioc->replyPostRegisterIndex);
> +
>  	if (ioc->chip_phys && ioc->chip)
>  		iounmap(ioc->chip);
>  	ioc->chip_phys = 0;
> @@ -4600,6 +4643,7 @@ mpt3sas_base_attach(struct MPT3SAS_ADAPTER *ioc)
>  {
>  	int r, i;
>  	int cpu_id, last_cpu_id = 0;
> +	u8 revision;
>  
>  	dinitprintk(ioc, pr_info(MPT3SAS_FMT "%s\n", ioc->name,
>  	    __func__));
> @@ -4618,6 +4662,18 @@ mpt3sas_base_attach(struct MPT3SAS_ADAPTER *ioc)
>  		r = -ENOMEM;
>  		goto out_free_resources;
>  	}
> +	/* Check whether the controller revision is C0 or above.
> +	    C0 and above revision controllers support 96 vectors */
> +	revision = ioc->pdev->revision;
> +
> +	if ((ioc->pdev->device == MPI25_MFGPAGE_DEVID_SAS3004 ||
> +	     ioc->pdev->device == MPI25_MFGPAGE_DEVID_SAS3008 ||
> +	     ioc->pdev->device == MPI25_MFGPAGE_DEVID_SAS3108_1 ||
> +	     ioc->pdev->device == MPI25_MFGPAGE_DEVID_SAS3108_2 ||
> +	     ioc->pdev->device == MPI25_MFGPAGE_DEVID_SAS3108_5 ||
> +	     ioc->pdev->device == MPI25_MFGPAGE_DEVID_SAS3108_6) &&
> +	     (revision >= 0x02))
> +		ioc->msix96_vector = 1;
>  
>  	ioc->rdpq_array_enable_assigned = 0;
>  	ioc->dma_mask = 0;
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h
> index afa8816..6b8d8f1 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.h
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
> @@ -728,7 +728,8 @@ typedef void (*MPT3SAS_FLUSH_RUNNING_CMDS)(struct MPT3SAS_ADAPTER *ioc);
>   *				is assigned only ones
>   * @reply_queue_count: number of reply queue's
>   * @reply_queue_list: link list contaning the reply queue info
> - * @reply_post_host_index: head index in the pool where FW completes IO
> + * @msix96_vector: 96 MSI-X vector support
> + * @replyPostRegisterIndex: index of next position in Reply Desc Post Queue
>   * @delayed_tr_list: target reset link list
>   * @delayed_tr_volume_list: volume target reset link list
>   * @@temp_sensors_count: flag to carry the number of temperature sensors
> @@ -937,6 +938,10 @@ struct MPT3SAS_ADAPTER {
>  	u8		reply_queue_count;
>  	struct list_head reply_queue_list;
>  
> +	u8		msix96_vector;
> +	/* reply post register index */
> +	resource_size_t	**replyPostRegisterIndex;
> +
>  	struct list_head delayed_tr_list;
>  	struct list_head delayed_tr_volume_list;
>  	u8		temp_sensors_count;
> -- 
> 2.0.2
>

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                                       Storage
jthumshirn@suse.de                             +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 01/20] [SCSI] mpt3sas: Added Combined Reply Queue feature to extend up-to 96 MSIX vector support

[Martin K. Petersen]

>>>>> "Sreekanth" == Sreekanth Reddy <sreekanth.reddy@avagotech.com> writes:

Sreekanth,

It's fine that you outline the 96 / 12 = 8 layout in the patch
description. But that relationship is not made clear when reading the
code. Please add a comment describing why things are set up this way.

> @@ -1009,8 +1009,15 @@ _base_interrupt(int irq, void *bus_id)
>  	}
>  
>  	wmb();
> -	writel(reply_q->reply_post_host_index | (msix_index <<
> -	    MPI2_RPHI_MSIX_INDEX_SHIFT), &ioc->chip->ReplyPostHostIndex);
> +	if (ioc->msix96_vector) {
> +		writel(reply_q->reply_post_host_index | ((msix_index  & 7) <<
> +			MPI2_RPHI_MSIX_INDEX_SHIFT),
> +				ioc->replyPostRegisterIndex[msix_index/8]);
> +	} else {
> +		writel(reply_q->reply_post_host_index | (msix_index <<
> +			MPI2_RPHI_MSIX_INDEX_SHIFT),
> +				&ioc->chip->ReplyPostHostIndex);
> +	}

Too many brackets. Why don't you do:

	index = reply_q->reply_post_host_index |
		((msix_index & 7) << MPI_RPHI_MSIX_INDEX_SHIFT);

	if (ioc->msix96_vector)
		writel(index, ioc->replyPostRegisterIndex[msix_index / 8]);
	else
        	writel(index, &ioc->chip->ReplyPostHostIndex);

> +	if (ioc->msix96_vector && ioc->reply_queue_count > 8) {
> +		/* If this is an 96 vector supported device,
> +		set up ReplyPostIndex addresses */

Bad comment formatting.

> +		ioc->replyPostRegisterIndex = kcalloc(12,
> +			sizeof(resource_size_t *), GFP_KERNEL);
[...]
> +		for (i = 0; i < 12; i++) {

Make 12 a constant or at the very least a variable with a comment.

> +			ioc->replyPostRegisterIndex[i] = (resource_size_t *)
> +				((u8 *)&ioc->chip->Doorbell +
> +				MPI25_SUP_REPLY_POST_HOST_INDEX_OFFSET +
> +				(i * 0x10));

0x10 - Another magic constant.

> @@ -4522,8 +4554,16 @@ _base_make_ioc_operational(struct MPT3SAS_ADAPTER *ioc, int sleep_flag)
>  
>  	/* initialize reply post host index */
>  	list_for_each_entry(reply_q, &ioc->reply_queue_list, list) {
> -		writel(reply_q->msix_index << MPI2_RPHI_MSIX_INDEX_SHIFT,
> -		    &ioc->chip->ReplyPostHostIndex);
> +		if (ioc->msix96_vector) {
> +			writel((reply_q->msix_index & 7)<<
> +			   MPI2_RPHI_MSIX_INDEX_SHIFT,
> +			   ioc->replyPostRegisterIndex[reply_q->msix_index/8]);
> +		} else {
> +			writel(reply_q->msix_index <<
> +				MPI2_RPHI_MSIX_INDEX_SHIFT,
> +					&ioc->chip->ReplyPostHostIndex);
> +		}
> +

Too many brackets.

-- 
Martin K. Petersen	Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 01/20] [SCSI] mpt3sas: Added Combined Reply Queue feature to extend up-to 96 MSIX vector support

[Sreekanth Reddy]

On Fri, Jun 19, 2015 at 8:27 PM, Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>>>>>> "Sreekanth" == Sreekanth Reddy <sreekanth.reddy@avagotech.com> writes:
>
> Sreekanth,
>
> It's fine that you outline the 96 / 12 = 8 layout in the patch
> description. But that relationship is not made clear when reading the
> code. Please add a comment describing why things are set up this way.

Accepted. I will add the comments where ever required.

>
>> @@ -1009,8 +1009,15 @@ _base_interrupt(int irq, void *bus_id)
>>       }
>>
>>       wmb();
>> -     writel(reply_q->reply_post_host_index | (msix_index <<
>> -         MPI2_RPHI_MSIX_INDEX_SHIFT), &ioc->chip->ReplyPostHostIndex);
>> +     if (ioc->msix96_vector) {
>> +             writel(reply_q->reply_post_host_index | ((msix_index  & 7) <<
>> +                     MPI2_RPHI_MSIX_INDEX_SHIFT),
>> +                             ioc->replyPostRegisterIndex[msix_index/8]);
>> +     } else {
>> +             writel(reply_q->reply_post_host_index | (msix_index <<
>> +                     MPI2_RPHI_MSIX_INDEX_SHIFT),
>> +                             &ioc->chip->ReplyPostHostIndex);
>> +     }
>
> Too many brackets. Why don't you do:

I will remove these extra brackets. but SAS3 HBA's less than C0
revision (which doesn't support this Combined Reply Queue feature)
will support up to 16 MSI-X vectors. so can't update the MSIxIndex
field with just from 0 to 7 by taking the modular of 8 on msix_index
value. so can't do below this.

>
>         index = reply_q->reply_post_host_index |
>                 ((msix_index & 7) << MPI_RPHI_MSIX_INDEX_SHIFT);
>
>         if (ioc->msix96_vector)
>                 writel(index, ioc->replyPostRegisterIndex[msix_index / 8]);
>         else
>                 writel(index, &ioc->chip->ReplyPostHostIndex);
>
>> +     if (ioc->msix96_vector && ioc->reply_queue_count > 8) {
>> +             /* If this is an 96 vector supported device,
>> +             set up ReplyPostIndex addresses */
>
> Bad comment formatting.

Accepted, will update this.

>
>> +             ioc->replyPostRegisterIndex = kcalloc(12,
>> +                     sizeof(resource_size_t *), GFP_KERNEL);
> [...]
>> +             for (i = 0; i < 12; i++) {
>
> Make 12 a constant or at the very least a variable with a comment.

Agreed. Will do this.

>
>> +                     ioc->replyPostRegisterIndex[i] = (resource_size_t *)
>> +                             ((u8 *)&ioc->chip->Doorbell +
>> +                             MPI25_SUP_REPLY_POST_HOST_INDEX_OFFSET +
>> +                             (i * 0x10));
>
> 0x10 - Another magic constant.

Accepted, Will update.

>
>> @@ -4522,8 +4554,16 @@ _base_make_ioc_operational(struct MPT3SAS_ADAPTER *ioc, int sleep_flag)
>>
>>       /* initialize reply post host index */
>>       list_for_each_entry(reply_q, &ioc->reply_queue_list, list) {
>> -             writel(reply_q->msix_index << MPI2_RPHI_MSIX_INDEX_SHIFT,
>> -                 &ioc->chip->ReplyPostHostIndex);
>> +             if (ioc->msix96_vector) {
>> +                     writel((reply_q->msix_index & 7)<<
>> +                        MPI2_RPHI_MSIX_INDEX_SHIFT,
>> +                        ioc->replyPostRegisterIndex[reply_q->msix_index/8]);
>> +             } else {
>> +                     writel(reply_q->msix_index <<
>> +                             MPI2_RPHI_MSIX_INDEX_SHIFT,
>> +                                     &ioc->chip->ReplyPostHostIndex);
>> +             }
>> +
>
> Too many brackets.

I will remove these brackets and also will post the next version of
this patch by considering all above comments.

>
> --
> Martin K. Petersen      Oracle Linux Engineering



-- 

Regards,
Sreekanth
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 01/20] [SCSI] mpt3sas: Added Combined Reply Queue feature to extend up-to 96 MSIX vector support

[Martin K. Petersen]

>>>>> "Sreekanth" == Sreekanth Reddy <sreekanth.reddy@avagotech.com> writes:

Sreekanth> I will remove these extra brackets. but SAS3 HBA's less than
Sreekanth> C0 revision (which doesn't support this Combined Reply Queue
Sreekanth> feature) will support up to 16 MSI-X vectors.

Ah, I missed that. Fair enough.

-- 
Martin K. Petersen	Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/