Re: [PATCH v2 2/5] scsi: elx: efct: free the HIO when the WQE cannot be built

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The Hardware I/O context (hio) is leaked on every successful completion of ELS, CT, and BLS commands.
--

--- Patch [2]: [PATCH v2 2/5] scsi: elx: efct: free the HIO when the WQE cannot be built ---
commit d739587d3893fac37f827b9d5a082304c53cf476
Author: Ali Ahmet Memis <[email protected]>
Date:   Thu Aug 6 20:22:02 2026 +0000

    scsi: elx: efct: free the HIO when the WQE cannot be built
    
    efct_els_hw_srrs_send() and efct_hw_bls_send() allocate an HIO, ask sli4
    to build a WQE into it, and give up when that fails without putting the
    HIO back:

[ ... ]

> diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
> index 15c37ff1d52e4..6cc48fa3e656a 100644
> --- a/drivers/scsi/elx/efct/efct_hw.c
> +++ b/drivers/scsi/elx/efct/efct_hw.c
> @@ -2609,6 +2609,7 @@ efct_hw_bls_send(struct efct *efct, u32 type, struct sli_bls_params *bls_params,
>  	if (sli_xmit_bls_rsp64_wqe(&hw->sli, hio->wqe.wqebuf,
>  				   &bls, bls_params)) {
>  		efc_log_err(hw->os, "XMIT_BLS_RSP64 WQE error\n");
> +		efct_hw_io_free(hw, hio);
>  		return -EIO;
>  	}
>  

[Severity: High]
This is a pre-existing issue, but does this code also leak the hio on the
success path?

Looking at the completion callback for BLS commands, efct_bls_send_rjt_cb()
receives the hio but completely ignores it:

drivers/scsi/elx/efct/efct_scsi.c:efct_bls_send_rjt_cb() {
    struct efct_io *io = app;

    efct_scsi_io_free(io);
    return 0;
}

Because the generic hardware completion handler efct_hw_wq_process_io() doesn't
free the hio either, doesn't this result in the hio being leaked on every
successful completion?

If the pool of hardware IO contexts exhausts, won't efct_hw_io_alloc() fail for
all subsequent requests, breaking Fibre Channel discovery, ELS, and BLS
operations?

> @@ -2820,24 +2821,27 @@ efct_els_hw_srrs_send(struct efc *efc, struct efc_disc_io *io)
>  		rc = -EIO;
>  	}
>  
> -	if (rc == 0) {
> -		hio->xbusy = true;
> +	if (rc) {
> +		efct_hw_io_free(hw, hio);
> +		return rc;
> +	}

[Severity: High]
This is a pre-existing issue, but does this function suffer from the same leak
on the success path?

The ELS completion callback receives the hio but also ignores it:

drivers/scsi/elx/efct/efct_hw.c:efct_els_ssrs_send_cb() {
    struct efc_disc_io *io = arg;

    efc_disc_io_complete(io, length, status, ext_status);
    return 0;
}

Should the completion callbacks be responsible for calling efct_hw_io_free(), or
should that be handled by the generic caller?

> -		/*
> -		 * Add IO to active io wqe list before submitting, in case the
> -		 * wcqe processing preempts this thread.
> -		 */
> -		hio->wq->use_count++;
> -		rc = efct_hw_wq_write(hio->wq, &hio->wqe);
> -		if (rc >= 0) {
> -			/* non-negative return is success */
> -			rc = 0;
> -		} else {
> -			/* failed to write wqe, remove from active wqe list */
> -			efc_log_err(hw->os,
> -				    "sli_queue_write failed: %d\n", rc);
> -			hio->xbusy = false;
> -		}
> +	hio->xbusy = true;
> +
> +	/*
> +	 * Add IO to active io wqe list before submitting, in case the
> +	 * wcqe processing preempts this thread.
> +	 */
> +	hio->wq->use_count++;
> +	rc = efct_hw_wq_write(hio->wq, &hio->wqe);
> +	if (rc >= 0) {
> +		/* non-negative return is success */
> +		rc = 0;
> +	} else {
> +		/* failed to write wqe, remove from active wqe list */
> +		efc_log_err(hw->os,
> +			    "sli_queue_write failed: %d\n", rc);
> +		hio->xbusy = false;
>  	}
>  
>  	return rc;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2