Re: [PATCH 5/8] csiostor: Chelsio FCoE offload driver submission (sources part 5).

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux