[PATCH] libata: disable_irq() during polling IDENTIFY

[PATCH] libata: disable_irq() during polling IDENTIFY

[Albert Lee]

Problem:
  Kernel got "irq 5: nobody cared" when using
  libata + polling IDENTIFY + Promise 20275 adapter + Benq DW1620 drive.

  Detail message available in bug 8441 (http://bugzilla.kernel.org/show_bug.cgi?id=8441).

Cause:
 The Benq DW1620 drive raises INTRQ during polling IDENTIFY PACKET DEVICE,
 even if nIEN = 1.

Proposed fix:
  disable_irq() during polling IDENTIFY to work around, the same as what IDE subsystem does.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
Some controller like Intel ICH4 is immune from the problem, with the same kernel
and the same Benq DW1620 drive. So, adding the ATA_FLAG_IDENT_IRQ_OFF flag for
those adapters that needs the workaround. Patch against 2.6.21.1 for your review, thanks.

diff -Nrup linux-2.6.21.1-ori/drivers/ata/libata-core.c linux-2.6.21.1-mod2/drivers/ata/libata-core.c
--- linux-2.6.21.1-ori/drivers/ata/libata-core.c	2007-05-04 11:22:23.000000000 +0800
+++ linux-2.6.21.1-mod2/drivers/ata/libata-core.c	2007-05-07 11:00:02.000000000 +0800
@@ -1427,6 +1427,7 @@ int ata_dev_read_id(struct ata_device *d
 		    unsigned int flags, u16 *id)
 {
 	struct ata_port *ap = dev->ap;
+	struct ata_host *host = ap->host;
 	unsigned int class = *p_class;
 	struct ata_taskfile tf;
 	unsigned int err_mask = 0;
@@ -1466,8 +1467,29 @@ int ata_dev_read_id(struct ata_device *d
 	 */
 	tf.flags |= ATA_TFLAG_POLLING;
 
+	/* Disable IRQ since some devices like Benq DW1620 raises INTRQ
+	 * when IDENTIFY PACKET DEVICE, even with polling IDENTIFY.
+	 */
+	if (ap->flags & ATA_FLAG_IDENT_IRQ_OFF) {
+		if (host->irq)
+			disable_irq(host->irq);
+
+		if (host->irq2)
+			disable_irq(host->irq2);
+	}
+
 	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE,
 				     id, sizeof(id[0]) * ATA_ID_WORDS);
+
+	/* Re-enable IRQ */
+	if (ap->flags & ATA_FLAG_IDENT_IRQ_OFF) {
+		if (host->irq)
+			enable_irq(host->irq);
+
+		if (host->irq2)
+			enable_irq(host->irq2);
+	}
+
 	if (err_mask) {
 		if (err_mask & AC_ERR_NODEV_HINT) {
 			DPRINTK("ata%u.%d: NODEV after polling detection\n",
diff -Nrup linux-2.6.21.1-ori/drivers/ata/pata_pdc2027x.c linux-2.6.21.1-mod2/drivers/ata/pata_pdc2027x.c
--- linux-2.6.21.1-ori/drivers/ata/pata_pdc2027x.c	2007-04-28 05:49:26.000000000 +0800
+++ linux-2.6.21.1-mod2/drivers/ata/pata_pdc2027x.c	2007-05-07 10:44:33.000000000 +0800
@@ -214,7 +214,7 @@ static struct ata_port_info pdc2027x_por
 	{
 		.sht		= &pdc2027x_sht,
 		.flags		= ATA_FLAG_NO_LEGACY | ATA_FLAG_SLAVE_POSS |
-		                  ATA_FLAG_MMIO,
+				  ATA_FLAG_MMIO | ATA_FLAG_IDENT_IRQ_OFF,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= ATA_UDMA5, /* udma0-5 */
@@ -224,7 +224,7 @@ static struct ata_port_info pdc2027x_por
 	{
 		.sht		= &pdc2027x_sht,
 		.flags		= ATA_FLAG_NO_LEGACY | ATA_FLAG_SLAVE_POSS |
-                        	  ATA_FLAG_MMIO,
+				  ATA_FLAG_MMIO | ATA_FLAG_IDENT_IRQ_OFF,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= ATA_UDMA6, /* udma0-6 */
diff -Nrup linux-2.6.21.1-ori/include/linux/libata.h linux-2.6.21.1-mod2/include/linux/libata.h
--- linux-2.6.21.1-ori/include/linux/libata.h	2007-04-28 05:49:26.000000000 +0800
+++ linux-2.6.21.1-mod2/include/linux/libata.h	2007-05-07 12:25:05.000000000 +0800
@@ -174,6 +174,7 @@ enum {
 	ATA_FLAG_SETXFER_POLLING= (1 << 14), /* use polling for SETXFER */
 	ATA_FLAG_IGN_SIMPLEX	= (1 << 15), /* ignore SIMPLEX */
 	ATA_FLAG_NO_IORDY	= (1 << 16), /* controller lacks iordy */
+	ATA_FLAG_IDENT_IRQ_OFF	= (1 << 17), /* disable irq when IDENTIFY */
 
 	/* The following flag belongs to ap->pflags but is kept in
 	 * ap->flags because it's referenced in many LLDs and will be

Re: [PATCH] libata: disable_irq() during polling IDENTIFY

[Tejun Heo]

[cc'ing Bartlomiej and Mark, hi]

Hello, Albert.

Albert Lee wrote:
> Problem:
>   Kernel got "irq 5: nobody cared" when using
>   libata + polling IDENTIFY + Promise 20275 adapter + Benq DW1620 drive.
> 
>   Detail message available in bug 8441 (http://bugzilla.kernel.org/show_bug.cgi?id=8441).
> 
> Cause:
>  The Benq DW1620 drive raises INTRQ during polling IDENTIFY PACKET DEVICE,
>  even if nIEN = 1.

Aieeee...

> Proposed fix:
>   disable_irq() during polling IDENTIFY to work around, the same as what IDE subsystem does.
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> ---
> Some controller like Intel ICH4 is immune from the problem, with the same kernel
> and the same Benq DW1620 drive. So, adding the ATA_FLAG_IDENT_IRQ_OFF flag for
> those adapters that needs the workaround. Patch against 2.6.21.1 for your review, thanks.

I guess piix is masking the interrupt at the host side.

Another interesting aspect is that the SATA spec says the device is
recommended to ignore nIEN while the controller is recommended to not
set nIEN when it sends FIS to the device.  ie. nIEN should be
implemented on the host controller.  I bet there are controllers out
there which doesn't do host-side masking and there will be more and more
devices which ignore nIEN, so we're likely to see similar problems on
SATA too.

> +	/* Disable IRQ since some devices like Benq DW1620 raises INTRQ
> +	 * when IDENTIFY PACKET DEVICE, even with polling IDENTIFY.
> +	 */
> +	if (ap->flags & ATA_FLAG_IDENT_IRQ_OFF) {
> +		if (host->irq)
> +			disable_irq(host->irq);
> +
> +		if (host->irq2)
> +			disable_irq(host->irq2);
> +	}
> +
>  	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE,
>  				     id, sizeof(id[0]) * ATA_ID_WORDS);
> +
> +	/* Re-enable IRQ */
> +	if (ap->flags & ATA_FLAG_IDENT_IRQ_OFF) {
> +		if (host->irq)
> +			enable_irq(host->irq);
> +
> +		if (host->irq2)
> +			enable_irq(host->irq2);
> +	}
> +

Yeap, this is how IDE deals with polling commands but I'm not sure how
it's supposed to work with PCI IRQ sharing.  Bartlomiej, can you
enlighten me here?

Also, this is a problem for not only IDENTIFY but all polling commands.

One solution I can think of is to let IRQ handler ack IRQ
unconditionally during polling commands - ie. just read the TF Status
register once and tell the IRQ subsystem that the IRQ is handled.  This
shouldn't affect the operation of polling as the only side effect of
reading Status is clearing pending IRQ && will give us a nice way to
deal with the SATA bridge chip which chokes on nIEN.  Considering the
sorry state of nIEN in SATA, I guess this might be the best way to deal
with this.

Albert, can you please test whether this works?  Modifying
ata_interrupt() such that it reads TF Status if ATA_TFLAG_POLLING should
do the trick.

Thanks.

-- 
tejun

Re: [PATCH] libata: disable_irq() during polling IDENTIFY

[Alan Cox]

> Yeap, this is how IDE deals with polling commands but I'm not sure how
> it's supposed to work with PCI IRQ sharing.  Bartlomiej, can you
> enlighten me here?

Simple answer: Badly. If you've got the IRQ shared it mucks up the
behaviour of the other device especially when its doing PIO.

Re: [PATCH] libata: disable_irq() during polling IDENTIFY

[Tejun Heo]

Alan Cox wrote:
>> Yeap, this is how IDE deals with polling commands but I'm not sure how
>> it's supposed to work with PCI IRQ sharing.  Bartlomiej, can you
>> enlighten me here?
> 
> Simple answer: Badly. If you've got the IRQ shared it mucks up the
> behaviour of the other device especially when its doing PIO.

OIC, I've been thinking I must be missing something but it's just
supposed to work that way.  Doesn't sound too attractive.  :-(

Thanks.

-- 
tejun

Re: [PATCH] libata: disable_irq() during polling IDENTIFY

[Mark Lord]

Tejun Heo wrote:
> Alan Cox wrote:
>>> Yeap, this is how IDE deals with polling commands but I'm not sure how
>>> it's supposed to work with PCI IRQ sharing.  Bartlomiej, can you
>>> enlighten me here?
>> Simple answer: Badly. If you've got the IRQ shared it mucks up the
>> behaviour of the other device especially when its doing PIO.
> 
> OIC, I've been thinking I must be missing something but it's just
> supposed to work that way.  Doesn't sound too attractive.  :-(

Yeah.  It's ugly -- designed for use at boot time only, really.

As I've said a few times here, the IDE Subsystem was designed/implemented
*pre-hotplug*, and though it does work well most of the time,
the world has moved on.

Cheers

Re: [PATCH] libata: disable_irq() during polling IDENTIFY

[Albert Lee]

Tejun Heo wrote:
> [cc'ing Bartlomiej and Mark, hi]
> 
> Hello, Albert.
> 
> Albert Lee wrote:
> 
>>Problem:
>>  Kernel got "irq 5: nobody cared" when using
>>  libata + polling IDENTIFY + Promise 20275 adapter + Benq DW1620 drive.
>>
>>  Detail message available in bug 8441 (http://bugzilla.kernel.org/show_bug.cgi?id=8441).
>>
>>Cause:
>> The Benq DW1620 drive raises INTRQ during polling IDENTIFY PACKET DEVICE,
>> even if nIEN = 1.
> 
> 
> Aieeee...
> 
> 
>>Proposed fix:
>>  disable_irq() during polling IDENTIFY to work around, the same as what IDE subsystem does.
>>
>>Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
>>---
>>Some controller like Intel ICH4 is immune from the problem, with the same kernel
>>and the same Benq DW1620 drive. So, adding the ATA_FLAG_IDENT_IRQ_OFF flag for
>>those adapters that needs the workaround. Patch against 2.6.21.1 for your review, thanks.
> 
> 
> I guess piix is masking the interrupt at the host side.
> 
> Another interesting aspect is that the SATA spec says the device is
> recommended to ignore nIEN while the controller is recommended to not
> set nIEN when it sends FIS to the device.  ie. nIEN should be
> implemented on the host controller.  I bet there are controllers out
> there which doesn't do host-side masking and there will be more and more
> devices which ignore nIEN, so we're likely to see similar problems on
> SATA too.
> 
> 
>>+	/* Disable IRQ since some devices like Benq DW1620 raises INTRQ
>>+	 * when IDENTIFY PACKET DEVICE, even with polling IDENTIFY.
>>+	 */
>>+	if (ap->flags & ATA_FLAG_IDENT_IRQ_OFF) {
>>+		if (host->irq)
>>+			disable_irq(host->irq);
>>+
>>+		if (host->irq2)
>>+			disable_irq(host->irq2);
>>+	}
>>+
>> 	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE,
>> 				     id, sizeof(id[0]) * ATA_ID_WORDS);
>>+
>>+	/* Re-enable IRQ */
>>+	if (ap->flags & ATA_FLAG_IDENT_IRQ_OFF) {
>>+		if (host->irq)
>>+			enable_irq(host->irq);
>>+
>>+		if (host->irq2)
>>+			enable_irq(host->irq2);
>>+	}
>>+
> 
> 
> Yeap, this is how IDE deals with polling commands but I'm not sure how
> it's supposed to work with PCI IRQ sharing.  Bartlomiej, can you
> enlighten me here?
> 
> Also, this is a problem for not only IDENTIFY but all polling commands.

Yes, other command might also assert INTRQ during polling.
However, for the specific BENQ DW1620 drive, only IDENTIFY_PACKET_DEVICE
has such behavior; other commands like READ or REQUEST_SENSE are ok.

> 
> One solution I can think of is to let IRQ handler ack IRQ
> unconditionally during polling commands - ie. just read the TF Status
> register once and tell the IRQ subsystem that the IRQ is handled.  This
> shouldn't affect the operation of polling as the only side effect of
> reading Status is clearing pending IRQ && will give us a nice way to
> deal with the SATA bridge chip which chokes on nIEN.  Considering the
> sorry state of nIEN in SATA, I guess this might be the best way to deal
> with this.
> 
> Albert, can you please test whether this works?  Modifying
> ata_interrupt() such that it reads TF Status if ATA_TFLAG_POLLING should
> do the trick.
> 

Yes, reading the Status register and acking interrupt also fixes the
problem (patch attached below). 

--
albert

diff -Nrup linux-2.6.21.1-ori/drivers/ata/libata-core.c linux-2.6.21.1-mod3/drivers/ata/libata-core.c
--- linux-2.6.21.1-ori/drivers/ata/libata-core.c	2007-05-04 11:22:23.000000000 +0800
+++ linux-2.6.21.1-mod3/drivers/ata/libata-core.c	2007-05-07 17:44:21.000000000 +0800
@@ -5224,9 +5224,14 @@ irqreturn_t ata_interrupt (int irq, void
 			struct ata_queued_cmd *qc;
 
 			qc = ata_qc_from_tag(ap, ap->active_tag);
-			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)) &&
-			    (qc->flags & ATA_QCFLAG_ACTIVE))
-				handled |= ata_host_intr(ap, qc);
+			if (qc && (qc->flags & ATA_QCFLAG_ACTIVE)) {
+				if (qc->tf.flags & ATA_TFLAG_POLLING) {
+					ata_chk_status(ap);
+					handled = 1;
+				} else {
+					handled |= ata_host_intr(ap, qc);
+				}
+			}
 		}
 	}
 

Re: [PATCH] libata: disable_irq() during polling IDENTIFY

[Tejun Heo]

Hello, Albert.

Albert Lee wrote:
> Tejun Heo wrote:
>> Also, this is a problem for not only IDENTIFY but all polling commands.
> 
> Yes, other command might also assert INTRQ during polling.
> However, for the specific BENQ DW1620 drive, only IDENTIFY_PACKET_DEVICE
> has such behavior; other commands like READ or REQUEST_SENSE are ok.

Oh I see.

>> One solution I can think of is to let IRQ handler ack IRQ
>> unconditionally during polling commands - ie. just read the TF Status
>> register once and tell the IRQ subsystem that the IRQ is handled.  This
>> shouldn't affect the operation of polling as the only side effect of
>> reading Status is clearing pending IRQ && will give us a nice way to
>> deal with the SATA bridge chip which chokes on nIEN.  Considering the
>> sorry state of nIEN in SATA, I guess this might be the best way to deal
>> with this.
>>
>> Albert, can you please test whether this works?  Modifying
>> ata_interrupt() such that it reads TF Status if ATA_TFLAG_POLLING should
>> do the trick.
> 
> Yes, reading the Status register and acking interrupt also fixes the
> problem (patch attached below). 

Good to know it works.  With or without nIEN, I think this change is a
good thing to have.  IRQ handler shouldn't interfere with polling as
both acquire lock during operation.  Polling is safer this way and
non-SFF drivers are doing something similar anyway.  I also think it
would be nice to have the SFF irq_handler clear IRQ while the port is
frozen.  This again is the default behavior of non-SFF drivers.

Any objections?

-- 
tejun

Re: [PATCH] libata: disable_irq() during polling IDENTIFY

[Albert Lee]

Tejun Heo wrote:
> Hello, Albert.
> 
> Albert Lee wrote:
> 
>>Tejun Heo wrote:
>>
>>>Also, this is a problem for not only IDENTIFY but all polling commands.
>>
>>Yes, other command might also assert INTRQ during polling.
>>However, for the specific BENQ DW1620 drive, only IDENTIFY_PACKET_DEVICE
>>has such behavior; other commands like READ or REQUEST_SENSE are ok.
> 
> 
> Oh I see.
> 
> 
>>>One solution I can think of is to let IRQ handler ack IRQ
>>>unconditionally during polling commands - ie. just read the TF Status
>>>register once and tell the IRQ subsystem that the IRQ is handled.  This
>>>shouldn't affect the operation of polling as the only side effect of
>>>reading Status is clearing pending IRQ && will give us a nice way to
>>>deal with the SATA bridge chip which chokes on nIEN.  Considering the
>>>sorry state of nIEN in SATA, I guess this might be the best way to deal
>>>with this.
>>>
>>>Albert, can you please test whether this works?  Modifying
>>>ata_interrupt() such that it reads TF Status if ATA_TFLAG_POLLING should
>>>do the trick.
>>
>>Yes, reading the Status register and acking interrupt also fixes the
>>problem (patch attached below). 
> 
> 
> Good to know it works.  With or without nIEN, I think this change is a
> good thing to have.  IRQ handler shouldn't interfere with polling as
> both acquire lock during operation. 

This reminds me of a possible issue with the patch:
Previously the polling code assumes that the interrupt handler won't interfere
with it and the polling code runs without holding ap->lock.
However, with the above patch, the interrupt handler might read the
Status register when the polling code is transfering data, etc. from the port.
Would this cause trouble to the ATA/ATAPI devices?

Should we have something like ATA_PFLAG_HSM_BUSY below, such that the interrupt
handler won't read the Status register when HSM is busy accessing the port?

--
albert

(Revised patch: Don't read the Status register when HSM is busy accessing the port)

diff -Nrup linux-2.6.21.1-ori/drivers/ata/libata-core.c linux-2.6.21.1-mod3/drivers/ata/libata-core.c
--- linux-2.6.21.1-ori/drivers/ata/libata-core.c	2007-05-04 11:22:23.000000000 +0800
+++ linux-2.6.21.1-mod3/drivers/ata/libata-core.c	2007-05-07 19:17:58.000000000 +0800
@@ -4389,6 +4389,14 @@ int ata_hsm_move(struct ata_port *ap, st
 	 */
 	WARN_ON(in_wq != ata_hsm_ok_in_wq(ap, qc));
 
+	if (in_wq) {
+		spin_lock_irqsave(ap->lock, flags);
+		ap->pflags |= ATA_PFLAG_HSM_BUSY;
+		spin_unlock_irqrestore(ap->lock, flags);
+	} else {
+		ap->pflags |= ATA_PFLAG_HSM_BUSY;
+	}
+
 fsm_start:
 	DPRINTK("ata%u: protocol %d task_state %d (dev_stat 0x%X)\n",
 		ap->print_id, qc->tf.protocol, ap->hsm_task_state, status);
@@ -4600,6 +4608,14 @@ fsm_start:
 		BUG();
 	}
 
+	if (in_wq) {
+		spin_lock_irqsave(ap->lock, flags);
+		ap->pflags &= ~ATA_PFLAG_HSM_BUSY;
+		spin_unlock_irqrestore(ap->lock, flags);
+	} else {
+		ap->pflags &= ~ATA_PFLAG_HSM_BUSY;
+	}
+
 	return poll_next;
 }
 
@@ -5224,9 +5240,14 @@ irqreturn_t ata_interrupt (int irq, void
 			struct ata_queued_cmd *qc;
 
 			qc = ata_qc_from_tag(ap, ap->active_tag);
-			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)) &&
-			    (qc->flags & ATA_QCFLAG_ACTIVE))
-				handled |= ata_host_intr(ap, qc);
+			if (qc && (qc->flags & ATA_QCFLAG_ACTIVE)) {
+				if (!(qc->tf.flags & ATA_TFLAG_POLLING)) {
+					handled |= ata_host_intr(ap, qc);
+				} else if (!(ap->pflags & ATA_PFLAG_HSM_BUSY)) {
+					ata_chk_status(ap);
+					handled = 1;
+				}
+			}
 		}
 	}
 
