[PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix

[PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix

[Tejun Heo]

 Hello, Jeff and Albert.

 This patchset fixes the following race.
(port A has ATA device and B ATAPI).

port B	: ata_issue_prot() (ATAPI_NODATA)
port B	: packet_task scheduled
port A	: ata_issue_prot() (DMA)
intr	: ata_interrupt()
port A	: ata_host_intr -> this is all good & dandy
port B	: ata_host_intr -> finishes ATAPI cmd w/ error (request sense)

 This is where the race is, we're polling for port B's qc, we must not
mess with it from interrupt.  Now, port B has dangling packet_task
which will race w/ whatever will run on port B.

 The problem is that we don't always protect polled ports from
interrupts with ata_qc_set_polling() and for non-DMA ATA commands
there's no way to discern if actually an IRQ has occurred (this
sucks), so we end up finishing the other port's command.

 This condition occurs quite often if both port A and B are busy.  The
most common result is assertion failure in atapi_packet_task
(assert(qc->flags & ATA_QCFLAG_ACTIVE)), but depending on timing and
on SMP, weirder things could happen, I think.

 Note that for ATAPI_DMA, interrupt from the other port won't mess
with a polled command as we can tell that it's not ours with
bmdma_status, but, if spurious interrupt occurs on the port, the
packet_task will go dangling.  That's why ATAPI_DMA also needs
protection.  The baseline is that all polled qc's need to be protected
with ata_qc_set_polling() until polling task is done with the command.

 Albert, I think, part of your assertion failures are from this.  We
also have unprotected qc_complete/EH race which can also cause other
assertion failures.  I'll post a patch for that soon.

[ Start of patch descriptions ]

01_atapi_update-ata_irq_on.patch
	: update ata_irq_on

	Update ata_irq_on for atapi_packet_task fix.  Changes are

	* IRQ is cleared _before_ unmasking interrupt.  This is
	  necessary as unmasking could raise a pending interrupt
	  which should have been ignored.

	* IRQ is cleared using both ata_chk_status and
          ap->ops->irq_clear.

	* ap->ctl value represents default ctl value and never gets
	  modified.  ap->last_ctl is the currently loaded ctl value.
	  Don't turn off ATA_NIEN on ap->ctl as the bit never gets set
	  on it.  Directly modify ap->last_ctl.

	* Don't do ata_wait_idle.  All current callers don't need it.
	  And it will always time out when used in atapi_packet_task.

02_atapi_fix-atapi_packet_task-race.patch
	: fix atapi_packet_task vs. ata_host_intr race

	Protect ATAPI_NODATA and ATAPI_DMA commands from being
	finished by ata_host_intr before atapi_packet_task is done w/
	the commands.  Previously, such ATAPI commands could have been
	completed by interrupts from other ports in the same host_set
	or by spurious interrupt, making atapi_packet_task access
	already finished commands.

[ End of patch descriptions ]

Thanks.

--
tejun

Re: [PATCH libata-dev-2.6:upstream 01/02] atapi: update ata_irq_on

[Tejun Heo]

01_atapi_update-ata_irq_on.patch

	Update ata_irq_on for atapi_packet_task fix.  Changes are

	* IRQ is cleared _before_ unmasking interrupt.  This is
	  necessary as unmasking could raise a pending interrupt
	  which should have been ignored.

	* IRQ is cleared using both ata_chk_status and
          ap->ops->irq_clear.

	* ap->ctl value represents default ctl value and never gets
	  modified.  ap->last_ctl is the currently loaded ctl value.
	  Don't turn off ATA_NIEN on ap->ctl as the bit never gets set
	  on it.  Directly modify ap->last_ctl.

	* Don't do ata_wait_idle.  All current callers don't need it.
	  And it will always time out when used in atapi_packet_task.

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

 libata.h |   28 +++++++++++++++-------------
 1 files changed, 15 insertions(+), 13 deletions(-)

Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h	2005-08-20 18:12:30.000000000 +0900
+++ work/include/linux/libata.h	2005-08-20 18:16:49.000000000 +0900
@@ -565,30 +565,32 @@ static inline void ata_tf_init(struct at
  *	ata_irq_on - Enable interrupts on a port.
  *	@ap: Port on which interrupts are enabled.
  *
- *	Enable interrupts on a legacy IDE device using MMIO or PIO,
- *	wait for idle, clear any pending interrupts.
+ *	Clear pending interrupts and enable interrupts on a legacy IDE
+ *	device using MMIO or PIO.
  *
  *	LOCKING:
  *	Inherited from caller.
  */
 
-static inline u8 ata_irq_on(struct ata_port *ap)
+static inline void ata_irq_on(struct ata_port *ap)
 {
 	struct ata_ioports *ioaddr = &ap->ioaddr;
-	u8 tmp;
 
-	ap->ctl &= ~ATA_NIEN;
-	ap->last_ctl = ap->ctl;
+	/* Clear pending interrupts. */
+	ata_chk_status(ap);
+	ap->ops->irq_clear(ap);
 
-	if (ap->flags & ATA_FLAG_MMIO)
-		writeb(ap->ctl, (void __iomem *) ioaddr->ctl_addr);
-	else
-		outb(ap->ctl, ioaddr->ctl_addr);
-	tmp = ata_wait_idle(ap);
+	ap->last_ctl &= ATA_NIEN;
 
-	ap->ops->irq_clear(ap);
+	/* smp_wmb() is here to make sure that other processors see
+	 * ATA_NIEN cleared on receiving interrupts.
+	 */
+	smp_wmb();
 
-	return tmp;
+	if (ap->flags & ATA_FLAG_MMIO)
+		writeb(ap->last_ctl, (void __iomem *) ioaddr->ctl_addr);
+	else
+		outb(ap->last_ctl, ioaddr->ctl_addr);
 }
 
 

Re: [PATCH libata-dev-2.6:upstream 02/02] atapi: fix atapi_packet_task vs. ata_host_intr race

[Tejun Heo]

02_atapi_fix-atapi_packet_task-race.patch

	Protect ATAPI_NODATA and ATAPI_DMA commands from being
	finished by ata_host_intr before atapi_packet_task is done w/
	the commands.  Previously, such ATAPI commands could have been
	completed by interrupts from other ports in the same host_set
	or by spurious interrupt, making atapi_packet_task access
	already finished commands.

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

 libata-core.c |   34 +++++++++++++++++++++-------------
 1 files changed, 21 insertions(+), 13 deletions(-)

Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2005-08-20 18:12:30.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2005-08-20 18:16:49.000000000 +0900
@@ -3218,11 +3218,13 @@ int ata_qc_issue_prot(struct ata_queued_
 		break;
 
 	case ATA_PROT_ATAPI_NODATA:
+		ata_qc_set_polling(qc);
 		ata_tf_to_host_nolock(ap, &qc->tf);
 		queue_work(ata_wq, &ap->packet_task);
 		break;
 
 	case ATA_PROT_ATAPI_DMA:
+		ata_qc_set_polling(qc);
 		ap->ops->tf_load(ap, &qc->tf);	 /* load tf registers */
 		ap->ops->bmdma_setup(qc);	    /* set up bmdma */
 		queue_work(ata_wq, &ap->packet_task);
@@ -3580,8 +3582,8 @@ 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.ctl & ATA_NIEN)) &&
-			    (qc->flags & ATA_QCFLAG_ACTIVE))
+			if ((!(ap->last_ctl & ATA_NIEN)) &&
+			    qc && (qc->flags & ATA_QCFLAG_ACTIVE))
 				handled |= ata_host_intr(ap, qc);
 		}
 	}
