From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <stable-owner@vger.kernel.org>
Received: from stargate.chelsio.com ([12.32.117.8]:49411 "EHLO
        stargate.chelsio.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1753595AbdEJQDK (ORCPT
        <rfc822;stable@vger.kernel.org>); Wed, 10 May 2017 12:03:10 -0400
Date: Wed, 10 May 2017 21:33:03 +0530
From: Varun Prakash <varun@chelsio.com>
To: "Nicholas A. Bellinger" <nab@linux-iscsi.org>
Cc: Bart Van Assche <bart.vanassche@sandisk.com>,
        target-devel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH 2/6] target/iscsi: Call .iscsit_release_cmd() once
Message-ID: <20170510160252.GA1674@chelsio.com>
References: <20170330171244.8346-1-bart.vanassche@sandisk.com>
 <20170330171244.8346-3-bart.vanassche@sandisk.com>
 <1491173945.8846.48.camel@haakon3.risingtidesystems.com>
 <20170404050649.GA1662@chelsio.com>
 <20170413074454.GA1724@chelsio.com>
 <1493699611.23202.30.camel@haakon3.risingtidesystems.com>
 <20170507125220.GA1706@chelsio.com>
 <1494316166.16894.55.camel@haakon3.risingtidesystems.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1494316166.16894.55.camel@haakon3.risingtidesystems.com>
Sender: stable-owner@vger.kernel.org
List-ID: <stable.vger.kernel.org>

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.