diff -Nrup linux-2.6.21.1-ori/include/linux/libata.h linux-2.6.21.1-mod3/include/linux/libata.h
--- linux-2.6.21.1-ori/include/linux/libata.h	2007-04-28 05:49:26.000000000 +0800
+++ linux-2.6.21.1-mod3/include/linux/libata.h	2007-05-07 18:41:01.000000000 +0800
@@ -195,6 +195,7 @@ enum {
 	ATA_PFLAG_FLUSH_PORT_TASK = (1 << 16), /* flush port task */
 	ATA_PFLAG_SUSPENDED	= (1 << 17), /* port is suspended (power) */
 	ATA_PFLAG_PM_PENDING	= (1 << 18), /* PM operation pending */
+	ATA_PFLAG_HSM_BUSY	= (1 << 19), /* HSM accessing the port */
 
 	/* struct ata_queued_cmd flags */
 	ATA_QCFLAG_ACTIVE	= (1 << 0), /* cmd not yet ack'd to scsi lyer */

Re: [PATCH] libata: disable_irq() during polling IDENTIFY

[Tejun Heo]

Albert Lee wrote:
>> Good to know it works.  With or without nIEN, I think this change is a
>> good thing to have.  IRQ handler shouldn't interfere with polling as
>> both acquire lock during operation. 
> 
> This reminds me of a possible issue with the patch:
> Previously the polling code assumes that the interrupt handler won't interfere
> with it and the polling code runs without holding ap->lock.
> However, with the above patch, the interrupt handler might read the
> Status register when the polling code is transfering data, etc. from the port.
> Would this cause trouble to the ATA/ATAPI devices?
> 
> Should we have something like ATA_PFLAG_HSM_BUSY below, such that the interrupt
> handler won't read the Status register when HSM is busy accessing the port?

Right, I forgot about that.  Wouldn't just grabbing port lock during PIO
be simpler?  There is some downside (IRQ latency) but also there are
controllers which get upset if PIO data transfer is interrupted by IRQ
from other devices.

-- 
tejun

[PATCH] libata: disable_irq() during polling IDENTIFY (take 2)

[Albert Lee]

Problem:
  Kernel got "irq 5: nobody cared" when using
  libata + polling IDENTIFY + Promise 20275 adapter + Benq DW1620 drive.

  Detail message available in bug 8441 (http://bugzilla.kernel.org/show_bug.cgi?id=8441).

Cause:
  The Benq DW1620 drive raises INTRQ during polling IDENTIFY PACKET DEVICE, 
  even if nIEN = 1.

Proposed fix:
  Ack the possibly unexpected irq in ata_interrupt().

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
Patch revised per Tejun's advice to ack the unexpected INTRQ in ata_interrupt().
Also, ap->lock is held when transfering PIO data from/to the port in case the
device doesn't like the status register to be read during data transfer.

Patch against 2.6.21.1 for your review, thanks.

diff -Nrup linux-2.6.21.1-ori/drivers/ata/libata-core.c linux-2.6.21.1-mod6/drivers/ata/libata-core.c
--- linux-2.6.21.1-ori/drivers/ata/libata-core.c	2007-05-04 11:22:23.000000000 +0800
+++ linux-2.6.21.1-mod6/drivers/ata/libata-core.c	2007-05-08 19:25:58.000000000 +0800
@@ -4490,8 +4490,17 @@ fsm_start:
 				goto fsm_start;
 			}
 
+			/* prevent ata_interrupt() from reading
+			 * status when transfering data for safty
+			 */
+			if (in_wq)
+				spin_lock_irqsave(ap->lock, flags);
+
 			atapi_pio_bytes(qc);
 
+			if (in_wq)
+				spin_unlock_irqrestore(ap->lock, flags);
+
 			if (unlikely(ap->hsm_task_state == HSM_ST_ERR))
 				/* bad ireason reported by device */
 				goto fsm_start;
@@ -4529,12 +4538,21 @@ fsm_start:
 				/* data might be corrputed */
 				qc->err_mask |= AC_ERR_DEV;
 
+				/* prevent ata_interrupt() from reading
+				 * status when transfering data for safty
+				 */
+				if (in_wq)
+					spin_lock_irqsave(ap->lock, flags);
+
 				if (!(qc->tf.flags & ATA_TFLAG_WRITE)) {
 					ata_pio_sectors(qc);
 					ata_altstatus(ap);
 					status = ata_wait_idle(ap);
 				}
 
+				if (in_wq)
+					spin_unlock_irqrestore(ap->lock, flags);
+
 				if (status & (ATA_BUSY | ATA_DRQ))
 					qc->err_mask |= AC_ERR_HSM;
 
@@ -4546,8 +4564,17 @@ fsm_start:
 				goto fsm_start;
 			}
 
