Re: [PATCH] aic94xx: Don't free ABORT_TASK SCBs that are timed out (Was: Re: aic94xx: failing on high load)

[Keith Hopkins <vger@hopnet.net>]

On 02/20/2008 02:44 AM, Darrick J. Wong wrote:
> If we send an ABORT_TASK ascb that doesn't return within the timeout period,
> we should not free that ascb because the sequencer is still holding onto it.
> Hopefully it will fix what James Bottomley describes below:
> 
> On Tue, Feb 19, 2008 at 10:22:20AM -0600, James Bottomley wrote:
> 
>> Unfortunately, there's a bug in TMF timeout handling in the driver, it
>> leaves the sequencer entry pending, but frees the ascb.  If the
>> sequencer ever picks this up it will get very confused, as it does a
>> while down in the trace:
>>
>>> aic94xx: BUG:sequencer:dl:no ascb?!
>>> aic94xx: BUG:sequencer:dl:no ascb?!
>> That's where the sequencer adds an ascb to the done list that we've
>> already freed.  From this point on confusion reigns and the error
>> handler eventually offlines the device.
>>
>> I'll see if I can come up with patches to fix this ... or at least
>> mitigate the problems it causes.
> 
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> ---
> 
>  drivers/scsi/aic94xx/aic94xx_tmf.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/aic94xx/aic94xx_tmf.c b/drivers/scsi/aic94xx/aic94xx_tmf.c
> index b52124f..4b24bd3 100644
> --- a/drivers/scsi/aic94xx/aic94xx_tmf.c
> +++ b/drivers/scsi/aic94xx/aic94xx_tmf.c
> @@ -463,7 +463,7 @@ int asd_abort_task(struct sas_task *task)
>  						       AIC94XX_SCB_TIMEOUT);
>  		spin_lock_irqsave(&task->task_state_lock, flags);
>  		if (leftover < 1)
> -			res = TMF_RESP_FUNC_FAILED;
> +			goto out_not_reported;
>  		if (task->task_state_flags & SAS_TASK_STATE_DONE)
>  			res = TMF_RESP_FUNC_COMPLETE;
>  		spin_unlock_irqrestore(&task->task_state_lock, flags);
> @@ -487,6 +487,11 @@ out:
>  	asd_ascb_free(ascb);
>  	ASD_DPRINTK("task 0x%p aborted, res: 0x%x\n", task, res);
>  	return res;
> +
> +out_not_reported:
> +	spin_unlock_irqrestore(&task->task_state_lock, flags);
> +	ASD_DPRINTK("task 0x%p aborted? but not reported.\n", task);
> +	return res;
>  }
>  
>  /**
> -

Hi Darrick,

  Is this the only patch for ascb sequencer use after free problems, or are you still looking into that?

--Keith