Re: [PATCH/RFC] libata-dev: update timeout for PIO polling

[Jeff Garzik <jgarzik@pobox.com>]

Albert Lee wrote:
> Jeff,
> 
> In the bug reported by Ralf Müller:
>   (http://marc.theaimsgroup.com/?l=linux-ide&m=113629906809278&w=2)
> 
>   When the command times out, the qc is completed by the time
> out handler. This might cause null pointer dereference if
> the command is running in the PIO polling thread.
> 
> Changes:
> - Update the timeout for PIO polling to 5 minutes.
>   This should be long enough to prevent the timeout handler from
>   interfering with the PIO polling thread.  
> 
> Patch against the libata-dev irq-pio branch 
> (61420e147a706ee7c7a902008045547fb2a2a330).
> 
> For your review, thanks.
> 
> Albert
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> 
> ==========
> 
> --- irq-pio/include/linux/libata.h	2006-01-05 15:16:32.000000000 +0800
> +++ update_tmout/include/linux/libata.h	2006-01-06 10:17:34.000000000 +0800
> @@ -141,6 +141,7 @@ enum {
>  	ATA_TMOUT_CDB_QUICK	= 5 * HZ,
>  	ATA_TMOUT_INTERNAL	= 30 * HZ,
>  	ATA_TMOUT_INTERNAL_QUICK = 5 * HZ,
> +	ATA_TMOUT_LONG		= 300 * HZ,	/* looks long enough */
>  
>  	/* ATA bus states */
>  	BUS_UNKNOWN		= 0,
> --- irq-pio/drivers/scsi/libata-core.c	2006-01-05 15:16:29.000000000 +0800
> +++ update_tmout/drivers/scsi/libata-core.c	2006-01-06 10:41:23.000000000 +0800
> @@ -3866,6 +3866,21 @@ int ata_qc_issue_prot(struct ata_queued_
>  		}
>  	}
>  
> +	/* The PIO polling thread has its own timeout handling.
> +	 * It won't loop forever. Even if it takes a long time,
> +	 * we can wait the PIO polling thread to finish its work.
> +	 * No need to stop the polling thread from the timeout handler.
> +	 * So the timeout is updated to a long time here. This can prevent
> +	 * the timeout handler from interfering with the PIO polling thread.
> +	 */
> +	if (qc->tf.flags & ATA_TFLAG_POLLING &&
> +	    qc->scsicmd &&
> +	    qc->scsicmd->eh_timeout.function) {
> +		DPRINTK("ata%u: timer updated for PIO polling\n", ap->id);
> +		mod_timer(&qc->scsicmd->eh_timeout,
> +			  jiffies + ATA_TMOUT_LONG);
> +	}

While this may solve the problem, unfortunately we really don't want to 
be directly touching the scsi EH timer.

A better method would be to implement ->eh_timed_out() SCSI host 
template hook, which is the preferred way to accomplish what you are 
attempting here.

	Jeff