Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933505AbbFVMik (ORCPT ); Mon, 22 Jun 2015 08:38:40 -0400 Received: from mail-wg0-f52.google.com ([74.125.82.52]:35114 "EHLO mail-wg0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754039AbbFVMi2 (ORCPT ); Mon, 22 Jun 2015 08:38:28 -0400 MIME-Version: 1.0 In-Reply-To: References: <1434711346-9970-1-git-send-email-Sreekanth.Reddy@avagotech.com> Date: Mon, 22 Jun 2015 18:08:27 +0530 Message-ID: Subject: Re: [PATCH v1 01/20] [SCSI] mpt3sas: Added Combined Reply Queue feature to extend up-to 96 MSIX vector support From: Sreekanth Reddy To: "Martin K. Petersen" Cc: "jejb@kernel.org" , Christoph Hellwig , Johannes Thumshirn , "linux-scsi@vger.kernel.org" , "James E.J. Bottomley" , Sathya Prakash , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3822 Lines: 110 On Fri, Jun 19, 2015 at 8:27 PM, Martin K. Petersen wrote: >>>>>> "Sreekanth" == Sreekanth Reddy 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/