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

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

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
>>
>> diff --git a/drivers/scsi/csiostor/csio_isr.c b/drivers/scsi/csiostor/csio_isr.c
>> new file mode 100644
>> index 0000000..96633e9
>> --- /dev/null
>> +++ b/drivers/scsi/csiostor/csio_isr.c
> 
> <SNIP>
> 
>> +#define csio_extra_msix_desc(_desc, _len, _str, _arg1, _arg2, _arg3)	\
>> +do {									\
>> +	memset((_desc), 0, (_len) + 1);					\
>> +	snprintf((_desc), (_len), (_str), (_arg1), (_arg2), (_arg3));	\
>> +} while (0)
>> +
> 
> This type of macro usage is not necessary for just two users below.
> Please inline code like this.
> 

OK, I will get rid of this macro.

>> +static void
>> +csio_add_msix_desc(struct csio_hw *hw)
>> +{
>> +	int i;
>> +	struct csio_msix_entries *entryp = &hw->msix_entries[0];
>> +	int k = CSIO_EXTRA_VECS;
>> +	int len = sizeof(entryp->desc) - 1;
>> +	int cnt = hw->num_sqsets + k;
>> +
>> +	/* Non-data vector */
>> +	csio_extra_msix_desc(entryp->desc, len, "csio-%02x:%02x:%x-nondata",
>> +			     CSIO_PCI_BUS(hw), CSIO_PCI_DEV(hw),
>> +			     CSIO_PCI_FUNC(hw));
>> +	entryp++;
>> +	csio_extra_msix_desc(entryp->desc, len, "csio-%02x:%02x:%x-fwevt",
>> +			     CSIO_PCI_BUS(hw), CSIO_PCI_DEV(hw),
>> +			     CSIO_PCI_FUNC(hw));
>> +	entryp++;
>> +
>> +	/* Name SCSI vecs */
>> +	for (i = k; i < cnt; i++, entryp++) {
>> +		memset(entryp->desc, 0, len + 1);
>> +		snprintf(entryp->desc, len, "csio-%02x:%02x:%x-scsi%d",
>> +			 CSIO_PCI_BUS(hw), CSIO_PCI_DEV(hw),
>> +			 CSIO_PCI_FUNC(hw), i - CSIO_EXTRA_VECS);
>> +	}
>> +}
>> +
> 
> 
>> diff --git a/drivers/scsi/csiostor/csio_scsi.c b/drivers/scsi/csiostor/csio_scsi.c
>> new file mode 100644
>> index 0000000..0f87b00
>> --- /dev/null
>> +++ b/drivers/scsi/csiostor/csio_scsi.c
> 
>> +
>> +/*
>> + * csio_scsi_match_io - Match an ioreq with the given SCSI level data.
>> + * @ioreq: The I/O request
>> + * @sld: Level information
>> + *
>> + * Should be called with lock held.
>> + *
>> + */
>> +static bool
>> +csio_scsi_match_io(struct csio_ioreq *ioreq, struct csio_scsi_level_data *sld)
>> +{
>> +	struct scsi_cmnd *scmnd = csio_scsi_cmnd(ioreq);
>> +
>> +	switch (sld->level) {
>> +	case CSIO_LEV_LUN:
>> +		if (scmnd == NULL)
>> +			return CSIO_FALSE;
>> +
>> +		return ((ioreq->lnode == sld->lnode) &&
>> +			(ioreq->rnode == sld->rnode) &&
>> +			((uint64_t)scmnd->device->lun == sld->oslun));
>> +
>> +	case CSIO_LEV_RNODE:
>> +		return ((ioreq->lnode == sld->lnode) &&
>> +				(ioreq->rnode == sld->rnode));
>> +	case CSIO_LEV_LNODE:
>> +		return (ioreq->lnode == sld->lnode);
>> +	case CSIO_LEV_ALL:
>> +		return CSIO_TRUE;
>> +	default:
>> +		return CSIO_FALSE;
>> +	}
>> +}
>> +
> 
> Why can't CSIO_[TRUE,FALSE] just use normal Boolean defines..?
> 

Is there a TRUE/FALSE define in a standard header file? I see a lot of
kernel code defining their own TRUE/FALSE.

