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 <naresh@xxxxxxxxxxx>
> >>>> ---
> >>>
> >>> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
