Re: [PATCH 1/5] firewire: core: expand close_transaction() into its callers and remove it

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

On Mon, Oct 05, 2026 at 08:25:40PM +0900, Takashi Sakamoto wrote:
> The return value from close_transaction() is useful only in
> fw_cancel_transaction(). The other caller,
> transmit_complete_callback(), simply discards it.
> 
> Additionally, the pending transaction queue should be enumerated as soon
> as possible to prevent the issued transaction from completing.
> 
> Expand close_transaction() into its callers, and remove it.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  drivers/firewire/core-transaction.c | 66 ++++++++++++++---------------
>  1 file changed, 31 insertions(+), 35 deletions(-)
> ... 
> @@ -201,9 +191,6 @@ static void transmit_complete_callback(struct fw_packet *packet,
>  					      packet->speed, status, packet->timestamp);
>  
>  	switch (status) {
> -	case ACK_COMPLETE:
> -		close_transaction(t, card, RCODE_COMPLETE, packet->timestamp);
> -		break;
>  	case ACK_PENDING:
>  	{
>  		unsigned int delta;
> @@ -218,27 +205,36 @@ static void transmit_complete_callback(struct fw_packet *packet,
>  		// local destination never runs in any type of IRQ context.
>  		scoped_guard(spinlock_irqsave, &card->transactions.lock)
>  			start_split_transaction_timeout(t, delta);
> -		break;
> +		return;
>  	}
> +	case ACK_COMPLETE:
> +		status = RCODE_COMPLETE;
> +		break;
>  	case ACK_BUSY_X:
>  	case ACK_BUSY_A:
>  	case ACK_BUSY_B:
> -		close_transaction(t, card, RCODE_BUSY, packet->timestamp);
> +		status = RCODE_BUSY;
>  		break;
>  	case ACK_DATA_ERROR:
> -		close_transaction(t, card, RCODE_DATA_ERROR, packet->timestamp);
> +		status = RCODE_DATA_ERROR;
>  		break;
>  	case ACK_TYPE_ERROR:
> -		close_transaction(t, card, RCODE_TYPE_ERROR, packet->timestamp);
> +		status = RCODE_TYPE_ERROR;
>  		break;
>  	default:
> -		/*
> -		 * In this case the ack is really a juju specific
> -		 * rcode, so just forward that to the callback.
> -		 */
> -		close_transaction(t, card, status, packet->timestamp);
> +		// In this case the ack is really a juju specific rcode, so just forward that to
> +		// the callback.
>  		break;
>  	}
> +
> +	// NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for
> +	// local destination never runs in any type of IRQ context.
> +	scoped_guard(spinlock_irqsave, &card->transactions.lock) {
> +		if (!find_and_pop_transaction_entry(card, iter == t))
> +			return;
> +	}

I realized that this expansion has a bug to suppress the following
callback, since the condition 'iter == t' includes the local variable 't' 
while 'find_and_pop_transaction_entry()' macro initiates the 't' with NULL
for its internal use. When receiving one of the above error acknowledge
codes (e.g. RCODE_GENERATION at bus reset),
'find_and_pop_transaction_entry()' is evaluated as NULL, then the above
return statement is executed always.

> +	invoke_callback(t, status, packet->timestamp, NULL, 0);
>  }
>  
>  static void fw_fill_request(struct fw_packet *packet, int tcode, int tlabel,

I'll post v2 patch after enough testing.


Regards

Takashi Sakamoto