>> +/*
>> + * csio_scsi_fcp_cmnd - Frame the SCSI FCP command paylod.
>> + * @req: IO req structure.
>> + * @addr: DMA location to place the payload.
>> + *
>> + * This routine is shared between FCP_WRITE, FCP_READ and FCP_CMD requests.
>> + */
>> +static inline void
>> +csio_scsi_fcp_cmnd(struct csio_ioreq *req, void *addr)
>> +{
>> +	struct csio_fcp_cmnd *fcp_cmnd = (struct csio_fcp_cmnd *)addr;
>> +	struct scsi_cmnd *scmnd = csio_scsi_cmnd(req);
>> +
>> +	/* Check for Task Management */
>> +	if (likely(scmnd->SCp.Message == 0)) {
>> +		int_to_scsilun(scmnd->device->lun,
>> +				(struct scsi_lun *)fcp_cmnd->lun);
>> +		fcp_cmnd->tm_flags = 0;
>> +		fcp_cmnd->cmdref = 0;
>> +		fcp_cmnd->pri_ta = 0;
>> +
>> +		memcpy(fcp_cmnd->cdb, scmnd->cmnd, 16);
>> +		csio_scsi_tag(scmnd, &fcp_cmnd->pri_ta,
>> +			      FCP_PTA_HEADQ, FCP_PTA_ORDERED, FCP_PTA_SIMPLE);
>> +		fcp_cmnd->dl = cpu_to_be32(scsi_bufflen(scmnd));
>> +
>> +		if (req->nsge)
>> +			if (req->datadir == CSIO_IOREQF_DMA_WRITE)
> 
> The same goes for CSIO_IOREQF_DMA_*...
> 
> Why can't this just be DMA_* defs from include/linux/dma-direction.h..?

OK, I will get rid of them and use the standard defines.

> 
>> +				fcp_cmnd->flags = FCP_CFL_WRDATA;
>> +			else
>> +				fcp_cmnd->flags = FCP_CFL_RDDATA;
>> +		else
>> +			fcp_cmnd->flags = 0;
>> +	} else {
>> +		memset(fcp_cmnd, 0, sizeof(*fcp_cmnd));
>> +		int_to_scsilun(scmnd->device->lun,
>> +				(struct scsi_lun *)fcp_cmnd->lun);
>> +		fcp_cmnd->tm_flags = (uint8_t)scmnd->SCp.Message;
>> +	}
>> +}
>> +
> 
> 
> 
>> +
>> +#define CSIO_SCSI_CMD_WR_SZ(_imm)					\
>> +	(sizeof(struct fw_scsi_cmd_wr) +		/* WR size */	\
>> +	 ALIGN((_imm), 16))				/* Immed data */
>> +
>> +#define CSIO_SCSI_CMD_WR_SZ_16(_imm)					\
>> +			(ALIGN(CSIO_SCSI_CMD_WR_SZ((_imm)), 16))
>> +
>> +/*
>> + * csio_scsi_cmd - Create a SCSI CMD WR.
>> + * @req: IO req structure.
>> + *
>> + * Gets a WR slot in the ingress queue and initializes it with SCSI CMD WR.
>> + *
>> + */
>> +static inline void
>> +csio_scsi_cmd(struct csio_ioreq *req)
>> +{
>> +	struct csio_wr_pair wrp;
>> +	struct csio_hw *hw = req->lnode->hwp;
>> +	struct csio_scsim *scsim = csio_hw_to_scsim(hw);
>> +	uint32_t size = CSIO_SCSI_CMD_WR_SZ_16(scsim->proto_cmd_len);
>> +
>> +	req->drv_status = csio_wr_get(hw, req->eq_idx, size, &wrp);
>> +	if (unlikely(req->drv_status != CSIO_SUCCESS))
>> +		return;
>> +
>> +	if (wrp.size1 >= size) {
>> +		/* Initialize WR in one shot */
>> +		csio_scsi_init_cmd_wr(req, wrp.addr1, size);
>> +	} else {
>> +		uint8_t tmpwr[512];
> 
> Mmmm, putting this large of a buffer on the local stack is probably not
> a good idea.
> 
> This should become an allocation..  If it's a hot path then you'll
> probably want to set this up before-hand.
> 

The else switch above is entered only when we near the end of the DMA
ring. This is not a frequent occurrence. If it is a strict no-no to have
so many on-stack bytes, I have to think of a way re-work it to use
pre-allocated memory. Please let me know.

>> +		/*
>> +		 * Make a temporary copy of the WR and write back
>> +		 * the copy into the WR pair.
>> +		 */
>> +		csio_scsi_init_cmd_wr(req, (void *)tmpwr, size);
>> +		memcpy(wrp.addr1, tmpwr, wrp.size1);
>> +		memcpy(wrp.addr2, tmpwr + wrp.size1, size - wrp.size1);
>> +	}
>> +}
>> +
>> +/*
>> + * The following is fast path code. Therefore it is inlined with multi-line
>> + * macros using name substitution, thus avoiding if-else switches for
>> + * operation (read/write), as well as serving the purpose of code re-use.
>> + */
>> +/*
>> + * csio_scsi_init_ulptx_dsgl - Fill in a ULP_TX_SC_DSGL
>> + * @hw: HW module
>> + * @req: IO request
>> + * @sgl: ULP TX SGL pointer.
>> + *
>> + */
>> +#define csio_scsi_init_ultptx_dsgl(hw, req, sgl)			       \
>> +do {									       \
>> +	struct ulptx_sge_pair *_sge_pair = NULL;			       \
>> +	struct scatterlist *_sgel;					       \
>> +	uint32_t _i = 0;						       \
>> +	uint32_t _xfer_len;						       \
>> +	struct list_head *_tmp;						       \
>> +	struct csio_dma_buf *_dma_buf;					       \
>> +	struct scsi_cmnd *scmnd = csio_scsi_cmnd((req));		       \
>> +									       \
>> +	(sgl)->cmd_nsge = htonl(ULPTX_CMD(ULP_TX_SC_DSGL) | ULPTX_MORE |       \
>> +				     ULPTX_NSGE((req)->nsge));		       \
>> +	/* Now add the data SGLs */					       \
>> +	if (likely(!(req)->dcopy)) {				               \
>> +		scsi_for_each_sg(scmnd, _sgel, (req)->nsge, _i) {	       \
>> +			if (_i == 0) {					       \
>> +				(sgl)->addr0 = cpu_to_be64(	               \
>> +						sg_dma_address(_sgel));	       \
>> +				(sgl)->len0 = cpu_to_be32(		       \
>> +						sg_dma_len(_sgel));	       \
>> +				_sge_pair =				       \
>> +					(struct ulptx_sge_pair *)((sgl) + 1);  \
>> +				continue;				       \
>> +			}						       \
>> +			if ((_i - 1) & 0x1) {				       \
>> +				_sge_pair->addr[1] = cpu_to_be64(	       \
>> +						sg_dma_address(_sgel));	       \
>> +				_sge_pair->len[1] = cpu_to_be32(	       \
>> +						sg_dma_len(_sgel));	       \
>> +				_sge_pair++;				       \
>> +			} else	{					       \
>> +				_sge_pair->addr[0] = cpu_to_be64(	       \
>> +						sg_dma_address(_sgel));	       \
>> +				_sge_pair->len[0] = cpu_to_be32(	       \
>> +						sg_dma_len(_sgel));	       \
>> +			}						       \
>> +		}							       \
>> +	} else {							       \
>> +		/* Program sg elements with driver's DDP buffer */	       \
>> +		_xfer_len = scsi_bufflen(scmnd);			       \
>> +		list_for_each(_tmp, &(req)->gen_list) {		       \
>> +			_dma_buf = (struct csio_dma_buf *)_tmp;		       \
>> +			if (_i == 0) {					       \
>> +				(sgl)->addr0 = cpu_to_be64(_dma_buf->paddr);   \
>> +				(sgl)->len0 = cpu_to_be32(		       \
>> +					min(_xfer_len, _dma_buf->len));        \
>> +				_sge_pair =				       \
>> +					(struct ulptx_sge_pair *)((sgl) + 1);  \
>> +			}						       \
>> +			else if ((_i - 1) & 0x1) {			       \
>> +				_sge_pair->addr[1] = cpu_to_be64(	       \
>> +							_dma_buf->paddr);      \
>> +				_sge_pair->len[1] = cpu_to_be32(	       \
>> +					min(_xfer_len, _dma_buf->len));        \
>> +				_sge_pair++;				       \
>> +			} else	{					       \
>> +				_sge_pair->addr[0] = cpu_to_be64(	       \
>> +							_dma_buf->paddr);      \
>> +				_sge_pair->len[0] = cpu_to_be32(	       \
>> +					min(_xfer_len, _dma_buf->len));        \
>> +			}						       \
>> +			_xfer_len -= min(_xfer_len, _dma_buf->len);            \
>> +			_i++;						       \
>> +		}							       \
>> +	}								       \
>> +} while (0)
>> +
> 
> I don't see any reason why this can't just be a static function..?  Why
> is the macro usage necessary here..?

OK, I will make this static-inline.

