Re: [PATCH 2/6] target/iscsi: Call .iscsit_release_cmd() once

[Varun Prakash <varun@chelsio.com>]

On Tue, May 09, 2017 at 12:49:26AM -0700, Nicholas A. Bellinger wrote:
> Hi Varun,
> 
> On Sun, 2017-05-07 at 18:22 +0530, Varun Prakash wrote:
> > On Mon, May 01, 2017 at 09:33:31PM -0700, Nicholas A. Bellinger wrote:
> 
> <SNIP>
> 
> Indeed.
> 
> Looking at other hw drivers like qla2xxx that have this same
> requirement, they do *_unmap_sg() once a completion interrupt has
> triggered, but before target_core_fabric_ops->release_cmd() is invoked
> and se_cmd->t_task_sg has already been freed.
> 
> So I'm open to adding a target_core_fabric_ops->unmap_sg() to do this
> ahead of target_core_transport.c:transport_free_pages().
> 
> That said, snother option is to perform *_unmap_sg() internally in
> cxgbit once DDP completion for WRITEs has completed, but before it's
> submitted into iscsi_target -> target_core.
> 
> AFAICT from a quick scan of cxgbit code, the two scenarios for this
> would be:
> 
> *) For ISCSI_OP_SCSI_CMD with immediate data, once
> cxgbit_get_immediate_data() is invoked.  Either for all cases when this
> is invoked (eg: does the DDP SGLs both immediate, unsolicited and
> solicited data when mapped..?), or only when iscsi_cmd->immediate_data =
> 1 && iscsi_cmd_flags & ICF_GOT_LAST_DATAOUT is true.
> 
> *) For ISCSI_OP_SCSI_DATA_OUT with FINAL_BIT set, before
> iscsit_check_dataout_payload() is called to invoke target_execute_cmd()
> 
> WDYT..?
>
 
Current cxgbit code does following in cxgbit_release_cmd() 
1. put_page() in case of PASSTHROUGH_SG_TO_MEM_NOALLOC.
2. dma_unmap_sg() DDP SGL.
3. free hw DDP resource.

If cxgbit does cleanup internally then lot of error handling code is
required in cxgbit driver, eg: - PDU with F bit set never arrives,
header digest errors etc.

Another approach to add target_core_fabrics_ops->unmap_sg() will not work
for ERL 2 case because for calling ->iscsit_unmap_sg() valid
cmd->conn pointer is required, in case of ERL 2 cmd->conn can be NULL,
but we can use this approach because in current cxgbit code I enable DDP
only for ERL 0 case, similiary I can add code
to enable PASSTHROUGH_SG_TO_MEM_NOALLOC only for ERL 0 case.