@@ -3628,19 +3630,25 @@ static void atapi_packet_task(void *_dat
 	/* send SCSI cdb */
 	DPRINTK("send cdb\n");
 	assert(ap->cdb_len >= 12);
-	ata_data_xfer(ap, qc->cdb, ap->cdb_len, 1);
 
-	/* if we are DMA'ing, irq handler takes over from here */
-	if (qc->tf.protocol == ATA_PROT_ATAPI_DMA)
-		ap->ops->bmdma_start(qc);	    /* initiate bmdma */
-
-	/* non-data commands are also handled via irq */
-	else if (qc->tf.protocol == ATA_PROT_ATAPI_NODATA) {
-		/* do nothing */
-	}
+	if (qc->tf.protocol == ATA_PROT_ATAPI_DMA ||
+	    qc->tf.protocol == ATA_PROT_ATAPI_NODATA) {
+		/* Once we're done issuing command and kicking bmdma,
+		 * irq handler takes over.  To not lose irq, we need
+		 * to enable irq before sending cdb, but interrupt
+		 * handler shouldn't be invoked before we're finished.
+		 * Hence, the following locking.
+		 */
+		spin_lock_irq(&ap->host_set->lock);
+		ata_irq_on(ap);
+		ata_data_xfer(ap, qc->cdb, ap->cdb_len, 1);
+		if (qc->tf.protocol == ATA_PROT_ATAPI_DMA)
+			ap->ops->bmdma_start(qc);	/* initiate bmdma */
+		spin_unlock_irq(&ap->host_set->lock);
+	} else {
+		ata_data_xfer(ap, qc->cdb, ap->cdb_len, 1);
 
-	/* PIO commands are handled by polling */
-	else {
+		/* PIO commands are handled by polling */
 		ap->pio_task_state = PIO_ST;
 		queue_work(ata_wq, &ap->pio_task);
 	}

Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix

[Jeff Garzik]

Tejun Heo wrote:
>  Hello, Jeff and Albert.
> 
>  This patchset fixes the following race.
> (port A has ATA device and B ATAPI).
> 
> port B	: ata_issue_prot() (ATAPI_NODATA)
> port B	: packet_task scheduled
> port A	: ata_issue_prot() (DMA)
> intr	: ata_interrupt()
> port A	: ata_host_intr -> this is all good & dandy
> port B	: ata_host_intr -> finishes ATAPI cmd w/ error (request sense)
> 
>  This is where the race is, we're polling for port B's qc, we must not
> mess with it from interrupt.  Now, port B has dangling packet_task
> which will race w/ whatever will run on port B.
> 
>  The problem is that we don't always protect polled ports from
> interrupts with ata_qc_set_polling() and for non-DMA ATA commands
> there's no way to discern if actually an IRQ has occurred (this
> sucks), so we end up finishing the other port's command.
> 
>  This condition occurs quite often if both port A and B are busy.  The
> most common result is assertion failure in atapi_packet_task
> (assert(qc->flags & ATA_QCFLAG_ACTIVE)), but depending on timing and
> on SMP, weirder things could happen, I think.
> 
>  Note that for ATAPI_DMA, interrupt from the other port won't mess
> with a polled command as we can tell that it's not ours with
> bmdma_status, but, if spurious interrupt occurs on the port, the
> packet_task will go dangling.  That's why ATAPI_DMA also needs
> protection.  The baseline is that all polled qc's need to be protected
> with ata_qc_set_polling() until polling task is done with the command.
> 
> 
> [ Start of patch descriptions ]

While this is something to look into, the supplied patches are 
definitely not the way we want to go.  We need to follow the state 
diagram for the PACKET command protocol.  ATA4 [1] diagrams are a good 
thing to read, since they include mention of the behavior of certain 
ATAPI devices that can send an interrupt between taskfile-out and 
write-cdb steps in the sequence.

In patch #2, you're making ata_irq_on() way too heavy.  In patch #1, 
calling ata_qc_set_polling() for non-polled commands is a hack.

The better solution is to track the PACKET command protocol state much 
more closely, so that the code _knows_ when it should and shouldn't be 
getting an interrupt.

This is required anyway because, as mentioned in another email, an ATA 
device might assert INTRQ for certain events, such as non-data commands, 
where the controller is not required to assert the BMDMA IRQ bit.

I _suspect_ that many host controllers cause the BMDMA IRQ bit to track 
the ATA INTRQ status precisely, but this theory has not been validated.

	Jeff


[1] http://www.t13.org/project/d1153r18-ATA-ATAPI-4.pdf

Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix

[Tejun Heo]

  Hello, Jeff.

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>>  Hello, Jeff and Albert.
>>
>>  This patchset fixes the following race.
>> (port A has ATA device and B ATAPI).
>>
>> port B    : ata_issue_prot() (ATAPI_NODATA)
>> port B    : packet_task scheduled
>> port A    : ata_issue_prot() (DMA)
>> intr    : ata_interrupt()
>> port A    : ata_host_intr -> this is all good & dandy
>> port B    : ata_host_intr -> finishes ATAPI cmd w/ error (request sense)
>>
>>  This is where the race is, we're polling for port B's qc, we must not
>> mess with it from interrupt.  Now, port B has dangling packet_task
>> which will race w/ whatever will run on port B.
>>
>>  The problem is that we don't always protect polled ports from
>> interrupts with ata_qc_set_polling() and for non-DMA ATA commands
>> there's no way to discern if actually an IRQ has occurred (this
>> sucks), so we end up finishing the other port's command.
>>
>>  This condition occurs quite often if both port A and B are busy.  The
>> most common result is assertion failure in atapi_packet_task
>> (assert(qc->flags & ATA_QCFLAG_ACTIVE)), but depending on timing and
>> on SMP, weirder things could happen, I think.
>>
>>  Note that for ATAPI_DMA, interrupt from the other port won't mess
>> with a polled command as we can tell that it's not ours with
>> bmdma_status, but, if spurious interrupt occurs on the port, the
>> packet_task will go dangling.  That's why ATAPI_DMA also needs
>> protection.  The baseline is that all polled qc's need to be protected
>> with ata_qc_set_polling() until polling task is done with the command.
>>
>>
>> [ Start of patch descriptions ]
> 
> 
> While this is something to look into, the supplied patches are 
> definitely not the way we want to go.  We need to follow the state 
> diagram for the PACKET command protocol.  ATA4 [1] diagrams are a good 
> thing to read, since they include mention of the behavior of certain 
> ATAPI devices that can send an interrupt between taskfile-out and 
> write-cdb steps in the sequence.
> 
> In patch #2, you're making ata_irq_on() way too heavy.  In patch #1, 
> calling ata_qc_set_polling() for non-polled commands is a hack.

  Have you received patches in the reverse order?  #1 changes 
ata_irq_on() and #2 adds ata_qc_set_polling().

  Hmmm, only a call to ata_chk_status() is added to ata_irq_on(), which 
I think is needed regardless of other changes, and ata_wait_idle() is 
removed.  Does that make the function heavy?

> 
> The better solution is to track the PACKET command protocol state much 
> more closely, so that the code _knows_ when it should and shouldn't be 
> getting an interrupt.
> 
> This is required anyway because, as mentioned in another email, an ATA 
> device might assert INTRQ for certain events, such as non-data commands, 
> where the controller is not required to assert the BMDMA IRQ bit.
> 
> I _suspect_ that many host controllers cause the BMDMA IRQ bit to track 
> the ATA INTRQ status precisely, but this theory has not been validated.
> 
>     Jeff
> 
> 
> [1] http://www.t13.org/project/d1153r18-ATA-ATAPI-4.pdf

  Without interrupt pending information from BMDMA bit for non-DMA 
commands (which I don't think we can use for non-DMA cmds as we'll never 
be sure if all controllers behave that way), the problem is that for 
many SATA controllers, more than one ports share single interrupt line. 
   And without interrupt pending bit, shared interrupt means a lot of 
spurious interrupts making it impossible to know when to expect interrupts.

  IDE driver deals with this by having only one command active per 
