Re: [PATCH 1/5] scsi: elx: efct: check the HW state before allocating an HIO

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

Pre-existing issues:
- [High] Hardware I/O object (hio) is permanently leaked on WQE submission failure.
- [High] Hardware I/O object (HIO) is permanently leaked on successful ELS and BLS completions.
--

--- Patch [1]: [PATCH 1/5] scsi: elx: efct: check the HW state before allocating an HIO ---
commit 9f7986d6f2e47353fa2d9861b16b7f4d623a7b71
Author: Ali Ahmet Memis <[email protected]>

scsi: elx: efct: check the HW state before allocating an HIO

This commit moves the hardware state check in efct_els_hw_srrs_send to
happen before allocating a hardware I/O (HIO) object. This prevents an
HIO object leak that previously occurred when the hardware state was not
active.

> diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
> index b79c6a7ea7912..15c37ff1d52e4 100644
> --- a/drivers/scsi/elx/efct/efct_hw.c
> +++ b/drivers/scsi/elx/efct/efct_hw.c
> @@ -2706,18 +2706,18 @@ efct_els_hw_srrs_send(struct efc *efc, struct efc_disc_io *io)
>  	u32 sge0_flags;
>  	u32 sge1_flags;
>  
> -	hio = efct_hw_io_alloc(hw);
> -	if (!hio) {
> -		pr_err("HIO alloc failed\n");
> -		return -EIO;
> -	}
> -
>  	if (hw->state != EFCT_HW_STATE_ACTIVE) {
>  		efc_log_debug(hw->os,
>  			      "cannot send SRRS, HW state=%d\n", hw->state);
>  		return -EIO;
>  	}
>  
> +	hio = efct_hw_io_alloc(hw);
> +	if (!hio) {
> +		pr_err("HIO alloc failed\n");
> +		return -EIO;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this function leak the
newly allocated hio object if WQE submission fails further down?

If efct_hw_wq_write() fails at the end of efct_els_hw_srrs_send():

	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;

The function returns the error but it doesn't appear to call
efct_hw_io_free(hw, hio), leaving the hio object stranded. Could this
eventually exhaust the adapter's hardware IO pool during transient queue
full conditions?

This same pattern appears to exist in efct_hw_bls_send() when either
sli_xmit_bls_rsp64_wqe() or efct_hw_wq_write() fails.

>  	hio->done = efct_els_ssrs_send_cb;

[Severity: High]
This is also a pre-existing issue, but is the hio object leaked on
successful completions as well?

Looking at the callback assigned here:

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;
}

The callback executes the completion handler but doesn't drop the
reference to the passed hio object. 

The overarching WQE completion handler efct_hw_wq_process_io() also seems
to drop the WQE and call done(), but leaves the hio pointer allocated.

Are we missing an efct_hw_io_free(hw, hio) call on the success paths
for ELS and BLS completions?

>  	hio->arg  = io;
>

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