+			/* prevent ata_interrupt() from reading status
+			 * when transfering data for safty
+			 */
+			if (in_wq)
+				spin_lock_irqsave(ap->lock, flags);
+
 			ata_pio_sectors(qc);
 
+			if (in_wq)
+				spin_unlock_irqrestore(ap->lock, flags);
+
 			if (ap->hsm_task_state == HSM_ST_LAST &&
 			    (!(qc->tf.flags & ATA_TFLAG_WRITE))) {
 				/* all data read */
@@ -5227,6 +5254,9 @@ irqreturn_t ata_interrupt (int irq, void
 			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)) &&
 			    (qc->flags & ATA_QCFLAG_ACTIVE))
 				handled |= ata_host_intr(ap, qc);
+			else
+				/* ack possibly unexpected irq */
+				ata_chk_status(ap);
 		}
 	}
 

Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)

[Tejun Heo]

Hello, Albert.

Just a few comments.

Albert Lee wrote:
> +			/* prevent ata_interrupt() from reading
> +			 * status when transfering data for safty
> +			 */
> +			if (in_wq)
> +				spin_lock_irqsave(ap->lock, flags);
> +

Can't we just remove in_wq and run the HSM the same way whether we're
invoking it from the interrupt handler or work queue?  The worker can
grab and release the lock outside of the HSM function and HSM can just
assume that it's running with lock held.  Am I missing something?

> @@ -5227,6 +5254,9 @@ irqreturn_t ata_interrupt (int irq, void
>  			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)) &&
>  			    (qc->flags & ATA_QCFLAG_ACTIVE))
>  				handled |= ata_host_intr(ap, qc);
> +			else
> +				/* ack possibly unexpected irq */
> +				ata_chk_status(ap);

I'm not so sure whether poking TF Status even when no command is flight
is a good idea.  The IRQ can be shared with a fairly busy device (e.g.
gigabit ethernet) and we'll be reading the status register a lot for
nothing.  I think we shouldn't do anything as before if no command is in
flight.

Also, please split it into two separate patches.  Both are pretty
significant changes and I think each deserves separate patch.

Thanks.

-- 
tejun

Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)

[Alan Cox]

On Tue, 08 May 2007 19:30:24 +0800
Albert Lee <albertcc@tw.ibm.com> wrote:

> Problem:
>   Kernel got "irq 5: nobody cared" when using
>   libata + polling IDENTIFY + Promise 20275 adapter + Benq DW1620 drive.
> 
>   Detail message available in bug 8441 (http://bugzilla.kernel.org/show_bug.cgi?id=8441).
> 
> Cause:
>   The Benq DW1620 drive raises INTRQ during polling IDENTIFY PACKET DEVICE, 
>   even if nIEN = 1.
> 
> Proposed fix:
>   Ack the possibly unexpected irq in ata_interrupt().
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

NAK

We can't do this. A PIO data transfer could take far far too long to be
under IRQ block (its already a problem on a big 32bit box when we bounce
buffer it sometimes).

Alan

Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)

[Tejun Heo]

Alan Cox wrote:
> On Tue, 08 May 2007 19:30:24 +0800
> Albert Lee <albertcc@tw.ibm.com> wrote:
> 
>> Problem:
>>   Kernel got "irq 5: nobody cared" when using
>>   libata + polling IDENTIFY + Promise 20275 adapter + Benq DW1620 drive.
>>
>>   Detail message available in bug 8441 (http://bugzilla.kernel.org/show_bug.cgi?id=8441).
>>
>> Cause:
>>   The Benq DW1620 drive raises INTRQ during polling IDENTIFY PACKET DEVICE, 
>>   even if nIEN = 1.
>>
>> Proposed fix:
>>   Ack the possibly unexpected irq in ata_interrupt().
>>
>> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> 
> NAK
> 
> We can't do this. A PIO data transfer could take far far too long to be
> under IRQ block (its already a problem on a big 32bit box when we bounce
> buffer it sometimes).

Oh well, that's how we do PIO data transfer when not polling anyway.  We
kick HSM directly from the interrupt handler and do PIO inside the
interrupt handler.  The change made here is to make polling PIO act the
same way.

-- 
tejun

Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)

[Alan Cox]

> Oh well, that's how we do PIO data transfer when not polling anyway.  We
> kick HSM directly from the interrupt handler and do PIO inside the
> interrupt handler.  The change made here is to make polling PIO act the
> same way.

Which is making a horrendous problem worse than it was before. In PIO CD
mode I'm actually losing serial characters and time its that bad.

Right now we can push the pio transfer out to run after the IRQ handler a
variety of ways and the locking for it isn't too bad. Make this change
and we are sunk.

Alan

Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)

[Tejun Heo]

Hello,

Alan Cox wrote:
>> Oh well, that's how we do PIO data transfer when not polling anyway.  We
>> kick HSM directly from the interrupt handler and do PIO inside the
>> interrupt handler.  The change made here is to make polling PIO act the
>> same way.
> 
> Which is making a horrendous problem worse than it was before. In PIO CD
> mode I'm actually losing serial characters and time its that bad.

Yeap, things get pretty choppy in PIO mode.

> Right now we can push the pio transfer out to run after the IRQ handler a
> variety of ways and the locking for it isn't too bad. Make this change
> and we are sunk.

So, are you suggesting pushing data transfer or the whole HSM to
workqueue?  I'm all for that.  I just thought it didn't make sense to
NACK the patch because it grabs locks while doing polling PIO, which
mostly happens only for EH commands when we do all the heavy lifting PIO
directly from the interrupt handler.

Anyways, if that's the plan, as Albert suggested earlier, we can just
mark that HSM is running in ap->pflags or somewhere and skip Status
clearing if it's running.  It's not like the controller is gonna raise
interrupt while it's transferring data anyway (we always have leading
status checks before data transfer).

One thing I'm worried about is that somehow I'm thinking there are
controllers/devices out there which choke if there PIO data transfer is
interrupted (timing-wise).  Is this something I just imagined up?

Thanks.

-- 
tejun

Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)

[Alan Cox]

> One thing I'm worried about is that somehow I'm thinking there are
> controllers/devices out there which choke if there PIO data transfer is
> interrupted (timing-wise).  Is this something I just imagined up?

There are a couple of problem controllers. They use the
ata_data_xfer_noirq method for ->data_xfer() which blocks local
interrupts in the method itself and in those cases we have no choice. Such
chips are all ancient thankfully.

Alan

Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)

[Tejun Heo]

Alan Cox wrote:
>> One thing I'm worried about is that somehow I'm thinking there are
>> controllers/devices out there which choke if there PIO data transfer is
>> interrupted (timing-wise).  Is this something I just imagined up?
> 
> There are a couple of problem controllers. They use the
> ata_data_xfer_noirq method for ->data_xfer() which blocks local
> interrupts in the method itself and in those cases we have no choice. Such
> chips are all ancient thankfully.

OIC, using separate data_xfer() definitely makes sense.

-- 
tejun

Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)

[Alan Cox]

> So, are you suggesting pushing data transfer or the whole HSM to
> workqueue?  I'm all for that.  I just thought it didn't make sense to

I was thinking data transfer but HSM itself might work too, I'd not even
thought of push that much out.