interrupt, but SATA doesn't have such scheme yet.  And I don't know if 
such a scheme is desirable at all.  Maybe just continuing to poll 
non-DMA commands (which isn't much a burden anyway) and keeping DMA 
commands fast is a better approach.

  So, the thing is that if we run non-DMA commands and other commands at 
the same time on ports which share interrupts, we'll get spurious 
interrupts very often regardless of ATA/ATAPI state machine.  Remember 
many spurious interrupts reports/fixes regarding ATAPI devices?  I 
suspect many of them are not actual spurious interrupts but interrupts 
from the other device sharing the host_set.

  So, how should we do here?  To follow ATA/ATAPI state machine, we need 
to implement exclusion among ports sharing an interrupt.  Is this the 
way to go?  Arggggggg...  Lack of interrupt pending bit is such a pain 
in the ass.  :-(

-- 
tejun

Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix

[Jeff Garzik]

Tejun Heo wrote:
> 
>  Hello, Jeff.
> 
> Jeff Garzik wrote:
> 
>> Tejun Heo wrote:
>>
>>>  Hello, Jeff and Albert.
>>>
>>>  This patchset fixes the following race.
>>> (port A has ATA device and B ATAPI).
>>>
>>> port B    : ata_issue_prot() (ATAPI_NODATA)
>>> port B    : packet_task scheduled
>>> port A    : ata_issue_prot() (DMA)
>>> intr    : ata_interrupt()
>>> port A    : ata_host_intr -> this is all good & dandy
>>> port B    : ata_host_intr -> finishes ATAPI cmd w/ error (request sense)
>>>
>>>  This is where the race is, we're polling for port B's qc, we must not
>>> mess with it from interrupt.  Now, port B has dangling packet_task
>>> which will race w/ whatever will run on port B.
>>>
>>>  The problem is that we don't always protect polled ports from
>>> interrupts with ata_qc_set_polling() and for non-DMA ATA commands
>>> there's no way to discern if actually an IRQ has occurred (this
>>> sucks), so we end up finishing the other port's command.
>>>
>>>  This condition occurs quite often if both port A and B are busy.  The
>>> most common result is assertion failure in atapi_packet_task
>>> (assert(qc->flags & ATA_QCFLAG_ACTIVE)), but depending on timing and
>>> on SMP, weirder things could happen, I think.
>>>
>>>  Note that for ATAPI_DMA, interrupt from the other port won't mess
>>> with a polled command as we can tell that it's not ours with
>>> bmdma_status, but, if spurious interrupt occurs on the port, the
>>> packet_task will go dangling.  That's why ATAPI_DMA also needs
>>> protection.  The baseline is that all polled qc's need to be protected
>>> with ata_qc_set_polling() until polling task is done with the command.
>>>
>>>
>>> [ Start of patch descriptions ]
>>
>>
>>
>> While this is something to look into, the supplied patches are 
>> definitely not the way we want to go.  We need to follow the state 
>> diagram for the PACKET command protocol.  ATA4 [1] diagrams are a good 
>> thing to read, since they include mention of the behavior of certain 
>> ATAPI devices that can send an interrupt between taskfile-out and 
>> write-cdb steps in the sequence.
>>
>> In patch #2, you're making ata_irq_on() way too heavy.  In patch #1, 
>> calling ata_qc_set_polling() for non-polled commands is a hack.
> 
> 
>  Have you received patches in the reverse order?  #1 changes 
> ata_irq_on() and #2 adds ata_qc_set_polling().
> 
>  Hmmm, only a call to ata_chk_status() is added to ata_irq_on(), which I 
> think is needed regardless of other changes, and ata_wait_idle() is 
> removed.  Does that make the function heavy?
> 
>>
>> The better solution is to track the PACKET command protocol state much 
>> more closely, so that the code _knows_ when it should and shouldn't be 
>> getting an interrupt.
>>
>> This is required anyway because, as mentioned in another email, an ATA 
>> device might assert INTRQ for certain events, such as non-data 
>> commands, where the controller is not required to assert the BMDMA IRQ 
>> bit.
>>
>> I _suspect_ that many host controllers cause the BMDMA IRQ bit to 
>> track the ATA INTRQ status precisely, but this theory has not been 
>> validated.
>>
>>     Jeff
>>
>>
>> [1] http://www.t13.org/project/d1153r18-ATA-ATAPI-4.pdf
> 
> 
>  Without interrupt pending information from BMDMA bit for non-DMA 
> commands (which I don't think we can use for non-DMA cmds as we'll never 
> be sure if all controllers behave that way), the problem is that for 
> many SATA controllers, more than one ports share single interrupt line. 
>   And without interrupt pending bit, shared interrupt means a lot of 
> spurious interrupts making it impossible to know when to expect interrupts.

Incorrect...  this is why I keep harping on the "ATA host state machine."

You rely on proper implementation to know when to expect interrupts. 
Read the state diagrams, they tell you precisely when an interrupt may 
be expected.  By definition, any other interrupt is probably a PCI 
shared interrupt, or a hardware or software bug.


>  IDE driver deals with this by having only one command active per 
> interrupt, but SATA doesn't have such scheme yet.  And I don't know if 
> such a scheme is desirable at all.  Maybe just continuing to poll 
> non-DMA commands (which isn't much a burden anyway) and keeping DMA 
> commands fast is a better approach.

The IDE driver has a high density, but look closely... it follows the 
ATA host state machine as well.


>  So, how should we do here?  To follow ATA/ATAPI state machine, we need 
> to implement exclusion among ports sharing an interrupt.  Is this the 
> way to go?  Arggggggg...  Lack of interrupt pending bit is such a pain 
> in the ass.  :-(

If the code knows what state its in, it knows whether or not to expect 
an interrupt.  All that state information should already be synchronized 
by spinlock(host-lock), too.

	Jeff

Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>>
>>  Hello, Jeff.
>>
>> Jeff Garzik wrote:
>>
>>> Tejun Heo wrote:
>>>
>>>>  Hello, Jeff and Albert.
>>>>
>>>>  This patchset fixes the following race.
>>>> (port A has ATA device and B ATAPI).
>>>>
>>>> port B    : ata_issue_prot() (ATAPI_NODATA)
>>>> port B    : packet_task scheduled
>>>> port A    : ata_issue_prot() (DMA)
>>>> intr    : ata_interrupt()
>>>> port A    : ata_host_intr -> this is all good & dandy
>>>> port B    : ata_host_intr -> finishes ATAPI cmd w/ error (request 
>>>> sense)
>>>>
>>>>  This is where the race is, we're polling for port B's qc, we must not
>>>> mess with it from interrupt.  Now, port B has dangling packet_task
>>>> which will race w/ whatever will run on port B.
>>>>
>>>>  The problem is that we don't always protect polled ports from
>>>> interrupts with ata_qc_set_polling() and for non-DMA ATA commands
>>>> there's no way to discern if actually an IRQ has occurred (this
>>>> sucks), so we end up finishing the other port's command.
>>>>
>>>>  This condition occurs quite often if both port A and B are busy.  The
>>>> most common result is assertion failure in atapi_packet_task
>>>> (assert(qc->flags & ATA_QCFLAG_ACTIVE)), but depending on timing and
>>>> on SMP, weirder things could happen, I think.
>>>>
>>>>  Note that for ATAPI_DMA, interrupt from the other port won't mess
>>>> with a polled command as we can tell that it's not ours with
>>>> bmdma_status, but, if spurious interrupt occurs on the port, the
>>>> packet_task will go dangling.  That's why ATAPI_DMA also needs
>>>> protection.  The baseline is that all polled qc's need to be protected
>>>> with ata_qc_set_polling() until polling task is done with the command.
>>>>
>>>>
>>>> [ Start of patch descriptions ]
>>>
>>>
>>>
>>>
>>> While this is something to look into, the supplied patches are 
>>> definitely not the way we want to go.  We need to follow the state 
>>> diagram for the PACKET command protocol.  ATA4 [1] diagrams are a 
>>> good thing to read, since they include mention of the behavior of 
>>> certain ATAPI devices that can send an interrupt between taskfile-out 
>>> and write-cdb steps in the sequence.
>>>
>>> In patch #2, you're making ata_irq_on() way too heavy.  In patch #1, 
>>> calling ata_qc_set_polling() for non-polled commands is a hack.
>>
>>
>>
>>  Have you received patches in the reverse order?  #1 changes 
>> ata_irq_on() and #2 adds ata_qc_set_polling().
>>
>>  Hmmm, only a call to ata_chk_status() is added to ata_irq_on(), which 
>> I think is needed regardless of other changes, and ata_wait_idle() is 
>> removed.  Does that make the function heavy?
>>
>>>
>>> The better solution is to track the PACKET command protocol state 
>>> much more closely, so that the code _knows_ when it should and 
>>> shouldn't be getting an interrupt.
>>>
>>> This is required anyway because, as mentioned in another email, an 
>>> ATA device might assert INTRQ for certain events, such as non-data 
>>> commands, where the controller is not required to assert the BMDMA 
>>> IRQ bit.
>>>
>>> I _suspect_ that many host controllers cause the BMDMA IRQ bit to 
>>> track the ATA INTRQ status precisely, but this theory has not been 
>>> validated.
>>>
>>>     Jeff
>>>
>>>
>>> [1] http://www.t13.org/project/d1153r18-ATA-ATAPI-4.pdf
>>
>>
>>
>>  Without interrupt pending information from BMDMA bit for non-DMA 
>> commands (which I don't think we can use for non-DMA cmds as we'll 
>> never be sure if all controllers behave that way), the problem is that 
>> for many SATA controllers, more than one ports share single interrupt 
>> line.   And without interrupt pending bit, shared interrupt means a 
>> lot of spurious interrupts making it impossible to know when to expect 
>> interrupts.
> 
> 
> Incorrect...  this is why I keep harping on the "ATA host state machine."
> 
> You rely on proper implementation to know when to expect interrupts. 
> Read the state diagrams, they tell you precisely when an interrupt may 
> be expected.  By definition, any other interrupt is probably a PCI 
> shared interrupt, or a hardware or software bug.
> 

  I think I'm familiar with ATA host state machine.  I don't think I'm 
currently saying things because I don't know about it.  Maybe I'm 
missing something here.  Please consider the following scenario.

1. non-DMA command issued.
2. device does something
3. another device sharing the IRQ line raises interrupt
4. D2H FIS received (BSY off, DRDY on, INTR on)
5. interrupt handler kicks in for #3
6. SATA controller raises interrupt
7. SATA handler invoked from #5 checks ATA_STATUS and determines that
    completes the command (we don't know if it's our interrupt or not)
8. interrupt handler from #5 returns.
9. interrupt handler for #6.
10. nobody cared.

  Did I miss something?

> 
>>  IDE driver deals with this by having only one command active per 
>> interrupt, but SATA doesn't have such scheme yet.  And I don't know if 
>> such a scheme is desirable at all.  Maybe just continuing to poll 
>> non-DMA commands (which isn't much a burden anyway) and keeping DMA 
>> commands fast is a better approach.
> 
> 
> The IDE driver has a high density, but look closely... it follows the 
> ATA host state machine as well.
> 

  Yes it does, and it does a lot of exclusion among ports to do so...

> 
>>  So, how should we do here?  To follow ATA/ATAPI state machine, we 
>> need to implement exclusion among ports sharing an interrupt.  Is this 
>> the way to go?  Arggggggg...  Lack of interrupt pending bit is such a 
>> pain in the ass.  :-(
> 
> 
> If the code knows what state its in, it knows whether or not to expect 
> an interrupt.  All that state information should already be synchronized 
> by spinlock(host-lock), too.
> 

  I think the problem is whether or not we're expecting interrupts, 
we'll get interrupts that are't ours and we have no way to tell if 
they're ours or not.  No?

  Thanks.

-- 
tejun

Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
> 
>> Tejun Heo wrote:
>>
>>>
>>>  Hello, Jeff.
>>>
>>> Jeff Garzik wrote:
>>>
>>>> Tejun Heo wrote:
>>>>
>>>>>  Hello, Jeff and Albert.
>>>>>
>>>>>  This patchset fixes the following race.
>>>>> (port A has ATA device and B ATAPI).
>>>>>
>>>>> port B    : ata_issue_prot() (ATAPI_NODATA)
>>>>> port B    : packet_task scheduled
>>>>> port A    : ata_issue_prot() (DMA)
>>>>> intr    : ata_interrupt()
>>>>> port A    : ata_host_intr -> this is all good & dandy
>>>>> port B    : ata_host_intr -> finishes ATAPI cmd w/ error (request 
>>>>> sense)
>>>>>
>>>>>  This is where the race is, we're polling for port B's qc, we must not
>>>>> mess with it from interrupt.  Now, port B has dangling packet_task
>>>>> which will race w/ whatever will run on port B.
>>>>>
>>>>>  The problem is that we don't always protect polled ports from
>>>>> interrupts with ata_qc_set_polling() and for non-DMA ATA commands
>>>>> there's no way to discern if actually an IRQ has occurred (this
>>>>> sucks), so we end up finishing the other port's command.
>>>>>
>>>>>  This condition occurs quite often if both port A and B are busy.  The
>>>>> most common result is assertion failure in atapi_packet_task
>>>>> (assert(qc->flags & ATA_QCFLAG_ACTIVE)), but depending on timing and
>>>>> on SMP, weirder things could happen, I think.
>>>>>
>>>>>  Note that for ATAPI_DMA, interrupt from the other port won't mess
>>>>> with a polled command as we can tell that it's not ours with
>>>>> bmdma_status, but, if spurious interrupt occurs on the port, the
>>>>> packet_task will go dangling.  That's why ATAPI_DMA also needs
>>>>> protection.  The baseline is that all polled qc's need to be protected
>>>>> with ata_qc_set_polling() until polling task is done with the command.
>>>>>
>>>>>
>>>>> [ Start of patch descriptions ]
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> While this is something to look into, the supplied patches are 
>>>> definitely not the way we want to go.  We need to follow the state 
>>>> diagram for the PACKET command protocol.  ATA4 [1] diagrams are a 
>>>> good thing to read, since they include mention of the behavior of 
>>>> certain ATAPI devices that can send an interrupt between 
>>>> taskfile-out and write-cdb steps in the sequence.
>>>>
>>>> In patch #2, you're making ata_irq_on() way too heavy.  In patch #1, 
>>>> calling ata_qc_set_polling() for non-polled commands is a hack.
>>>
>>>
>>>
>>>
>>>  Have you received patches in the reverse order?  #1 changes 
>>> ata_irq_on() and #2 adds ata_qc_set_polling().
>>>
>>>  Hmmm, only a call to ata_chk_status() is added to ata_irq_on(), 
>>> which I think is needed regardless of other changes, and 
>>> ata_wait_idle() is removed.  Does that make the function heavy?
>>>
>>>>
>>>> The better solution is to track the PACKET command protocol state 
>>>> much more closely, so that the code _knows_ when it should and 
>>>> shouldn't be getting an interrupt.
>>>>
>>>> This is required anyway because, as mentioned in another email, an 
>>>> ATA device might assert INTRQ for certain events, such as non-data 
>>>> commands, where the controller is not required to assert the BMDMA 
>>>> IRQ bit.
>>>>
>>>> I _suspect_ that many host controllers cause the BMDMA IRQ bit to 
>>>> track the ATA INTRQ status precisely, but this theory has not been 
>>>> validated.
>>>>
>>>>     Jeff
>>>>
>>>>
>>>> [1] http://www.t13.org/project/d1153r18-ATA-ATAPI-4.pdf
>>>
>>>
>>>
>>>
>>>  Without interrupt pending information from BMDMA bit for non-DMA 
>>> commands (which I don't think we can use for non-DMA cmds as we'll 
>>> never be sure if all controllers behave that way), the problem is 
>>> that for many SATA controllers, more than one ports share single 
>>> interrupt line.   And without interrupt pending bit, shared interrupt 
>>> means a lot of spurious interrupts making it impossible to know when 
>>> to expect interrupts.
>>
>>
>>
>> Incorrect...  this is why I keep harping on the "ATA host state machine."
>>
>> You rely on proper implementation to know when to expect interrupts. 
>> Read the state diagrams, they tell you precisely when an interrupt may 
>> be expected.  By definition, any other interrupt is probably a PCI 
>> shared interrupt, or a hardware or software bug.
>>
> 
>  I think I'm familiar with ATA host state machine.  I don't think I'm 
> currently saying things because I don't know about it.  Maybe I'm 
> missing something here.  Please consider the following scenario.
> 
> 1. non-DMA command issued.
> 2. device does something
> 3. another device sharing the IRQ line raises interrupt
> 4. D2H FIS received (BSY off, DRDY on, INTR on)
> 5. interrupt handler kicks in for #3
> 6. SATA controller raises interrupt
> 7. SATA handler invoked from #5 checks ATA_STATUS and determines that
>    completes the command (we don't know if it's our interrupt or not)
> 8. interrupt handler from #5 returns.
> 9. interrupt handler for #6.
> 10. nobody cared.
> 
>  Did I miss something?

This gets into interrupt controller hardware, how interrupts are 
asserted and delivered, and similar not-IDE-specific things.

If you clear the condition (by checking ATA Status) that caused the 
interrupt hardware to raise an interrupt, during #5 and #7, then the 
interrupt condition is cleared in the interrupt controller hardware, and 
the kernel should not invoke the interrupt handler again.


>  I think the problem is whether or not we're expecting interrupts, we'll 
> get interrupts that are't ours and we have no way to tell if they're 
> ours or not.  No?

You can tell just by following normal interrupt assertion rules, and by 
the "if it's nor ours it's either somebody else's interrupt, or 
out-of-spec hardware"

In addition, (slight tangent)
* if DMA is active, you can check the DMA status bits to see if you have 
an interrupt condition to handle
* if DMA is not active, you can check the AltStatus register, to test 
the state without actually clearing anything

Though beware, on rare PATA controllers (old Macs?) the AltStatus 
register doesn't exist.

	Jeff

Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix

[Tejun Heo]

  Howdy, Jeff.

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> Jeff Garzik wrote:
>>
>>> Tejun Heo wrote:
>>>
>>>>
>>>>  Hello, Jeff.
>>>>
>>>> Jeff Garzik wrote:
>>>>
>>>>> Tejun Heo wrote:
>>>>>
>>>>>>  Hello, Jeff and Albert.
>>>>>>
>>>>>>  This patchset fixes the following race.
>>>>>> (port A has ATA device and B ATAPI).
>>>>>>
>>>>>> port B    : ata_issue_prot() (ATAPI_NODATA)
>>>>>> port B    : packet_task scheduled
>>>>>> port A    : ata_issue_prot() (DMA)
>>>>>> intr    : ata_interrupt()
>>>>>> port A    : ata_host_intr -> this is all good & dandy
>>>>>> port B    : ata_host_intr -> finishes ATAPI cmd w/ error (request 
>>>>>> sense)
>>>>>>
>>>>>>  This is where the race is, we're polling for port B's qc, we must 
>>>>>> not
>>>>>> mess with it from interrupt.  Now, port B has dangling packet_task
>>>>>> which will race w/ whatever will run on port B.
>>>>>>
>>>>>>  The problem is that we don't always protect polled ports from
>>>>>> interrupts with ata_qc_set_polling() and for non-DMA ATA commands
>>>>>> there's no way to discern if actually an IRQ has occurred (this
>>>>>> sucks), so we end up finishing the other port's command.
>>>>>>
>>>>>>  This condition occurs quite often if both port A and B are busy.  
>>>>>> The
>>>>>> most common result is assertion failure in atapi_packet_task
>>>>>> (assert(qc->flags & ATA_QCFLAG_ACTIVE)), but depending on timing and
>>>>>> on SMP, weirder things could happen, I think.
>>>>>>
>>>>>>  Note that for ATAPI_DMA, interrupt from the other port won't mess
>>>>>> with a polled command as we can tell that it's not ours with
>>>>>> bmdma_status, but, if spurious interrupt occurs on the port, the
>>>>>> packet_task will go dangling.  That's why ATAPI_DMA also needs
>>>>>> protection.  The baseline is that all polled qc's need to be 
>>>>>> protected
>>>>>> with ata_qc_set_polling() until polling task is done with the 
>>>>>> command.
>>>>>>
>>>>>>
>>>>>> [ Start of patch descriptions ]
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> While this is something to look into, the supplied patches are 
>>>>> definitely not the way we want to go.  We need to follow the state 
>>>>> diagram for the PACKET command protocol.  ATA4 [1] diagrams are a 
>>>>> good thing to read, since they include mention of the behavior of 
>>>>> certain ATAPI devices that can send an interrupt between 
>>>>> taskfile-out and write-cdb steps in the sequence.
>>>>>
>>>>> In patch #2, you're making ata_irq_on() way too heavy.  In patch 
>>>>> #1, calling ata_qc_set_polling() for non-polled commands is a hack.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>  Have you received patches in the reverse order?  #1 changes 
>>>> ata_irq_on() and #2 adds ata_qc_set_polling().
>>>>
>>>>  Hmmm, only a call to ata_chk_status() is added to ata_irq_on(), 
>>>> which I think is needed regardless of other changes, and 
>>>> ata_wait_idle() is removed.  Does that make the function heavy?
>>>>
>>>>>
>>>>> The better solution is to track the PACKET command protocol state 
>>>>> much more closely, so that the code _knows_ when it should and 
>>>>> shouldn't be getting an interrupt.
>>>>>
>>>>> This is required anyway because, as mentioned in another email, an 
>>>>> ATA device might assert INTRQ for certain events, such as non-data 
>>>>> commands, where the controller is not required to assert the BMDMA 
>>>>> IRQ bit.
>>>>>
>>>>> I _suspect_ that many host controllers cause the BMDMA IRQ bit to 
>>>>> track the ATA INTRQ status precisely, but this theory has not been 
>>>>> validated.
>>>>>
>>>>>     Jeff
>>>>>
>>>>>
>>>>> [1] http://www.t13.org/project/d1153r18-ATA-ATAPI-4.pdf
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>  Without interrupt pending information from BMDMA bit for non-DMA 
>>>> commands (which I don't think we can use for non-DMA cmds as we'll 
>>>> never be sure if all controllers behave that way), the problem is 
>>>> that for many SATA controllers, more than one ports share single 
>>>> interrupt line.   And without interrupt pending bit, shared 
>>>> interrupt means a lot of spurious interrupts making it impossible to 
>>>> know when to expect interrupts.
>>>
>>>
>>>
>>>
>>> Incorrect...  this is why I keep harping on the "ATA host state 
>>> machine."
>>>
>>> You rely on proper implementation to know when to expect interrupts. 
>>> Read the state diagrams, they tell you precisely when an interrupt 
>>> may be expected.  By definition, any other interrupt is probably a 
>>> PCI shared interrupt, or a hardware or software bug.
>>>
>>
>>  I think I'm familiar with ATA host state machine.  I don't think I'm 
>> currently saying things because I don't know about it.  Maybe I'm 
>> missing something here.  Please consider the following scenario.
>>
>> 1. non-DMA command issued.
>> 2. device does something
>> 3. another device sharing the IRQ line raises interrupt
>> 4. D2H FIS received (BSY off, DRDY on, INTR on)
>> 5. interrupt handler kicks in for #3
>> 6. SATA controller raises interrupt
>> 7. SATA handler invoked from #5 checks ATA_STATUS and determines that
>>    completes the command (we don't know if it's our interrupt or not)
>> 8. interrupt handler from #5 returns.
>> 9. interrupt handler for #6.
>> 10. nobody cared.
>>
>>  Did I miss something?
> 
> 
> This gets into interrupt controller hardware, how interrupts are 
> asserted and delivered, and similar not-IDE-specific things.
> 
> If you clear the condition (by checking ATA Status) that caused the 
> interrupt hardware to raise an interrupt, during #5 and #7, then the 
> interrupt condition is cleared in the interrupt controller hardware, and 
> the kernel should not invoke the interrupt handler again.
> 

  Ah... yes right.  Gotta be level interrupt to be shared.

> 
>>  I think the problem is whether or not we're expecting interrupts, 
>> we'll get interrupts that are't ours and we have no way to tell if 
>> they're ours or not.  No?
> 
> 
> You can tell just by following normal interrupt assertion rules, and by 
> the "if it's nor ours it's either somebody else's interrupt, or 
> out-of-spec hardware"
> 
> In addition, (slight tangent)
> * if DMA is active, you can check the DMA status bits to see if you have 
> an interrupt condition to handle
> * if DMA is not active, you can check the AltStatus register, to test 
> the state without actually clearing anything
> 
> Though beware, on rare PATA controllers (old Macs?) the AltStatus 
> register doesn't exist.
> 

  Now I see what you're saying.  We should be able to know/clear 
interrupt by reading ATA_STATUS.  So, not having explicit interrupt 
pending bit can be compensated by following changes in ATA_STATUS (so 
the state machine), right?  Thanks a lot for putting up with my ignorance.

  So, how about a port status bit to tell interrupt handler whether or 
not we are expecting an interrupt currently?  That would solve the race 
from I tried to fix in the first patchset.  If that's okay with you, 
I'll redo all four patches accordingly.

  Again, thanks a lot. :-)

-- 
tejun

Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix

[Jeff Garzik]

Tejun Heo wrote:
>  Now I see what you're saying.  We should be able to know/clear 
> interrupt by reading ATA_STATUS.  So, not having explicit interrupt 
> pending bit can be compensated by following changes in ATA_STATUS (so 
> the state machine), right?  Thanks a lot for putting up with my ignorance.

Correct.  Just never forget there is a side effect to reading the Status 
register -- it clears any interrupt event it also signals.


>  So, how about a port status bit to tell interrupt handler whether or 
> not we are expecting an interrupt currently?  That would solve the race 
> from I tried to fix in the first patchset.  If that's okay with you, 
> I'll redo all four patches accordingly.

I'm a bit skeptical, TBH.  I would prefer some sort of approach whereby 
the code knows where the port is, inside the host state machine, and may 
deduce from that "location" whether or not an interrupt is expected. 
That's precisely how the code functions now with ATA DMA and non-data, 
at least (or should...).

If we find out where the bug in the "I know when I expect an interrupt" 
logic is, then your race should be easy to fix.  Currently, I can see 
that (a) ATAPI interrupt can race with called-immediately EH, and (b) 
ATAPI may throw an interrupt to indicate CDB is ready to be sent [see 
ATA-4], and libata doesn't yet support such a state in its host state 
machine (HSM) implementation.

That said, if you still wish to send such a patch, I will give it an 
open and honest review.

Maybe your bit flag approach plugs the hole in the existing HSM -- I 
would just like to know _where_ this hole is.  So far I don't see it, 
except for (a) and (b) ATAPI examples above.

	Jeff

Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix

[Tejun Heo]

On Sun, Aug 21, 2005 at 05:59:31PM -0400, Jeff Garzik wrote:
> Tejun Heo wrote:
> > Now I see what you're saying.  We should be able to know/clear 
> >interrupt by reading ATA_STATUS.  So, not having explicit interrupt 
> >pending bit can be compensated by following changes in ATA_STATUS (so 
> >the state machine), right?  Thanks a lot for putting up with my ignorance.
> 
> Correct.  Just never forget there is a side effect to reading the Status 
> register -- it clears any interrupt event it also signals.
> 
> 
> > So, how about a port status bit to tell interrupt handler whether or 
> >not we are expecting an interrupt currently?  That would solve the race 
> >from I tried to fix in the first patchset.  If that's okay with you, 
> >I'll redo all four patches accordingly.
> 
> I'm a bit skeptical, TBH.  I would prefer some sort of approach whereby 
> the code knows where the port is, inside the host state machine, and may 
> deduce from that "location" whether or not an interrupt is expected. 
> That's precisely how the code functions now with ATA DMA and non-data, 
> at least (or should...).
> 
> If we find out where the bug in the "I know when I expect an interrupt" 
> logic is, then your race should be easy to fix.  Currently, I can see 
> that (a) ATAPI interrupt can race with called-immediately EH, and (b) 
> ATAPI may throw an interrupt to indicate CDB is ready to be sent [see 
> ATA-4], and libata doesn't yet support such a state in its host state 
> machine (HSM) implementation.
> 
> That said, if you still wish to send such a patch, I will give it an 
> open and honest review.
> 
> Maybe your bit flag approach plugs the hole in the existing HSM -- I 
> would just like to know _where_ this hole is.  So far I don't see it, 
> except for (a) and (b) ATAPI examples above.

 For example, ATA_PROT_NODATA is processed as follows.

   Driver				Controller
   -------------------------------------------------------------------------
1. issue ATA_CMD_PACKET
2.					Turns on BSY and sends H2D regs
3. atapi_packet_task poll for BSY
4.					D2H regs FIS (!BSY, DRQ)
5. packet_task sends CDB
6.					Turns on BSY and sends DATA FIS
7.					D2H regs FIS (!BSY, DRDY, INTR)
8. intr handler completes the cmd

 In step #3, we're waiting for BSY to clear - command is active and
NIEN is clear.  After step #4 but before #5, command is active, BSY
clear and DRQ set, if an interrupt from the other port occurs here, it
will incorrectly fail this qc causing EH to kick in for sense data.

 The problem is that the current interrupt handler is unaware of the
HSM state where NIEN and BSY are clear but interrupts are not
expected.  So, the problem is that the interrupt handler doesn't know
enough about HSM state to be able to determine that the interrupt is
not its.  The flag I've mentioned is to tell just that to the
interrupt handler.

 Otherwise, I think we can implement something like ap->ata_state and
fully record where the HSM is, which would also be useful to implement
interrupt-driven PIO.  What do you think?

 Thanks.  :-)

-- 
tejun

Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix

[Jeff Garzik]

Tejun Heo wrote:
>  For example, ATA_PROT_NODATA is processed as follows.

You mean ATA_PROT_ATAPI_NODATA, I presume.

>    Driver				Controller
>    -------------------------------------------------------------------------
> 1. issue ATA_CMD_PACKET
> 2.					Turns on BSY and sends H2D regs
> 3. atapi_packet_task poll for BSY
> 4.					D2H regs FIS (!BSY, DRQ)
> 5. packet_task sends CDB
> 6.					Turns on BSY and sends DATA FIS

but first PIO Setup FIS


> 7.					D2H regs FIS (!BSY, DRDY, INTR)
> 8. intr handler completes the cmd
> 
>  In step #3, we're waiting for BSY to clear - command is active and
> NIEN is clear.  After step #4 but before #5, command is active, BSY
> clear and DRQ set, if an interrupt from the other port occurs here, it
> will incorrectly fail this qc causing EH to kick in for sense data.

Agreed, that is a problem w/ libata's ATAPI HSM.  Two problems actually:

(a) [what you found] at that point in the HSM, unexpected interrupts can 
incorrectly complete a command
(b) [a 'todo' item'] at that point in the HSM, there may actually be an 
expected interrupt, before which the CDB should not be sent, per ATA/ATAPI-4


>  The problem is that the current interrupt handler is unaware of the
> HSM state where NIEN and BSY are clear but interrupts are not
> expected.  So, the problem is that the interrupt handler doesn't know
> enough about HSM state to be able to determine that the interrupt is
> not its.

Agreed.


> The flag I've mentioned is to tell just that to the
> interrupt handler.

>  Otherwise, I think we can implement something like ap->ata_state and
> fully record where the HSM is, which would also be useful to implement
> interrupt-driven PIO.  What do you think?

I tend to prefer ap->ata_state method, since I definitely would like to 
implement interrupt-driven PIO.  But if the patch is too ugly, maybe a 
bit flag would be OK.

Long term, PIO w/ interrupts is probably the best thing to do under 
SATA.  That prevents problems with PATA->SATA emulation that lead to 
screaming interrupts.  For SATA, it seems like polling is a bad idea in 
all cases [save suspend/resume edge cases].

SATA technology is inherently more suited to an interrupt-driven 
architecture anyway.  Polling makes less sense, and needlessly increases 
PCI bus traffic.

	Jeff

Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix

[Tejun Heo]

 Jeff, replies to your comments here and I'll post another reply to
post new patch.

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>>  For example, ATA_PROT_NODATA is processed as follows.
> 
> 
> You mean ATA_PROT_ATAPI_NODATA, I presume.
> 

 Yeap.

>>    Driver                Controller
>>    
>> -------------------------------------------------------------------------
>> 1. issue ATA_CMD_PACKET
>> 2.                    Turns on BSY and sends H2D regs
>> 3. atapi_packet_task poll for BSY
>> 4.                    D2H regs FIS (!BSY, DRQ)
>> 5. packet_task sends CDB
>> 6.                    Turns on BSY and sends DATA FIS
> 
> 
> but first PIO Setup FIS
> 

 Thanks for the correction.

> 
>> 7.                    D2H regs FIS (!BSY, DRDY, INTR)
>> 8. intr handler completes the cmd
>>
>>  In step #3, we're waiting for BSY to clear - command is active and
>> NIEN is clear.  After step #4 but before #5, command is active, BSY
>> clear and DRQ set, if an interrupt from the other port occurs here, it
>> will incorrectly fail this qc causing EH to kick in for sense data.
> 
> 
> Agreed, that is a problem w/ libata's ATAPI HSM.  Two problems actually:
> 
> (a) [what you found] at that point in the HSM, unexpected interrupts can 
> incorrectly complete a command
> (b) [a 'todo' item'] at that point in the HSM, there may actually be an 
> expected interrupt, before which the CDB should not be sent, per 
> ATA/ATAPI-4

 This is deprecated since ATA/ATAPI-4.  Will we see this on SATA ATAPI
devices?  Maybe for PATA ATAPI's?

> 
> 
>>  The problem is that the current interrupt handler is unaware of the
>> HSM state where NIEN and BSY are clear but interrupts are not
>> expected.  So, the problem is that the interrupt handler doesn't know
>> enough about HSM state to be able to determine that the interrupt is
>> not its.
> 
> 
> Agreed.
> 
> 
>> The flag I've mentioned is to tell just that to the
>> interrupt handler.
> 
> 
>>  Otherwise, I think we can implement something like ap->ata_state and
>> fully record where the HSM is, which would also be useful to implement
>> interrupt-driven PIO.  What do you think?
> 
> 
> I tend to prefer ap->ata_state method, since I definitely would like to 
> implement interrupt-driven PIO.  But if the patch is too ugly, maybe a 
> bit flag would be OK.

 I just went the flag way for the following reason.  As the larger
chunk (PIO state machine) is not in place yet, implementing small part
would look out of place and it's gonna need be modified anyway later
when actual state machine is implemented.  I've added a FIXME: comment
after the flag to signify that this should be merged into state
machine.

> 
> Long term, PIO w/ interrupts is probably the best thing to do under 
> SATA.  That prevents problems with PATA->SATA emulation that lead to 
> screaming interrupts.  For SATA, it seems like polling is a bad idea in 
> all cases [save suspend/resume edge cases].
> 
> SATA technology is inherently more suited to an interrupt-driven 
> architecture anyway.  Polling makes less sense, and needlessly increases 
> PCI bus traffic.

 I liked the simplicity of polling and, as PIO is not a very hot path,
thought it would be okay, but I agree that implementing full interrupt
driven HSM is a better thing to do.

 Thanks.

--
tejun

Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
>>(b) [a 'todo' item'] at that point in the HSM, there may actually be an 
>>expected interrupt, before which the CDB should not be sent, per 
>>ATA/ATAPI-4

>  This is deprecated since ATA/ATAPI-4.  Will we see this on SATA ATAPI
> devices?  Maybe for PATA ATAPI's?

Early PATA, though maybe it carried through ATA-6 or so.  For generic 
ATAPI HSM, we need to handle it if a "I have a CDB interrupt" flag is 
set somewhere.

	Jeff

[PATCH libata:upstream] fix atapi_packet_task vs. intr race

[Tejun Heo]

 Interrupts from devices sharing the same IRQ could cause
ata_host_intr to finish commands being processed by atapi_packet_task
if the commands are using ATA_PROT_ATAPI_NODATA or ATA_PROT_ATAPI_DMA
protocol.  This is because libata interrupt handler is unaware that
interrupts are not expected during that period.  This patch adds
ATA_FLAG_NOINTR flag to tell the interrupt handler that we're not
expecting interrupts.

 Note that once proper HSM is implemented for interrupt-driven PIO,
this should be merged into it and this flag will be removed.

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

diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -3218,11 +3218,13 @@ int ata_qc_issue_prot(struct ata_queued_
 		break;
 
 	case ATA_PROT_ATAPI_NODATA:
+		ap->flags |= ATA_FLAG_NOINTR;
 		ata_tf_to_host_nolock(ap, &qc->tf);
 		queue_work(ata_wq, &ap->packet_task);
 		break;
 
 	case ATA_PROT_ATAPI_DMA:
+		ap->flags |= ATA_FLAG_NOINTR;
 		ap->ops->tf_load(ap, &qc->tf);	 /* load tf registers */
 		ap->ops->bmdma_setup(qc);	    /* set up bmdma */
 		queue_work(ata_wq, &ap->packet_task);
@@ -3576,7 +3578,8 @@ irqreturn_t ata_interrupt (int irq, void
 		struct ata_port *ap;
 
 		ap = host_set->ports[i];
-		if (ap && (!(ap->flags & ATA_FLAG_PORT_DISABLED))) {
+		if (ap &&
+		    !(ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR))) {
 			struct ata_queued_cmd *qc;
 
 			qc = ata_qc_from_tag(ap, ap->active_tag);
@@ -3628,19 +3631,25 @@ static void atapi_packet_task(void *_dat
 	/* send SCSI cdb */
 	DPRINTK("send cdb\n");
 	assert(ap->cdb_len >= 12);
-	ata_data_xfer(ap, qc->cdb, ap->cdb_len, 1);
 
-	/* if we are DMA'ing, irq handler takes over from here */
-	if (qc->tf.protocol == ATA_PROT_ATAPI_DMA)
-		ap->ops->bmdma_start(qc);	    /* initiate bmdma */
-
-	/* non-data commands are also handled via irq */
-	else if (qc->tf.protocol == ATA_PROT_ATAPI_NODATA) {
-		/* do nothing */
-	}
+	if (qc->tf.protocol == ATA_PROT_ATAPI_DMA ||
+	    qc->tf.protocol == ATA_PROT_ATAPI_NODATA) {
+		/* Once we're done issuing command and kicking bmdma,
+		 * irq handler takes over.  To not lose irq, we need
+		 * to clear NOINTR flag before sending cdb, but
+		 * interrupt handler shouldn't be invoked before we're
+		 * finished.  Hence, the following locking.
+		 */
+		spin_lock_irq(&ap->host_set->lock);
+		ap->flags &= ~ATA_FLAG_NOINTR;
+		ata_data_xfer(ap, qc->cdb, ap->cdb_len, 1);
+		if (qc->tf.protocol == ATA_PROT_ATAPI_DMA)
+			ap->ops->bmdma_start(qc);	/* initiate bmdma */
+		spin_unlock_irq(&ap->host_set->lock);
+	} else {
+		ata_data_xfer(ap, qc->cdb, ap->cdb_len, 1);
 
-	/* PIO commands are handled by polling */
-	else {
+		/* PIO commands are handled by polling */
 		ap->pio_task_state = PIO_ST;
 		queue_work(ata_wq, &ap->pio_task);
 	}
diff --git a/include/linux/libata.h b/include/linux/libata.h
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -113,6 +113,8 @@ enum {
 	ATA_FLAG_MMIO		= (1 << 6), /* use MMIO, not PIO */
 	ATA_FLAG_SATA_RESET	= (1 << 7), /* use COMRESET */
 	ATA_FLAG_PIO_DMA	= (1 << 8), /* PIO cmds via DMA */
+	ATA_FLAG_NOINTR		= (1 << 9), /* FIXME: Remove this once
+					     * proper HSM is in place. */
 
 	ATA_QCFLAG_ACTIVE	= (1 << 1), /* cmd not yet ack'd to scsi lyer */
 	ATA_QCFLAG_SG		= (1 << 3), /* have s/g table? */

Re: [PATCH libata:upstream] fix atapi_packet_task vs. intr race

[Jeff Garzik]

Tejun Heo wrote:
>  Interrupts from devices sharing the same IRQ could cause
> ata_host_intr to finish commands being processed by atapi_packet_task
> if the commands are using ATA_PROT_ATAPI_NODATA or ATA_PROT_ATAPI_DMA
> protocol.  This is because libata interrupt handler is unaware that
> interrupts are not expected during that period.  This patch adds
> ATA_FLAG_NOINTR flag to tell the interrupt handler that we're not
> expecting interrupts.
> 
>  Note that once proper HSM is implemented for interrupt-driven PIO,
> this should be merged into it and this flag will be removed.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

Looks good except for two things:

* use spin_lock_irqsave()/spin_unlock_irqrestore(), not spin_lock_irq()

* need to audit every driver[1].  if it implements its own 
->irq_handler() hook, then you likely need to update the driver.

	Jeff


[1] drivers/scsi/ahci.c drivers/scsi/ata_*.c drivers/scsi/sata_*.[ch]

Re: [PATCH libata:upstream] fix atapi_packet_task vs. intr race (take 2)

[Tejun Heo]

 Hello, Jeff.

 Here's the updated patch.  Though I have a question.  Why always use
spin_lock_irqsave()?  To improve maintainability?

--

 Interrupts from devices sharing the same IRQ could cause
ata_host_intr to finish commands being processed by atapi_packet_task
if the commands are using ATA_PROT_ATAPI_NODATA or ATA_PROT_ATAPI_DMA
protocol.  This is because libata interrupt handler is unaware that
interrupts are not expected during that period.  This patch adds
ATA_FLAG_NOINTR flag to tell the interrupt handler that we're not
expecting interrupts.

 Note that once proper HSM is implemented for interrupt-driven PIO,
this should be merged into it and this flag will be removed.

 ahci.c is a different kind of beast, so it's left alone.

* The following drivers use ata_qc_issue_prot and ata_interrupt, so
  changes in libata core will do.

  ata_piix sata_sil sata_svw sata_via sata_sis sata_uli

* The following drivers use ata_qc_issue_prot and custom intr handler.
  They need this change to work correctly.

  sata_nv sata_vsc

* The following drivers use custom issue function and intr handler.
  Currently all custom issue functions don't support ATAPI, so this
  change is irrelevant, updated for consistency and to avoid later
  mistakes.

  sata_promise sata_qstor sata_sx4

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

diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -3218,11 +3218,13 @@ int ata_qc_issue_prot(struct ata_queued_
 		break;
 
 	case ATA_PROT_ATAPI_NODATA:
+		ap->flags |= ATA_FLAG_NOINTR;
 		ata_tf_to_host_nolock(ap, &qc->tf);
 		queue_work(ata_wq, &ap->packet_task);
 		break;
 
 	case ATA_PROT_ATAPI_DMA:
+		ap->flags |= ATA_FLAG_NOINTR;
 		ap->ops->tf_load(ap, &qc->tf);	 /* load tf registers */
 		ap->ops->bmdma_setup(qc);	    /* set up bmdma */
 		queue_work(ata_wq, &ap->packet_task);
@@ -3576,7 +3578,8 @@ irqreturn_t ata_interrupt (int irq, void
 		struct ata_port *ap;
 
 		ap = host_set->ports[i];
-		if (ap && (!(ap->flags & ATA_FLAG_PORT_DISABLED))) {
+		if (ap &&
+		    !(ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR))) {
 			struct ata_queued_cmd *qc;
 
 			qc = ata_qc_from_tag(ap, ap->active_tag);
@@ -3628,19 +3631,27 @@ static void atapi_packet_task(void *_dat
 	/* send SCSI cdb */
 	DPRINTK("send cdb\n");
 	assert(ap->cdb_len >= 12);
-	ata_data_xfer(ap, qc->cdb, ap->cdb_len, 1);
 
-	/* if we are DMA'ing, irq handler takes over from here */
-	if (qc->tf.protocol == ATA_PROT_ATAPI_DMA)
-		ap->ops->bmdma_start(qc);	    /* initiate bmdma */
-
-	/* non-data commands are also handled via irq */
-	else if (qc->tf.protocol == ATA_PROT_ATAPI_NODATA) {
-		/* do nothing */
-	}
+	if (qc->tf.protocol == ATA_PROT_ATAPI_DMA ||
+	    qc->tf.protocol == ATA_PROT_ATAPI_NODATA) {
+		unsigned long flags;
+
+		/* Once we're done issuing command and kicking bmdma,
+		 * irq handler takes over.  To not lose irq, we need
+		 * to clear NOINTR flag before sending cdb, but
+		 * interrupt handler shouldn't be invoked before we're
+		 * finished.  Hence, the following locking.
+		 */
+		spin_lock_irqsave(&ap->host_set->lock, flags);
+		ap->flags &= ~ATA_FLAG_NOINTR;
+		ata_data_xfer(ap, qc->cdb, ap->cdb_len, 1);
+		if (qc->tf.protocol == ATA_PROT_ATAPI_DMA)
+			ap->ops->bmdma_start(qc);	/* initiate bmdma */
+		spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	} else {
+		ata_data_xfer(ap, qc->cdb, ap->cdb_len, 1);
 
-	/* PIO commands are handled by polling */
-	else {
+		/* PIO commands are handled by polling */
 		ap->pio_task_state = PIO_ST;
 		queue_work(ata_wq, &ap->pio_task);
 	}
diff --git a/drivers/scsi/sata_nv.c b/drivers/scsi/sata_nv.c
--- a/drivers/scsi/sata_nv.c
+++ b/drivers/scsi/sata_nv.c
@@ -291,7 +291,8 @@ static irqreturn_t nv_interrupt (int irq
 		struct ata_port *ap;
 
 		ap = host_set->ports[i];
-		if (ap && (!(ap->flags & ATA_FLAG_PORT_DISABLED))) {
+		if (ap &&
+		    !(ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR))) {
 			struct ata_queued_cmd *qc;
 
 			qc = ata_qc_from_tag(ap, ap->active_tag);
diff --git a/drivers/scsi/sata_promise.c b/drivers/scsi/sata_promise.c
--- a/drivers/scsi/sata_promise.c
+++ b/drivers/scsi/sata_promise.c
@@ -441,7 +441,8 @@ static irqreturn_t pdc_interrupt (int ir
 		VPRINTK("port %u\n", i);
 		ap = host_set->ports[i];
 		tmp = mask & (1 << (i + 1));
-		if (tmp && ap && (!(ap->flags & ATA_FLAG_PORT_DISABLED))) {
+		if (tmp && ap &&
+		    !(ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR))) {
 			struct ata_queued_cmd *qc;
 
 			qc = ata_qc_from_tag(ap, ap->active_tag);
diff --git a/drivers/scsi/sata_qstor.c b/drivers/scsi/sata_qstor.c
--- a/drivers/scsi/sata_qstor.c
+++ b/drivers/scsi/sata_qstor.c
@@ -386,7 +386,8 @@ static inline unsigned int qs_intr_pkt(s
 			DPRINTK("SFF=%08x%08x: sCHAN=%u sHST=%d sDST=%02x\n",
 					sff1, sff0, port_no, sHST, sDST);
 			handled = 1;
-			if (ap && (!(ap->flags & ATA_FLAG_PORT_DISABLED))) {
+			if (ap && !(ap->flags &
+				    (ATA_FLAG_PORT_DISABLED|ATA_FLAG_NOINTR))) {
 				struct ata_queued_cmd *qc;
 				struct qs_port_priv *pp = ap->private_data;
 				if (!pp || pp->state != qs_state_pkt)
@@ -417,7 +418,8 @@ static inline unsigned int qs_intr_mmio(
 	for (port_no = 0; port_no < host_set->n_ports; ++port_no) {
 		struct ata_port *ap;
 		ap = host_set->ports[port_no];
-		if (ap && (!(ap->flags & ATA_FLAG_PORT_DISABLED))) {
+		if (ap &&
+		    !(ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR))) {
 			struct ata_queued_cmd *qc;
 			struct qs_port_priv *pp = ap->private_data;
 			if (!pp || pp->state != qs_state_mmio)
diff --git a/drivers/scsi/sata_sx4.c b/drivers/scsi/sata_sx4.c
--- a/drivers/scsi/sata_sx4.c
+++ b/drivers/scsi/sata_sx4.c
@@ -825,7 +825,8 @@ static irqreturn_t pdc20621_interrupt (i
 			ap = host_set->ports[port_no];
 		tmp = mask & (1 << i);
 		VPRINTK("seq %u, port_no %u, ap %p, tmp %x\n", i, port_no, ap, tmp);
-		if (tmp && ap && (!(ap->flags & ATA_FLAG_PORT_DISABLED))) {
+		if (tmp && ap &&
+		    !(ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR))) {
 			struct ata_queued_cmd *qc;
 
 			qc = ata_qc_from_tag(ap, ap->active_tag);
diff --git a/drivers/scsi/sata_vsc.c b/drivers/scsi/sata_vsc.c
--- a/drivers/scsi/sata_vsc.c
+++ b/drivers/scsi/sata_vsc.c
@@ -173,7 +173,8 @@ static irqreturn_t vsc_sata_interrupt (i
 			struct ata_port *ap;
 
 			ap = host_set->ports[i];
-			if (ap && (!(ap->flags & ATA_FLAG_PORT_DISABLED))) {
+			if (ap && !(ap->flags &
+				    (ATA_FLAG_PORT_DISABLED|ATA_FLAG_NOINTR))) {
 				struct ata_queued_cmd *qc;
 
 				qc = ata_qc_from_tag(ap, ap->active_tag);
diff --git a/include/linux/libata.h b/include/linux/libata.h
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -113,6 +113,8 @@ enum {
 	ATA_FLAG_MMIO		= (1 << 6), /* use MMIO, not PIO */
 	ATA_FLAG_SATA_RESET	= (1 << 7), /* use COMRESET */
 	ATA_FLAG_PIO_DMA	= (1 << 8), /* PIO cmds via DMA */
+	ATA_FLAG_NOINTR		= (1 << 9), /* FIXME: Remove this once
+					     * proper HSM is in place. */
 
 	ATA_QCFLAG_ACTIVE	= (1 << 1), /* cmd not yet ack'd to scsi lyer */
 	ATA_QCFLAG_SG		= (1 << 3), /* have s/g table? */

Re: [PATCH libata:upstream] fix atapi_packet_task vs. intr race (take 2)

[Jeff Garzik]

Tejun Heo wrote:
>  Hello, Jeff.
> 
>  Here's the updated patch.  Though I have a question.  Why always use

Looks OK to me.  Have you tested it?  :)


> spin_lock_irqsave()?  To improve maintainability?

Code is less fragile, since it works regardless of whether the code path 
even gets moved to a different or not.  And I think its more friendly to 
architectures (portable), as it has available a 'cookie' for which 
random arch-specific info can be stored.

	Jeff

Re: [PATCH libata:upstream] fix atapi_packet_task vs. intr race (take 2)

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>>  Hello, Jeff.
>>
>>  Here's the updated patch.  Though I have a question.  Why always use
> 
> 
> Looks OK to me.  Have you tested it?  :)
> 

  Yes, I've tested both normal and error cases.  I don't get assertion 
failures anymore with ongoing disk traffic.

  Thanks.

-- 
tejun

Re: [PATCH libata:upstream] fix atapi_packet_task vs. intr race (take 2)

[Jeff Garzik]

applied

Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix

[Jeff Garzik]

Tejun Heo wrote:
>  Have you received patches in the reverse order?  #1 changes 
> ata_irq_on() and #2 adds ata_qc_set_polling().
> 
>  Hmmm, only a call to ata_chk_status() is added to ata_irq_on(), which I 
> think is needed regardless of other changes, and ata_wait_idle() is 
> removed.  Does that make the function heavy?


You're also adding a call to ->irq_clear() in there.

The overall meaning/effect of the patch is heavy:  "I don't know why 
these interrupts are popping up, so I'll whack it with a hammer every 
chance I get, to make them go away."  ;-) ;-)

That's taking the easy way out ;-)  It's a bandaid over whatever the 
-real- problem is.

	Jeff

Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix

[Mark Lord]

Tejun Heo wrote:
>  
>   And without interrupt pending bit, shared interrupt means a lot of 
> spurious interrupts making it impossible to know when to expect interrupts.
> 
>  IDE driver deals with this by having only one command active per 
> interrupt, but SATA doesn't have such scheme yet.  And I don't know if 
> such a scheme is desirable at all.

I'm not sure which part of the IDE driver this refers to,
but there is/was code in there from 1994 or so, which enabled
sharing a single IRQ among multiple ISA IDE interfaces using
edge-triggered interrupts (rather than level-triggered).

That particular code did indeed only permit one command active per
interrupt, but only for that situation.  But later on, when simplex
DMA host controllers appeared, the scheme got reorganized into
"hardware groups" (hwgroups), which were collections of IDE interfaces
which required mutual exclusion among themselves.

If only the hardware were as simple and as well documented back then
as it is (mostly) today!

Cheers