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.