[PATCH libata-dev-2.6:sil24 00/07] sil24: misc fixes

[PATCH libata-dev-2.6:sil24 00/07] sil24: misc fixes

[Tejun Heo]

 Hello, Jeff & Carlos.

 These patches are misc fixes suggested by Jeff for rewritten sil24
driver.  The follwing issues are not fixed in this patchset.  I need
more info to work on these.

 * Device signature access on reset.
 * How to discern completion interrupts from error ones.
 * 64bit activation
 * Removal of PORT_RST in sil24_init_one().

[ Start of patch descriptions ]

01_sil24_add-status-emulation.patch
	: implement status register emulation

	Add back status register emulation.  It's very simple.  If the
	previous command completed successfully, we return ATA_DRDY
	for all following status register queries; otherwise, we
	return ATA_DRDY | ATA_ERR.  We can improve error reporting by
	reading CMD_ERROR register and emulating status/error values
	accordingly.

02_sil24_separate-out-error-path.patch
	: move error handling out of hot interrupt path

	Move error handling from sil24_host_intr into separate
	function - sil24_error_intr.

	Jeff, I don't think this patch actually improves readability
	and/or performance.  Is this what you wanted?

03_sil24_add-test-for-PCI-fault.patch
	: add testing for PCI fault

	On entry to interrupt handler, PORT_SLOT_STAT register is read
	first.  Check if PCI fault or device removal has occurred by
	testing the value for 0xffffffff.

04_sil24_remove-irq-disable-on-spurious-interrupt.patch
	: remove irq disable code on spurious interrupt

	If interrupt occurs on a disabled port, the driver used to
	mask the port's interrupt, but we don't know if such action is
	necessary yet and that's not what other drives do.  So, just
	do nothing and tell IRQ subsystem that it's not our interrupt.

05_sil24_mdelay-instead-of-udelay.patch
	: use longer delay function and less iteration in reset_controller

	loop 100 times with mdelay(1) instead of 1000 times with
	udelay(100) in sil24_reset_controller.

	Jeff, is this what you wanted?  If not, just ignore this
	patch.  The following patches will apply without this one.

06_sil24_add-flusing-after-masking-irq.patch
	: add IO flushing after masking irq during initialization

	Add IO flushing after masking irq during initialization.

07_sil24_add-FIXME-ata_device_add-return_value.patch
	: add FIXME comment above ata_device_add

	Add FIXME comment above ata_device_add.

[ End of patch descriptions ]

 Thanks.

--
tejun

Re: [PATCH libata-dev-2.6:sil24 01/07] sil24: implement status register emulation

[Tejun Heo]

01_sil24_add-status-emulation.patch

	Add back status register emulation.  It's very simple.  If the
	previous command completed successfully, we return ATA_DRDY
	for all following status register queries; otherwise, we
	return ATA_DRDY | ATA_ERR.  We can improve error reporting by
	reading CMD_ERROR register and emulating status/error values
	accordingly.

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

 sata_sil24.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletion(-)

Index: work/drivers/scsi/sata_sil24.c
===================================================================
--- work.orig/drivers/scsi/sata_sil24.c	2005-07-30 19:13:39.000000000 +0900
+++ work/drivers/scsi/sata_sil24.c	2005-07-30 19:13:39.000000000 +0900
@@ -217,6 +217,7 @@ struct sil24_cmd_block {
  * here from the previous interrupt.
  */
 struct sil24_port_priv {
+	u8 stat;
 	void *port;
 	struct sil24_cmd_block *cmd_block;	/* 32 cmd blocks */
 	dma_addr_t cmd_block_dma;		/* DMA base addr for them */
@@ -328,7 +329,8 @@ static struct ata_port_info sil24_port_i
 
 static u8 sil24_check_status(struct ata_port *ap)
 {
-	return ATA_DRDY;
+	struct sil24_port_priv *pp = ap->private_data;
+	return pp->stat;
 }
 
 static u8 sil24_check_err(struct ata_port *ap)
@@ -489,6 +491,7 @@ static inline void sil24_host_intr(struc
 
 	slot_stat = readl(port + PORT_SLOT_STAT);
 	if (!(slot_stat & HOST_SSTAT_ATTN)) {
+		pp->stat = ATA_DRDY;
 		if (qc)
 			ata_qc_complete(qc, 0);
 	} else {
@@ -510,6 +513,8 @@ static inline void sil24_host_intr(struc
 		       "  stat=0x%x irq=0x%x cmd_err=%d sstatus=0x%x serror=0x%x\n",
 		       ap->id, ap->port_no, slot_stat, irq_stat, cmd_err, sstatus, serror);
 
+		/* FIXME: We can do better and emulate the err register too. */
+		pp->stat = ATA_DRDY | ATA_ERR;
 		if (qc)
 			ata_qc_complete(qc, ATA_ERR);
 
@@ -574,6 +579,7 @@ static int sil24_port_start(struct ata_p
 	}
 	memset(cb, 0, cb_size);
 
+	pp->stat = ATA_DRDY;
 	pp->port = hpriv->port_base + ap->port_no * PORT_REGS_SIZE;
 	pp->cmd_block = cb;
 	pp->cmd_block_dma = cb_dma;

Re: [PATCH libata-dev-2.6:sil24 01/07] sil24: implement status register emulation

[Jeff Garzik]

Tejun Heo wrote:
> 01_sil24_add-status-emulation.patch
> 
> 	Add back status register emulation.  It's very simple.  If the
> 	previous command completed successfully, we return ATA_DRDY
> 	for all following status register queries; otherwise, we
> 	return ATA_DRDY | ATA_ERR.  We can improve error reporting by
> 	reading CMD_ERROR register and emulating status/error values
> 	accordingly.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

NAK.  As described and demonstrated in other emails, we want to obtain 
the Error/Status values from the D2H Register FIS, -not- emulate them.

	Jeff

Re: [PATCH libata-dev-2.6:sil24 02/07] sil24: move error handling out of hot interrupt path

[Tejun Heo]

02_sil24_separate-out-error-path.patch

	Move error handling from sil24_host_intr into separate
	function - sil24_error_intr.

	Jeff, I don't think this patch actually improves readability
	and/or performance.  Is this what you wanted?

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

 sata_sil24.c |   59 +++++++++++++++++++++++++++++++++--------------------------
 1 files changed, 33 insertions(+), 26 deletions(-)

Index: work/drivers/scsi/sata_sil24.c
===================================================================
--- work.orig/drivers/scsi/sata_sil24.c	2005-07-30 19:13:39.000000000 +0900
+++ work/drivers/scsi/sata_sil24.c	2005-07-30 19:13:40.000000000 +0900
@@ -482,6 +482,37 @@ static void sil24_eng_timeout(struct ata
 	sil24_reset_controller(ap);
 }
 
+static void sil24_error_intr(struct ata_port *ap, u32 slot_stat)
+{
+	struct ata_queued_cmd *qc = ata_qc_from_tag(ap, ap->active_tag);
+	struct sil24_port_priv *pp = ap->private_data;
+	void *port = pp->port;
+	u32 irq_stat, cmd_err, sstatus, serror;
+
+	irq_stat = readl(port + PORT_IRQ_STAT);
+	cmd_err = readl(port + PORT_CMD_ERR);
+	sstatus = readl(port + PORT_SSTATUS);
+	serror = readl(port + PORT_SERROR);
+
+	/* Clear IRQ/errors */
+	writel(irq_stat, port + PORT_IRQ_STAT);
+	if (cmd_err)
+		writel(cmd_err, port + PORT_CMD_ERR);
+	if (serror)
+		writel(serror, port + PORT_SERROR);
+
+	printk(KERN_ERR DRV_NAME " ata%u: error interrupt on port%d\n"
+	       "  stat=0x%x irq=0x%x cmd_err=%d sstatus=0x%x serror=0x%x\n",
+	       ap->id, ap->port_no, slot_stat, irq_stat, cmd_err, sstatus, serror);
+
+	/* FIXME: We can do better and emulate the err register too. */
+	pp->stat = ATA_DRDY | ATA_ERR;
+	if (qc)
+		ata_qc_complete(qc, ATA_ERR);
+
+	sil24_reset_controller(ap);
+}
+
 static inline void sil24_host_intr(struct ata_port *ap)
 {
 	struct ata_queued_cmd *qc = ata_qc_from_tag(ap, ap->active_tag);
@@ -494,32 +525,8 @@ static inline void sil24_host_intr(struc
 		pp->stat = ATA_DRDY;
 		if (qc)
 			ata_qc_complete(qc, 0);
-	} else {
-		u32 irq_stat, cmd_err, sstatus, serror;
-
-		irq_stat = readl(port + PORT_IRQ_STAT);
-		cmd_err = readl(port + PORT_CMD_ERR);
-		sstatus = readl(port + PORT_SSTATUS);
-		serror = readl(port + PORT_SERROR);
-
-		/* Clear IRQ/errors */
-		writel(irq_stat, port + PORT_IRQ_STAT);
-		if (cmd_err)
-			writel(cmd_err, port + PORT_CMD_ERR);
-		if (serror)
-			writel(serror, port + PORT_SERROR);
-
-		printk(KERN_ERR DRV_NAME " ata%u: error interrupt on port%d\n"
-		       "  stat=0x%x irq=0x%x cmd_err=%d sstatus=0x%x serror=0x%x\n",
-		       ap->id, ap->port_no, slot_stat, irq_stat, cmd_err, sstatus, serror);
-
-		/* FIXME: We can do better and emulate the err register too. */
-		pp->stat = ATA_DRDY | ATA_ERR;
-		if (qc)
-			ata_qc_complete(qc, ATA_ERR);
-
-		sil24_reset_controller(ap);
-	}
+	} else
+		sil24_error_intr(ap, slot_stat);
 }
 
 static irqreturn_t sil24_interrupt(int irq, void *dev_instance, struct pt_regs *regs)

Re: [PATCH libata-dev-2.6:sil24 02/07] sil24: move error handling out of hot interrupt path

[Jeff Garzik]

Tejun Heo wrote:
> 02_sil24_separate-out-error-path.patch
> 
> 	Move error handling from sil24_host_intr into separate
> 	function - sil24_error_intr.
> 
> 	Jeff, I don't think this patch actually improves readability
> 	and/or performance.  Is this what you wanted?

Yes.

It improves readability and maintainability by clearly separating the 
error and non-error paths.

It improves performance by removing an uncommon path -- error -- from 
the hot path (command completion).

	Jeff

Re: [PATCH libata-dev-2.6:sil24 03/07] sil24: add testing for PCI fault

[Tejun Heo]

03_sil24_add-test-for-PCI-fault.patch

	On entry to interrupt handler, PORT_SLOT_STAT register is read
	first.  Check if PCI fault or device removal has occurred by
	testing the value for 0xffffffff.

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

 sata_sil24.c |    6 ++++++
 1 files changed, 6 insertions(+)

Index: work/drivers/scsi/sata_sil24.c
===================================================================
--- work.orig/drivers/scsi/sata_sil24.c	2005-07-30 19:13:40.000000000 +0900
+++ work/drivers/scsi/sata_sil24.c	2005-07-30 19:13:40.000000000 +0900
@@ -539,6 +539,12 @@ static irqreturn_t sil24_interrupt(int i
 
 	status = readl(hpriv->host_base + HOST_IRQ_STAT);
 
+	if (status == 0xffffffff) {
+		printk(KERN_ERR DRV_NAME ": IRQ status == 0xffffffff, "
+		       "possible PCI fault or device removal\n");
+		goto out;
+	}
+
 	if (!(status & IRQ_STAT_4PORTS))
 		goto out;
 

Re: [PATCH libata-dev-2.6:sil24 03/07] sil24: add testing for PCI fault

[Jeff Garzik]

Tejun Heo wrote:
> 03_sil24_add-test-for-PCI-fault.patch
> 
> 	On entry to interrupt handler, PORT_SLOT_STAT register is read
> 	first.  Check if PCI fault or device removal has occurred by
> 	testing the value for 0xffffffff.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK

Re: [PATCH libata-dev-2.6:sil24 04/07] sil24: remove irq disable code on spurious interrupt

[Tejun Heo]

04_sil24_remove-irq-disable-on-spurious-interrupt.patch

	If interrupt occurs on a disabled port, the driver used to
	mask the port's interrupt, but we don't know if such action is
	necessary yet and that's not what other drives do.  So, just
	do nothing and tell IRQ subsystem that it's not our interrupt.

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

 sata_sil24.c |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)

Index: work/drivers/scsi/sata_sil24.c
===================================================================
--- work.orig/drivers/scsi/sata_sil24.c	2005-07-30 19:13:40.000000000 +0900
+++ work/drivers/scsi/sata_sil24.c	2005-07-30 19:13:40.000000000 +0900
@@ -553,17 +553,12 @@ static irqreturn_t sil24_interrupt(int i
 	for (i = 0; i < host_set->n_ports; i++)
 		if (status & (1 << i)) {
 			struct ata_port *ap = host_set->ports[i];
-			if (ap && !(ap->flags & ATA_FLAG_PORT_DISABLED))
+			if (ap && !(ap->flags & ATA_FLAG_PORT_DISABLED)) {
 				sil24_host_intr(host_set->ports[i]);
-			else {
-				u32 tmp;
-				printk(KERN_WARNING DRV_NAME
-				       ": spurious interrupt from port %d\n", i);
-				tmp = readl(hpriv->host_base + HOST_CTRL);
-				tmp &= ~(1 << i);
-				writel(tmp, hpriv->host_base + HOST_CTRL);
-			}
-			handled++;
+				handled++;
+			} else
+				printk(KERN_ERR DRV_NAME
+				       ": interrupt from disabled port %d\n", i);
 		}
 
 	spin_unlock(&host_set->lock);

Re: [PATCH libata-dev-2.6:sil24 04/07] sil24: remove irq disable code on spurious interrupt

[Jeff Garzik]

Tejun Heo wrote:
> 04_sil24_remove-irq-disable-on-spurious-interrupt.patch
> 
> 	If interrupt occurs on a disabled port, the driver used to
> 	mask the port's interrupt, but we don't know if such action is
> 	necessary yet and that's not what other drives do.  So, just
> 	do nothing and tell IRQ subsystem that it's not our interrupt.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK

Re: [PATCH libata-dev-2.6:sil24 05/07] sil24: use longer delay function and less iteration in reset_controller

[Tejun Heo]

05_sil24_mdelay-instead-of-udelay.patch

	loop 100 times with mdelay(1) instead of 1000 times with
	udelay(100) in sil24_reset_controller.

	Jeff, is this what you wanted?  If not, just ignore this
	patch.  The following patches will apply without this one.

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

 sata_sil24.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

Index: work/drivers/scsi/sata_sil24.c
===================================================================
--- work.orig/drivers/scsi/sata_sil24.c	2005-07-30 19:13:40.000000000 +0900
+++ work/drivers/scsi/sata_sil24.c	2005-07-30 19:13:40.000000000 +0900
@@ -445,9 +445,13 @@ static void sil24_reset_controller(struc
 	writel(PORT_CS_DEV_RST, port + PORT_CTRL_STAT);
 	readl(port + PORT_CTRL_STAT);	/* sync */
 
-	/* Max ~100ms */
-	for (cnt = 0; cnt < 1000; cnt++) {
-		udelay(100);
+	/*
+	 * Max ~100ms.
+	 * FIXME: 100ms is an arbitrary value, get spec and use
+	 *        accurate value.
+	 */
+	for (cnt = 0; cnt < 100; cnt++) {
+		mdelay(1);
 		tmp = readl(port + PORT_CTRL_STAT);
 		if (!(tmp & PORT_CS_DEV_RST))
 			break;

Re: [PATCH libata-dev-2.6:sil24 05/07] sil24: use longer delay function and less iteration in reset_controller

[Jeff Garzik]

Tejun Heo wrote:
> 05_sil24_mdelay-instead-of-udelay.patch
> 
> 	loop 100 times with mdelay(1) instead of 1000 times with
> 	udelay(100) in sil24_reset_controller.
> 
> 	Jeff, is this what you wanted?  If not, just ignore this
> 	patch.  The following patches will apply without this one.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
>  sata_sil24.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> Index: work/drivers/scsi/sata_sil24.c
> ===================================================================
> --- work.orig/drivers/scsi/sata_sil24.c	2005-07-30 19:13:40.000000000 +0900
> +++ work/drivers/scsi/sata_sil24.c	2005-07-30 19:13:40.000000000 +0900
> @@ -445,9 +445,13 @@ static void sil24_reset_controller(struc
>  	writel(PORT_CS_DEV_RST, port + PORT_CTRL_STAT);
>  	readl(port + PORT_CTRL_STAT);	/* sync */
>  
> -	/* Max ~100ms */
> -	for (cnt = 0; cnt < 1000; cnt++) {
> -		udelay(100);
> +	/*
> +	 * Max ~100ms.
> +	 * FIXME: 100ms is an arbitrary value, get spec and use
> +	 *        accurate value.
> +	 */
> +	for (cnt = 0; cnt < 100; cnt++) {
> +		mdelay(1);
>  		tmp = readl(port + PORT_CTRL_STAT);

I forget what the discussion resulted in, for this change.

For error handling, we typically want to move to process context (if not 
there already), and then use msleep() and friends.

	Jeff

Re: [PATCH libata-dev-2.6:sil24 05/07] sil24: use longer delay function and less iteration in reset_controller

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> 05_sil24_mdelay-instead-of-udelay.patch
>>
>>     loop 100 times with mdelay(1) instead of 1000 times with
>>     udelay(100) in sil24_reset_controller.
>>
>>     Jeff, is this what you wanted?  If not, just ignore this
>>     patch.  The following patches will apply without this one.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>
>>  sata_sil24.c |   10 +++++++---
>>  1 files changed, 7 insertions(+), 3 deletions(-)
>>
>> Index: work/drivers/scsi/sata_sil24.c
>> ===================================================================
>> --- work.orig/drivers/scsi/sata_sil24.c    2005-07-30 
>> 19:13:40.000000000 +0900
>> +++ work/drivers/scsi/sata_sil24.c    2005-07-30 19:13:40.000000000 +0900
>> @@ -445,9 +445,13 @@ static void sil24_reset_controller(struc
>>      writel(PORT_CS_DEV_RST, port + PORT_CTRL_STAT);
>>      readl(port + PORT_CTRL_STAT);    /* sync */
>>  
>> -    /* Max ~100ms */
>> -    for (cnt = 0; cnt < 1000; cnt++) {
>> -        udelay(100);
>> +    /*
>> +     * Max ~100ms.
>> +     * FIXME: 100ms is an arbitrary value, get spec and use
>> +     *        accurate value.
>> +     */
>> +    for (cnt = 0; cnt < 100; cnt++) {
>> +        mdelay(1);
>>          tmp = readl(port + PORT_CTRL_STAT);
> 
> 
> I forget what the discussion resulted in, for this change.
> 
> For error handling, we typically want to move to process context (if not 
> there already), and then use msleep() and friends.
> 

  Yeap, that's exactly what I did in sil24 driver against new EH/NCQ 
helpers.  As we don't have EH thread luxury in mainline yet, I had to 
convert it to mdelay.  Once new EH (in whatever form) is in place, we 
should be able to convert all mdelay's in reset routines to msleep's.

-- 
tejun

Re: [PATCH libata-dev-2.6:sil24 06/07] sil24: add IO flushing after masking irq during initialization

[Tejun Heo]

06_sil24_add-flusing-after-masking-irq.patch

	Add IO flushing after masking irq during initialization.

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

 sata_sil24.c |    1 +
 1 files changed, 1 insertion(+)

Index: work/drivers/scsi/sata_sil24.c
===================================================================
--- work.orig/drivers/scsi/sata_sil24.c	2005-07-30 19:13:40.000000000 +0900
+++ work/drivers/scsi/sata_sil24.c	2005-07-30 19:13:41.000000000 +0900
@@ -706,6 +706,7 @@ static int sil24_init_one(struct pci_dev
 
 	/* Mask interrupts during initialization */
 	writel(0, host_base + HOST_CTRL);
+	readl(host_base + HOST_CTRL);	/* sync */
 
 	for (i = 0; i < probe_ent->n_ports; i++) {
 		void *port = port_base + i * PORT_REGS_SIZE;

Re: [PATCH libata-dev-2.6:sil24 06/07] sil24: add IO flushing after masking irq during initialization

[Jeff Garzik]

Tejun Heo wrote:
> 06_sil24_add-flusing-after-masking-irq.patch
> 
> 	Add IO flushing after masking irq during initialization.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK

Re: [PATCH libata-dev-2.6:sil24 07/07] sil24: add FIXME comment above ata_device_add

[Tejun Heo]

07_sil24_add-FIXME-ata_device_add-return_value.patch

	Add FIXME comment above ata_device_add.

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

 sata_sil24.c |    1 +
 1 files changed, 1 insertion(+)

Index: work/drivers/scsi/sata_sil24.c
===================================================================
--- work.orig/drivers/scsi/sata_sil24.c	2005-07-30 19:13:41.000000000 +0900
+++ work/drivers/scsi/sata_sil24.c	2005-07-30 19:13:41.000000000 +0900
@@ -768,6 +768,7 @@ static int sil24_init_one(struct pci_dev
 
 	pci_set_master(pdev);
 
+	/* FIXME: check ata_device_add return value */
 	ata_device_add(probe_ent);
 
 	kfree(probe_ent);

Re: [PATCH libata-dev-2.6:sil24 07/07] sil24: add FIXME comment above ata_device_add

[Jeff Garzik]

Tejun Heo wrote:
> 07_sil24_add-FIXME-ata_device_add-return_value.patch
> 
> 	Add FIXME comment above ata_device_add.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK

Re: [PATCH libata-dev-2.6:sil24 00/07] sil24: misc fixes

[Jeff Garzik]

Tejun Heo wrote:
>  Hello, Jeff & Carlos.
> 
>  These patches are misc fixes suggested by Jeff for rewritten sil24
> driver.  The follwing issues are not fixed in this patchset.  I need
> more info to work on these.
> 
>  * Device signature access on reset.
>  * How to discern completion interrupts from error ones.
>  * 64bit activation
>  * Removal of PORT_RST in sil24_init_one().
> 
> [ Start of patch descriptions ]

Just sent review comments on this patchset.  I'll now delete it from my 
inbox, and wait for the next iteration.

	Jeff

Re: [PATCH libata-dev-2.6:sil24 00/07] sil24: misc fixes

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>>  Hello, Jeff & Carlos.
>>
>>  These patches are misc fixes suggested by Jeff for rewritten sil24
>> driver.  The follwing issues are not fixed in this patchset.  I need
>> more info to work on these.
>>
>>  * Device signature access on reset.
>>  * How to discern completion interrupts from error ones.
>>  * 64bit activation
>>  * Removal of PORT_RST in sil24_init_one().
>>
>> [ Start of patch descriptions ]
> 
> 
> Just sent review comments on this patchset.  I'll now delete it from my 
> inbox, and wait for the next iteration.
> 
>     Jeff

  Cool, I'll soon repost acked patches + some trivial patches.  Edward 
Falk is currently working on new sil24 driver and will follow with 
patches implementing missing features.

  Thanks. ;-)

-- 
tejun