On 8/26/2012 12:13 AM, Nicholas A. Bellinger wrote: > On Sun, 2012-08-26 at 00:06 +0530, Naresh Kumar Inna wrote: >> On 8/25/2012 2:26 AM, Nicholas A. Bellinger wrote: >>> On Fri, 2012-08-24 at 23:10 +0530, Naresh Kumar Inna wrote: >>>> On 8/24/2012 1:18 AM, Nicholas A. Bellinger wrote: >>>>> On Fri, 2012-08-24 at 03:57 +0530, Naresh Kumar Inna wrote: >>>>>> This patch contains code to implement the interrupt handling and the fast >>>>>> path I/O functionality. The interrupt handling includes allocation of >>>>>> MSIX vectors, registering and implemeting the interrupt service routines. >>>>>> The fast path I/O functionality includes posting the I/O request to >>>>>> firmware >>>>>> via Work Requests, tracking/completing them, and handling task management >>>>>> requests. SCSI midlayer host template implementation is also covered by >>>>>> this patch. >>>>>> >>>>>> Signed-off-by: Naresh Kumar Inna <nar...@chelsio.com> >>>>>> --- >>>>> >>>>> Hi Naresh, >>>>> >>>>> My review comments are inline below.. >>>> >>>> Hi Nicholas, >>>> >>>> Thanks for taking the time to review the driver. Please find my replies >>>> inline. >>>> >>>> Regards, >>>> Naresh. >>>> >>>>> >>>>>> drivers/scsi/csiostor/csio_isr.c | 631 ++++++++++ >>>>>> drivers/scsi/csiostor/csio_scsi.c | 2498 >>>>>> +++++++++++++++++++++++++++++++++++++ >>>>>> 2 files changed, 3129 insertions(+), 0 deletions(-) >>>>>> create mode 100644 drivers/scsi/csiostor/csio_isr.c >>>>>> create mode 100644 drivers/scsi/csiostor/csio_scsi.c >>>>>> > > <SNIP> > >>>>> >>>>>> +/* >>>>>> + * csio_scsi_init_data_wr - Initialize the READ/WRITE SCSI WR. >>>>>> + * @req: IO req structure. >>>>>> + * @oper: read/write >>>>>> + * @wrp: DMA location to place the payload. >>>>>> + * @size: Size of WR (including FW WR + immed data + rsp SG entry + >>>>>> data SGL >>>>>> + * @wrop: _READ_/_WRITE_ >>>>>> + * >>>>>> + * Wrapper for populating fw_scsi_read_wr/fw_scsi_write_wr. >>>>>> + */ >>>>>> +#define csio_scsi_init_data_wr(req, oper, wrp, size, wrop) >>>>>> \ >>>>>> +do { >>>>>> \ >>>>>> + struct csio_hw *_hw = (req)->lnode->hwp; >>>>>> \ >>>>>> + struct csio_rnode *_rn = (req)->rnode; >>>>>> \ >>>>>> + struct fw_scsi_##oper##_wr *__wr = (struct fw_scsi_##oper##_wr >>>>>> *)(wrp);\ >>>>>> + struct ulptx_sgl *_sgl; >>>>>> \ >>>>>> + struct csio_dma_buf *_dma_buf; >>>>>> \ >>>>>> + uint8_t _imm = csio_hw_to_scsim(_hw)->proto_cmd_len; >>>>>> \ >>>>>> + struct scsi_cmnd *scmnd = csio_scsi_cmnd((req)); >>>>>> \ >>>>>> + >>>>>> \ >>>>>> + __wr->op_immdlen = cpu_to_be32(FW_WR_OP(FW_SCSI##wrop##WR) | >>>>>> \ >>>>>> + >>>>>> FW_SCSI##wrop##WR_IMMDLEN(_imm)); \ >>>>>> + __wr->flowid_len16 = cpu_to_be32(FW_WR_FLOWID(_rn->flowid) | >>>>>> \ >>>>>> + FW_WR_LEN16( >>>>>> \ >>>>>> + CSIO_ROUNDUP((size), >>>>>> 16))); \ >>>>>> + __wr->cookie = (uintptr_t) (req); >>>>>> \ >>>>>> + __wr->iqid = (uint16_t)cpu_to_be16(csio_q_physiqid(_hw, >>>>>> \ >>>>>> + >>>>>> (req)->iq_idx));\ >>>>>> + __wr->tmo_val = (uint8_t)((req)->tmo); >>>>>> \ >>>>>> + __wr->use_xfer_cnt = 1; >>>>>> \ >>>>>> + __wr->xfer_cnt = cpu_to_be32(scsi_bufflen(scmnd)); >>>>>> \ >>>>>> + __wr->ini_xfer_cnt = cpu_to_be32(scsi_bufflen(scmnd)); >>>>>> \ >>>>>> + /* Get RSP DMA buffer */ >>>>>> \ >>>>>> + _dma_buf = &(req)->dma_buf; >>>>>> \ >>>>>> + >>>>>> \ >>>>>> + /* Prepare RSP SGL */ >>>>>> \ >>>>>> + __wr->rsp_dmalen = cpu_to_be32(_dma_buf->len); >>>>>> \ >>>>>> + __wr->rsp_dmaaddr = cpu_to_be64(_dma_buf->paddr); >>>>>> \ >>>>>> + >>>>>> \ >>>>>> + __wr->r4 = 0; >>>>>> \ >>>>>> + >>>>>> \ >>>>>> + __wr->u.fcoe.ctl_pri = 0; >>>>>> \ >>>>>> + __wr->u.fcoe.cp_en_class = 0; >>>>>> \ >>>>>> + __wr->u.fcoe.r3_lo[0] = 0; >>>>>> \ >>>>>> + __wr->u.fcoe.r3_lo[1] = 0; >>>>>> \ >>>>>> + csio_scsi_fcp_cmnd((req), (void *)((uintptr_t)(wrp) + >>>>>> \ >>>>>> + sizeof(struct >>>>>> fw_scsi_##oper##_wr))); \ >>>>>> + >>>>>> \ >>>>>> + /* Move WR pointer past command and immediate data */ >>>>>> \ >>>>>> + _sgl = (struct ulptx_sgl *) ((uintptr_t)(wrp) + >>>>>> \ >>>>>> + sizeof(struct fw_scsi_##oper##_wr) + >>>>>> \ >>>>>> + ALIGN(_imm, 16)); >>>>>> \ >>>>>> + >>>>>> \ >>>>>> + /* Fill in the DSGL */ >>>>>> \ >>>>>> + csio_scsi_init_ultptx_dsgl(_hw, (req), _sgl); >>>>>> \ >>>>>> + >>>>>> \ >>>>>> +} while (0) >>>>>> + >>>>> >>>>> This one has four uses of CPP keys. Just turn those into macros, and >>>>> leave the rest of the code in a static function. >>>>> >>>> >>>> So what you are suggesting is to have all the lines of the macro >>>> csio_scsi_init_data_wr() added into a static function, but for the ones >>>> with the 4 keys. csio_scsi_init_data_wr() will then invoke this new >>>> function. Is that correct? >>>> >>> >>> Not sure how the above should actually look without actually doing it, >>> but IMHO the usage of macro just obfuscates what is going on.. >>> >>> If it's only used a few times, just inline the code into seperate static >>> functions. If it's used more than a few times, then use a single static >>> funciton with macro accessors for the assignment of the various '__wr' >>> structure members. >>> >>> The larger problem with all of these macros is that you can't tell what >>> is a macro and what is a function. >>> >>> If you need to use a CPP macro, please make sure to capitalize the name >>> of the macro in order to tell the difference between the two. >>> >> >> OK, I will see what I can do to convert this macro into a function. >> > > Also please change all of the remaining macro names to be capitalized so > someone reading the code knows the difference between function or macro. >
Hi Nicholas, Did you get a chance to review the rest of the driver patches? Thanks, Naresh. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html