Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix
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. DriverController - 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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix
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
Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix
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? Arggg... 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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix
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? Arggg... Lack of interrupt pending bit is such a pain in the ass. :-( -- tejun - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix
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
Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix
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.
Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH libata-dev-2.6:upstream 00/02] atapi: packet task vs. intr race fix
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 - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