> Anyways, if that's the plan, as Albert suggested earlier, we can just
> mark that HSM is running in ap->pflags or somewhere and skip Status
> clearing if it's running.  It's not like the controller is gonna raise
> interrupt while it's transferring data anyway (we always have leading
> status checks before data transfer).

And the controllers with a hidden FIFO magically defer the IRQ until the
FIFO empties so that the events occur in the expected sequence for the
spec even if the drive transfer to the fifo already finished and the host
is still emptying it.

Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)

[Tejun Heo]

Alan Cox wrote:
>> So, are you suggesting pushing data transfer or the whole HSM to
>> workqueue?  I'm all for that.  I just thought it didn't make sense to
> 
> I was thinking data transfer but HSM itself might work too, I'd not even
> thought of push that much out.

I thought pushing the whole HSM out to workqueue will be the simplest to
implement as we already have everything needed for polling.

>> Anyways, if that's the plan, as Albert suggested earlier, we can just
>> mark that HSM is running in ap->pflags or somewhere and skip Status
>> clearing if it's running.  It's not like the controller is gonna raise
>> interrupt while it's transferring data anyway (we always have leading
>> status checks before data transfer).
> 
> And the controllers with a hidden FIFO magically defer the IRQ until the
> FIFO empties so that the events occur in the expected sequence for the
> spec even if the drive transfer to the fifo already finished and the host
> is still emptying it.

Yeah, that sadly makes things a bit tricky tho.  We need to make the
last transfer and clearing HSM running flag atomic; otherwise, we're
likely to get nobody-cared right after we finish the last transfer.

I guess it's about time to listen what Jeff thinks.

Albert, if Jeff agrees with pushing HSM or data transfer to workqueue,
are you interested in doing that?  The HSM is your baby after all.  :-)

Thanks.

-- 
tejun

Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)

[Albert Lee]

Tejun Heo wrote:

> 
> I guess it's about time to listen what Jeff thinks.
> 
> Albert, if Jeff agrees with pushing HSM or data transfer to workqueue,
> are you interested in doing that?  The HSM is your baby after all.  :-)
> 

Moving HSM task to the workqueue looks good idea. The mouse cursor of my
X desktop doesn't move smoothly when blocks of PIO are going in hard irq.
Hopefully we can do better with that.

Yes, I am interested in doing it, if Jeff also ack.
--
albert

Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)

[Jeff Garzik]

Tejun Heo wrote:
> Albert, if Jeff agrees with pushing HSM or data transfer to workqueue,
> are you interested in doing that?  The HSM is your baby after all.  :-)


It's always been my desire to do PIO from a workqueue if possible, since 
it is so slow.

	Jeff

[PATCH/RFC 0/7] libata: push part of irq driven pio to workqueue

[Albert Lee]

1/7: set the state after "PIO data-in" to HSM_ST_IDLE instead of HSM_ST_LAST
2/7: fix the ata_altstatus() in ata_hsm_qc_complete()
3/7: move ata_altstatus() out from ata_hsm_move() to the pio data xfer functions
4/7: move polling idle irq check to ata_host_intr()
5/7: move and reduce locking to the pio data xfer functions
6/7: push part of the irq driven pio out to workqueue
7/7: ack unexpected INTRQ when polling

This is not complete yet, still needs to check if this creates new race
conditions with EH or causing trouble to other part.

Draft patch against 2.6.21.1 for your review.

[PATCH 1/7] libata: set the state after "PIO data-in" to HSM_ST_IDLE instead of HSM_ST_LAST

[Albert Lee]

patch 1/7:
  Set the state after "PIO data-in" to HSM_ST_IDLE instead of HSM_ST_LAST.
To prevent the irq handler from thinking "HSM is waiting for the last irq".
Needed later when part of the irq driven pio pushed out to workqueue.

---

diff -Nrup 00_linux-2.6.21.1/drivers/ata/libata-core.c 01_last_idle/drivers/ata/libata-core.c
--- 00_linux-2.6.21.1/drivers/ata/libata-core.c	2007-05-04 11:22:23.000000000 +0800
+++ 01_last_idle/drivers/ata/libata-core.c	2007-05-11 10:24:16.000000000 +0800
@@ -4041,7 +4041,7 @@ static void ata_pio_sector(struct ata_qu
 	unsigned char *buf;
 
 	if (qc->curbytes == qc->nbytes - ATA_SECT_SIZE)
-		ap->hsm_task_state = HSM_ST_LAST;
+		ap->hsm_task_state = do_write ? HSM_ST_LAST : HSM_ST_IDLE;
 
 	page = sg[qc->cursg].page;
 	offset = sg[qc->cursg].offset + qc->cursg_ofs;
@@ -4389,6 +4389,8 @@ int ata_hsm_move(struct ata_port *ap, st
 	 */
 	WARN_ON(in_wq != ata_hsm_ok_in_wq(ap, qc));
 
+	WARN_ON(ap->hsm_task_state == HSM_ST_IDLE);
+
 fsm_start:
 	DPRINTK("ata%u: protocol %d task_state %d (dev_stat 0x%X)\n",
 		ap->print_id, qc->tf.protocol, ap->hsm_task_state, status);
@@ -4548,8 +4550,7 @@ fsm_start:
 
 			ata_pio_sectors(qc);
 
-			if (ap->hsm_task_state == HSM_ST_LAST &&
-			    (!(qc->tf.flags & ATA_TFLAG_WRITE))) {
+			if (ap->hsm_task_state == HSM_ST_IDLE) {
 				/* all data read */
 				ata_altstatus(ap);
 				status = ata_wait_idle(ap);
@@ -4562,6 +4563,7 @@ fsm_start:
 		break;
 
 	case HSM_ST_LAST:
+	case HSM_ST_IDLE:
 		if (unlikely(!ata_ok(status))) {
 			qc->err_mask |= __ac_err_mask(status);
 			ap->hsm_task_state = HSM_ST_ERR;

[PATCH 2/7] libata: move the ata_altstatus() in ata_hsm_qc_complete()

[Albert Lee]

patch 2/7:
 Calling ata_altstatus() after the qc completed looks incorrect.
Move it to before the qc is completed.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---

diff -Nrup 01_last_idle/drivers/ata/libata-core.c 02_flush_fix/drivers/ata/libata-core.c
--- 01_last_idle/drivers/ata/libata-core.c	2007-05-11 10:24:16.000000000 +0800
+++ 02_flush_fix/drivers/ata/libata-core.c	2007-05-11 11:14:04.000000000 +0800
@@ -4329,6 +4329,8 @@ static void ata_hsm_qc_complete(struct a
 	struct ata_port *ap = qc->ap;
 	unsigned long flags;
 
+	ata_altstatus(ap); /* flush */
+
 	if (ap->ops->error_handler) {
 		if (in_wq) {
 			spin_lock_irqsave(ap->lock, flags);
@@ -4361,8 +4363,6 @@ static void ata_hsm_qc_complete(struct a
 		} else
 			ata_qc_complete(qc);
 	}
-
-	ata_altstatus(ap); /* flush */
 }
 
 /**

[PATCH 3/7] libata: move ata_altstatus() out to the pio data xfer functions

[Albert Lee]

patch 3/7:
 move ata_altstatus() out from ata_hsm_move() to the pio data xfer functions.
Functions like ata_pio_sectors() and atapi_pio_bytes() know better if the
flush is needed.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---

diff -Nrup 02_flush_fix/drivers/ata/libata-core.c 03_smart_flush/drivers/ata/libata-core.c
--- 02_flush_fix/drivers/ata/libata-core.c	2007-05-11 11:14:04.000000000 +0800
+++ 03_smart_flush/drivers/ata/libata-core.c	2007-05-11 10:24:19.000000000 +0800
@@ -4031,7 +4031,7 @@ void ata_data_xfer_noirq(struct ata_devi
  *	Inherited from caller.
  */
 
-static void ata_pio_sector(struct ata_queued_cmd *qc)
+static void ata_pio_sector(struct ata_queued_cmd *qc, int last)
 {
 	int do_write = (qc->tf.flags & ATA_TFLAG_WRITE);
 	struct scatterlist *sg = qc->__sg;
@@ -4069,6 +4069,9 @@ static void ata_pio_sector(struct ata_qu
 		ap->ops->data_xfer(qc->dev, buf + offset, ATA_SECT_SIZE, do_write);
 	}
 
+	if (last)
+		ata_altstatus(ap); /* flush */
+
 	qc->curbytes += ATA_SECT_SIZE;
 	qc->cursg_ofs += ATA_SECT_SIZE;
 
@@ -4100,9 +4103,9 @@ static void ata_pio_sectors(struct ata_q
 		nsect = min((qc->nbytes - qc->curbytes) / ATA_SECT_SIZE,
 			    qc->dev->multi_count);
 		while (nsect--)
-			ata_pio_sector(qc);
+			ata_pio_sector(qc, !nsect);
 	} else
-		ata_pio_sector(qc);
+		ata_pio_sector(qc, 1);
 }
 
 /**
@@ -4185,6 +4188,8 @@ next_sg:
 		for (i = 0; i < words; i++)
 			ap->ops->data_xfer(qc->dev, (unsigned char*)pad_buf, 2, do_write);
 
+		ata_altstatus(ap); /* flush */
+
 		ap->hsm_task_state = HSM_ST_LAST;
 		return;
 	}
@@ -4234,6 +4239,8 @@ next_sg:
 
 	if (bytes)
 		goto next_sg;
+
+	ata_altstatus(ap); /* flush */
 }
 
 /**
@@ -4452,7 +4459,6 @@ fsm_start:
 			 */
 			ap->hsm_task_state = HSM_ST;
 			ata_pio_sectors(qc);
-			ata_altstatus(ap); /* flush */
 		} else
 			/* send CDB */
 			atapi_send_cdb(ap, qc);
@@ -4533,7 +4539,6 @@ fsm_start:
 
 				if (!(qc->tf.flags & ATA_TFLAG_WRITE)) {
 					ata_pio_sectors(qc);
-					ata_altstatus(ap);
 					status = ata_wait_idle(ap);
 				}
 
@@ -4552,13 +4557,11 @@ fsm_start:
 
 			if (ap->hsm_task_state == HSM_ST_IDLE) {
 				/* all data read */
-				ata_altstatus(ap);
 				status = ata_wait_idle(ap);
 				goto fsm_start;
 			}
 		}
 
-		ata_altstatus(ap); /* flush */
 		poll_next = 1;
 		break;
 

[PATCH 4/7] libata: move polling idle irq check to ata_host_intr()

[Albert Lee]

patch 4/7:
move the polling idle irq check from ata_interrupt() to ata_host_intr(),
where it makes more sense.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---

diff -Nrup 03_smart_flush/drivers/ata/libata-core.c 04_polling_check/drivers/ata/libata-core.c
--- 03_smart_flush/drivers/ata/libata-core.c	2007-05-11 10:24:19.000000000 +0800
+++ 04_polling_check/drivers/ata/libata-core.c	2007-05-11 10:25:09.000000000 +0800
@@ -5119,6 +5119,10 @@ inline unsigned int ata_host_intr (struc
 	VPRINTK("ata%u: protocol %d task_state %d\n",
 		ap->print_id, qc->tf.protocol, ap->hsm_task_state);
 
+	/* polling */
+	if (qc->tf.flags & ATA_TFLAG_POLLING)
+		goto idle_irq;
+
 	/* Check whether we are expecting interrupt in this state */
 	switch (ap->hsm_task_state) {
 	case HSM_ST_FIRST:
@@ -5229,8 +5233,7 @@ irqreturn_t ata_interrupt (int irq, void
 			struct ata_queued_cmd *qc;
 
 			qc = ata_qc_from_tag(ap, ap->active_tag);
-			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)) &&
-			    (qc->flags & ATA_QCFLAG_ACTIVE))
+			if (qc && (qc->flags & ATA_QCFLAG_ACTIVE))
 				handled |= ata_host_intr(ap, qc);
 		}
 	}

