[PATCH 2/5] sil24: add sil24_restart_controller

[PATCH 2/5] sil24: add sil24_restart_controller

[Tejun Heo]

When an error condition is raised by device via D2H FIS or SDB.  sil24
controller should be restarted by setting PORT_CS_INIT and waiting
until PORT_CS_RDY is asserted instead of resetting the controller.
This patch implements sil24_restart_controller for those cases.  This
patch also makes sure that PORT_CS_RDY is asserted on
sil24_reset_controller completion.

Signed-off-by: Tejun Heo <htejun@gmail.com>

--

Jeff, the patch named "sil24: add constants" is 1/5 and should be
applied first.  Sorry.

Index: work/drivers/scsi/sata_sil24.c
===================================================================
--- work.orig/drivers/scsi/sata_sil24.c	2005-11-16 16:54:10.000000000 +0900
+++ work/drivers/scsi/sata_sil24.c	2005-11-16 16:58:01.000000000 +0900
@@ -486,6 +486,31 @@ static void sil24_irq_clear(struct ata_p
 	/* unused */
 }
 
+static int __sil24_restart_controller(void __iomem *port)
+{
+	u32 tmp;
+	int cnt;
+
+	writel(PORT_CS_INIT, port + PORT_CTRL_STAT);
+
+	/* Max ~100ms */
+	for (cnt = 0; cnt < 1000; cnt++) {
+		tmp = readl(port + PORT_CTRL_STAT);
+		if (tmp & PORT_CS_RDY)
+			return 0;
+		udelay(100);
+	}
+
+	return -1;
+}
+
+static void sil24_restart_controller(struct ata_port *ap)
+{
+	if (__sil24_restart_controller((void __iomem *)ap->ioaddr.cmd_addr))
+		printk(KERN_ERR DRV_NAME
+		       " ata%u: failed to restart controller\n", ap->id);
+}
+
 static int __sil24_reset_controller(void __iomem *port)
 {
 	int cnt;
@@ -505,7 +530,11 @@ static int __sil24_reset_controller(void
 
 	if (tmp & PORT_CS_DEV_RST)
 		return -1;
-	return 0;
+
+	if (tmp & PORT_CS_RDY)
+		return 0;
+
+	return __sil24_restart_controller(port);
 }
 
 static void sil24_reset_controller(struct ata_port *ap)
@@ -577,6 +606,7 @@ static void sil24_error_intr(struct ata_
 		 */
 		sil24_update_tf(ap);
 		err_mask = ac_err_mask(pp->tf.command);
+		sil24_restart_controller(ap);
 	} else {
 		/*
 		 * Other errors.  libata currently doesn't have any
@@ -584,12 +614,11 @@ static void sil24_error_intr(struct ata_
 		 * ATA_ERR.
 		 */
 		err_mask = AC_ERR_OTHER;
+		sil24_reset_controller(ap);
 	}
 
 	if (qc)
 		ata_qc_complete(qc, err_mask);
-
-	sil24_reset_controller(ap);
 }
 
 static inline void sil24_host_intr(struct ata_port *ap)

Re: [PATCH 2/5] sil24: add sil24_restart_controller

[Jeff Garzik]

Tejun Heo wrote:
> When an error condition is raised by device via D2H FIS or SDB.  sil24
> controller should be restarted by setting PORT_CS_INIT and waiting
> until PORT_CS_RDY is asserted instead of resetting the controller.
> This patch implements sil24_restart_controller for those cases.  This
> patch also makes sure that PORT_CS_RDY is asserted on
> sil24_reset_controller completion.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> --
> 
> Jeff, the patch named "sil24: add constants" is 1/5 and should be
> applied first.  Sorry.
> 
> Index: work/drivers/scsi/sata_sil24.c
> ===================================================================
> --- work.orig/drivers/scsi/sata_sil24.c	2005-11-16 16:54:10.000000000 +0900
> +++ work/drivers/scsi/sata_sil24.c	2005-11-16 16:58:01.000000000 +0900
> @@ -486,6 +486,31 @@ static void sil24_irq_clear(struct ata_p
>  	/* unused */
>  }
>  
> +static int __sil24_restart_controller(void __iomem *port)
> +{
> +	u32 tmp;
> +	int cnt;
> +
> +	writel(PORT_CS_INIT, port + PORT_CTRL_STAT);
> +
> +	/* Max ~100ms */
> +	for (cnt = 0; cnt < 1000; cnt++) {
> +		tmp = readl(port + PORT_CTRL_STAT);
> +		if (tmp & PORT_CS_RDY)
> +			return 0;
> +		udelay(100);
> +	}
> +
> +	return -1;
> +}

NAK, this potentially soaks up an entire timer tick (~100ms).

It's probably better to schedule_work(), and poll using msleep().  Or 
you could cut the worst case down to 10ms.

FWIW, I have no idea what the max is, I don't see it in the docs :(

	Jeff

