Re: [PATCH v5 07/10] lpfc: Rework I/O flush ordering when unloading driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: lpfc: memory leak of iocbq left on txcmplq during unload
--

commit 0147a0023ef276c5ea7aa246240c55db9bc5ed52
Author: Nigel Kirkland <nkirkland2304@gmail.com>

lpfc: Rework I/O flush ordering when unloading driver

This commit reworks I/O flush ordering during driver unload by avoiding
cancelling IOCB_ABORTING returns in lpfc_els_abort() and flushing the
PHBA WQ prior to resource unset.

> diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
> index 6460127bcc7b0..7352cb6e584bf 100644
> --- a/drivers/scsi/lpfc/lpfc_init.c
> +++ b/drivers/scsi/lpfc/lpfc_init.c
> @@ -13514,6 +13514,9 @@ lpfc_sli4_hba_unset(struct lpfc_hba *phba)
>  	/* Stop the SLI4 device port */
>  	if (phba->pport)
>  		phba->pport->work_port_events = 0;
> +
> +	/* All IO completed and queues released. Free the IOCBs. */
> +	lpfc_free_iocb_list(phba);
>  }

[ ... ]

> diff --git a/drivers/scsi/lpfc/lpfc_nportdisc.c b/drivers/scsi/lpfc/lpfc_nportdisc.c
> index 2c8d995a45bf8..f917a5bcfd025 100644
> --- a/drivers/scsi/lpfc/lpfc_nportdisc.c
> +++ b/drivers/scsi/lpfc/lpfc_nportdisc.c
[ ... ]
> @@ -271,7 +272,13 @@ lpfc_els_abort(struct lpfc_hba *phba, struct lpfc_nodelist *ndlp)
>  		retval = lpfc_sli_issue_abort_iotag(phba, pring, iocb, NULL);
>  		spin_unlock_irq(&phba->hbalock);
>  
> -		if (retval && test_bit(FC_UNLOADING, &phba->pport->load_flag)) {
> +		/* An abort that fails here is just cancelled when the driver is
> +		 * going offline.  However, if the abort failure is because the
> +		 * IOCB is already getting aborted, don't cancel.  Just let it
> +		 * complete.
> +		 */
> +		if (test_bit(FC_UNLOADING, &phba->pport->load_flag) &&
> +		    retval && retval != IOCB_ABORTING) {

[Severity: Medium]
If an ELS abort returns IOCB_ABORTING during driver unload, this explicitly
leaves the IOCB on the ring's txcmplq to complete naturally.

However, does the unload path guarantee this will complete? In lpfc_init.c,
lpfc_sli4_hba_unset() calls lpfc_sli4_xri_exchange_busy_wait() which checks
abts_* lists, but does not wait for txcmplq to empty before calling
lpfc_sli4_disable_intr() to disable interrupts.

When lpfc_sli4_hba_unset() finally calls lpfc_free_iocb_list(), it only frees
the available IOCBs in phba->lpfc_iocb_list:

lpfc_init.c:lpfc_free_iocb_list() {
    ...
    list_for_each_entry_safe(iocbq_entry, iocbq_next,
                             &phba->lpfc_iocb_list, list) {
        list_del(&iocbq_entry->list);
        kfree(iocbq_entry);
    ...
}

Because the pending IOCB is still linked in txcmplq and hasn't been returned
to the free list, does this result in a memory leak of the lpfc_iocbq
structure?

>  			list_del_init(&iocb->list);
>  			list_add_tail(&iocb->list, &drv_cmpl_list);
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928181757.21959-1-nkirkland2304@gmail.com?part=7