Re: [PATCH 4/7] libata: move polling idle irq check to ata_host_intr()

[Tejun Heo]

Albert Lee wrote:
> patch 4/7:
> move the polling idle irq check from ata_interrupt() to ata_host_intr(),
> where it makes more sense.
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> ---
> 
> diff -Nrup 03_smart_flush/drivers/ata/libata-core.c 04_polling_check/drivers/ata/libata-core.c
> --- 03_smart_flush/drivers/ata/libata-core.c	2007-05-11 10:24:19.000000000 +0800
> +++ 04_polling_check/drivers/ata/libata-core.c	2007-05-11 10:25:09.000000000 +0800
> @@ -5119,6 +5119,10 @@ inline unsigned int ata_host_intr (struc
>  	VPRINTK("ata%u: protocol %d task_state %d\n",
>  		ap->print_id, qc->tf.protocol, ap->hsm_task_state);
>  
> +	/* polling */
> +	if (qc->tf.flags & ATA_TFLAG_POLLING)
> +		goto idle_irq;
> +
>  	/* Check whether we are expecting interrupt in this state */
>  	switch (ap->hsm_task_state) {
>  	case HSM_ST_FIRST:
> @@ -5229,8 +5233,7 @@ irqreturn_t ata_interrupt (int irq, void
>  			struct ata_queued_cmd *qc;
>  
>  			qc = ata_qc_from_tag(ap, ap->active_tag);
> -			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)) &&
> -			    (qc->flags & ATA_QCFLAG_ACTIVE))
> +			if (qc && (qc->flags & ATA_QCFLAG_ACTIVE))

There are several LLD specific IRQ handlers which have similar part.
Care to update them together?

-- 
tejun

Re: [PATCH 4/7] libata: move polling idle irq check to ata_host_intr()

[Albert Lee]

Tejun Heo wrote:

>>
>>diff -Nrup 03_smart_flush/drivers/ata/libata-core.c 04_polling_check/drivers/ata/libata-core.c
>>--- 03_smart_flush/drivers/ata/libata-core.c	2007-05-11 10:24:19.000000000 +0800
>>+++ 04_polling_check/drivers/ata/libata-core.c	2007-05-11 10:25:09.000000000 +0800
>>@@ -5119,6 +5119,10 @@ inline unsigned int ata_host_intr (struc
>> 	VPRINTK("ata%u: protocol %d task_state %d\n",
>> 		ap->print_id, qc->tf.protocol, ap->hsm_task_state);
>> 
>>+	/* polling */
>>+	if (qc->tf.flags & ATA_TFLAG_POLLING)
>>+		goto idle_irq;
>>+
>> 	/* Check whether we are expecting interrupt in this state */
>> 	switch (ap->hsm_task_state) {
>> 	case HSM_ST_FIRST:
>>@@ -5229,8 +5233,7 @@ irqreturn_t ata_interrupt (int irq, void
>> 			struct ata_queued_cmd *qc;
>> 
>> 			qc = ata_qc_from_tag(ap, ap->active_tag);
>>-			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)) &&
>>-			    (qc->flags & ATA_QCFLAG_ACTIVE))
>>+			if (qc && (qc->flags & ATA_QCFLAG_ACTIVE))
> 
> 
> There are several LLD specific IRQ handlers which have similar part.
> Care to update them together?
> 

Sure, will do.
--
albert

[PATCH 5/7] libata: move and reduce locking to the pio data xfer functions

[Albert Lee]

patch 5/7: 
- move the locking out from the ata_hsm_move() to the data xfer functions like ata_pio_sectors().
- added the ATA_PFLAG_HSM_WQ (HSM running in workqueue) flag to serialize irq and workqueue.
- the time holding ap->lock is reduced to last piece of pio transfer and clearing the ATA_PFLAG_HSM_WQ flag

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---

