diff mbox series

[05/36] target: Don't remove command too early

Message ID bb728dcbc73942cda3ae64d0d26195c9a284996d.1657149962.git.Thinh.Nguyen@synopsys.com (mailing list archive)
State Superseded
Headers show
Series usb: gadget: f_tcm: Enhance UASP driver | expand

Commit Message

Thinh Nguyen July 6, 2022, 11:34 p.m. UTC
A command completion is asynchronous, regardless if an abort command is
executed. Don't just free the command before its completion. Similarly,
a TMR command is not completed until its response is completed. The
freeing of the command can be done by the target user through
target_generic_free_cmd().

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/target/target_core_transport.c | 7 -------
 1 file changed, 7 deletions(-)

Comments

Dmitry Bogdanov July 7, 2022, 1:16 p.m. UTC | #1
Hi Thinh,

On Wed, Jul 06, 2022 at 04:34:55PM -0700, Thinh Nguyen wrote:
> A command completion is asynchronous, regardless if an abort command is
> executed. Don't just free the command before its completion. Similarly,
> a TMR command is not completed until its response is completed. The
> freeing of the command can be done by the target user through
> target_generic_free_cmd().
> 
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>  drivers/target/target_core_transport.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 7838dc20f713..105d3b0e470f 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -836,10 +836,6 @@ static void target_handle_abort(struct se_cmd *cmd)
>  	}
>  
>  	WARN_ON_ONCE(kref_read(&cmd->cmd_kref) == 0);
> -
> -	transport_lun_remove_cmd(cmd);
> -
> -	transport_cmd_check_stop_to_fabric(cmd);
>  }
>  
>  static void target_abort_work(struct work_struct *work)
> @@ -3553,9 +3549,6 @@ static void target_tmr_work(struct work_struct *work)
>  		goto aborted;
>  
>  	cmd->se_tfo->queue_tm_rsp(cmd);
> -
> -	transport_lun_remove_cmd(cmd);
> -	transport_cmd_check_stop_to_fabric(cmd);
>  	return;
>  
>  aborted:
Those functions are not about to free the command.
transport_lun_remove_cmd is for remove command from the state/tmr list.
transport_cmd_check_stop_to_fabric is for notify a fabric driver to
decrease the command kref that it owns. And eventually to wake
target_put_cmd_and_wait() in core_tmr_abort_task().

Those functions do always are called after a final response has been sent
(STATUS, CHECK_CONDITION,etc).
Those functions do not break the abort functionality. But this patch
does.
Thinh Nguyen July 8, 2022, 11:40 p.m. UTC | #2
On 7/7/2022, Dmitry Bogdanov wrote:
> Hi Thinh,
>
> On Wed, Jul 06, 2022 at 04:34:55PM -0700, Thinh Nguyen wrote:
>> A command completion is asynchronous, regardless if an abort command is
>> executed. Don't just free the command before its completion. Similarly,
>> a TMR command is not completed until its response is completed. The
>> freeing of the command can be done by the target user through
>> target_generic_free_cmd().
>>
>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> ---
>>   drivers/target/target_core_transport.c | 7 -------
>>   1 file changed, 7 deletions(-)
>>
>> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
>> index 7838dc20f713..105d3b0e470f 100644
>> --- a/drivers/target/target_core_transport.c
>> +++ b/drivers/target/target_core_transport.c
>> @@ -836,10 +836,6 @@ static void target_handle_abort(struct se_cmd *cmd)
>>   	}
>>   
>>   	WARN_ON_ONCE(kref_read(&cmd->cmd_kref) == 0);
>> -
>> -	transport_lun_remove_cmd(cmd);
>> -
>> -	transport_cmd_check_stop_to_fabric(cmd);
>>   }
>>   
>>   static void target_abort_work(struct work_struct *work)
>> @@ -3553,9 +3549,6 @@ static void target_tmr_work(struct work_struct *work)
>>   		goto aborted;
>>   
>>   	cmd->se_tfo->queue_tm_rsp(cmd);
>> -
>> -	transport_lun_remove_cmd(cmd);
>> -	transport_cmd_check_stop_to_fabric(cmd);
>>   	return;
>>   
>>   aborted:
> Those functions are not about to free the command.
> transport_lun_remove_cmd is for remove command from the state/tmr list.
> transport_cmd_check_stop_to_fabric is for notify a fabric driver to
> decrease the command kref that it owns. And eventually to wake
> target_put_cmd_and_wait() in core_tmr_abort_task().
>
> Those functions do always are called after a final response has been sent
> (STATUS, CHECK_CONDITION,etc).
> Those functions do not break the abort functionality. But this patch
> does.
>
>

Looks like I misunderstood those functions' role.

Thanks,
Thinh
diff mbox series

Patch

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 7838dc20f713..105d3b0e470f 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -836,10 +836,6 @@  static void target_handle_abort(struct se_cmd *cmd)
 	}
 
 	WARN_ON_ONCE(kref_read(&cmd->cmd_kref) == 0);
-
-	transport_lun_remove_cmd(cmd);
-
-	transport_cmd_check_stop_to_fabric(cmd);
 }
 
 static void target_abort_work(struct work_struct *work)
@@ -3553,9 +3549,6 @@  static void target_tmr_work(struct work_struct *work)
 		goto aborted;
 
 	cmd->se_tfo->queue_tm_rsp(cmd);
-
-	transport_lun_remove_cmd(cmd);
-	transport_cmd_check_stop_to_fabric(cmd);
 	return;
 
 aborted: