On Fri, 2012-05-11 at 10:38 -0400, Jörn Engel wrote:
> This patch isn't done yet. I'm having enough trouble to untangle the
> qla2xxx side alone, so all other transports are ignored for now and
> some break on build. But ignoring that issue, is it correct(er) to
> always return 1 from tcm_qla2xxx_check_stop_free()?
>
Correct, this is intended for users of target_submit_cmd() w/
TARGET_SCF_ACK_KREF, and used by the response callback accounting done
within transport_cmd_check_stop() -> TFO->check_stop_free().
> It clearly doesn't make any sense to check the return value of kref_put.
> However, it is unclear to me what purpose this return value is supposed
> to serve. It seems to be indicating whether the command in question has
> been freed. Well, it has in the sense that the kfref is gone and it is
> no longer safe to access it.
>
You'll recall this was originally added to the target-core response path
to signal the final release of the descriptor between the target-core
response accountting, and HW fabric ACK generated by qla2xxx to release
qla_tgt_cmd descriptor memory.
So the way things currently work, always returning 1 here for
->check_stop_free() with TARGET_SCF_ACK_KREF usage is certainly not the
right thing to do..
Are you intending to try to get rid of the TFO->check_stop_free() return
value all together, or something else..?
--nab
> Signed-off-by: Joern Engel <joern@xxxxxxxxx>
> ---
> drivers/scsi/qla2xxx/tcm_qla2xxx.c | 3 ++-
> drivers/target/target_core_transport.c | 4 ++--
> include/target/target_core_fabric.h | 2 +-
> 3 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> index b360746..d6a4dd6 100644
> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> @@ -431,7 +431,8 @@ static void tcm_qla2xxx_free_cmd(struct qla_tgt_cmd *cmd)
> */
> static int tcm_qla2xxx_check_stop_free(struct se_cmd *se_cmd)
> {
> - return target_put_sess_cmd(se_cmd->se_sess, se_cmd);
> + target_put_sess_cmd(se_cmd->se_sess, se_cmd);
> + return 1;
> }
>
> /* tcm_qla2xxx_release_cmd - Callback from TCM Core to release underlying
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 9742f10..99343e0 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -3658,9 +3658,9 @@ static void target_release_cmd_kref(struct kref *kref)
> * @se_sess: session to reference
> * @se_cmd: command descriptor to drop
> */
> -int target_put_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cmd)
> +void target_put_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cmd)
> {
> - return kref_put(&se_cmd->cmd_kref, target_release_cmd_kref);
> + kref_put(&se_cmd->cmd_kref, target_release_cmd_kref);
> }
> EXPORT_SYMBOL(target_put_sess_cmd);
>
> diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
> index 4e3f7a4..efee5f5 100644
> --- a/include/target/target_core_fabric.h
> +++ b/include/target/target_core_fabric.h
> @@ -130,7 +130,7 @@ int transport_check_aborted_status(struct se_cmd *, int);
> int transport_send_check_condition_and_sense(struct se_cmd *, u8, int);
>
> void target_get_sess_cmd(struct se_session *, struct se_cmd *, bool);
> -int target_put_sess_cmd(struct se_session *, struct se_cmd *);
> +void target_put_sess_cmd(struct se_session *, struct se_cmd *);
> void target_splice_sess_cmd_list(struct se_session *);
> void target_wait_for_sess_cmds(struct se_session *, int);
>
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