diff -Nrup 04_polling_check/drivers/ata/libata-core.c 05_narrow_lock/drivers/ata/libata-core.c
--- 04_polling_check/drivers/ata/libata-core.c	2007-05-11 10:25:09.000000000 +0800
+++ 05_narrow_lock/drivers/ata/libata-core.c	2007-05-11 11:46:41.000000000 +0800
@@ -4031,7 +4031,7 @@ void ata_data_xfer_noirq(struct ata_devi
  *	Inherited from caller.
  */
 
-static void ata_pio_sector(struct ata_queued_cmd *qc, int last)
+static void ata_pio_sector(struct ata_queued_cmd *qc, int last, int lock)
 {
 	int do_write = (qc->tf.flags & ATA_TFLAG_WRITE);
 	struct scatterlist *sg = qc->__sg;
@@ -4039,6 +4039,7 @@ static void ata_pio_sector(struct ata_qu
 	struct page *page;
 	unsigned int offset;
 	unsigned char *buf;
+	unsigned long irq_flags = 0;
 
 	if (qc->curbytes == qc->nbytes - ATA_SECT_SIZE)
 		ap->hsm_task_state = do_write ? HSM_ST_LAST : HSM_ST_IDLE;
@@ -4055,6 +4056,9 @@ static void ata_pio_sector(struct ata_qu
 	if (PageHighMem(page)) {
 		unsigned long flags;
 
+		if (lock)
+			spin_lock_irqsave(ap->lock, irq_flags);
+
 		/* FIXME: use a bounce buffer */
 		local_irq_save(flags);
 		buf = kmap_atomic(page, KM_IRQ0);
@@ -4065,8 +4069,18 @@ static void ata_pio_sector(struct ata_qu
 		kunmap_atomic(buf, KM_IRQ0);
 		local_irq_restore(flags);
 	} else {
+		unsigned int head = 0, tail = ATA_SECT_SIZE;
+
 		buf = page_address(page);
-		ap->ops->data_xfer(qc->dev, buf + offset, ATA_SECT_SIZE, do_write);
+
+		if (lock) {
+			tail = 8;
+			head = ATA_SECT_SIZE - tail; /* multiple of 8 bytes */
+			ap->ops->data_xfer(qc->dev, buf + offset, head, do_write);
+			spin_lock_irqsave(ap->lock, irq_flags);
+		}
+
+		ap->ops->data_xfer(qc->dev, buf + offset + head, tail, do_write);
 	}
 
 	if (last)
@@ -4079,6 +4093,11 @@ static void ata_pio_sector(struct ata_qu
 		qc->cursg++;
 		qc->cursg_ofs = 0;
 	}
+
+	if (lock) {
+		ap->pflags &= ~ATA_PFLAG_HSM_WQ;
+		spin_unlock_irqrestore(ap->lock, irq_flags);
+	}
 }
 
 /**
@@ -4092,7 +4111,7 @@ static void ata_pio_sector(struct ata_qu
  *	Inherited from caller.
  */
 
-static void ata_pio_sectors(struct ata_queued_cmd *qc)
+static void ata_pio_sectors(struct ata_queued_cmd *qc, int lock)
 {
 	if (is_multi_taskfile(&qc->tf)) {
 		/* READ/WRITE MULTIPLE */
@@ -4103,9 +4122,9 @@ static void ata_pio_sectors(struct ata_q
 		nsect = min((qc->nbytes - qc->curbytes) / ATA_SECT_SIZE,
 			    qc->dev->multi_count);
 		while (nsect--)
-			ata_pio_sector(qc, !nsect);
+			ata_pio_sector(qc, !nsect, lock && !nsect);
 	} else
-		ata_pio_sector(qc, 1);
+		ata_pio_sector(qc, 1, lock);
 }
 
 /**
@@ -4120,12 +4139,17 @@ static void ata_pio_sectors(struct ata_q
  *	caller.
  */
 
-static void atapi_send_cdb(struct ata_port *ap, struct ata_queued_cmd *qc)
+static void atapi_send_cdb(struct ata_port *ap, struct ata_queued_cmd *qc, int lock)
 {
+	unsigned long irq_flags = 0;
+
 	/* send SCSI cdb */
 	DPRINTK("send cdb\n");
 	WARN_ON(qc->dev->cdb_len < 12);
 
+	if (lock)
+		spin_lock_irqsave(ap->lock, irq_flags);
+
 	ap->ops->data_xfer(qc->dev, qc->cdb, qc->dev->cdb_len, 1);
 	ata_altstatus(ap); /* flush */
 
@@ -4142,6 +4166,11 @@ static void atapi_send_cdb(struct ata_po
 		ap->ops->bmdma_start(qc);
 		break;
 	}
+
+	if (lock) {
+		ap->pflags &= ~ATA_PFLAG_HSM_WQ;
+		spin_unlock_irqrestore(ap->lock, irq_flags);
+	}
 }
 
 /**
@@ -4156,7 +4185,7 @@ static void atapi_send_cdb(struct ata_po
  *
  */
 
-static void __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes)
+static void __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes, int lock)
 {
 	int do_write = (qc->tf.flags & ATA_TFLAG_WRITE);
 	struct scatterlist *sg = qc->__sg;
@@ -4164,6 +4193,8 @@ static void __atapi_pio_bytes(struct ata
 	struct page *page;
 	unsigned char *buf;
 	unsigned int offset, count;
+	unsigned long irq_flags = 0;
+	int transfer_lock = 0;
 
 	if (qc->curbytes + bytes >= qc->nbytes)
 		ap->hsm_task_state = HSM_ST_LAST;
@@ -4181,6 +4212,10 @@ next_sg:
 		unsigned int words = bytes >> 1;
 		unsigned int i;
 
+		WARN_ON(transfer_lock);
+		if (lock)
+			spin_lock_irqsave(ap->lock, irq_flags);
+
 		if (words) /* warning if bytes > 1 */
 			ata_dev_printk(qc->dev, KERN_WARNING,
 				       "%u bytes trailing data\n", bytes);
@@ -4191,6 +4226,12 @@ next_sg:
 		ata_altstatus(ap); /* flush */
 
 		ap->hsm_task_state = HSM_ST_LAST;
+
+		if (lock) {
+			ap->pflags &= ~ATA_PFLAG_HSM_WQ;
+			spin_unlock_irqrestore(ap->lock, irq_flags);
+		}
+
 		return;
 	}
 
@@ -4210,10 +4251,16 @@ next_sg:
 	count = min(count, (unsigned int)PAGE_SIZE - offset);
 
 	DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
+	
+	if (lock && bytes <= count)
+		transfer_lock = 1;
 
 	if (PageHighMem(page)) {
 		unsigned long flags;
 
+		if (transfer_lock)
+			spin_lock_irqsave(ap->lock, irq_flags);
+
 		/* FIXME: use bounce buffer */
 		local_irq_save(flags);
 		buf = kmap_atomic(page, KM_IRQ0);
@@ -4224,8 +4271,21 @@ next_sg:
 		kunmap_atomic(buf, KM_IRQ0);
 		local_irq_restore(flags);
 	} else {
+		unsigned int head = 0, tail = count;
+
 		buf = page_address(page);
-		ap->ops->data_xfer(qc->dev,  buf + offset, count, do_write);
+
+		if (transfer_lock) {
+			if (count > 20) {
+				tail = 8 + count % 8;
+				head = count - tail; /* multiple of 8 bytes */
+				ap->ops->data_xfer(qc->dev,  buf + offset, head, do_write);
+			}
+
+			spin_lock_irqsave(ap->lock, irq_flags);
+		}
+
+		ap->ops->data_xfer(qc->dev,  buf + offset + head, tail, do_write);
 	}
 
 	bytes -= count;
@@ -4241,6 +4301,12 @@ next_sg:
 		goto next_sg;
 
 	ata_altstatus(ap); /* flush */
+
+	if (lock) {
+		WARN_ON(!transfer_lock);
+		ap->pflags &= ~ATA_PFLAG_HSM_WQ;
+		spin_unlock_irqrestore(ap->lock, irq_flags);
+	}
 }
 
 /**
@@ -4253,7 +4319,7 @@ next_sg:
  *	Inherited from caller.
  */
 
-static void atapi_pio_bytes(struct ata_queued_cmd *qc)
+static void atapi_pio_bytes(struct ata_queued_cmd *qc, int lock)
 {
 	struct ata_port *ap = qc->ap;
 	struct ata_device *dev = qc->dev;
@@ -4283,7 +4349,7 @@ static void atapi_pio_bytes(struct ata_q
 
 	VPRINTK("ata%u: xfering %d bytes\n", ap->print_id, bytes);
 
-	__atapi_pio_bytes(qc, bytes);
+	__atapi_pio_bytes(qc, bytes, lock);
 
 	return;
 
@@ -4385,7 +4451,6 @@ static void ata_hsm_qc_complete(struct a
 int ata_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
 		 u8 status, int in_wq)
 {
-	unsigned long flags = 0;
 	int poll_next;
 
 	WARN_ON((qc->flags & ATA_QCFLAG_ACTIVE) == 0);
@@ -4443,11 +4508,10 @@ fsm_start:
 		/* Send the CDB (atapi) or the first data block (ata pio out).
 		 * During the state transition, interrupt handler shouldn't
 		 * be invoked before the data transfer is complete and
-		 * hsm_task_state is changed. Hence, the following locking.
+		 * hsm_task_state is changed. This is ensured by the
+		 * ATA_PFLAG_HSM_WQ flag and holding ap->lock in the PIO
+		 * data transfer functions.
 		 */
-		if (in_wq)
-			spin_lock_irqsave(ap->lock, flags);
-
 		if (qc->tf.protocol == ATA_PROT_PIO) {
 			/* PIO data out protocol.
 			 * send first data block.
@@ -4458,13 +4522,10 @@ fsm_start:
 			 * before ata_pio_sectors().
 			 */
 			ap->hsm_task_state = HSM_ST;
-			ata_pio_sectors(qc);
+			ata_pio_sectors(qc, in_wq);
 		} else
 			/* send CDB */
-			atapi_send_cdb(ap, qc);
-
-		if (in_wq)
-			spin_unlock_irqrestore(ap->lock, flags);
+			atapi_send_cdb(ap, qc, in_wq);
 
 		/* if polling, ata_pio_task() handles the rest.
 		 * otherwise, interrupt handler takes over from here.
@@ -4498,7 +4559,7 @@ fsm_start:
 				goto fsm_start;
 			}
 
-			atapi_pio_bytes(qc);
+			atapi_pio_bytes(qc, 0);
 
 			if (unlikely(ap->hsm_task_state == HSM_ST_ERR))
 				/* bad ireason reported by device */
@@ -4538,7 +4599,7 @@ fsm_start:
 				qc->err_mask |= AC_ERR_DEV;
 
 				if (!(qc->tf.flags & ATA_TFLAG_WRITE)) {
-					ata_pio_sectors(qc);
+					ata_pio_sectors(qc, 0);
 					status = ata_wait_idle(ap);
 				}
 
@@ -4553,7 +4614,7 @@ fsm_start:
 				goto fsm_start;
 			}
 
-			ata_pio_sectors(qc);
+			ata_pio_sectors(qc, 0);
 
 			if (ap->hsm_task_state == HSM_ST_IDLE) {
 				/* all data read */
@@ -4615,6 +4676,7 @@ static void ata_pio_task(struct work_str
 	struct ata_queued_cmd *qc = ap->port_task_data;
 	u8 status;
 	int poll_next;
+	unsigned long irq_flags;
 
 fsm_start:
 	WARN_ON(ap->hsm_task_state == HSM_ST_IDLE);
@@ -4636,6 +4698,11 @@ fsm_start:
 		}
 	}
 
+	/* Let the irq handler know wq is accessing the port */
+	spin_lock_irqsave(ap->lock, irq_flags);
+	ap->pflags |= ATA_PFLAG_HSM_WQ;
+	spin_unlock_irqrestore(ap->lock, irq_flags);
+
 	/* move the HSM */
 	poll_next = ata_hsm_move(ap, qc, status, 1);
 
@@ -4749,6 +4816,7 @@ void __ata_qc_complete(struct ata_queued
 	 */
 	qc->flags &= ~ATA_QCFLAG_ACTIVE;
 	ap->qc_active &= ~(1 << qc->tag);
+	ap->pflags &= ~ATA_PFLAG_HSM_WQ;
 
 	/* call completion callback */
 	qc->complete_fn(qc);
@@ -5119,6 +5187,10 @@ inline unsigned int ata_host_intr (struc
 	VPRINTK("ata%u: protocol %d task_state %d\n",
 		ap->print_id, qc->tf.protocol, ap->hsm_task_state);
 
+	/* HSM accessing the port from the workqueue */
+	if (ap->pflags & ATA_PFLAG_HSM_WQ)
+		goto idle_irq;
+
 	/* polling */
 	if (qc->tf.flags & ATA_TFLAG_POLLING)
 		goto idle_irq;
diff -Nrup 04_polling_check/include/linux/libata.h 05_narrow_lock/include/linux/libata.h
--- 04_polling_check/include/linux/libata.h	2007-04-28 05:49:26.000000000 +0800
+++ 05_narrow_lock/include/linux/libata.h	2007-05-10 12:12:35.000000000 +0800
@@ -195,6 +195,7 @@ enum {
 	ATA_PFLAG_FLUSH_PORT_TASK = (1 << 16), /* flush port task */
 	ATA_PFLAG_SUSPENDED	= (1 << 17), /* port is suspended (power) */
 	ATA_PFLAG_PM_PENDING	= (1 << 18), /* PM operation pending */
+	ATA_PFLAG_HSM_WQ	= (1 << 19), /* hsm accessing the port in wq */
 
 	/* struct ata_queued_cmd flags */
 	ATA_QCFLAG_ACTIVE	= (1 << 0), /* cmd not yet ack'd to scsi lyer */

Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions

[Tejun Heo]

Albert Lee wrote:
> -static void ata_pio_sector(struct ata_queued_cmd *qc, int last)
> +static void ata_pio_sector(struct ata_queued_cmd *qc, int last, int lock)

I think the naming of @lock is a bit confusing here.  @clr_hsm_wq or
@last_sector, maybe?

> +		if (lock) {
> +			tail = 8;
> +			head = ATA_SECT_SIZE - tail; /* multiple of 8 bytes */
> +			ap->ops->data_xfer(qc->dev, buf + offset, head, do_write);
> +			spin_lock_irqsave(ap->lock, irq_flags);
> +		}
> +
> +		ap->ops->data_xfer(qc->dev, buf + offset + head, tail, do_write);

Aieee, we have to transfer the whole last sector while holding the spin
lock and IRQ disabled.  That's sad but pushing locking into ->data_xfer
doesn't sound attractive either.  Any better ideas?

> @@ -5119,6 +5187,10 @@ inline unsigned int ata_host_intr (struc
>  	VPRINTK("ata%u: protocol %d task_state %d\n",
>  		ap->print_id, qc->tf.protocol, ap->hsm_task_state);
>  
> +	/* HSM accessing the port from the workqueue */
> +	if (ap->pflags & ATA_PFLAG_HSM_WQ)
> +		goto idle_irq;
> +

Please update LLDs with private IRQ handlers too.  Maybe we need
ata_idle_irq(ap) test?

Thanks.

-- 
tejun

Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions

[Alan Cox]

> Aieee, we have to transfer the whole last sector while holding the spin
> lock and IRQ disabled.  That's sad but pushing locking into ->data_xfer
> doesn't sound attractive either.  Any better ideas?

I'd say this is a non-starter. It solves nothing and means PIO in libata
is still basically unusable.

Alan

Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions

[Tejun Heo]

Alan Cox wrote:
>> Aieee, we have to transfer the whole last sector while holding the spin
>> lock and IRQ disabled.  That's sad but pushing locking into ->data_xfer
>> doesn't sound attractive either.  Any better ideas?
> 
> I'd say this is a non-starter. It solves nothing and means PIO in libata
> is still basically unusable.

It doesn't solve the problem completely but still helps, FWIW.  I was
hoping we could lock only for the last transfer (word).  Would it
complicate ->data_xfer() too much?

-- 
tejun

Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions

[Alan Cox]

> > I'd say this is a non-starter. It solves nothing and means PIO in libata
> > is still basically unusable.
> 
> It doesn't solve the problem completely but still helps, FWIW.  I was

Most transfers for PIO are a single 512 byte transfer per command.

> hoping we could lock only for the last transfer (word).  Would it
> complicate ->data_xfer() too much?

I don't think so. We need to complicate it far more to add 32bit
transfers. We also only have a few ->data_xfer implementations so its
easy to propogate the chance.

Why do we need the lock on the last word transferred anyway. I'm missing
a detail here ?

Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions

[Tejun Heo]

Alan Cox wrote:
>>> I'd say this is a non-starter. It solves nothing and means PIO in libata
>>> is still basically unusable.
>> It doesn't solve the problem completely but still helps, FWIW.  I was
> 
> Most transfers for PIO are a single 512 byte transfer per command.

Not for disks but, yeah, who uses PIO for disks.

>> hoping we could lock only for the last transfer (word).  Would it
>> complicate ->data_xfer() too much?
> 
> I don't think so. We need to complicate it far more to add 32bit
> transfers. We also only have a few ->data_xfer implementations so its
> easy to propogate the chance.
> 
> Why do we need the lock on the last word transferred anyway. I'm missing
> a detail here ?

The problem is that controllers queue IRQ till the end of transfer and
raise it right after the last transfer completes.  If
WQ-active-ignore-IRQ flag is set at that point && we're not holding the
lock, the IRQ handler will ignore the IRQ without clearing it, so we get
nobody-cared right after the last transfer.  So, the last transfer and
clearing of WQ-active-ignore-IRQ flag should be atomic w.r.t. the IRQ
handler.

-- 
tejun

Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions

[Alan Cox]

On Fri, 11 May 2007 17:14:35 +0200
Tejun Heo <htejun@gmail.com> wrote:

> Alan Cox wrote:
> >>> I'd say this is a non-starter. It solves nothing and means PIO in libata
> >>> is still basically unusable.
> >> It doesn't solve the problem completely but still helps, FWIW.  I was
> > 
> > Most transfers for PIO are a single 512 byte transfer per command.
> 
> Not for disks but, yeah, who uses PIO for disks.

PCMCIA, embedded, CF devices .. basically everyone.

> The problem is that controllers queue IRQ till the end of transfer and
> raise it right after the last transfer completes.  If
> WQ-active-ignore-IRQ flag is set at that point && we're not holding the
> lock, the IRQ handler will ignore the IRQ without clearing it, so we get
> nobody-cared right after the last transfer.  So, the last transfer and
> clearing of WQ-active-ignore-IRQ flag should be atomic w.r.t. the IRQ
> handler.

Ok that makes sense. And we can't safely peek at DRQ at this point
either. Holding the lock for the last word shouldn't be a problem. It
isn't ideal but it is a lot less ugly than holding it for the full
transfer. The private data_xfer methods in PATA are for locking IRQ off
entirely so they'll convert trivially, not looked for others.

Alan

Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions

[Alan Cox]

> The problem is that controllers queue IRQ till the end of transfer and
> raise it right after the last transfer completes.  If
> WQ-active-ignore-IRQ flag is set at that point && we're not holding the
> lock, the IRQ handler will ignore the IRQ without clearing it, so we get
> nobody-cared right after the last transfer.  So, the last transfer and
> clearing of WQ-active-ignore-IRQ flag should be atomic w.r.t. the IRQ
> handler.

There does seem to be an alternative. In the pio handler we do

	clear_bit(COMPLETION_RUN, ->flags);
	xfer bytes
	if (test_and_set_bit(COMPLETION_RUN, ->flags) == 0)
		run_completion_routine();
	->active-ignore-irq = 0;

and in the IRQ case after checking we are not active-ignore-IRQ we
similarly do

	if (test_and_set_bit(COMPLETION_RUN, ->flags) == 0)
		run_completion_routine_irq();

Now if we are unlucky and the IRQ gets in between the last byte of
transfer and clearing the active ignore IRQ flag we will still run the
completion handler

Alan

Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions

[Tejun Heo]

Alan Cox wrote:
>> The problem is that controllers queue IRQ till the end of transfer and
>> raise it right after the last transfer completes.  If
>> WQ-active-ignore-IRQ flag is set at that point && we're not holding the
>> lock, the IRQ handler will ignore the IRQ without clearing it, so we get
>> nobody-cared right after the last transfer.  So, the last transfer and
>> clearing of WQ-active-ignore-IRQ flag should be atomic w.r.t. the IRQ
>> handler.
> 
> There does seem to be an alternative. In the pio handler we do
> 
> 	clear_bit(COMPLETION_RUN, ->flags);
> 	xfer bytes
> 	if (test_and_set_bit(COMPLETION_RUN, ->flags) == 0)
> 		run_completion_routine();
> 	->active-ignore-irq = 0;
> 
> and in the IRQ case after checking we are not active-ignore-IRQ we
> similarly do
> 
> 	if (test_and_set_bit(COMPLETION_RUN, ->flags) == 0)
> 		run_completion_routine_irq();
> 
> Now if we are unlucky and the IRQ gets in between the last byte of
> transfer and clearing the active ignore IRQ flag we will still run the
> completion handler

I don't really get this.  What happens if the IRQ is shared and the
other device raises interrupt while the data transfer is still in
progress?  What prevents it from running the completion routine?

-- 
tejun

Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions

[Alan Cox]

> > 	clear_bit(COMPLETION_RUN, ->flags);
> > 	xfer bytes
> > 	if (test_and_set_bit(COMPLETION_RUN, ->flags) == 0)
> > 		run_completion_routine();
> > 	->active-ignore-irq = 0;
> > 
> > and in the IRQ case after checking we are not active-ignore-IRQ we
> > similarly do
> > 
> > 	if (test_and_set_bit(COMPLETION_RUN, ->flags) == 0)
> > 		run_completion_routine_irq();
> > 
> > Now if we are unlucky and the IRQ gets in between the last byte of
> > transfer and clearing the active ignore IRQ flag we will still run the
> > completion handler
> 
> I don't really get this.  What happens if the IRQ is shared and the
> other device raises interrupt while the data transfer is still in
> progress?  What prevents it from running the completion routine?

Before the transfer begins you set your flag to indicate you want to
ignore this IRQ. You then set the flag indicating completion has not
occurred.

Next you do the data transfer

An IRQ during the transfer will not complete as we are ignoring the
IRQ/polling at the driver level.

On completion either

a) the IRQ beats the test_and_set_bit in which case it runs the completion

or

b) the test_and_set_bit in the I/O path beats the IRQ handler in which
case the IRQ handler will not run the completion as the bit is atomically
set

or

c) the IRQ beats the clearing of the ignore irq flag in which case it
won't execute the handler but the data transfer path will


I'm sure there are even simpler ways we can do this they key is to make
sure that whatever the event order we only run the completion path once,
and it doesn't matter which path runs it.

Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions

[Tejun Heo]

Alan Cox wrote:
>>> Now if we are unlucky and the IRQ gets in between the last byte of
>>> transfer and clearing the active ignore IRQ flag we will still run the
>>> completion handler
>> I don't really get this.  What happens if the IRQ is shared and the
>> other device raises interrupt while the data transfer is still in
>> progress?  What prevents it from running the completion routine?
[--snip--]
> c) the IRQ beats the clearing of the ignore irq flag in which case it
> won't execute the handler but the data transfer path will

This case is where I fail to understand how it's supposed to work.  If
IRQ beats the clearing of the ignore irq flag && execution of completion
routine from wq, it ignores the IRQ, right?  The IRQ line remains
asserted, so when the IRQ handler finishes, it's entered again
immediately till nobody-cared kicks in and completely disable the IRQ
line.  ie.

	clear_bit(COMPLETION_RUN, ->flags);
	xfer bytes
	>> IRQ hits here
	if (test_and_set_bit(COMPLETION_RUN, ->flags) == 0)
		run_completion_routine();
	->active-ignore-irq = 0;

-- 
tejun

Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions

[Alan Cox]

> This case is where I fail to understand how it's supposed to work.  If
> IRQ beats the clearing of the ignore irq flag && execution of completion
> routine from wq, it ignores the IRQ, right?  The IRQ line remains

I was assuming your polling handler would be polling and clearing the IRQ
status otherwise it doens't work anyway - cable noise or drive error and
an early DRQ de-assert would do the same thing during a transfer with IRQ
blocked only on the last dword.

Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions

[Tejun Heo]

Alan Cox wrote:
>> This case is where I fail to understand how it's supposed to work.  If
>> IRQ beats the clearing of the ignore irq flag && execution of completion
>> routine from wq, it ignores the IRQ, right?  The IRQ line remains
> 
> I was assuming your polling handler would be polling and clearing the IRQ
> status otherwise it doens't work anyway - cable noise or drive error and
> an early DRQ de-assert would do the same thing during a transfer with IRQ
> blocked only on the last dword.

Yeah, it's not a water-tight solution.  I don't think we can do that
with IRQ enabled during transfer.  So, we can have either highly jerky
but reliable system if PIO is used or usually better behaving PIO with
some chance of getting nobody-cared.  Also, we can flag known buggy
devices/controllers such that IRQ is disabled over PIO by default.  I
think it's worth doing but this definitely should be debated better.

-- 
tejun

Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions

[Alan Cox]

> Yeah, it's not a water-tight solution.  I don't think we can do that
> with IRQ enabled during transfer.  So, we can have either highly jerky
> but reliable system if PIO is used or usually better behaving PIO with
> some chance of getting nobody-cared.  Also, we can flag known buggy
> devices/controllers such that IRQ is disabled over PIO by default.  I
> think it's worth doing but this definitely should be debated better.

Which leaves us back at enable/disable_irq as with the old IDE - which
*are* much better because most of the time the IRQ isn't shared. Sucks
when it is but that is life.

Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions

[Albert Lee]

Tejun Heo wrote:
> Albert Lee wrote:
> 
>>-static void ata_pio_sector(struct ata_queued_cmd *qc, int last)
>>+static void ata_pio_sector(struct ata_queued_cmd *qc, int last, int lock)
> 
> 
> I think the naming of @lock is a bit confusing here.  @clr_hsm_wq or
> @last_sector, maybe?
> 

How about "irq_handover"? When set to "true", it means the workqueue is going to
handover the control of the port to the irq handler.

> 
>>+		if (lock) {
>>+			tail = 8;
>>+			head = ATA_SECT_SIZE - tail; /* multiple of 8 bytes */
>>+			ap->ops->data_xfer(qc->dev, buf + offset, head, do_write);
>>+			spin_lock_irqsave(ap->lock, irq_flags);
>>+		}
>>+
>>+		ap->ops->data_xfer(qc->dev, buf + offset + head, tail, do_write);
> 
> 
> Aieee, we have to transfer the whole last sector while holding the spin
> lock and IRQ disabled.  That's sad but pushing locking into ->data_xfer
> doesn't sound attractive either.  Any better ideas?
> 
> 

Why need to transfer the last sector as a whole?
Spliting it into 504 (unlocked) + 8 (holding ap->lock) works on my machine...

--
albert

Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions

[Tejun Heo]

Albert Lee wrote:
> Tejun Heo wrote:
>> Albert Lee wrote:
>>
>>> -static void ata_pio_sector(struct ata_queued_cmd *qc, int last)
>>> +static void ata_pio_sector(struct ata_queued_cmd *qc, int last, int lock)
>>
>> I think the naming of @lock is a bit confusing here.  @clr_hsm_wq or
>> @last_sector, maybe?
>>
> 
> How about "irq_handover"? When set to "true", it means the workqueue is going to
> handover the control of the port to the irq handler.
> 
>>> +		if (lock) {
>>> +			tail = 8;
>>> +			head = ATA_SECT_SIZE - tail; /* multiple of 8 bytes */
>>> +			ap->ops->data_xfer(qc->dev, buf + offset, head, do_write);
>>> +			spin_lock_irqsave(ap->lock, irq_flags);
>>> +		}
>>> +
>>> +		ap->ops->data_xfer(qc->dev, buf + offset + head, tail, do_write);
>>
>> Aieee, we have to transfer the whole last sector while holding the spin
>> lock and IRQ disabled.  That's sad but pushing locking into ->data_xfer
>> doesn't sound attractive either.  Any better ideas?
>>
> 
> Why need to transfer the last sector as a whole?
> Spliting it into 504 (unlocked) + 8 (holding ap->lock) works on my machine...

That would be the cleanest way to do it but I'm not sure whether all
controllers can live with that.  If I understand correctly, Some
controllers just have to have the whole transfer done atomically.  I
think Alan knows much better about this.  Alan?

-- 
tejun

Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions

[Alan Cox]

> That would be the cleanest way to do it but I'm not sure whether all
> controllers can live with that.  If I understand correctly, Some
> controllers just have to have the whole transfer done atomically.  I
> think Alan knows much better about this.  Alan?

Those controllers already do a local IRQ disable in their private
->data_xfer method so ought to be ok.

Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions

[Tejun Heo]

Alan Cox wrote:
>> That would be the cleanest way to do it but I'm not sure whether all
>> controllers can live with that.  If I understand correctly, Some
>> controllers just have to have the whole transfer done atomically.  I
>> think Alan knows much better about this.  Alan?
> 
> Those controllers already do a local IRQ disable in their private
> ->data_xfer method so ought to be ok.

I think what Albert is suggesting is...

	if (last_sector) {
		for (i = 0; i < WORDS_IN_SECTORS - 1; i++)
			ops->data_xfer(word);
		/* HERE */
		spin_lock_irq(ap->lock);
		ops->data_xfer(last word);
		ap->pflags &= ~WQ_IN_PROGRESS_STAY_AWAY_IRQHNDL;
		spin_unlock_irq(ap->lock);
	}

So, it basically splits the last sector into two transfers and nothing
prevents the machine from taking an interrupt at HERE.

Am I understanding it correctly Albert?

-- 
tejun

Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions

[Tejun Heo]

Albert Lee wrote:
> Tejun Heo wrote:
>> Albert Lee wrote:
>>
>>> -static void ata_pio_sector(struct ata_queued_cmd *qc, int last)
>>> +static void ata_pio_sector(struct ata_queued_cmd *qc, int last, int lock)
>>
>> I think the naming of @lock is a bit confusing here.  @clr_hsm_wq or
>> @last_sector, maybe?
>>
> 
> How about "irq_handover"? When set to "true", it means the workqueue is going to
> handover the control of the port to the irq handler.

Oops, forgot to write about this.  Yeah, that sounds good too.  I just
thought @lock was too vague.

Thanks.

-- 
tejun

[PATCH 6/7] libata: push part of the irq driven pio out to workqueue

[Albert Lee]

patch 6/7:
 Push part of the irq driven pio out to workqueue.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
It seems this creates new race condition with ata_exec_internal_sg() and EH?

diff -Nrup 05_narrow_lock/drivers/ata/libata-core.c 06_irq_wq/drivers/ata/libata-core.c
--- 05_narrow_lock/drivers/ata/libata-core.c	2007-05-11 11:46:41.000000000 +0800
+++ 06_irq_wq/drivers/ata/libata-core.c	2007-05-11 11:54:56.000000000 +0800
@@ -4370,20 +4370,15 @@ err_out:
 
 static inline int ata_hsm_ok_in_wq(struct ata_port *ap, struct ata_queued_cmd *qc)
 {
-	if (qc->tf.flags & ATA_TFLAG_POLLING)
-		return 1;
-
-	if (ap->hsm_task_state == HSM_ST_FIRST) {
-		if (qc->tf.protocol == ATA_PROT_PIO &&
-		    (qc->tf.flags & ATA_TFLAG_WRITE))
-		    return 1;
-
-		if (is_atapi_taskfile(&qc->tf) &&
-		    !(qc->dev->flags & ATA_DFLAG_CDB_INTR))
-			return 1;
+	switch (qc->tf.protocol) {
+	case ATA_PROT_DMA:
+		return 0;
+	case ATA_PROT_ATAPI_DMA:
+		if (ap->hsm_task_state != HSM_ST_FIRST)
+			return 0;
 	}
 
-	return 0;
+	return (ap->pflags & ATA_PFLAG_HSM_WQ) ? 1 : 0;
 }
 
 /**
@@ -4559,7 +4554,7 @@ fsm_start:
 				goto fsm_start;
 			}
 
-			atapi_pio_bytes(qc, 0);
+			atapi_pio_bytes(qc, in_wq);
 
 			if (unlikely(ap->hsm_task_state == HSM_ST_ERR))
 				/* bad ireason reported by device */
@@ -4614,7 +4609,7 @@ fsm_start:
 				goto fsm_start;
 			}
 
-			ata_pio_sectors(qc, 0);
+			ata_pio_sectors(qc, in_wq);
 
 			if (ap->hsm_task_state == HSM_ST_IDLE) {
 				/* all data read */
@@ -4713,6 +4708,31 @@ fsm_start:
 		goto fsm_start;
 }
 
+static void ata_irq_task(struct work_struct *work)
+{
+	struct ata_port *ap =
+		container_of(work, struct ata_port, port_task.work);
+	struct ata_queued_cmd *qc = ap->port_task_data;
+	u8 status;
+
+	WARN_ON(ap->hsm_task_state == HSM_ST_IDLE);
+
+	/* double check the device is not busy */
+	status = ata_chk_status(ap);
+	if (status & ATA_BUSY) {
+		ata_port_printk(ap, KERN_ERR, "Unexpected device busy "
+				"(Status 0x%x)\n", status);
+		return;
+	}
+
+	/* move the HSM */
+	ata_hsm_move(ap, qc, status, 1);
+
+	/* another command or interrupt handler
+	 * may be running at this point.
+	 */
+}
+
 /**
  *	ata_qc_new - Request an available ATA command, for queueing
  *	@ap: Port associated with device @dev
@@ -5250,11 +5270,20 @@ inline unsigned int ata_host_intr (struc
 	/* ack bmdma irq events */
 	ap->ops->irq_clear(ap);
 
-	ata_hsm_move(ap, qc, status, 0);
+	/* invoke HSM */
+	switch (ap->hsm_task_state) {
+	case HSM_ST_FIRST:
+	case HSM_ST:
+		ap->pflags |= ATA_PFLAG_HSM_WQ;
+		ata_port_queue_task(ap, ata_irq_task, qc, 0);
+		break;
+	default:
+		ata_hsm_move(ap, qc, status, 0);
 
-	if (unlikely(qc->err_mask) && (qc->tf.protocol == ATA_PROT_DMA ||
-				       qc->tf.protocol == ATA_PROT_ATAPI_DMA))
-		ata_ehi_push_desc(ehi, "BMDMA stat 0x%x", host_stat);
+		if (unlikely(qc->err_mask) && (qc->tf.protocol == ATA_PROT_DMA ||
+					       qc->tf.protocol == ATA_PROT_ATAPI_DMA))
+			ata_ehi_push_desc(ehi, "BMDMA stat 0x%x", host_stat);
+	}
 
 	return 1;	/* irq handled */
 

[PATCH 7/7] libata: ack unexpected INTRQ when polling

[Albert Lee]

patch 7/7:
 ack unexpected INTRQ when polling.
(Some device asserts INTRQ even if polling and nIEN = 1.
http://bugzilla.kernel.org/show_bug.cgi?id=8441)

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---

diff -Nrup 06_irq_wq/drivers/ata/libata-core.c 07_ack_random_irq/drivers/ata/libata-core.c
--- 06_irq_wq/drivers/ata/libata-core.c	2007-05-11 11:54:56.000000000 +0800
+++ 07_ack_random_irq/drivers/ata/libata-core.c	2007-05-11 15:40:37.000000000 +0800
@@ -5211,9 +5211,12 @@ inline unsigned int ata_host_intr (struc
 	if (ap->pflags & ATA_PFLAG_HSM_WQ)
 		goto idle_irq;
 
-	/* polling */
-	if (qc->tf.flags & ATA_TFLAG_POLLING)
+	/* polling, while HSM not yet active in wq */
+	if (qc->tf.flags & ATA_TFLAG_POLLING) {
+		/* ack random irq */
+		ata_chk_status(ap);
 		goto idle_irq;
+	}
 
 	/* Check whether we are expecting interrupt in this state */
 	switch (ap->hsm_task_state) {

Re: [PATCH] libata: disable_irq() during polling IDENTIFY

[Alan Cox]

> This reminds me of a possible issue with the patch:
> Previously the polling code assumes that the interrupt handler won't interfere
> with it and the polling code runs without holding ap->lock.
> However, with the above patch, the interrupt handler might read the
> Status register when the polling code is transfering data, etc. from the port.
> Would this cause trouble to the ATA/ATAPI devices?

Oh yes, and also some controllers will corrupt their data stream if you
do this during a PIO transfer of a block of ATA data.

Alan

Re: [PATCH] libata: disable_irq() during polling IDENTIFY

[Mark Lord]

Alan Cox wrote:
>> This reminds me of a possible issue with the patch:
>> Previously the polling code assumes that the interrupt handler won't interfere
>> with it and the polling code runs without holding ap->lock.
>> However, with the above patch, the interrupt handler might read the
>> Status register when the polling code is transfering data, etc. from the port.
>> Would this cause trouble to the ATA/ATAPI devices?
> 
> Oh yes, and also some controllers will corrupt their data stream if you
> do this during a PIO transfer of a block of ATA data.

I also believe that to be true, but don't know exactly which hardware
has that issue.

On the other hand, if we don't read the status from the IRQ handler,
then how does the IRQ get cleared ????

This whole thing is quite messy.

Re: [PATCH] libata: disable_irq() during polling IDENTIFY

[Alan Cox]

> > Oh yes, and also some controllers will corrupt their data stream if you
> > do this during a PIO transfer of a block of ATA data.
> 
> I also believe that to be true, but don't know exactly which hardware
> has that issue.

Some stuff which needs to block local irqs during the transfer anyway (eg
ancient VIA) which therefore doesn't raise that problem much if our
locking is careful (right now we block local irq but we can spin_lock
happily in these as they are almost always uniprocessor anyway)

There is also some intel stuff with this problem (PIIX4 errata 15, 440MX
errata 13.