> 
>> +/*
>> + * 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?

>> +/* Calculate WR size needed for fw_scsi_read_wr/fw_scsi_write_wr */
>> +#define csio_scsi_data_wrsz(req, oper, sz, imm)				       \
>> +do {									       \
>> +	(sz) = sizeof(struct fw_scsi_##oper##_wr) +	/* WR size */          \
>> +	       ALIGN((imm), 16) +			/* Immed data */       \
>> +	       sizeof(struct ulptx_sgl);		/* ulptx_sgl */	       \
>> +									       \
>> +	if (unlikely((req)->nsge > 1))				               \
>> +		(sz) += (sizeof(struct ulptx_sge_pair) *		       \
>> +				(ALIGN(((req)->nsge - 1), 2) / 2));            \
>> +							/* Data SGE */	       \
>> +} while (0)
>> +
>> +/*
>> + * csio_scsi_data - Create a SCSI WRITE/READ WR.
>> + * @req: IO req structure.
>> + * @oper: read/write
>> + * @wrop:  _READ_/_WRITE_ (string subsitutions to use with the FW bit field
>> + *         macros).
>> + *
>> + * Gets a WR slot in the ingress queue and initializes it with
>> + * SCSI CMD READ/WRITE WR.
>> + *
>> + */
>> +#define csio_scsi_data(req, oper, wrop)					       \
>> +do {									       \
>> +	struct csio_wr_pair _wrp;					       \
>> +	uint32_t _size;							       \
>> +	struct csio_hw *_hw = (req)->lnode->hwp;			       \
>> +	struct csio_scsim *_scsim = csio_hw_to_scsim(_hw);		       \
>> +									       \
>> +	csio_scsi_data_wrsz((req), oper, _size, _scsim->proto_cmd_len);	       \
>> +	_size = ALIGN(_size, 16);					       \
>> +									       \
>> +	(req)->drv_status = csio_wr_get(_hw, (req)->eq_idx, _size, &_wrp);     \
>> +	if (likely((req)->drv_status == CSIO_SUCCESS)) {		       \
>> +		if (likely(_wrp.size1 >= _size)) {			       \
>> +			/* Initialize WR in one shot */			       \
>> +			csio_scsi_init_data_wr((req), oper, _wrp.addr1,        \
>> +						    _size, wrop);	       \
>> +		} else {						       \
>> +			uint8_t tmpwr[512];				       \
>> +			/*						       \
>> +			 * Make a temporary copy of the WR and write back      \
>> +			 * the copy into the WR pair.			       \
>> +			 */						       \
>> +			csio_scsi_init_data_wr((req), oper, (void *)tmpwr,     \
>> +						    _size, wrop);	       \
>> +			memcpy(_wrp.addr1, tmpwr, _wrp.size1);	               \
>> +			memcpy(_wrp.addr2, tmpwr + _wrp.size1,	               \
>> +				    _size - _wrp.size1);		       \
>> +		}							       \
>> +	}								       \
>> +} while (0)
>> +
> 
> Ditto on this one, along with the tmpwr[512] stack usage..
> 
>> +static inline void
>> +csio_scsi_abrt_cls(struct csio_ioreq *req, bool abort)
>> +{
>> +	struct csio_wr_pair wrp;
>> +	struct csio_hw *hw = req->lnode->hwp;
>> +	uint32_t size = ALIGN(sizeof(struct fw_scsi_abrt_cls_wr), 16);
>> +
>> +	req->drv_status = csio_wr_get(hw, req->eq_idx, size, &wrp);
>> +	if (req->drv_status != CSIO_SUCCESS)
>> +		return;
>> +
>> +	if (wrp.size1 >= size) {
>> +		/* Initialize WR in one shot */
>> +		csio_scsi_init_abrt_cls_wr(req, wrp.addr1, size, abort);
>> +	} else {
>> +		uint8_t tmpwr[512];
> 
> Ditto here on local scope stack usage..
> 
>> +		/*
>> +		 * Make a temporary copy of the WR and write back
>> +		 * the copy into the WR pair.
>> +		 */
>> +		csio_scsi_init_abrt_cls_wr(req, (void *)tmpwr, size, abort);
>> +		memcpy(wrp.addr1, tmpwr, wrp.size1);
>> +		memcpy(wrp.addr2, tmpwr + wrp.size1, size - wrp.size1);
>> +	}
>> +}
>> +
>> +/*****************************************************************************/
>> +/* START: SCSI SM                                                            */
>> +/*****************************************************************************/
>> +static void
>> +csio_scsis_uninit(struct csio_ioreq *req, enum csio_scsi_ev evt)
>> +{
>> +	struct csio_hw *hw = req->lnode->hwp;
>> +	struct csio_scsim *scsim = csio_hw_to_scsim(hw);
>> +
>> +	switch (evt) {
>> +
>> +	case CSIO_SCSIE_START_IO:
> 
> Extra space between start of first switch case

OK, I will remove it. Is there any tool that catches such deviations?
checkpath.pl didnt report it.

> 
>> +
>> +		/* There is data */
> 
> Point-less comment

I will remove it.

> 
>> +		if (req->nsge) {
>> +			if (req->datadir == CSIO_IOREQF_DMA_WRITE) {
>> +				req->dcopy = 0;
>> +				csio_scsi_data(req, write, _WRITE_);
>> +			} else
>> +				csio_setup_ddp(scsim, req);
>> +		} else {
>> +			csio_scsi_cmd(req);
>> +		}
>> +
>> +		if (likely(req->drv_status == CSIO_SUCCESS)) {
>> +			/* change state and enqueue on active_q */
>> +			csio_set_state(&req->sm, csio_scsis_io_active);
>> +			list_add_tail(&req->sm.sm_list, &scsim->active_q);
>> +			csio_wr_issue(hw, req->eq_idx, CSIO_FALSE);
>> +			csio_inc_stats(scsim, n_active);
>> +
>> +			return;
>> +		}
>> +		break;
>> +
>> +	case CSIO_SCSIE_START_TM:
>> +		csio_scsi_cmd(req);
>> +		if (req->drv_status == CSIO_SUCCESS) {
>> +			/*
>> +			 * NOTE: We collect the affected I/Os prior to issuing
>> +			 * LUN reset, and not after it. This is to prevent
>> +			 * aborting I/Os that get issued after the LUN reset,
>> +			 * but prior to LUN reset completion (in the event that
>> +			 * the host stack has not blocked I/Os to a LUN that is
>> +			 * being reset.
>> +			 */
>> +			csio_set_state(&req->sm, csio_scsis_tm_active);
>> +			list_add_tail(&req->sm.sm_list, &scsim->active_q);
>> +			csio_wr_issue(hw, req->eq_idx, CSIO_FALSE);
>> +			csio_inc_stats(scsim, n_tm_active);
>> +		}
>> +		return;
>> +
>> +	case CSIO_SCSIE_ABORT:
>> +	case CSIO_SCSIE_CLOSE:
>> +		/*
>> +		 * NOTE:
>> +		 * We could get here due to  :
>> +		 * - a window in the cleanup path of the SCSI module
>> +		 *   (csio_scsi_abort_io()). Please see NOTE in this function.
>> +		 * - a window in the time we tried to issue an abort/close
>> +		 *   of a request to FW, and the FW completed the request
>> +		 *   itself.
>> +		 *   Print a message for now, and return INVAL either way.
>> +		 */
>> +		req->drv_status = CSIO_INVAL;
>> +		csio_warn(hw, "Trying to abort/close completed IO:%p!\n", req);
>> +		break;
>> +
>> +	default:
>> +		csio_dbg(hw, "Unhandled event:%d sent to req:%p\n", evt, req);
>> +		CSIO_DB_ASSERT(0);
>> +	}
>> +}
>> +
>> +static void
>> +csio_scsis_io_active(struct csio_ioreq *req, enum csio_scsi_ev evt)
>> +{
>> +	struct csio_hw *hw = req->lnode->hwp;
>> +	struct csio_scsim *scm = csio_hw_to_scsim(hw);
>> +	struct csio_rnode *rn;
>> +
>> +	switch (evt) {
>> +
>> +	case CSIO_SCSIE_COMPLETED:
> 
> Ditto
> 
>> +		csio_dec_stats(scm, n_active);
>> +		list_del_init(&req->sm.sm_list);
>> +		csio_set_state(&req->sm, csio_scsis_uninit);
>> +		/*
>> +		 * In MSIX mode, with multiple queues, the SCSI compeltions
>> +		 * could reach us sooner than the FW events sent to indicate
>> +		 * I-T nexus loss (link down, remote device logo etc). We
>> +		 * dont want to be returning such I/Os to the upper layer
>> +		 * immediately, since we wouldnt have reported the I-T nexus
>> +		 * loss itself. This forces us to serialize such completions
>> +		 * with the reporting of the I-T nexus loss. Therefore, we
>> +		 * internally queue up such up such completions in the rnode.
>> +		 * The reporting of I-T nexus loss to the upper layer is then
>> +		 * followed by the returning of I/Os in this internal queue.
>> +		 * Having another state alongwith another queue helps us take
>> +		 * actions for events such as ABORT received while we are
>> +		 * in this rnode queue.
>> +		 */
>> +		if (unlikely(req->wr_status != FW_SUCCESS)) {
>> +			rn = req->rnode;
>> +			/*
>> +			 * FW says remote device is lost, but rnode
>> +			 * doesnt reflect it.
>> +			 */
>> +			if (csio_scsi_itnexus_loss_error(req->wr_status) &&
>> +						csio_is_rnode_ready(rn)) {
>> +				csio_set_state(&req->sm,
>> +						csio_scsis_shost_cmpl_await);
>> +				list_add_tail(&req->sm.sm_list,
>> +					      &rn->host_cmpl_q);
>> +			}
>> +		}
>> +
>> +		break;
>> +
>> +	case CSIO_SCSIE_ABORT:
>> +		csio_scsi_abrt_cls(req, SCSI_ABORT);
>> +		if (req->drv_status == CSIO_SUCCESS) {
>> +			csio_wr_issue(hw, req->eq_idx, CSIO_FALSE);
>> +			csio_set_state(&req->sm, csio_scsis_aborting);
>> +		}
>> +		break;
>> +
>> +	case CSIO_SCSIE_CLOSE:
>> +		csio_scsi_abrt_cls(req, SCSI_CLOSE);
>> +		if (req->drv_status == CSIO_SUCCESS) {
>> +			csio_wr_issue(hw, req->eq_idx, CSIO_FALSE);
>> +			csio_set_state(&req->sm, csio_scsis_closing);
>> +		}
>> +		break;
>> +
>> +	case CSIO_SCSIE_DRVCLEANUP:
>> +		req->wr_status = FW_HOSTERROR;
>> +		csio_dec_stats(scm, n_active);
>> +		csio_set_state(&req->sm, csio_scsis_uninit);
>> +		break;
>> +
>> +	default:
>> +		csio_dbg(hw, "Unhandled event:%d sent to req:%p\n", evt, req);
>> +		CSIO_DB_ASSERT(0);
>> +	}
>> +}
>> +
>> +static void
>> +csio_scsis_tm_active(struct csio_ioreq *req, enum csio_scsi_ev evt)
>> +{
>> +	struct csio_hw *hw = req->lnode->hwp;
>> +	struct csio_scsim *scm = csio_hw_to_scsim(hw);
>> +
>> +	switch (evt) {
>> +
>> +	case CSIO_SCSIE_COMPLETED:
> 
> Ditto
> 
>> +		csio_dec_stats(scm, n_tm_active);
>> +		list_del_init(&req->sm.sm_list);
>> +		csio_set_state(&req->sm, csio_scsis_uninit);
>> +
>> +		break;
>> +
>> +	case CSIO_SCSIE_ABORT:
>> +		csio_scsi_abrt_cls(req, SCSI_ABORT);
>> +		if (req->drv_status == CSIO_SUCCESS) {
>> +			csio_wr_issue(hw, req->eq_idx, CSIO_FALSE);
>> +			csio_set_state(&req->sm, csio_scsis_aborting);
>> +		}
>> +		break;
>> +
>> +
>> +	case CSIO_SCSIE_CLOSE:
>> +		csio_scsi_abrt_cls(req, SCSI_CLOSE);
>> +		if (req->drv_status == CSIO_SUCCESS) {
>> +			csio_wr_issue(hw, req->eq_idx, CSIO_FALSE);
>> +			csio_set_state(&req->sm, csio_scsis_closing);
>> +		}
>> +		break;
>> +
>> +	case CSIO_SCSIE_DRVCLEANUP:
>> +		req->wr_status = FW_HOSTERROR;
>> +		csio_dec_stats(scm, n_tm_active);
>> +		csio_set_state(&req->sm, csio_scsis_uninit);
>> +		break;
>> +
>> +	default:
>> +		csio_dbg(hw, "Unhandled event:%d sent to req:%p\n", evt, req);
>> +		CSIO_DB_ASSERT(0);
>> +	}
>> +}
>> +
>> +static void
>> +csio_scsis_aborting(struct csio_ioreq *req, enum csio_scsi_ev evt)
>> +{
>> +	struct csio_hw *hw = req->lnode->hwp;
>> +	struct csio_scsim *scm = csio_hw_to_scsim(hw);
>> +
>> +	switch (evt) {
>> +
>> +	case CSIO_SCSIE_COMPLETED:
> 
> Ditto
> 
>> +		csio_dbg(hw,
>> +			 "ioreq %p recvd cmpltd (wr_status:%d) "
>> +			 "in aborting st\n", req, req->wr_status);
>> +		/*
>> +		 * Use CSIO_CANCELLED to explicitly tell the ABORTED event that
>> +		 * the original I/O was returned to driver by FW.
>> +		 * We dont really care if the I/O was returned with success by
>> +		 * FW (because the ABORT and completion of the I/O crossed each
>> +		 * other), or any other return value. Once we are in aborting
>> +		 * state, the success or failure of the I/O is unimportant to
>> +		 * us.
>> +		 */
>> +		req->drv_status = CSIO_CANCELLED;
>> +		break;
>> +
>> +	case CSIO_SCSIE_ABORT:
>> +		csio_inc_stats(scm, n_abrt_dups);
>> +		break;
>> +
>> +	case CSIO_SCSIE_ABORTED:
>> +
>> +		csio_dbg(hw, "abort of %p return status:0x%x drv_status:%x\n",
>> +			 req, req->wr_status, req->drv_status);
>> +		/*
>> +		 * Check if original I/O WR completed before the Abort
>> +		 * completion.
>> +		 */
>> +		if (req->drv_status != CSIO_CANCELLED) {
>> +			csio_fatal(hw,
>> +				   "Abort completed before original I/O,"
>> +				   " req:%p\n", req);
>> +			CSIO_DB_ASSERT(0);
>> +		}
>> +
>> +		/*
>> +		 * There are the following possible scenarios:
>> +		 * 1. The abort completed successfully, FW returned FW_SUCCESS.
>> +		 * 2. The completion of an I/O and the receipt of
>> +		 *    abort for that I/O by the FW crossed each other.
>> +		 *    The FW returned FW_EINVAL. The original I/O would have
>> +		 *    returned with FW_SUCCESS or any other SCSI error.
>> +		 * 3. The FW couldnt sent the abort out on the wire, as there
>> +		 *    was an I-T nexus loss (link down, remote device logged
>> +		 *    out etc). FW sent back an appropriate IT nexus loss status
>> +		 *    for the abort.
>> +		 * 4. FW sent an abort, but abort timed out (remote device
>> +		 *    didnt respond). FW replied back with
>> +		 *    FW_SCSI_ABORT_TIMEDOUT.
>> +		 * 5. FW couldnt genuinely abort the request for some reason,
>> +		 *    and sent us an error.
>> +		 *
>> +		 * The first 3 scenarios are treated as  succesful abort
>> +		 * operations by the host, while the last 2 are failed attempts
>> +		 * to abort. Manipulate the return value of the request
>> +		 * appropriately, so that host can convey these results
>> +		 * back to the upper layer.
>> +		 */
>> +		if ((req->wr_status == FW_SUCCESS) ||
>> +		    (req->wr_status == FW_EINVAL) ||
>> +		     csio_scsi_itnexus_loss_error(req->wr_status))
>> +			req->wr_status = FW_SCSI_ABORT_REQUESTED;
>> +
>> +		csio_dec_stats(scm, n_active);
>> +		list_del_init(&req->sm.sm_list);
>> +		csio_set_state(&req->sm, csio_scsis_uninit);
>> +		break;
>> +
>> +	case CSIO_SCSIE_DRVCLEANUP:
>> +		req->wr_status = FW_HOSTERROR;
>> +		csio_dec_stats(scm, n_active);
>> +		csio_set_state(&req->sm, csio_scsis_uninit);
>> +		break;
>> +
>> +	case CSIO_SCSIE_CLOSE:
>> +		/*
>> +		 * We can receive this event from the module
>> +		 * cleanup paths, if the FW forgot to reply to the ABORT WR
>> +		 * and left this ioreq in this state. For now, just ignore
>> +		 * the event. The CLOSE event is sent to this state, as
>> +		 * the LINK may have already gone down.
>> +		 */
>> +		break;
>> +
>> +	default:
>> +		csio_dbg(hw, "Unhandled event:%d sent to req:%p\n", evt, req);
>> +		CSIO_DB_ASSERT(0);
>> +	}
>> +}
>> +
>> +static void
>> +csio_scsis_closing(struct csio_ioreq *req, enum csio_scsi_ev evt)
>> +{
>> +	struct csio_hw *hw = req->lnode->hwp;
>> +	struct csio_scsim *scm = csio_hw_to_scsim(hw);
>> +
>> +	switch (evt) {
>> +
>> +	case CSIO_SCSIE_COMPLETED:
> 
> Ditto
> 
>> +		csio_dbg(hw,
>> +			 "ioreq %p recvd cmpltd (wr_status:%d) "
>> +			 "in closing st\n", req, req->wr_status);
>> +		/*
>> +		 * Use CSIO_CANCELLED to explicitly tell the CLOSED event that
>> +		 * the original I/O was returned to driver by FW.
>> +		 * We dont really care if the I/O was returned with success by
>> +		 * FW (because the CLOSE and completion of the I/O crossed each
>> +		 * other), or any other return value. Once we are in aborting
>> +		 * state, the success or failure of the I/O is unimportant to
>> +		 * us.
>> +		 */
>> +		req->drv_status = CSIO_CANCELLED;
>> +		break;
>> +
>> +	case CSIO_SCSIE_CLOSED:
>> +		/*
>> +		 * Check if original I/O WR completed before the Close
>> +		 * completion.
>> +		 */
>> +		if (req->drv_status != CSIO_CANCELLED) {
>> +			csio_fatal(hw,
>> +				   "Close completed before original I/O,"
>> +				   " req:%p\n", req);
>> +			CSIO_DB_ASSERT(0);
>> +		}
>> +
>> +		/*
>> +		 * Either close succeeded, or we issued close to FW at the
>> +		 * same time FW compelted it to us. Either way, the I/O
>> +		 * is closed.
>> +		 */
>> +		CSIO_DB_ASSERT((req->wr_status == FW_SUCCESS) ||
>> +					(req->wr_status == FW_EINVAL));
>> +		req->wr_status = FW_SCSI_CLOSE_REQUESTED;
>> +
>> +		csio_dec_stats(scm, n_active);
>> +		list_del_init(&req->sm.sm_list);
>> +		csio_set_state(&req->sm, csio_scsis_uninit);
>> +		break;
>> +
>> +	case CSIO_SCSIE_CLOSE:
>> +		break;
>> +
>> +	case CSIO_SCSIE_DRVCLEANUP:
>> +		req->wr_status = FW_HOSTERROR;
>> +		csio_dec_stats(scm, n_active);
>> +		csio_set_state(&req->sm, csio_scsis_uninit);
>> +		break;
>> +
>> +	default:
>> +		csio_dbg(hw, "Unhandled event:%d sent to req:%p\n", evt, req);
>> +		CSIO_DB_ASSERT(0);
>> +	}
>> +}
>> +
>> +static void
>> +csio_scsis_shost_cmpl_await(struct csio_ioreq *req, enum csio_scsi_ev evt)
>> +{
>> +	switch (evt) {
>> +	case CSIO_SCSIE_ABORT:
>> +	case CSIO_SCSIE_CLOSE:
>> +		/*
>> +		 * Just succeed the abort request, and hope that
>> +		 * the remote device unregister path will cleanup
>> +		 * this I/O to the upper layer within a sane
>> +		 * amount of time.
>> +		 */
>> +		/*
>> +		 * A close can come in during a LINK DOWN. The FW would have
>> +		 * returned us the I/O back, but not the remote device lost
>> +		 * FW event. In this interval, if the I/O times out at the upper
>> +		 * layer, a close can come in. Take the same action as abort:
>> +		 * return success, and hope that the remote device unregister
>> +		 * path will cleanup this I/O. If the FW still doesnt send
>> +		 * the msg, the close times out, and the upper layer resorts
>> +		 * to the next level of error recovery.
>> +		 */
>> +		req->drv_status = CSIO_SUCCESS;
>> +		break;
>> +	case CSIO_SCSIE_DRVCLEANUP:
>> +		csio_set_state(&req->sm, csio_scsis_uninit);
>> +		break;
>> +	default:
>> +		csio_dbg(req->lnode->hwp, "Unhandled event:%d sent to req:%p\n",
>> +			 evt, req);
>> +		CSIO_DB_ASSERT(0);
>> +	}
>> +}
>> +
> 
> 
>> +
>> +/**
>> + * csio_queuecommand_lck - Entry point to kickstart an I/O request.
>> + * @cmnd:	The I/O request from ML.
>> + * @done:	The ML callback routine.
>> + *
>> + * This routine does the following:
>> + *	- Checks for HW and Rnode module readiness.
>> + *	- Gets a free ioreq structure (which is already initialized
>> + *	  to uninit during its allocation).
>> + *	- Maps SG elements.
>> + *	- Initializes ioreq members.
>> + *	- Kicks off the SCSI state machine for this IO.
>> + *	- Returns busy status on error.
>> + */
>> +static int
>> +csio_queuecommand_lck(struct scsi_cmnd *cmnd, void (*done)(struct scsi_cmnd *))
>> +{
>> +	struct csio_lnode *ln = shost_priv(cmnd->device->host);
>> +	struct csio_hw *hw = csio_lnode_to_hw(ln);
>> +	struct csio_scsim *scsim = csio_hw_to_scsim(hw);
>> +	struct csio_rnode *rn = (struct csio_rnode *)(cmnd->device->hostdata);
>> +	struct csio_ioreq *ioreq = NULL;
>> +	unsigned long flags;
>> +	int nsge = 0;
>> +	int rv = SCSI_MLQUEUE_HOST_BUSY, nr;
>> +	csio_retval_t retval;
>> +	int cpu;
>> +	struct csio_scsi_qset *sqset;
>> +	struct fc_rport *rport = starget_to_rport(scsi_target(cmnd->device));
>> +
>> +	if (!blk_rq_cpu_valid(cmnd->request))
>> +		cpu = smp_processor_id();
>> +	else
>> +		cpu = cmnd->request->cpu;
>> +
>> +	sqset = &hw->sqset[ln->portid][cpu];
>> +
>> +	nr = fc_remote_port_chkready(rport);
>> +	if (nr) {
>> +		cmnd->result = nr;
>> +		csio_inc_stats(scsim, n_rn_nr_error);
>> +		goto err_done;
>> +	}
>> +
>> +	if (unlikely(!csio_is_hw_ready(hw))) {
>> +		cmnd->result = (DID_REQUEUE << 16);
>> +		csio_inc_stats(scsim, n_hw_nr_error);
>> +		goto err_done;
>> +	}
>> +
>> +	/* Get req->nsge, if there are SG elements to be mapped  */
>> +	nsge = scsi_dma_map(cmnd);
>> +	if (unlikely(nsge < 0)) {
>> +		csio_inc_stats(scsim, n_dmamap_error);
>> +		goto err;
>> +	}
>> +
>> +	/* Do we support so many mappings? */
>> +	if (unlikely(nsge > scsim->max_sge)) {
>> +		csio_warn(hw,
>> +			  "More SGEs than can be supported."
>> +			  " SGEs: %d, Max SGEs: %d\n", nsge, scsim->max_sge);
>> +		csio_inc_stats(scsim, n_unsupp_sge_error);
>> +		goto err_dma_unmap;
>> +	}
>> +
>> +	/* Get a free ioreq structure - SM is already set to uninit */
>> +	ioreq = csio_get_scsi_ioreq_lock(hw, scsim);
>> +	if (!ioreq) {
>> +		csio_err(hw, "Out of I/O request elements. Active #:%d\n",
>> +			 scsim->stats.n_active);
>> +		csio_inc_stats(scsim, n_no_req_error);
>> +		goto err_dma_unmap;
>> +	}
>> +
>> +	ioreq->nsge		= nsge;
>> +	ioreq->lnode		= ln;
>> +	ioreq->rnode		= rn;
>> +	ioreq->iq_idx		= sqset->iq_idx;
>> +	ioreq->eq_idx		= sqset->eq_idx;
>> +	ioreq->wr_status	= 0;
>> +	ioreq->drv_status	= CSIO_SUCCESS;
>> +	csio_scsi_cmnd(ioreq)	= (void *)cmnd;
>> +	ioreq->tmo		= 0;
>> +
>> +	switch (cmnd->sc_data_direction) {
>> +	case DMA_BIDIRECTIONAL:
>> +		ioreq->datadir = CSIO_IOREQF_DMA_BIDI;
>> +		csio_inc_stats(ln, n_control_requests);
>> +		break;
>> +	case DMA_TO_DEVICE:
>> +		ioreq->datadir = CSIO_IOREQF_DMA_WRITE;
>> +		csio_inc_stats(ln, n_output_requests);
>> +		ln->stats.n_output_bytes += scsi_bufflen(cmnd);
>> +		break;
>> +	case DMA_FROM_DEVICE:
>> +		ioreq->datadir = CSIO_IOREQF_DMA_READ;
>> +		csio_inc_stats(ln, n_input_requests);
>> +		ln->stats.n_input_bytes += scsi_bufflen(cmnd);
>> +		break;
>> +	case DMA_NONE:
>> +		ioreq->datadir = CSIO_IOREQF_DMA_NONE;
>> +		csio_inc_stats(ln, n_control_requests);
>> +		break;
>> +	default:
>> +		CSIO_DB_ASSERT(0);
>> +		break;
>> +	}
>> +
>> +	/* Set cbfn */
>> +	ioreq->io_cbfn = csio_scsi_cbfn;
>> +
>> +	/* Needed during abort */
>> +	cmnd->host_scribble = (unsigned char *)ioreq;
>> +	cmnd->scsi_done = done;
>> +	cmnd->SCp.Message = 0;
>> +
>> +	/* Kick off SCSI IO SM on the ioreq */
>> +	spin_lock_irqsave(&hw->lock, flags);
>> +	retval = csio_scsi_start_io(ioreq);
>> +	spin_unlock_irqrestore(&hw->lock, flags);
>> +
>> +	if (retval != CSIO_SUCCESS) {
>> +		csio_err(hw, "ioreq: %p couldnt be started, status:%d\n",
>> +			 ioreq, retval);
>> +		csio_inc_stats(scsim, n_busy_error);
>> +		goto err_put_req;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_put_req:
>> +	csio_put_scsi_ioreq_lock(hw, scsim, ioreq);
>> +err_dma_unmap:
>> +	if (nsge > 0)
>> +		scsi_dma_unmap(cmnd);
>> +err:
>> +	return rv;
>> +
>> +err_done:
>> +	done(cmnd);
>> +	return 0;
>> +}
>> +
>> +static DEF_SCSI_QCMD(csio_queuecommand);
>> +
> 
> This means that your running with the host_lock held..  I'm not sure if
> that is really what you want to do as it really end's up killing
> multi-lun small packet performance..
> 
> How about dropping DEF_SCSI_QCMD usage here, and figure out what
> actually needs to be protected by the SCSI host_lock within
> csio_queuecommand_lck()..?

It is on my TODO list for the next version of the driver, after the
initial submission. Per the current design, we shouldnt need the
host_lock to be held, but I would like to test this change thoroughly
before I submit it.

>> +static csio_retval_t
>> +csio_do_abrt_cls(struct csio_hw *hw, struct csio_ioreq *ioreq, bool abort)
>> +{
>> +	csio_retval_t rv;
>> +	int cpu = smp_processor_id();
>> +	struct csio_lnode *ln = ioreq->lnode;
>> +	struct csio_scsi_qset *sqset = &hw->sqset[ln->portid][cpu];
>> +
>> +	ioreq->tmo = CSIO_SCSI_ABRT_TMO_MS;
>> +	/*
>> +	 * Use current processor queue for posting the abort/close, but retain
>> +	 * the ingress queue ID of the original I/O being aborted/closed - we
>> +	 * need the abort/close completion to be received on the same queue
>> +	 * as the original I/O.
>> +	 */
>> +	ioreq->eq_idx = sqset->eq_idx;
>> +
>> +	if (abort == SCSI_ABORT)
>> +		rv = csio_scsi_abort(ioreq);
>> +	else /* close */
> 
> Point-less comment.

I will remove it.

> 
>> +		rv = csio_scsi_close(ioreq);
>> +
>> +	return rv;
>> +}
>> +
>> +static int
>> +csio_eh_abort_handler(struct scsi_cmnd *cmnd)
>> +{
>> +	struct csio_ioreq *ioreq;
>> +	struct csio_lnode *ln = shost_priv(cmnd->device->host);
>> +	struct csio_hw *hw = csio_lnode_to_hw(ln);
>> +	struct csio_scsim *scsim = csio_hw_to_scsim(hw);
>> +	int ready = 0, ret;
>> +	unsigned long tmo = 0;
>> +	csio_retval_t rv;
>> +	struct csio_rnode *rn = (struct csio_rnode *)(cmnd->device->hostdata);
>> +
>> +	ret = fc_block_scsi_eh(cmnd);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ioreq = (struct csio_ioreq *)cmnd->host_scribble;
>> +	if (!ioreq)
>> +		return SUCCESS;
>> +
>> +	if (!rn)
>> +		return FAILED;
>> +
>> +	csio_dbg(hw,
>> +		 "Request to abort ioreq:%p cmd:%p cdb:%08llx"
>> +		 " ssni:0x%x lun:%d iq:0x%x\n",
>> +		ioreq, cmnd, *((uint64_t *)cmnd->cmnd), rn->flowid,
>> +		cmnd->device->lun, csio_q_physiqid(hw, ioreq->iq_idx));
>> +
>> +	if (((struct scsi_cmnd *)csio_scsi_cmnd(ioreq)) != cmnd) {
>> +		csio_inc_stats(scsim, n_abrt_race_comp);
>> +		return SUCCESS;
>> +	}
>> +
>> +	ready = csio_is_lnode_ready(ln);
>> +	tmo = CSIO_SCSI_ABRT_TMO_MS;
>> +
>> +	spin_lock_irq(&hw->lock);
>> +	rv = csio_do_abrt_cls(hw, ioreq, (ready ? SCSI_ABORT : SCSI_CLOSE));
>> +	spin_unlock_irq(&hw->lock);
>> +
>> +	if (rv != CSIO_SUCCESS) {
>> +		if (rv == CSIO_INVAL) {
>> +			/* Return success, if abort/close request issued on
>> +			 * already completed IO
>> +			 */
>> +			return SUCCESS;
>> +		}
>> +		if (ready)
>> +			csio_inc_stats(scsim, n_abrt_busy_error);
>> +		else
>> +			csio_inc_stats(scsim, n_cls_busy_error);
>> +
>> +		goto inval_scmnd;
>> +	}
>> +
>> +	/* Wait for completion */
>> +	init_completion(&ioreq->cmplobj);
>> +	wait_for_completion_timeout(&ioreq->cmplobj, msecs_to_jiffies(tmo));
>> +
>> +	/* FW didnt respond to abort within our timeout */
>> +	if (((struct scsi_cmnd *)csio_scsi_cmnd(ioreq)) == cmnd) {
>> +
>> +		csio_err(hw, "Abort timed out -- req: %p\n", ioreq);
>> +		csio_inc_stats(scsim, n_abrt_timedout);
>> +
>> +inval_scmnd:
>> +		if (ioreq->nsge > 0)
>> +			scsi_dma_unmap(cmnd);
>> +
>> +		spin_lock_irq(&hw->lock);
>> +		csio_scsi_cmnd(ioreq) = NULL;
>> +		spin_unlock_irq(&hw->lock);
>> +
>> +		cmnd->result = (DID_ERROR << 16);
>> +		cmnd->scsi_done(cmnd);
>> +
>> +		return FAILED;
>> +	}
>> +
>> +	/* FW successfully aborted the request */
>> +	if (host_byte(cmnd->result) == DID_REQUEUE) {
>> +		csio_info(hw,
>> +			"Aborted SCSI command to (%d:%d) serial#:0x%lx\n",
>> +			cmnd->device->id, cmnd->device->lun,
>> +			cmnd->serial_number);
>> +		return SUCCESS;
>> +	} else {
>> +		csio_info(hw,
>> +			"Failed to abort SCSI command, (%d:%d) serial#:0x%lx\n",
>> +			cmnd->device->id, cmnd->device->lun,
>> +			cmnd->serial_number);
>> +		return FAILED;
>> +	}
>> +}
>> +
> 
>> +struct scsi_host_template csio_fcoe_shost_template = {
>> +	.module			= THIS_MODULE,
>> +	.name			= CSIO_DRV_DESC,
>> +	.proc_name		= KBUILD_MODNAME,
>> +	.queuecommand		= csio_queuecommand,
>> +	.eh_abort_handler	= csio_eh_abort_handler,
>> +	.eh_device_reset_handler = csio_eh_lun_reset_handler,
>> +	.slave_alloc		= csio_slave_alloc,
>> +	.slave_configure	= csio_slave_configure,
>> +	.slave_destroy		= csio_slave_destroy,
>> +	.scan_finished		= csio_scan_finished,
>> +	.this_id		= -1,
>> +	.sg_tablesize		= CSIO_SCSI_MAX_SGE,
>> +	.cmd_per_lun		= CSIO_MAX_CMD_PER_LUN,
>> +	.use_clustering		= ENABLE_CLUSTERING,
>> +	.shost_attrs		= csio_fcoe_lport_attrs,
>> +	.max_sectors		= CSIO_MAX_SECTOR_SIZE,
>> +};
>> +
>> +struct scsi_host_template csio_fcoe_shost_vport_template = {
>> +	.module			= THIS_MODULE,
>> +	.name			= CSIO_DRV_DESC,
>> +	.proc_name		= KBUILD_MODNAME,
>> +	.queuecommand		= csio_queuecommand,
>> +	.eh_abort_handler	= csio_eh_abort_handler,
>> +	.eh_device_reset_handler = csio_eh_lun_reset_handler,
>> +	.slave_alloc		= csio_slave_alloc,
>> +	.slave_configure	= csio_slave_configure,
>> +	.slave_destroy		= csio_slave_destroy,
>> +	.scan_finished		= csio_scan_finished,
>> +	.this_id		= -1,
>> +	.sg_tablesize		= CSIO_SCSI_MAX_SGE,
>> +	.cmd_per_lun		= CSIO_MAX_CMD_PER_LUN,
>> +	.use_clustering		= ENABLE_CLUSTERING,
>> +	.shost_attrs		= csio_fcoe_vport_attrs,
>> +	.max_sectors		= CSIO_MAX_SECTOR_SIZE,
>> +};
>> +
>> +/*
>> + * csio_scsi_alloc_ddp_bufs - Allocate buffers for DDP of unaligned SGLs.
>> + * @scm: SCSI Module
>> + * @hw: HW device.
>> + * @buf_size: buffer size
>> + * @num_buf : Number of buffers.
>> + *
>> + * This routine allocates DMA buffers required for SCSI Data xfer, if
>> + * each SGL buffer for a SCSI Read request posted by SCSI midlayer are
>> + * not virtually contiguous.
>> + */
>> +static csio_retval_t
>> +csio_scsi_alloc_ddp_bufs(struct csio_scsim *scm, struct csio_hw *hw,
>> +			 int buf_size, int num_buf)
>> +{
>> +	int n = 0;
>> +	struct list_head *tmp;
>> +	struct csio_dma_buf *ddp_desc = NULL;
>> +	uint32_t unit_size = 0;
>> +
>> +	if (!num_buf)
>> +		return CSIO_SUCCESS;
> 
> Just return 0..?

Since there is a comment about driver-internal return values in your
review of patch 6, I would like to take up this discussion in that thread.


> 
>> +
>> +	if (!buf_size)
>> +		return CSIO_INVAL;
> 
> Just return -EINVAL..?
> 
>> +
>> +	INIT_LIST_HEAD(&scm->ddp_freelist);
>> +
>> +	/* Align buf size to page size */
>> +	buf_size = (buf_size + PAGE_SIZE - 1) & PAGE_MASK;
>> +	/* Initialize dma descriptors */
>> +	for (n = 0; n < num_buf; n++) {
>> +		/* Set unit size to request size */
>> +		unit_size = buf_size;
>> +		ddp_desc = kzalloc(sizeof(struct csio_dma_buf), GFP_KERNEL);
>> +		if (!ddp_desc) {
>> +			csio_err(hw,
>> +				 "Failed to allocate ddp descriptors,"
>> +				 " Num allocated = %d.\n",
>> +				 scm->stats.n_free_ddp);
>> +			goto no_mem;
>> +		}
>> +
>> +		/* Allocate Dma buffers for DDP */
>> +		ddp_desc->vaddr = pci_alloc_consistent(hw->pdev, unit_size,
>> +							&ddp_desc->paddr);
>> +		if (!ddp_desc->vaddr) {
>> +			csio_err(hw,
>> +				 "SCSI response DMA buffer (ddp) allocation"
>> +				 " failed!\n");
>> +			kfree(ddp_desc);
>> +			goto no_mem;
>> +		}
>> +
>> +		ddp_desc->len = unit_size;
>> +
>> +		/* Added it to scsi ddp freelist */
>> +		list_add_tail(&ddp_desc->list, &scm->ddp_freelist);
>> +		csio_inc_stats(scm, n_free_ddp);
>> +	}
>> +
>> +	return CSIO_SUCCESS;
> 
> Just return 0..?
> 
>> +no_mem:
>> +	/* release dma descs back to freelist and free dma memory */
>> +	list_for_each(tmp, &scm->ddp_freelist) {
>> +		ddp_desc = (struct csio_dma_buf *) tmp;
>> +		tmp = csio_list_prev(tmp);
>> +		pci_free_consistent(hw->pdev, ddp_desc->len, ddp_desc->vaddr,
>> +				    ddp_desc->paddr);
>> +		list_del_init(&ddp_desc->list);
>> +		kfree(ddp_desc);
>> +	}
>> +	scm->stats.n_free_ddp = 0;
>> +
>> +	return CSIO_NOMEM;
> 
> This should be just -ENOMEM..?
> 
>> +}
>> +
>> +/*
>> + * csio_scsi_free_ddp_bufs - free DDP buffers of unaligned SGLs.
>> + * @scm: SCSI Module
>> + * @hw: HW device.
>> + *
>> + * This routine frees ddp buffers.
>> + */
>> +static csio_retval_t
>> +csio_scsi_free_ddp_bufs(struct csio_scsim *scm, struct csio_hw *hw)
>> +{
>> +	struct list_head *tmp;
>> +	struct csio_dma_buf *ddp_desc;
>> +
>> +	/* release dma descs back to freelist and free dma memory */
>> +	list_for_each(tmp, &scm->ddp_freelist) {
>> +		ddp_desc = (struct csio_dma_buf *) tmp;
>> +		tmp = csio_list_prev(tmp);
>> +		pci_free_consistent(hw->pdev, ddp_desc->len, ddp_desc->vaddr,
>> +				    ddp_desc->paddr);
>> +		list_del_init(&ddp_desc->list);
>> +		kfree(ddp_desc);
>> +	}
>> +	scm->stats.n_free_ddp = 0;
>> +
>> +	return CSIO_NOMEM;
>> +}
> 
> Ditto..  Just -ENOMEM..?
> 
>