Re: [PATCH 2/5] sil24: add sil24_restart_controller

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> When an error condition is raised by device via D2H FIS or SDB.  sil24
>> controller should be restarted by setting PORT_CS_INIT and waiting
>> until PORT_CS_RDY is asserted instead of resetting the controller.
>> This patch implements sil24_restart_controller for those cases.  This
>> patch also makes sure that PORT_CS_RDY is asserted on
>> sil24_reset_controller completion.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>
>> -- 
>>
>> Jeff, the patch named "sil24: add constants" is 1/5 and should be
>> applied first.  Sorry.
>>
>> Index: work/drivers/scsi/sata_sil24.c
>> ===================================================================
>> --- work.orig/drivers/scsi/sata_sil24.c    2005-11-16 
>> 16:54:10.000000000 +0900
>> +++ work/drivers/scsi/sata_sil24.c    2005-11-16 16:58:01.000000000 +0900
>> @@ -486,6 +486,31 @@ static void sil24_irq_clear(struct ata_p
>>      /* unused */
>>  }
>>  
>> +static int __sil24_restart_controller(void __iomem *port)
>> +{
>> +    u32 tmp;
>> +    int cnt;
>> +
>> +    writel(PORT_CS_INIT, port + PORT_CTRL_STAT);
>> +
>> +    /* Max ~100ms */
>> +    for (cnt = 0; cnt < 1000; cnt++) {
>> +        tmp = readl(port + PORT_CTRL_STAT);
>> +        if (tmp & PORT_CS_RDY)
>> +            return 0;
>> +        udelay(100);
>> +    }
>> +
>> +    return -1;
>> +}
> 
> 
> NAK, this potentially soaks up an entire timer tick (~100ms).
> 
> It's probably better to schedule_work(), and poll using msleep().  Or 
> you could cut the worst case down to 10ms.
> 
> FWIW, I have no idea what the max is, I don't see it in the docs :(
> 

Yeap, it might consume quite some ticks.  So did 
sili24_reset_controller.  I almost infinitely prefer to take all these 
resume/reset/request sense stuff into EH thread rather than using 
separate mechanism.  No?

-- 
tejun

Re: [PATCH 2/5] sil24: add sil24_restart_controller

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
> 
>> Tejun Heo wrote:
>>
>>> When an error condition is raised by device via D2H FIS or SDB.  sil24
>>> controller should be restarted by setting PORT_CS_INIT and waiting
>>> until PORT_CS_RDY is asserted instead of resetting the controller.
>>> This patch implements sil24_restart_controller for those cases.  This
>>> patch also makes sure that PORT_CS_RDY is asserted on
>>> sil24_reset_controller completion.
>>>
>>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>>
>>> -- 
>>>
>>> Jeff, the patch named "sil24: add constants" is 1/5 and should be
>>> applied first.  Sorry.
>>>
>>> Index: work/drivers/scsi/sata_sil24.c
>>> ===================================================================
>>> --- work.orig/drivers/scsi/sata_sil24.c    2005-11-16 
>>> 16:54:10.000000000 +0900
>>> +++ work/drivers/scsi/sata_sil24.c    2005-11-16 16:58:01.000000000 
>>> +0900
>>> @@ -486,6 +486,31 @@ static void sil24_irq_clear(struct ata_p
>>>      /* unused */
>>>  }
>>>  
>>> +static int __sil24_restart_controller(void __iomem *port)
>>> +{
>>> +    u32 tmp;
>>> +    int cnt;
>>> +
>>> +    writel(PORT_CS_INIT, port + PORT_CTRL_STAT);
>>> +
>>> +    /* Max ~100ms */
>>> +    for (cnt = 0; cnt < 1000; cnt++) {
>>> +        tmp = readl(port + PORT_CTRL_STAT);
>>> +        if (tmp & PORT_CS_RDY)
>>> +            return 0;
>>> +        udelay(100);
>>> +    }
>>> +
>>> +    return -1;
>>> +}
>>
>>
>>
>> NAK, this potentially soaks up an entire timer tick (~100ms).
>>
>> It's probably better to schedule_work(), and poll using msleep().  Or 
>> you could cut the worst case down to 10ms.
>>
>> FWIW, I have no idea what the max is, I don't see it in the docs :(
>>
> 
> Yeap, it might consume quite some ticks.  So did 
> sili24_reset_controller.  I almost infinitely prefer to take all these 
> resume/reset/request sense stuff into EH thread rather than using 
> separate mechanism.  No?

Well, I clearly like autosensing over doing it in the EH thread.  :)

But in general, as long as its done in a way that is maintainable across 
all drivers that implement ->eng_timeout, I am definitely open to 
approaches using the EH thread.  The concept of the EH thread should 
continue to exist even after SCSI is made optional.

	Jeff

Re: [PATCH 2/5] sil24: add sil24_restart_controller

[Edward Falk]

> +static int __sil24_restart_controller(void __iomem *port)

Hi; I'm curious:  why the leading underscores in the function name?

	-ed falk

Re: [PATCH 2/5] sil24: add sil24_restart_controller

[Tejun Heo]

On Wed, Nov 16, 2005 at 10:26:02AM -0800, Edward Falk wrote:
> 
> 
> >+static int __sil24_restart_controller(void __iomem *port)
> 
> Hi; I'm curious:  why the leading underscores in the function name?
> 
> 	-ed falk

Hi, Edward.

It's the same as sil24_reset_controller.  __sil24_restart_controller
takes 'void __iomem *port' and sil24_restart_controller takes 'struct
ata_port *ap)' and prints error message when it fails.  The
underscored versions are needed as they are called from sil24_init_one
where we don't have ap yet.  __sil24_restart_controller isn't directly
 called from sil24_init_one but __sil24_reset_controller calls it to
kick the controller if the controller doesn't turn on PORT_CS_RDY
after reset is complete.

I don't know if this is necessary.  I was just trying to be on the
safe side.

-- 
tejun