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

[Naresh Kumar Inna <naresh@chelsio.com>]

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 <naresh@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.