On 15.1.2016 10:48, Hannes Reinecke wrote:
> Upon command completion the lpfc driver would call ->done()
> on the scsi command before taking the host lock and
> releasing the command internally.
> This opens up a race window there this command might be re-used
> after ->done(), leading to a double completion on the same command.
I agree that a driver should clean up the command before calling
->done, but this driver uses a list based system where a command
can't be reused only until it was returned to the list,
so I don't understand how a 'done' before internal free could
cause an issue other than a failed lpfc_get_scsi_buf in .queuecommand.
Is your issue related to the abort_handler
(maybe cmd->host_scribble = NULL; changes the abort handler flow)?
Tomas
>
> This patch takes the host lock before accessing the scsi command,
> and disconnect the internal command under the same lock.
>
> Signed-off-by: Hannes Reinecke <hare@xxxxxxx>
> ---
> drivers/scsi/lpfc/lpfc_scsi.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
> index 4679ed4..974af28 100644
> --- a/drivers/scsi/lpfc/lpfc_scsi.c
> +++ b/drivers/scsi/lpfc/lpfc_scsi.c
> @@ -3908,9 +3908,16 @@ lpfc_scsi_cmd_iocb_cmpl(struct lpfc_hba *phba, struct lpfc_iocbq *pIocbIn,
> uint32_t logit = LOG_FCP;
>
> /* Sanity check on return of outstanding command */
> - if (!(lpfc_cmd->pCmd))
> + spin_lock_irqsave(&phba->hbalock, flags);
> + if (!(lpfc_cmd->pCmd)) {
> + spin_unlock_irqrestore(&phba->hbalock, flags);
> return;
> + }
> cmd = lpfc_cmd->pCmd;
> + cmd->host_scribble = NULL;
> + lpfc_cmd->pCmd = NULL;
> + spin_unlock_irqrestore(&phba->hbalock, flags);
> +
> shost = cmd->device->host;
>
> lpfc_cmd->result = (pIocbOut->iocb.un.ulpWord[4] & IOERR_PARAM_MASK);
> @@ -4125,10 +4132,6 @@ lpfc_scsi_cmd_iocb_cmpl(struct lpfc_hba *phba, struct lpfc_iocbq *pIocbIn,
> cmd->scsi_done(cmd);
>
> if (phba->cfg_poll & ENABLE_FCP_RING_POLLING) {
> - spin_lock_irqsave(&phba->hbalock, flags);
> - lpfc_cmd->pCmd = NULL;
> - spin_unlock_irqrestore(&phba->hbalock, flags);
> -
> /*
> * If there is a thread waiting for command completion
> * wake up the thread.
> @@ -4141,10 +4144,6 @@ lpfc_scsi_cmd_iocb_cmpl(struct lpfc_hba *phba, struct lpfc_iocbq *pIocbIn,
> return;
> }
>
> - spin_lock_irqsave(&phba->hbalock, flags);
> - lpfc_cmd->pCmd = NULL;
> - spin_unlock_irqrestore(&phba->hbalock, flags);
> -
> /*
> * If there is a thread waiting for command completion
> * wake up the thread.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
