libata: disable_irq() during polling IDENTIFY
PATCH 1/1 libata: disable_irq() during polling IDENTIFY
Problem:
Kernel got "irq 5: nobody cared" when using
libata + polling IDENTIFY + Promise 20275 adapter + Benq DW1620 drive.
Detail message available in bug 8441 (http://bugzilla.kernel.org/show_bug.cgi?id=8441).
Cause:
The Benq DW1620 drive raises INTRQ during polling IDENTIFY PACKET DEVICE, even if nIEN = 1.
Proposed fix:
disable_irq() during polling IDENTIFY to work around, the same as what IDE subsystem does.
---
Some controller like Intel ICH4 is immune from the problem, with the same kernel and the same Benq DW1620 drive. So, adding the ATA_FLAG_IDENT_IRQ_OFF flag for those adapters that needs the workaround. Patch against 2.6.21.1 for your review, thanks.
drivers/ata/libata-core.c +22 -0
@@ -1427,6 +1427,7 @@ int ata_dev_read_id(struct ata_device *d unsigned int flags, u16 *id) { struct ata_port *ap = dev->ap; +struct ata_host *host = ap->host;unsigned int class = *p_class;struct ata_taskfile tf;unsigned int err_mask =0 ;
@@ -1466,8 +1467,29 @@ int ata_dev_read_id(struct ata_device *d*/ tf.flags |=ATA_TFLAG_POLLING ; +/* Disable IRQ since some devices like Benq DW1620 raises INTRQ +* when IDENTIFY PACKET DEVICE, even with polling IDENTIFY. +*/ +if (ap->flags &ATA_FLAG_IDENT_IRQ_OFF ) { +if (host->irq) +disable_irq (host->irq); + +if (host->irq2) +disable_irq (host->irq2); + } + err_mask =ata_exec_internal (dev, &tf,NULL ,DMA_FROM_DEVICE , id,sizeof (id[0 ]) *ATA_ID_WORDS ); + +/* Re-enable IRQ */ +if (ap->flags &ATA_FLAG_IDENT_IRQ_OFF ) { +if (host->irq) +enable_irq (host->irq); + +if (host->irq2) +enable_irq (host->irq2); + } +if (err_mask) {if (err_mask &AC_ERR_NODEV_HINT ) { DPRINTK("ata%u.%d: NODEV after polling detection\n",
drivers/ata/pata_pdc2027x.c +2 -2
@@ -214,7 +214,7 @@ static struct ata_port_info pdc2027x_por { .sht = &pdc2027x_sht, .flags =ATA_FLAG_NO_LEGACY |ATA_FLAG_SLAVE_POSS | - ATA_FLAG_MMIO, + ATA_FLAG_MMIO | ATA_FLAG_IDENT_IRQ_OFF, .pio_mask = 0x1f, /* pio0-4 */ .mwdma_mask = 0x07, /* mwdma0-2 */ .udma_mask = ATA_UDMA5, /* udma0-5 */
@@ -224,7 +224,7 @@ static struct ata_port_info pdc2027x_por { .sht = &pdc2027x_sht, .flags =ATA_FLAG_NO_LEGACY |ATA_FLAG_SLAVE_POSS | - ATA_FLAG_MMIO, + ATA_FLAG_MMIO | ATA_FLAG_IDENT_IRQ_OFF, .pio_mask = 0x1f, /* pio0-4 */ .mwdma_mask = 0x07, /* mwdma0-2 */ .udma_mask = ATA_UDMA6, /* udma0-6 */
include/linux/libata.h +1 -0
@@ -174,6 +174,7 @@ enum {ATA_FLAG_SETXFER_POLLING = (1 <<14 ),/* use polling for SETXFER */ ATA_FLAG_IGN_SIMPLEX = (1 <<15 ),/* ignore SIMPLEX */ ATA_FLAG_NO_IORDY = (1 <<16 ),/* controller lacks iordy */ +ATA_FLAG_IDENT_IRQ_OFF = (1 <<17 ),/* disable irq when IDENTIFY */ /* The following flag belongs to ap->pflags but is kept in * ap->flags because it's referenced in many LLDs and will be
[cc'ing Bartlomiej and Mark, hi]
Hello, Albert.
Quoted 9 lines · Albert Lee
Problem:
Kernel got "irq 5: nobody cared" when using
libata + polling IDENTIFY + Promise 20275 adapter + Benq DW1620 drive.Detail message available in bug 8441 (http://bugzilla.kernel.org/show_bug.cgi?id=8441).
Cause:
The Benq DW1620 drive raises INTRQ during polling IDENTIFY PACKET DEVICE, even if nIEN = 1.
Aieeee...
Quoted 8 lines
Proposed fix:
disable_irq()during polling IDENTIFY to work around, the same as what IDE subsystem does.Signed-off-by Albert Lee <[email protected]>---
Some controller like Intel ICH4 is immune from the problem, with the same kernel and the same Benq DW1620 drive. So, adding theATA_FLAG_IDENT_IRQ_OFFflag for those adapters that needs the workaround. Patch against 2.6.21.1 for your review, thanks.
I guess piix is masking the interrupt at the host side.
Another interesting aspect is that the SATA spec says the device is recommended to ignore nIEN while the controller is recommended to not set nIEN when it sends FIS to the device. ie. nIEN should be implemented on the host controller. I bet there are controllers out there which doesn't do host-side masking and there will be more and more devices which ignore nIEN, so we're likely to see similar problems on SATA too.
Quoted 13 lines
drivers/ata/libata-core.c +11
-0+/* Disable IRQ since some devices like Benq DW1620 raises INTRQ +* when IDENTIFY PACKET DEVICE, even with polling IDENTIFY. +*/ +if (ap->flags &ATA_FLAG_IDENT_IRQ_OFF ) { +if (host->irq) +disable_irq (host->irq); + +if (host->irq2) +disable_irq (host->irq2); + } + err_mask =ata_exec_internal (dev, &tf,NULL ,DMA_FROM_DEVICE , id,sizeof (id[0 ]) *ATA_ID_WORDS );
drivers/ata/libata-core.c int ata_dev_read_id(struct ata_device *d · line 1492
Quoted 10 lines
drivers/ata/libata-core.c +10
-0+ +/* Re-enable IRQ */ +if (ap->flags &ATA_FLAG_IDENT_IRQ_OFF ) { +if (host->irq) +enable_irq (host->irq); + +if (host->irq2) +enable_irq (host->irq2); + } +
Yeap, this is how IDE deals with polling commands but I'm not sure how it's supposed to work with PCI IRQ sharing. Bartlomiej, can you enlighten me here?
Also, this is a problem for not only IDENTIFY but all polling commands.
One solution I can think of is to let IRQ handler ack IRQ unconditionally during polling commands - ie. just read the TF Status register once and tell the IRQ subsystem that the IRQ is handled. This shouldn't affect the operation of polling as the only side effect of reading Status is clearing pending IRQ && will give us a nice way to deal with the SATA bridge chip which chokes on nIEN. Considering the sorry state of nIEN in SATA, I guess this might be the best way to deal with this.
Albert, can you please test whether this works? Modifying ata_interrupt() such that it reads TF Status if ATA_TFLAG_POLLING should do the trick.
Thanks.
Signature · 1 line
tejun
Quoted 3 lines
Yeap, this is how IDE deals with polling commands but I'm not sure how it's supposed to work with PCI IRQ sharing. Bartlomiej, can you enlighten me here?
Simple answer: Badly. If you've got the IRQ shared it mucks up the behaviour of the other device especially when its doing PIO.
Quoted 6 lines · Alan Cox
Quoted 3 lines
Yeap, this is how IDE deals with polling commands but I'm not sure how it's supposed to work with PCI IRQ sharing. Bartlomiej, can you enlighten me here?
Simple answer: Badly. If you've got the IRQ shared it mucks up the behaviour of the other device especially when its doing PIO.
OIC, I've been thinking I must be missing something but it's just supposed to work that way. Doesn't sound too attractive. :-(
Thanks.
Signature · 1 line
tejun
Quoted 9 lines · Tejun Heo
Quoted 5 lines · Alan Cox
Quoted 3 lines
Yeap, this is how IDE deals with polling commands but I'm not sure how it's supposed to work with PCI IRQ sharing. Bartlomiej, can you enlighten me here?
Simple answer: Badly. If you've got the IRQ shared it mucks up the behaviour of the other device especially when its doing PIO.
OIC, I've been thinking I must be missing something but it's just supposed to work that way. Doesn't sound too attractive. :-(
Yeah. It's ugly -- designed for use at boot time only, really.
As I've said a few times here, the IDE Subsystem was designed/implemented *pre-hotplug*, and though it does work well most of the time, the world has moved on.
Cheers
Quoted 70 lines · Tejun Heo
[cc'ing Bartlomiej and Mark, hi]
Hello, Albert.
Quoted 9 lines · Albert Lee
Problem:
Kernel got "irq 5: nobody cared" when using
libata + polling IDENTIFY + Promise 20275 adapter + Benq DW1620 drive.Detail message available in bug 8441 (http://bugzilla.kernel.org/show_bug.cgi?id=8441).
Cause:
The Benq DW1620 drive raises INTRQ during polling IDENTIFY PACKET DEVICE, even if nIEN = 1.Aieeee...
Quoted 8 lines
Proposed fix:
disable_irq()during polling IDENTIFY to work around, the same as what IDE subsystem does.Signed-off-by Albert Lee <[email protected]>---
Some controller like Intel ICH4 is immune from the problem, with the same kernel and the same Benq DW1620 drive. So, adding theATA_FLAG_IDENT_IRQ_OFFflag for those adapters that needs the workaround. Patch against 2.6.21.1 for your review, thanks.I guess piix is masking the interrupt at the host side.
Another interesting aspect is that the SATA spec says the device is recommended to ignore nIEN while the controller is recommended to not set nIEN when it sends FIS to the device. ie. nIEN should be implemented on the host controller. I bet there are controllers out there which doesn't do host-side masking and there will be more and more devices which ignore nIEN, so we're likely to see similar problems on SATA too.
Quoted 13 lines
(quoted diff) +11
-0+ /* Disable IRQ since some devices like Benq DW1620 raises INTRQ + * when IDENTIFY PACKET DEVICE, even with polling IDENTIFY. + */ + if (ap->flags & ATA_FLAG_IDENT_IRQ_OFF) { + if (host->irq) + disable_irq(host->irq); + + if (host->irq2) + disable_irq(host->irq2); + } +err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE, id, sizeof(id[0]) * ATA_ID_WORDS);Review
Quoted 10 lines
(quoted diff) +10
-0+ + /* Re-enable IRQ */ + if (ap->flags & ATA_FLAG_IDENT_IRQ_OFF) { + if (host->irq) + enable_irq(host->irq); + + if (host->irq2) + enable_irq(host->irq2); + } +Yeap, this is how IDE deals with polling commands but I'm not sure how it's supposed to work with PCI IRQ sharing. Bartlomiej, can you enlighten me here?
Quoted 1 line
Also, this is a problem for not only IDENTIFY but all polling commands.
Yes, other command might also assert INTRQ during polling. However, for the specific BENQ DW1620 drive, only IDENTIFY_PACKET_DEVICE has such behavior; other commands like READ or REQUEST_SENSE are ok.
Quoted 10 lines
One solution I can think of is to let IRQ handler ack IRQ unconditionally during polling commands - ie. just read the TF Status register once and tell the IRQ subsystem that the IRQ is handled. This shouldn't affect the operation of polling as the only side effect of reading Status is clearing pending IRQ && will give us a nice way to deal with the SATA bridge chip which chokes on nIEN. Considering the sorry state of nIEN in SATA, I guess this might be the best way to deal with this.
Quoted 4 lines
Albert, can you please test whether this works? Modifying
ata_interrupt()such that it reads TF Status ifATA_TFLAG_POLLINGshould do the trick.
Yes, reading the Status register and acking interrupt also fixes the problem (patch attached below).
--
albert
drivers/ata/libata-core.c +8 -3
@@ -5224,9 +5224,14 @@ irqreturn_t ata_interrupt (int irq, voidstruct ata_queued_cmd *qc; qc =ata_qc_from_tag (ap, ap->active_tag); -if (qc && (!(qc->tf.flags &ATA_TFLAG_POLLING )) && - (qc->flags &ATA_QCFLAG_ACTIVE )) - handled |=ata_host_intr (ap, qc); +if (qc && (qc->flags &ATA_QCFLAG_ACTIVE )) { +if (qc->tf.flags &ATA_TFLAG_POLLING ) { +ata_chk_status (ap); + handled =1 ; + }else { + handled |=ata_host_intr (ap, qc); + } + } } }
Hello, Albert.
Quoted 6 lines · Albert Lee
Quoted 1 line · Tejun Heo
Also, this is a problem for not only IDENTIFY but all polling commands.
Yes, other command might also assert INTRQ during polling. However, for the specific BENQ DW1620 drive, only
IDENTIFY_PACKET_DEVICEhas such behavior; other commands like READ orREQUEST_SENSEare ok.
Oh I see.
Quoted 9 lines
Quoted 9 lines
One solution I can think of is to let IRQ handler ack IRQ unconditionally during polling commands - ie. just read the TF Status register once and tell the IRQ subsystem that the IRQ is handled. This shouldn't affect the operation of polling as the only side effect of reading Status is clearing pending IRQ && will give us a nice way to deal with the SATA bridge chip which chokes on nIEN. Considering the sorry state of nIEN in SATA, I guess this might be the best way to deal with this.
Quoted 6 lines
Quoted 3 lines
Albert, can you please test whether this works? Modifying
ata_interrupt()such that it reads TF Status ifATA_TFLAG_POLLINGshould do the trick.Yes, reading the Status register and acking interrupt also fixes the problem (patch attached below).
Good to know it works. With or without nIEN, I think this change is a good thing to have. IRQ handler shouldn't interfere with polling as both acquire lock during operation. Polling is safer this way and non-SFF drivers are doing something similar anyway. I also think it would be nice to have the SFF irq_handler clear IRQ while the port is frozen. This again is the default behavior of non-SFF drivers.
Any objections?
Signature · 1 line
tejun
Quoted 29 lines · Tejun Heo
Hello, Albert.
Quoted 7 lines · Albert Lee
Quoted 1 line · Tejun Heo
Also, this is a problem for not only IDENTIFY but all polling commands.
Yes, other command might also assert INTRQ during polling. However, for the specific BENQ DW1620 drive, only
IDENTIFY_PACKET_DEVICEhas such behavior; other commands like READ orREQUEST_SENSEare ok.Oh I see.
Quoted 13 lines
Quoted 12 lines
One solution I can think of is to let IRQ handler ack IRQ unconditionally during polling commands - ie. just read the TF Status register once and tell the IRQ subsystem that the IRQ is handled. This shouldn't affect the operation of polling as the only side effect of reading Status is clearing pending IRQ && will give us a nice way to deal with the SATA bridge chip which chokes on nIEN. Considering the sorry state of nIEN in SATA, I guess this might be the best way to deal with this.
Albert, can you please test whether this works? Modifying
ata_interrupt()such that it reads TF Status ifATA_TFLAG_POLLINGshould do the trick.
Quoted 7 lines
Quoted 2 lines
Yes, reading the Status register and acking interrupt also fixes the problem (patch attached below).
Good to know it works. With or without nIEN, I think this change is a good thing to have. IRQ handler shouldn't interfere with polling as both acquire lock during operation.
This reminds me of a possible issue with the patch:
Previously the polling code assumes that the interrupt handler won't interfere with it and the polling code runs without holding ap->lock. However, with the above patch, the interrupt handler might read the Status register when the polling code is transfering data, etc. from the port. Would this cause trouble to the ATA/ATAPI devices?
Should we have something like ATA_PFLAG_HSM_BUSY below, such that the interrupt handler won't read the Status register when HSM is busy accessing the port?
--
albert
(Revised patch: Don't read the Status register when HSM is busy accessing the port)
drivers/ata/libata-core.c +24 -3
@@ -4389,6 +4389,14 @@ int ata_hsm_move(struct ata_port *ap, st*/ WARN_ON (in_wq !=ata_hsm_ok_in_wq (ap, qc)); +if (in_wq) { +spin_lock_irqsave (ap->lock, flags); + ap->pflags |=ATA_PFLAG_HSM_BUSY ; +spin_unlock_irqrestore (ap->lock, flags); + }else { + ap->pflags |=ATA_PFLAG_HSM_BUSY ; + } + fsm_start:DPRINTK ("ata%u: protocol %d task_state %d (dev_stat 0x%X)\n" , ap->print_id, qc->tf.protocol, ap->hsm_task_state, status);
@@ -4600,6 +4608,14 @@ fsm_start:BUG (); } +if (in_wq) { +spin_lock_irqsave (ap->lock, flags); + ap->pflags &= ~ATA_PFLAG_HSM_BUSY ; +spin_unlock_irqrestore (ap->lock, flags); + }else { + ap->pflags &= ~ATA_PFLAG_HSM_BUSY ; + } +return poll_next; }
@@ -5224,9 +5240,14 @@ irqreturn_t ata_interrupt (int irq, voidstruct ata_queued_cmd *qc; qc =ata_qc_from_tag (ap, ap->active_tag); -if (qc && (!(qc->tf.flags &ATA_TFLAG_POLLING )) && - (qc->flags &ATA_QCFLAG_ACTIVE )) - handled |=ata_host_intr (ap, qc); +if (qc && (qc->flags &ATA_QCFLAG_ACTIVE )) { +if (!(qc->tf.flags &ATA_TFLAG_POLLING )) { + handled |=ata_host_intr (ap, qc); + }else if (!(ap->pflags &ATA_PFLAG_HSM_BUSY )) { +ata_chk_status (ap); + handled =1 ; + } + } } }
include/linux/libata.h +1 -0
@@ -195,6 +195,7 @@ enum {ATA_PFLAG_FLUSH_PORT_TASK = (1 <<16 ),/* flush port task */ ATA_PFLAG_SUSPENDED = (1 <<17 ),/* port is suspended (power) */ ATA_PFLAG_PM_PENDING = (1 <<18 ),/* PM operation pending */ +ATA_PFLAG_HSM_BUSY = (1 <<19 ),/* HSM accessing the port */ /* struct ata_queued_cmd flags */ ATA_QCFLAG_ACTIVE = (1 << 0), /* cmd not yet ack'd to scsi lyer */
Quoted 11 lines · Albert Lee
Quoted 3 lines
Good to know it works. With or without nIEN, I think this change is a good thing to have. IRQ handler shouldn't interfere with polling as both acquire lock during operation.
This reminds me of a possible issue with the patch:
Previously the polling code assumes that the interrupt handler won't interfere with it and the polling code runs without holdingap->lock. However, with the above patch, the interrupt handler might read the Status register when the polling code is transfering data, etc. from the port. Would this cause trouble to the ATA/ATAPI devices?
Quoted 2 lines
Should we have something like
ATA_PFLAG_HSM_BUSYbelow, such that the interrupt handler won't read the Status register when HSM is busy accessing the port?
Right, I forgot about that. Wouldn't just grabbing port lock during PIO be simpler? There is some downside (IRQ latency) but also there are controllers which get upset if PIO data transfer is interrupted by IRQ from other devices.
Signature · 1 line
tejun
PATCH 1/1 libata: disable_irq() during polling IDENTIFY (take 2)
Problem:
Kernel got "irq 5: nobody cared" when using
libata + polling IDENTIFY + Promise 20275 adapter + Benq DW1620 drive.
Detail message available in bug 8441 (http://bugzilla.kernel.org/show_bug.cgi?id=8441).
Cause:
The Benq DW1620 drive raises INTRQ during polling IDENTIFY PACKET DEVICE, even if nIEN = 1.
Proposed fix:
Ack the possibly unexpected irq in ata_interrupt().
---
Patch revised per Tejun's advice to ack the unexpected INTRQ in ata_interrupt(). Also, ap->lock is held when transfering PIO data from/to the port in case the device doesn't like the status register to be read during data transfer.
Patch against 2.6.21.1 for your review, thanks.
drivers/ata/libata-core.c +30 -0
@@ -4490,8 +4490,17 @@ fsm_start: goto fsm_start; } +/* prevent ata_interrupt() from reading +* status when transfering data for safty +*/ +if (in_wq) +spin_lock_irqsave (ap->lock, flags); +atapi_pio_bytes (qc); +if (in_wq) +spin_unlock_irqrestore (ap->lock, flags); +if (unlikely (ap->hsm_task_state ==HSM_ST_ERR ))/* bad ireason reported by device */ goto fsm_start;
@@ -4529,12 +4538,21 @@ fsm_start:/* data might be corrputed */ qc->err_mask |=AC_ERR_DEV ; +/* prevent ata_interrupt() from reading +* status when transfering data for safty +*/ +if (in_wq) +spin_lock_irqsave (ap->lock, flags); +if (!(qc->tf.flags &ATA_TFLAG_WRITE )) {ata_pio_sectors (qc);ata_altstatus (ap); status =ata_wait_idle (ap); } +if (in_wq) +spin_unlock_irqrestore (ap->lock, flags); +if (status & (ATA_BUSY |ATA_DRQ )) qc->err_mask |=AC_ERR_HSM ;
@@ -4546,8 +4564,17 @@ fsm_start: goto fsm_start; } +/* prevent ata_interrupt() from reading status +* when transfering data for safty +*/ +if (in_wq) +spin_lock_irqsave (ap->lock, flags); +ata_pio_sectors (qc); +if (in_wq) +spin_unlock_irqrestore (ap->lock, flags); +if (ap->hsm_task_state ==HSM_ST_LAST && (!(qc->tf.flags &ATA_TFLAG_WRITE ))) {/* all data read */
@@ -5227,6 +5254,9 @@ irqreturn_t ata_interrupt (int irq, voidif (qc && (!(qc->tf.flags &ATA_TFLAG_POLLING )) && (qc->flags &ATA_QCFLAG_ACTIVE )) handled |=ata_host_intr (ap, qc); +else +/* ack possibly unexpected irq */ +ata_chk_status (ap); } }
Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)
Hello, Albert.
Just a few comments.
drivers/ata/libata-core.c fsm_start: · line 4498
Quoted 6 lines · Albert Lee
drivers/ata/libata-core.c +6
-0+/* prevent ata_interrupt() from reading +* status when transfering data for safty +*/ +if (in_wq) +spin_lock_irqsave (ap->lock, flags); +
Can't we just remove in_wq and run the HSM the same way whether we're invoking it from the interrupt handler or work queue? The worker can grab and release the lock outside of the HSM function and HSM can just assume that it's running with lock held. Am I missing something?
drivers/ata/libata-core.c irqreturn_t ata_interrupt (int irq, void · line 5259
Quoted 7 lines
drivers/ata/libata-core.c +3
-0@@ -5227,6 +5254,9 @@ irqreturn_t ata_interrupt (int irq, voidif (qc && (!(qc->tf.flags &ATA_TFLAG_POLLING )) && (qc->flags &ATA_QCFLAG_ACTIVE )) handled |=ata_host_intr (ap, qc); +else +/* ack possibly unexpected irq */ +ata_chk_status (ap);
I'm not so sure whether poking TF Status even when no command is flight is a good idea. The IRQ can be shared with a fairly busy device (e.g. gigabit ethernet) and we'll be reading the status register a lot for nothing. I think we shouldn't do anything as before if no command is in flight.
Also, please split it into two separate patches. Both are pretty significant changes and I think each deserves separate patch.
Thanks.
Signature · 1 line
tejun
Alan Cox
NAKRe: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)
In reply to Albert LeeQuoted 6 lines · On Tue, 08 May 2007 19:30:24 +0800 Albert Lee <[email protected]> wrote:
Problem:
Kernel got "irq 5: nobody cared" when using
libata + polling IDENTIFY + Promise 20275 adapter + Benq DW1620 drive.Detail message available in bug 8441 (http://bugzilla.kernel.org/show_bug.cgi?id=8441).
Quoted 8 lines
Cause:
The Benq DW1620 drive raises INTRQ during polling IDENTIFY PACKET DEVICE, even if nIEN = 1.Proposed fix:
Ack the possibly unexpected irq inata_interrupt().Signed-off-by Albert Lee <[email protected]>
NAK
We can't do this. A PIO data transfer could take far far too long to be under IRQ block (its already a problem on a big 32bit box when we bounce buffer it sometimes).
Alan
Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)
Quoted 16 lines · Alan Cox
Quoted 13 lines · On Tue, 08 May 2007 19:30:24 +0800 Albert Lee <[email protected]> wrote:
Problem:
Kernel got "irq 5: nobody cared" when using
libata + polling IDENTIFY + Promise 20275 adapter + Benq DW1620 drive.Detail message available in bug 8441 (http://bugzilla.kernel.org/show_bug.cgi?id=8441).
Cause:
The Benq DW1620 drive raises INTRQ during polling IDENTIFY PACKET DEVICE, even if nIEN = 1.Proposed fix:
Ack the possibly unexpected irq inata_interrupt().
Quoted 7 lines
Quoted 1 line
Signed-off-by Albert Lee <[email protected]>NAK
We can't do this. A PIO data transfer could take far far too long to be under IRQ block (its already a problem on a big 32bit box when we bounce buffer it sometimes).
Oh well, that's how we do PIO data transfer when not polling anyway. We kick HSM directly from the interrupt handler and do PIO inside the interrupt handler. The change made here is to make polling PIO act the same way.
Signature · 1 line
tejun
Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)
Quoted 4 lines
Oh well, that's how we do PIO data transfer when not polling anyway. We kick HSM directly from the interrupt handler and do PIO inside the interrupt handler. The change made here is to make polling PIO act the same way.
Which is making a horrendous problem worse than it was before. In PIO CD mode I'm actually losing serial characters and time its that bad.
Right now we can push the pio transfer out to run after the IRQ handler a variety of ways and the locking for it isn't too bad. Make this change and we are sunk.
Alan
Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)
Hello,
Quoted 7 lines · Alan Cox
Quoted 4 lines
Oh well, that's how we do PIO data transfer when not polling anyway. We kick HSM directly from the interrupt handler and do PIO inside the interrupt handler. The change made here is to make polling PIO act the same way.
Which is making a horrendous problem worse than it was before. In PIO CD mode I'm actually losing serial characters and time its that bad.
Yeap, things get pretty choppy in PIO mode.
Quoted 3 lines
Right now we can push the pio transfer out to run after the IRQ handler a variety of ways and the locking for it isn't too bad. Make this change and we are sunk.
So, are you suggesting pushing data transfer or the whole HSM to workqueue? I'm all for that. I just thought it didn't make sense to NACK the patch because it grabs locks while doing polling PIO, which mostly happens only for EH commands when we do all the heavy lifting PIO directly from the interrupt handler.
Anyways, if that's the plan, as Albert suggested earlier, we can just mark that HSM is running in ap->pflags or somewhere and skip Status clearing if it's running. It's not like the controller is gonna raise interrupt while it's transferring data anyway (we always have leading status checks before data transfer).
One thing I'm worried about is that somehow I'm thinking there are controllers/devices out there which choke if there PIO data transfer is interrupted (timing-wise). Is this something I just imagined up?
Thanks.
Signature · 1 line
tejun
Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)
Quoted 3 lines
One thing I'm worried about is that somehow I'm thinking there are controllers/devices out there which choke if there PIO data transfer is interrupted (timing-wise). Is this something I just imagined up?
There are a couple of problem controllers. They use the ata_data_xfer_noirq method for ->data_xfer() which blocks local interrupts in the method itself and in those cases we have no choice. Such chips are all ancient thankfully.
Alan
Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)
Quoted 8 lines · Alan Cox
Quoted 3 lines
One thing I'm worried about is that somehow I'm thinking there are controllers/devices out there which choke if there PIO data transfer is interrupted (timing-wise). Is this something I just imagined up?
There are a couple of problem controllers. They use the
ata_data_xfer_noirqmethod for ->data_xfer() which blocks local interrupts in the method itself and in those cases we have no choice. Such chips are all ancient thankfully.
OIC, using separate data_xfer() definitely makes sense.
Signature · 1 line
tejun
Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)
In reply to Tejun HeoQuoted 2 lines
So, are you suggesting pushing data transfer or the whole HSM to workqueue? I'm all for that. I just thought it didn't make sense to
I was thinking data transfer but HSM itself might work too, I'd not even thought of push that much out.
Quoted 5 lines
Anyways, if that's the plan, as Albert suggested earlier, we can just mark that HSM is running in
ap->pflagsor somewhere and skip Status clearing if it's running. It's not like the controller is gonna raise interrupt while it's transferring data anyway (we always have leading status checks before data transfer).
And the controllers with a hidden FIFO magically defer the IRQ until the FIFO empties so that the events occur in the expected sequence for the spec even if the drive transfer to the fifo already finished and the host is still emptying it.
Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)
Quoted 5 lines · Alan Cox
Quoted 2 lines
So, are you suggesting pushing data transfer or the whole HSM to workqueue? I'm all for that. I just thought it didn't make sense to
I was thinking data transfer but HSM itself might work too, I'd not even thought of push that much out.
I thought pushing the whole HSM out to workqueue will be the simplest to implement as we already have everything needed for polling.
Quoted 10 lines
Quoted 5 lines
Anyways, if that's the plan, as Albert suggested earlier, we can just mark that HSM is running in
ap->pflagsor somewhere and skip Status clearing if it's running. It's not like the controller is gonna raise interrupt while it's transferring data anyway (we always have leading status checks before data transfer).And the controllers with a hidden FIFO magically defer the IRQ until the FIFO empties so that the events occur in the expected sequence for the spec even if the drive transfer to the fifo already finished and the host is still emptying it.
Yeah, that sadly makes things a bit tricky tho. We need to make the last transfer and clearing HSM running flag atomic; otherwise, we're likely to get nobody-cared right after we finish the last transfer.
I guess it's about time to listen what Jeff thinks.
Albert, if Jeff agrees with pushing HSM or data transfer to workqueue, are you interested in doing that? The HSM is your baby after all. :-)
Thanks.
Signature · 1 line
tejun
Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)
Quoted 6 lines · Tejun Heo
I guess it's about time to listen what Jeff thinks.
Albert, if Jeff agrees with pushing HSM or data transfer to workqueue, are you interested in doing that? The HSM is your baby after all. :-)
Moving HSM task to the workqueue looks good idea. The mouse cursor of my X desktop doesn't move smoothly when blocks of PIO are going in hard irq. Hopefully we can do better with that.
Yes, I am interested in doing it, if Jeff also ack. --
albert
Re: [PATCH] libata: disable_irq() during polling IDENTIFY (take 2)
In reply to Tejun HeoQuoted 2 lines · Tejun Heo
Albert, if Jeff agrees with pushing HSM or data transfer to workqueue, are you interested in doing that? The HSM is your baby after all. :-)
It's always been my desire to do PIO from a workqueue if possible, since it is so slow.
Jeff
cover libata: push part of irq driven pio to workqueue
In reply to Tejun Heo1/7: set the state after "PIO data-in" to HSM_ST_IDLE instead of HSM_ST_LAST 2/7: fix the ata_altstatus() in ata_hsm_qc_complete()
3/7: move ata_altstatus() out from ata_hsm_move() to the pio data xfer functions 4/7: move polling idle irq check to ata_host_intr()
5/7: move and reduce locking to the pio data xfer functions
6/7: push part of the irq driven pio out to workqueue
7/7: ack unexpected INTRQ when polling
This is not complete yet, still needs to check if this creates new race conditions with EH or causing trouble to other part.
Draft patch against 2.6.21.1 for your review.
PATCH 1/7 libata: set the state after "PIO data-in" to HSM_ST_IDLE instead of HSM_ST_LAST
patch 1/7:
Set the state after "PIO data-in" to HSM_ST_IDLE instead of HSM_ST_LAST.
To prevent the irq handler from thinking "HSM is waiting for the last irq". Needed later when part of the irq driven pio pushed out to workqueue.
---
drivers/ata/libata-core.c +5 -3
@@ -4041,7 +4041,7 @@ static void ata_pio_sector(struct ata_quunsigned char *buf;if (qc->curbytes == qc->nbytes -ATA_SECT_SIZE ) - ap->hsm_task_state =HSM_ST_LAST ; + ap->hsm_task_state = do_write ?HSM_ST_LAST :HSM_ST_IDLE ; page = sg[qc->cursg].page; offset = sg[qc->cursg].offset + qc->cursg_ofs;
@@ -4389,6 +4389,8 @@ int ata_hsm_move(struct ata_port *ap, st*/ WARN_ON (in_wq !=ata_hsm_ok_in_wq (ap, qc)); +WARN_ON (ap->hsm_task_state ==HSM_ST_IDLE ); + fsm_start:DPRINTK ("ata%u: protocol %d task_state %d (dev_stat 0x%X)\n" , ap->print_id, qc->tf.protocol, ap->hsm_task_state, status);
@@ -4548,8 +4550,7 @@ fsm_start:ata_pio_sectors (qc); -if (ap->hsm_task_state ==HSM_ST_LAST && - (!(qc->tf.flags &ATA_TFLAG_WRITE ))) { +if (ap->hsm_task_state ==HSM_ST_IDLE ) {/* all data read */ ata_altstatus (ap); status =ata_wait_idle (ap);
@@ -4562,6 +4563,7 @@ fsm_start:break ;case HSM_ST_LAST : +case HSM_ST_IDLE :if (unlikely (!ata_ok (status))) { qc->err_mask |=__ac_err_mask (status); ap->hsm_task_state =HSM_ST_ERR ;
PATCH 2/7 libata: move the ata_altstatus() in ata_hsm_qc_complete()
In reply to Albert Leepatch 2/7:
Calling ata_altstatus() after the qc completed looks incorrect.
Move it to before the qc is completed.
---
drivers/ata/libata-core.c +2 -2
@@ -4329,6 +4329,8 @@ static void ata_hsm_qc_complete(struct astruct ata_port *ap = qc->ap;unsigned long flags; +ata_altstatus (ap);/* flush */ +if (ap->ops->error_handler) {if (in_wq) {spin_lock_irqsave (ap->lock, flags);
@@ -4361,8 +4363,6 @@ static void ata_hsm_qc_complete(struct a }else ata_qc_complete (qc); } - -ata_altstatus (ap);/* flush */ }/**
PATCH 3/7 libata: move ata_altstatus() out to the pio data xfer functions
In reply to Albert Leepatch 3/7:
move ata_altstatus() out from ata_hsm_move() to the pio data xfer functions.
Functions like ata_pio_sectors() and atapi_pio_bytes() know better if the flush is needed.
---
drivers/ata/libata-core.c +10 -7
@@ -4031,7 +4031,7 @@ void ata_data_xfer_noirq(struct ata_devi* Inherited from caller. */ -static void ata_pio_sector (struct ata_queued_cmd *qc) +static void ata_pio_sector (struct ata_queued_cmd *qc,int last) {int do_write = (qc->tf.flags &ATA_TFLAG_WRITE );struct scatterlist *sg = qc->__sg;
@@ -4069,6 +4069,9 @@ static void ata_pio_sector(struct ata_qu ap->ops->data_xfer (qc->dev, buf + offset,ATA_SECT_SIZE , do_write); } +if (last) +ata_altstatus (ap);/* flush */ + qc->curbytes +=ATA_SECT_SIZE ; qc->cursg_ofs +=ATA_SECT_SIZE ;
@@ -4100,9 +4103,9 @@ static void ata_pio_sectors(struct ata_q nsect =min ((qc->nbytes - qc->curbytes) /ATA_SECT_SIZE , qc->dev->multi_count);while (nsect--) -ata_pio_sector (qc); +ata_pio_sector (qc, !nsect); }else -ata_pio_sector (qc); +ata_pio_sector (qc,1 ); }/**
@@ -4185,6 +4188,8 @@ next_sg:for (i =0 ; i < words; i++) ap->ops->data_xfer (qc->dev, (unsigned char *)pad_buf,2 , do_write); +ata_altstatus (ap);/* flush */ + ap->hsm_task_state =HSM_ST_LAST ;return ; }
@@ -4234,6 +4239,8 @@ next_sg:if (bytes) goto next_sg; + +ata_altstatus (ap);/* flush */ }/**
@@ -4452,7 +4459,6 @@ fsm_start:*/ ap->hsm_task_state =HSM_ST ;ata_pio_sectors (qc); -ata_altstatus (ap);/* flush */ }else /* send CDB */ atapi_send_cdb (ap, qc);
@@ -4533,7 +4539,6 @@ fsm_start:if (!(qc->tf.flags &ATA_TFLAG_WRITE )) {ata_pio_sectors (qc); -ata_altstatus (ap); status =ata_wait_idle (ap); }
@@ -4552,13 +4557,11 @@ fsm_start:if (ap->hsm_task_state ==HSM_ST_IDLE ) {/* all data read */ -ata_altstatus (ap); status =ata_wait_idle (ap); goto fsm_start; } } -ata_altstatus (ap);/* flush */ poll_next =1 ;break ;
PATCH 4/7 libata: move polling idle irq check to ata_host_intr()
In reply to Albert Leepatch 4/7:
move the polling idle irq check from ata_interrupt() to ata_host_intr(), where it makes more sense.
---
drivers/ata/libata-core.c +5 -2
@@ -5119,6 +5119,10 @@ inline unsigned int ata_host_intr (strucVPRINTK ("ata%u: protocol %d task_state %d\n" , ap->print_id, qc->tf.protocol, ap->hsm_task_state); +/* polling */ +if (qc->tf.flags &ATA_TFLAG_POLLING ) + goto idle_irq; +/* Check whether we are expecting interrupt in this state */ switch (ap->hsm_task_state) {case HSM_ST_FIRST :
@@ -5229,8 +5233,7 @@ irqreturn_t ata_interrupt (int irq, voidstruct ata_queued_cmd *qc; qc =ata_qc_from_tag (ap, ap->active_tag); -if (qc && (!(qc->tf.flags &ATA_TFLAG_POLLING )) && - (qc->flags &ATA_QCFLAG_ACTIVE )) +if (qc && (qc->flags &ATA_QCFLAG_ACTIVE )) handled |=ata_host_intr (ap, qc); } }
Re: [PATCH 4/7] libata: move polling idle irq check to ata_host_intr()
Quoted 22 lines · Albert Lee
patch 4/7:
move the polling idle irq check fromata_interrupt()toata_host_intr(), where it makes more sense.Signed-off-by Albert Lee <[email protected]>---
diff -Nrup
03_smart_flush/drivers/ata/libata-core.c04_polling_check/drivers/ata/libata-core.cdrivers/ata/libata-core.c +4
-0@@ -5119,6 +5119,10 @@ inline unsigned int ata_host_intr (strucVPRINTK ("ata%u: protocol %d task_state %d\n" , ap->print_id, qc->tf.protocol, ap->hsm_task_state); +/* polling */ +if (qc->tf.flags &ATA_TFLAG_POLLING ) + goto idle_irq; +/* Check whether we are expecting interrupt in this state */ switch (ap->hsm_task_state) {case HSM_ST_FIRST :
drivers/ata/libata-core.c irqreturn_t ata_interrupt (int irq, void · line 5236
Quoted 6 lines
drivers/ata/libata-core.c +1
-2@@ -5229,8 +5233,7 @@ irqreturn_t ata_interrupt (int irq, voidstruct ata_queued_cmd *qc; qc =ata_qc_from_tag (ap, ap->active_tag); -if (qc && (!(qc->tf.flags &ATA_TFLAG_POLLING )) && - (qc->flags &ATA_QCFLAG_ACTIVE )) +if (qc && (qc->flags &ATA_QCFLAG_ACTIVE ))
There are several LLD specific IRQ handlers which have similar part. Care to update them together?
Signature · 1 line
tejun
Re: [PATCH 4/7] libata: move polling idle irq check to ata_host_intr()
Quoted 18 lines · Tejun Heo
Quoted 18 lines
diff -Nrup
03_smart_flush/drivers/ata/libata-core.c04_polling_check/drivers/ata/libata-core.cdrivers/ata/libata-core.c +4
-0@@ -5119,6 +5119,10 @@ inline unsigned int ata_host_intr (strucVPRINTK ("ata%u: protocol %d task_state %d\n" , ap->print_id, qc->tf.protocol, ap->hsm_task_state); +/* polling */ +if (qc->tf.flags &ATA_TFLAG_POLLING ) + goto idle_irq; +/* Check whether we are expecting interrupt in this state */ switch (ap->hsm_task_state) {case HSM_ST_FIRST :@@ -5229,8 +5233,7 @@ irqreturn_t ata_interrupt (int irq, voidstruct ata_queued_cmd *qc;
Quoted 9 lines
drivers/ata/libata-core.c irqreturn_t ata_interrupt (int irq, void · line 5236
Quoted 4 lines
drivers/ata/libata-core.c +1
-2qc =ata_qc_from_tag (ap, ap->active_tag); -if (qc && (!(qc->tf.flags &ATA_TFLAG_POLLING )) && - (qc->flags &ATA_QCFLAG_ACTIVE )) +if (qc && (qc->flags &ATA_QCFLAG_ACTIVE ))There are several LLD specific IRQ handlers which have similar part. Care to update them together?
Sure, will do.
--
albert
PATCH 5/7 libata: move and reduce locking to the pio data xfer functions
In reply to Albert Leepatch 5/7:
- move the locking out from the ata_hsm_move() to the data xfer functions like ata_pio_sectors().
- added the ATA_PFLAG_HSM_WQ (HSM running in workqueue) flag to serialize irq and workqueue.
- the time holding ap->lock is reduced to last piece of pio transfer and clearing the ATA_PFLAG_HSM_WQ flag
---
drivers/ata/libata-core.c +95 -23
@@ -4031,7 +4031,7 @@ void ata_data_xfer_noirq(struct ata_devi* Inherited from caller. */ -static void ata_pio_sector (struct ata_queued_cmd *qc,int last) +static void ata_pio_sector (struct ata_queued_cmd *qc,int last,int lock) {int do_write = (qc->tf.flags &ATA_TFLAG_WRITE );struct scatterlist *sg = qc->__sg;
@@ -4039,6 +4039,7 @@ static void ata_pio_sector(struct ata_qustruct page *page;unsigned int offset;unsigned char *buf; +unsigned long irq_flags =0 ;if (qc->curbytes == qc->nbytes -ATA_SECT_SIZE ) ap->hsm_task_state = do_write ?HSM_ST_LAST :HSM_ST_IDLE ;
@@ -4055,6 +4056,9 @@ static void ata_pio_sector(struct ata_quif (PageHighMem (page)) {unsigned long flags; +if (lock) +spin_lock_irqsave (ap->lock, irq_flags); +/* FIXME: use a bounce buffer */ local_irq_save (flags); buf =kmap_atomic (page,KM_IRQ0 );
@@ -4065,8 +4069,18 @@ static void ata_pio_sector(struct ata_qukunmap_atomic (buf,KM_IRQ0 );local_irq_restore (flags); }else { +unsigned int head =0 , tail =ATA_SECT_SIZE ; + buf =page_address (page); - ap->ops->data_xfer (qc->dev, buf + offset,ATA_SECT_SIZE , do_write); + +if (lock) { + tail =8 ; + head =ATA_SECT_SIZE - tail;/* multiple of 8 bytes */ + ap->ops->data_xfer (qc->dev, buf + offset, head, do_write); +spin_lock_irqsave (ap->lock, irq_flags); + } + + ap->ops->data_xfer (qc->dev, buf + offset + head, tail, do_write); }if (last)
@@ -4079,6 +4093,11 @@ static void ata_pio_sector(struct ata_qu qc->cursg++; qc->cursg_ofs =0 ; } + +if (lock) { + ap->pflags &= ~ATA_PFLAG_HSM_WQ ; +spin_unlock_irqrestore (ap->lock, irq_flags); + } }/**
@@ -4092,7 +4111,7 @@ static void ata_pio_sector(struct ata_qu* Inherited from caller. */ -static void ata_pio_sectors (struct ata_queued_cmd *qc) +static void ata_pio_sectors (struct ata_queued_cmd *qc,int lock) {if (is_multi_taskfile (&qc->tf)) {/* READ/WRITE MULTIPLE */
@@ -4103,9 +4122,9 @@ static void ata_pio_sectors(struct ata_q nsect =min ((qc->nbytes - qc->curbytes) /ATA_SECT_SIZE , qc->dev->multi_count);while (nsect--) -ata_pio_sector (qc, !nsect); +ata_pio_sector (qc, !nsect, lock && !nsect); }else -ata_pio_sector (qc,1 ); +ata_pio_sector (qc,1 , lock); }/**
@@ -4120,12 +4139,17 @@ static void ata_pio_sectors(struct ata_q* caller. */ -static void atapi_send_cdb (struct ata_port *ap,struct ata_queued_cmd *qc) +static void atapi_send_cdb (struct ata_port *ap,struct ata_queued_cmd *qc,int lock) { +unsigned long irq_flags =0 ; +/* send SCSI cdb */ DPRINTK ("send cdb\n" );WARN_ON (qc->dev->cdb_len <12 ); +if (lock) +spin_lock_irqsave (ap->lock, irq_flags); + ap->ops->data_xfer (qc->dev, qc->cdb, qc->dev->cdb_len,1 );ata_altstatus (ap);/* flush */
@@ -4142,6 +4166,11 @@ static void atapi_send_cdb(struct ata_po ap->ops->bmdma_start (qc);break ; } + +if (lock) { + ap->pflags &= ~ATA_PFLAG_HSM_WQ ; +spin_unlock_irqrestore (ap->lock, irq_flags); + } }/**
@@ -4156,7 +4185,7 @@ static void atapi_send_cdb(struct ata_po* */ -static void __atapi_pio_bytes (struct ata_queued_cmd *qc,unsigned int bytes) +static void __atapi_pio_bytes (struct ata_queued_cmd *qc,unsigned int bytes,int lock) {int do_write = (qc->tf.flags &ATA_TFLAG_WRITE );struct scatterlist *sg = qc->__sg;
@@ -4164,6 +4193,8 @@ static void __atapi_pio_bytes(struct atastruct page *page;unsigned char *buf;unsigned int offset, count; +unsigned long irq_flags =0 ; +int transfer_lock =0 ;if (qc->curbytes + bytes >= qc->nbytes) ap->hsm_task_state =HSM_ST_LAST ;
@@ -4181,6 +4212,10 @@ next_sg:unsigned int words = bytes >>1 ;unsigned int i; +WARN_ON (transfer_lock); +if (lock) +spin_lock_irqsave (ap->lock, irq_flags); +if (words)/* warning if bytes > 1 */ ata_dev_printk (qc->dev,KERN_WARNING ,"%u bytes trailing data\n" , bytes);
@@ -4191,6 +4226,12 @@ next_sg:ata_altstatus (ap);/* flush */ ap->hsm_task_state =HSM_ST_LAST ; + +if (lock) { + ap->pflags &= ~ATA_PFLAG_HSM_WQ ; +spin_unlock_irqrestore (ap->lock, irq_flags); + } +return ; }
@@ -4210,10 +4251,16 @@ next_sg: count =min (count, (unsigned int )PAGE_SIZE - offset);DPRINTK ("data %s\n" , qc->tf.flags &ATA_TFLAG_WRITE ?"write" :"read" ); + +if (lock && bytes <= count) + transfer_lock =1 ;if (PageHighMem (page)) {unsigned long flags; +if (transfer_lock) +spin_lock_irqsave (ap->lock, irq_flags); +/* FIXME: use bounce buffer */ local_irq_save (flags); buf =kmap_atomic (page,KM_IRQ0 );
@@ -4224,8 +4271,21 @@ next_sg:kunmap_atomic (buf,KM_IRQ0 );local_irq_restore (flags); }else { +unsigned int head =0 , tail = count; + buf =page_address (page); - ap->ops->data_xfer (qc->dev, buf + offset, count, do_write); + +if (transfer_lock) { +if (count >20 ) { + tail =8 + count %8 ; + head = count - tail;/* multiple of 8 bytes */ + ap->ops->data_xfer (qc->dev, buf + offset, head, do_write); + } + +spin_lock_irqsave (ap->lock, irq_flags); + } + + ap->ops->data_xfer (qc->dev, buf + offset + head, tail, do_write); } bytes -= count;
@@ -4241,6 +4301,12 @@ next_sg: goto next_sg;ata_altstatus (ap);/* flush */ + +if (lock) { +WARN_ON (!transfer_lock); + ap->pflags &= ~ATA_PFLAG_HSM_WQ ; +spin_unlock_irqrestore (ap->lock, irq_flags); + } }/**
@@ -4253,7 +4319,7 @@ next_sg:* Inherited from caller. */ -static void atapi_pio_bytes (struct ata_queued_cmd *qc) +static void atapi_pio_bytes (struct ata_queued_cmd *qc,int lock) {struct ata_port *ap = qc->ap;struct ata_device *dev = qc->dev;
@@ -4283,7 +4349,7 @@ static void atapi_pio_bytes(struct ata_qVPRINTK ("ata%u: xfering %d bytes\n" , ap->print_id, bytes); -__atapi_pio_bytes (qc, bytes); +__atapi_pio_bytes (qc, bytes, lock);return ;
@@ -4385,7 +4451,6 @@ static void ata_hsm_qc_complete(struct aint ata_hsm_move (struct ata_port *ap,struct ata_queued_cmd *qc,u8 status,int in_wq) { -unsigned long flags =0 ;int poll_next;WARN_ON ((qc->flags &ATA_QCFLAG_ACTIVE ) ==0 );
@@ -4443,11 +4508,10 @@ fsm_start:/* Send the CDB (atapi) or the first data block (ata pio out). * During the state transition, interrupt handler shouldn't * be invoked before the data transfer is complete and -* hsm_task_state is changed. Hence, the following locking. +* hsm_task_state is changed. This is ensured by the +* ATA_PFLAG_HSM_WQ flag and holding ap->lock in the PIO +* data transfer functions. */ -if (in_wq) -spin_lock_irqsave (ap->lock, flags); -if (qc->tf.protocol ==ATA_PROT_PIO ) {/* PIO data out protocol. * send first data block.
@@ -4458,13 +4522,10 @@ fsm_start:* before ata_pio_sectors(). */ ap->hsm_task_state =HSM_ST ; -ata_pio_sectors (qc); +ata_pio_sectors (qc, in_wq); }else /* send CDB */ -atapi_send_cdb (ap, qc); - -if (in_wq) -spin_unlock_irqrestore (ap->lock, flags); +atapi_send_cdb (ap, qc, in_wq);/* if polling, ata_pio_task() handles the rest. * otherwise, interrupt handler takes over from here.
@@ -4498,7 +4559,7 @@ fsm_start: goto fsm_start; } -atapi_pio_bytes (qc); +atapi_pio_bytes (qc,0 );if (unlikely (ap->hsm_task_state ==HSM_ST_ERR ))/* bad ireason reported by device */
@@ -4538,7 +4599,7 @@ fsm_start: qc->err_mask |=AC_ERR_DEV ;if (!(qc->tf.flags &ATA_TFLAG_WRITE )) { -ata_pio_sectors (qc); +ata_pio_sectors (qc,0 ); status =ata_wait_idle (ap); }
@@ -4553,7 +4614,7 @@ fsm_start: goto fsm_start; } -ata_pio_sectors (qc); +ata_pio_sectors (qc,0 );if (ap->hsm_task_state ==HSM_ST_IDLE ) {/* all data read */
@@ -4615,6 +4676,7 @@ static void ata_pio_task(struct work_strstruct ata_queued_cmd *qc = ap->port_task_data;u8 status;int poll_next; +unsigned long irq_flags; fsm_start:WARN_ON (ap->hsm_task_state ==HSM_ST_IDLE );
@@ -4636,6 +4698,11 @@ fsm_start: } } +/* Let the irq handler know wq is accessing the port */ +spin_lock_irqsave (ap->lock, irq_flags); + ap->pflags |=ATA_PFLAG_HSM_WQ ; +spin_unlock_irqrestore (ap->lock, irq_flags); +/* move the HSM */ poll_next =ata_hsm_move (ap, qc, status,1 );
@@ -4749,6 +4816,7 @@ void __ata_qc_complete(struct ata_queued*/ qc->flags &= ~ATA_QCFLAG_ACTIVE ; ap->qc_active &= ~(1 << qc->tag); + ap->pflags &= ~ATA_PFLAG_HSM_WQ ;/* call completion callback */ qc->complete_fn (qc);
@@ -5119,6 +5187,10 @@ inline unsigned int ata_host_intr (strucVPRINTK ("ata%u: protocol %d task_state %d\n" , ap->print_id, qc->tf.protocol, ap->hsm_task_state); +/* HSM accessing the port from the workqueue */ +if (ap->pflags &ATA_PFLAG_HSM_WQ ) + goto idle_irq; +/* polling */ if (qc->tf.flags &ATA_TFLAG_POLLING ) goto idle_irq;
include/linux/libata.h +1 -0
@@ -195,6 +195,7 @@ enum {ATA_PFLAG_FLUSH_PORT_TASK = (1 <<16 ),/* flush port task */ ATA_PFLAG_SUSPENDED = (1 <<17 ),/* port is suspended (power) */ ATA_PFLAG_PM_PENDING = (1 <<18 ),/* PM operation pending */ +ATA_PFLAG_HSM_WQ = (1 <<19 ),/* hsm accessing the port in wq */ /* struct ata_queued_cmd flags */ ATA_QCFLAG_ACTIVE = (1 << 0), /* cmd not yet ack'd to scsi lyer */
Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions
drivers/ata/libata-core.c void ata_data_xfer_noirq(struct ata_devi · line 4034
Quoted 2 lines · Albert Lee
drivers/ata/libata-core.c +1
-1-static void ata_pio_sector (struct ata_queued_cmd *qc,int last) +static void ata_pio_sector (struct ata_queued_cmd *qc,int last,int lock)
I think the naming of @lock is a bit confusing here. @clr_hsm_wq or @last_sector, maybe?
drivers/ata/libata-core.c static void ata_pio_sector(struct ata_qu · line 4083
Quoted 8 lines
drivers/ata/libata-core.c +8
-0+if (lock) { + tail =8 ; + head =ATA_SECT_SIZE - tail;/* multiple of 8 bytes */ + ap->ops->data_xfer (qc->dev, buf + offset, head, do_write); +spin_lock_irqsave (ap->lock, irq_flags); + } + + ap->ops->data_xfer (qc->dev, buf + offset + head, tail, do_write);
Aieee, we have to transfer the whole last sector while holding the spin lock and IRQ disabled. That's sad but pushing locking into ->data_xfer doesn't sound attractive either. Any better ideas?
drivers/ata/libata-core.c inline unsigned int ata_host_intr (struc · line 5193
Quoted 8 lines
drivers/ata/libata-core.c +4
-0@@ -5119,6 +5187,10 @@ inline unsigned int ata_host_intr (strucVPRINTK ("ata%u: protocol %d task_state %d\n" , ap->print_id, qc->tf.protocol, ap->hsm_task_state); +/* HSM accessing the port from the workqueue */ +if (ap->pflags &ATA_PFLAG_HSM_WQ ) + goto idle_irq; +
Please update LLDs with private IRQ handlers too. Maybe we need ata_idle_irq(ap) test?
Thanks.
Signature · 1 line
tejun
Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions
Quoted 3 lines
Aieee, we have to transfer the whole last sector while holding the spin lock and IRQ disabled. That's sad but pushing locking into ->data_xfer doesn't sound attractive either. Any better ideas?
I'd say this is a non-starter. It solves nothing and means PIO in libata is still basically unusable.
Alan
Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions
Quoted 6 lines · Alan Cox
Quoted 3 lines
Aieee, we have to transfer the whole last sector while holding the spin lock and IRQ disabled. That's sad but pushing locking into ->data_xfer doesn't sound attractive either. Any better ideas?
I'd say this is a non-starter. It solves nothing and means PIO in libata is still basically unusable.
It doesn't solve the problem completely but still helps, FWIW. I was hoping we could lock only for the last transfer (word). Would it complicate ->data_xfer() too much?
Signature · 1 line
tejun
Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions
Quoted 4 lines
Quoted 2 lines
I'd say this is a non-starter. It solves nothing and means PIO in libata is still basically unusable.
It doesn't solve the problem completely but still helps, FWIW. I was
Most transfers for PIO are a single 512 byte transfer per command.
Quoted 2 lines
hoping we could lock only for the last transfer (word). Would it complicate ->data_xfer() too much?
I don't think so. We need to complicate it far more to add 32bit transfers. We also only have a few ->data_xfer implementations so its easy to propogate the chance.
Why do we need the lock on the last word transferred anyway. I'm missing a detail here ?
Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions
Quoted 5 lines · Alan Cox
Quoted 3 lines
Quoted 2 lines
I'd say this is a non-starter. It solves nothing and means PIO in libata is still basically unusable.
It doesn't solve the problem completely but still helps, FWIW. I was
Most transfers for PIO are a single 512 byte transfer per command.
Not for disks but, yeah, who uses PIO for disks.
Quoted 9 lines
Quoted 2 lines
hoping we could lock only for the last transfer (word). Would it complicate ->data_xfer() too much?
I don't think so. We need to complicate it far more to add 32bit transfers. We also only have a few ->data_xfer implementations so its easy to propogate the chance.
Why do we need the lock on the last word transferred anyway. I'm missing a detail here ?
The problem is that controllers queue IRQ till the end of transfer and raise it right after the last transfer completes. If WQ-active-ignore-IRQ flag is set at that point && we're not holding the lock, the IRQ handler will ignore the IRQ without clearing it, so we get nobody-cared right after the last transfer. So, the last transfer and clearing of WQ-active-ignore-IRQ flag should be atomic w.r.t. the IRQ handler.
Signature · 1 line
tejun
Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions
Quoted 8 lines · On Fri, 11 May 2007 17:14:35 +0200 Tejun Heo <[email protected]> wrote:
Quoted 5 lines · Alan Cox
Quoted 3 lines
Quoted 2 lines
I'd say this is a non-starter. It solves nothing and means PIO in libata is still basically unusable.
It doesn't solve the problem completely but still helps, FWIW. I was
Most transfers for PIO are a single 512 byte transfer per command.
Not for disks but, yeah, who uses PIO for disks.
PCMCIA, embedded, CF devices .. basically everyone.
Quoted 7 lines
The problem is that controllers queue IRQ till the end of transfer and raise it right after the last transfer completes. If WQ-active-ignore-IRQ flag is set at that point && we're not holding the lock, the IRQ handler will ignore the IRQ without clearing it, so we get nobody-cared right after the last transfer. So, the last transfer and clearing of WQ-active-ignore-IRQ flag should be atomic w.r.t. the IRQ handler.
Ok that makes sense. And we can't safely peek at DRQ at this point either. Holding the lock for the last word shouldn't be a problem. It isn't ideal but it is a lot less ugly than holding it for the full transfer. The private data_xfer methods in PATA are for locking IRQ off entirely so they'll convert trivially, not looked for others.
Alan
Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions
In reply to Tejun HeoQuoted 7 lines
The problem is that controllers queue IRQ till the end of transfer and raise it right after the last transfer completes. If WQ-active-ignore-IRQ flag is set at that point && we're not holding the lock, the IRQ handler will ignore the IRQ without clearing it, so we get nobody-cared right after the last transfer. So, the last transfer and clearing of WQ-active-ignore-IRQ flag should be atomic w.r.t. the IRQ handler.
There does seem to be an alternative. In the pio handler we do
clear_bit(COMPLETION_RUN, ->flags); xfer bytes if (test_and_set_bit(COMPLETION_RUN, ->flags) == 0) run_completion_routine(); ->active-ignore-irq = 0;
and in the IRQ case after checking we are not active-ignore-IRQ we similarly do
if (test_and_set_bit(COMPLETION_RUN, ->flags) == 0) run_completion_routine_irq();
Now if we are unlucky and the IRQ gets in between the last byte of transfer and clearing the active ignore IRQ flag we will still run the completion handler
Alan
Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions
Quoted 19 lines · Alan Cox
Quoted 7 lines
The problem is that controllers queue IRQ till the end of transfer and raise it right after the last transfer completes. If WQ-active-ignore-IRQ flag is set at that point && we're not holding the lock, the IRQ handler will ignore the IRQ without clearing it, so we get nobody-cared right after the last transfer. So, the last transfer and clearing of WQ-active-ignore-IRQ flag should be atomic w.r.t. the IRQ handler.
There does seem to be an alternative. In the pio handler we do
clear_bit(COMPLETION_RUN, ->flags); xfer bytes if (test_and_set_bit(COMPLETION_RUN, ->flags) == 0) run_completion_routine(); ->active-ignore-irq = 0;and in the IRQ case after checking we are not active-ignore-IRQ we similarly do
Quoted 6 lines
if (test_and_set_bit(COMPLETION_RUN, ->flags) == 0) run_completion_routine_irq();Now if we are unlucky and the IRQ gets in between the last byte of transfer and clearing the active ignore IRQ flag we will still run the completion handler
I don't really get this. What happens if the IRQ is shared and the other device raises interrupt while the data transfer is still in progress? What prevents it from running the completion routine?
Signature · 1 line
tejun
Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions
Quoted 12 lines
Quoted 12 lines
clear_bit(COMPLETION_RUN, ->flags); xfer bytes if (test_and_set_bit(COMPLETION_RUN, ->flags) == 0) run_completion_routine(); ->active-ignore-irq = 0;and in the IRQ case after checking we are not active-ignore-IRQ we similarly do
if (test_and_set_bit(COMPLETION_RUN, ->flags) == 0) run_completion_routine_irq();
Quoted 7 lines
Quoted 3 lines
Now if we are unlucky and the IRQ gets in between the last byte of transfer and clearing the active ignore IRQ flag we will still run the completion handler
I don't really get this. What happens if the IRQ is shared and the other device raises interrupt while the data transfer is still in progress? What prevents it from running the completion routine?
Before the transfer begins you set your flag to indicate you want to ignore this IRQ. You then set the flag indicating completion has not occurred.
Next you do the data transfer
An IRQ during the transfer will not complete as we are ignoring the IRQ/polling at the driver level.
On completion either
a) the IRQ beats the test_and_set_bit in which case it runs the completion
or
b) the test_and_set_bit in the I/O path beats the IRQ handler in which case the IRQ handler will not run the completion as the bit is atomically set
or
c) the IRQ beats the clearing of the ignore irq flag in which case it won't execute the handler but the data transfer path will
I'm sure there are even simpler ways we can do this they key is to make sure that whatever the event order we only run the completion path once, and it doesn't matter which path runs it.
Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions
Quoted 6 lines · Alan Cox
Quoted 6 lines
Quoted 3 lines
Now if we are unlucky and the IRQ gets in between the last byte of transfer and clearing the active ignore IRQ flag we will still run the completion handler
I don't really get this. What happens if the IRQ is shared and the other device raises interrupt while the data transfer is still in progress? What prevents it from running the completion routine?
[--snip--]
Quoted 2 lines
c) the IRQ beats the clearing of the ignore irq flag in which case it won't execute the handler but the data transfer path will
This case is where I fail to understand how it's supposed to work. If IRQ beats the clearing of the ignore irq flag && execution of completion routine from wq, it ignores the IRQ, right? The IRQ line remains asserted, so when the IRQ handler finishes, it's entered again immediately till nobody-cared kicks in and completely disable the IRQ line. ie.
clear_bit(COMPLETION_RUN, ->flags); xfer bytes >> IRQ hits here if (test_and_set_bit(COMPLETION_RUN, ->flags) == 0) run_completion_routine(); ->active-ignore-irq = 0;
Signature · 1 line
tejun
Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions
Quoted 3 lines
This case is where I fail to understand how it's supposed to work. If IRQ beats the clearing of the ignore irq flag && execution of completion routine from wq, it ignores the IRQ, right? The IRQ line remains
I was assuming your polling handler would be polling and clearing the IRQ status otherwise it doens't work anyway - cable noise or drive error and an early DRQ de-assert would do the same thing during a transfer with IRQ blocked only on the last dword.
Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions
Quoted 8 lines · Alan Cox
Quoted 3 lines
This case is where I fail to understand how it's supposed to work. If IRQ beats the clearing of the ignore irq flag && execution of completion routine from wq, it ignores the IRQ, right? The IRQ line remains
I was assuming your polling handler would be polling and clearing the IRQ status otherwise it doens't work anyway - cable noise or drive error and an early DRQ de-assert would do the same thing during a transfer with IRQ blocked only on the last dword.
Yeah, it's not a water-tight solution. I don't think we can do that with IRQ enabled during transfer. So, we can have either highly jerky but reliable system if PIO is used or usually better behaving PIO with some chance of getting nobody-cared. Also, we can flag known buggy devices/controllers such that IRQ is disabled over PIO by default. I think it's worth doing but this definitely should be debated better.
Signature · 1 line
tejun
Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions
Quoted 6 lines
Yeah, it's not a water-tight solution. I don't think we can do that with IRQ enabled during transfer. So, we can have either highly jerky but reliable system if PIO is used or usually better behaving PIO with some chance of getting nobody-cared. Also, we can flag known buggy devices/controllers such that IRQ is disabled over PIO by default. I think it's worth doing but this definitely should be debated better.
Which leaves us back at enable/disable_irq as with the old IDE - which *are* much better because most of the time the IRQ isn't shared. Sucks when it is but that is life.
Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions
In reply to Tejun HeoQuoted 9 lines · Tejun Heo
drivers/ata/libata-core.c void ata_data_xfer_noirq(struct ata_devi · line 4034
Quoted 2 lines · Albert Lee
drivers/ata/libata-core.c +1
-1-static void ata_pio_sector (struct ata_queued_cmd *qc,int last) +static void ata_pio_sector (struct ata_queued_cmd *qc,int last,int lock)I think the naming of @lock is a bit confusing here. @clr_hsm_wq or @last_sector, maybe?
How about "irq_handover"? When set to "true", it means the workqueue is going to handover the control of the port to the irq handler.
Quoted 16 lines
drivers/ata/libata-core.c static void ata_pio_sector(struct ata_qu · line 4083
Quoted 9 lines
drivers/ata/libata-core.c +8
-0+if (lock) { + tail =8 ; + head =ATA_SECT_SIZE - tail;/* multiple of 8 bytes */ + ap->ops->data_xfer (qc->dev, buf + offset, head, do_write); +spin_lock_irqsave (ap->lock, irq_flags); + } + + ap->ops->data_xfer (qc->dev, buf + offset + head, tail, do_write);Aieee, we have to transfer the whole last sector while holding the spin lock and IRQ disabled. That's sad but pushing locking into ->data_xfer doesn't sound attractive either. Any better ideas?
Why need to transfer the last sector as a whole?
Spliting it into 504 (unlocked) + 8 (holding ap->lock) works on my machine...
--
albert
Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions
Quoted 27 lines · Albert Lee
Quoted 8 lines · Tejun Heo
drivers/ata/libata-core.c void ata_data_xfer_noirq(struct ata_devi · line 4034
Quoted 2 lines · Albert Lee
drivers/ata/libata-core.c +1
-1-static void ata_pio_sector (struct ata_queued_cmd *qc,int last) +static void ata_pio_sector (struct ata_queued_cmd *qc,int last,int lock)I think the naming of @lock is a bit confusing here. @clr_hsm_wq or @last_sector, maybe?
How about "
irq_handover"? When set to "true", it means the workqueue is going to handover the control of the port to the irq handler.Quoted 4 lines
Quoted 4 lines
drivers/ata/libata-core.c +4
-0+if (lock) { + tail =8 ; + head =ATA_SECT_SIZE - tail;/* multiple of 8 bytes */ + ap->ops->data_xfer (qc->dev, buf + offset, head, do_write);Quoted 9 lines
drivers/ata/libata-core.c static void ata_pio_sector(struct ata_qu · line 4083
Quoted 4 lines
drivers/ata/libata-core.c +4
-0+spin_lock_irqsave (ap->lock, irq_flags); + } + + ap->ops->data_xfer (qc->dev, buf + offset + head, tail, do_write);Aieee, we have to transfer the whole last sector while holding the spin lock and IRQ disabled. That's sad but pushing locking into ->data_xfer doesn't sound attractive either. Any better ideas?
Quoted 2 lines
Why need to transfer the last sector as a whole?
Spliting it into 504 (unlocked) + 8 (holdingap->lock) works on my machine...
That would be the cleanest way to do it but I'm not sure whether all controllers can live with that. If I understand correctly, Some controllers just have to have the whole transfer done atomically. I think Alan knows much better about this. Alan?
Signature · 1 line
tejun
Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions
Quoted 4 lines
That would be the cleanest way to do it but I'm not sure whether all controllers can live with that. If I understand correctly, Some controllers just have to have the whole transfer done atomically. I think Alan knows much better about this. Alan?
Those controllers already do a local IRQ disable in their private ->data_xfer method so ought to be ok.
Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions
Quoted 7 lines · Alan Cox
Quoted 4 lines
That would be the cleanest way to do it but I'm not sure whether all controllers can live with that. If I understand correctly, Some controllers just have to have the whole transfer done atomically. I think Alan knows much better about this. Alan?
Those controllers already do a local IRQ disable in their private ->data_xfer method so ought to be ok.
I think what Albert is suggesting is...
if (last_sector) {
for (i = 0; i < WORDS_IN_SECTORS - 1; i++)
ops->data_xfer(word);
/* HERE */
spin_lock_irq(ap->lock);
ops->data_xfer(last word);
ap->pflags &= ~WQ_IN_PROGRESS_STAY_AWAY_IRQHNDL;
spin_unlock_irq(ap->lock);
}So, it basically splits the last sector into two transfers and nothing prevents the machine from taking an interrupt at HERE.
Am I understanding it correctly Albert?
Signature · 1 line
tejun
Re: [PATCH 5/7] libata: move and reduce locking to the pio data xfer functions
In reply to Albert LeeQuoted 10 lines · Albert Lee
Quoted 8 lines · Tejun Heo
drivers/ata/libata-core.c void ata_data_xfer_noirq(struct ata_devi · line 4034
Quoted 2 lines · Albert Lee
drivers/ata/libata-core.c +1
-1-static void ata_pio_sector (struct ata_queued_cmd *qc,int last) +static void ata_pio_sector (struct ata_queued_cmd *qc,int last,int lock)I think the naming of @lock is a bit confusing here. @clr_hsm_wq or @last_sector, maybe?
Quoted 2 lines
How about "
irq_handover"? When set to "true", it means the workqueue is going to handover the control of the port to the irq handler.
Oops, forgot to write about this. Yeah, that sounds good too. I just thought @lock was too vague.
Thanks.
Signature · 1 line
tejun
PATCH 6/7 libata: push part of the irq driven pio out to workqueue
In reply to Albert Leepatch 6/7:
Push part of the irq driven pio out to workqueue.
---
It seems this creates new race condition with ata_exec_internal_sg() and EH?
drivers/ata/libata-core.c +47 -18
@@ -4370,20 +4370,15 @@ err_out:static inline int ata_hsm_ok_in_wq (struct ata_port *ap,struct ata_queued_cmd *qc) { -if (qc->tf.flags &ATA_TFLAG_POLLING ) -return 1 ; - -if (ap->hsm_task_state ==HSM_ST_FIRST ) { -if (qc->tf.protocol ==ATA_PROT_PIO && - (qc->tf.flags &ATA_TFLAG_WRITE )) -return 1 ; - -if (is_atapi_taskfile (&qc->tf) && - !(qc->dev->flags &ATA_DFLAG_CDB_INTR )) -return 1 ; +switch (qc->tf.protocol) { +case ATA_PROT_DMA : +return 0 ; +case ATA_PROT_ATAPI_DMA : +if (ap->hsm_task_state !=HSM_ST_FIRST ) +return 0 ; } -return 0 ; +return (ap->pflags &ATA_PFLAG_HSM_WQ ) ?1 :0 ; }/**
@@ -4559,7 +4554,7 @@ fsm_start: goto fsm_start; } -atapi_pio_bytes (qc,0 ); +atapi_pio_bytes (qc, in_wq);if (unlikely (ap->hsm_task_state ==HSM_ST_ERR ))/* bad ireason reported by device */
@@ -4614,7 +4609,7 @@ fsm_start: goto fsm_start; } -ata_pio_sectors (qc,0 ); +ata_pio_sectors (qc, in_wq);if (ap->hsm_task_state ==HSM_ST_IDLE ) {/* all data read */
@@ -4713,6 +4708,31 @@ fsm_start: goto fsm_start; } +static void ata_irq_task (struct work_struct *work) +{ +struct ata_port *ap = + container_of(work, struct ata_port, port_task.work); +struct ata_queued_cmd *qc = ap->port_task_data; +u8 status; + +WARN_ON (ap->hsm_task_state ==HSM_ST_IDLE ); + +/* double check the device is not busy */ + status =ata_chk_status (ap); +if (status &ATA_BUSY ) { +ata_port_printk (ap,KERN_ERR ,"Unexpected device busy " +"(Status 0x%x)\n" , status); +return ; + } + +/* move the HSM */ +ata_hsm_move (ap, qc, status,1 ); + +/* another command or interrupt handler +* may be running at this point. +*/ +} +/** * ata_qc_new - Request an available ATA command, for queueing * @ap: Port associated with device @dev
@@ -5250,11 +5270,20 @@ inline unsigned int ata_host_intr (struc/* ack bmdma irq events */ ap->ops->irq_clear (ap); -ata_hsm_move (ap, qc, status,0 ); +/* invoke HSM */ +switch (ap->hsm_task_state) { +case HSM_ST_FIRST : +case HSM_ST : + ap->pflags |=ATA_PFLAG_HSM_WQ ; +ata_port_queue_task (ap, ata_irq_task, qc,0 ); +break ; +default : +ata_hsm_move (ap, qc, status,0 ); -if (unlikely (qc->err_mask) && (qc->tf.protocol ==ATA_PROT_DMA || - qc->tf.protocol ==ATA_PROT_ATAPI_DMA )) -ata_ehi_push_desc (ehi,"BMDMA stat 0x%x" , host_stat); +if (unlikely (qc->err_mask) && (qc->tf.protocol ==ATA_PROT_DMA || + qc->tf.protocol ==ATA_PROT_ATAPI_DMA )) +ata_ehi_push_desc (ehi,"BMDMA stat 0x%x" , host_stat); + }return 1 ;/* irq handled */
PATCH 7/7 libata: ack unexpected INTRQ when polling
In reply to Albert Leepatch 7/7:
ack unexpected INTRQ when polling.
(Some device asserts INTRQ even if polling and nIEN = 1.
http://bugzilla.kernel.org/show_bug.cgi?id=8441)
---
drivers/ata/libata-core.c +5 -2
@@ -5211,9 +5211,12 @@ inline unsigned int ata_host_intr (strucif (ap->pflags &ATA_PFLAG_HSM_WQ ) goto idle_irq; -/* polling */ -if (qc->tf.flags &ATA_TFLAG_POLLING ) +/* polling, while HSM not yet active in wq */ +if (qc->tf.flags &ATA_TFLAG_POLLING ) { +/* ack random irq */ +ata_chk_status (ap); goto idle_irq; + }/* Check whether we are expecting interrupt in this state */ switch (ap->hsm_task_state) {
Quoted 6 lines
This reminds me of a possible issue with the patch:
Previously the polling code assumes that the interrupt handler won't interfere with it and the polling code runs without holdingap->lock. However, with the above patch, the interrupt handler might read the Status register when the polling code is transfering data, etc. from the port. Would this cause trouble to the ATA/ATAPI devices?
Oh yes, and also some controllers will corrupt their data stream if you do this during a PIO transfer of a block of ATA data.
Alan
Quoted 9 lines · Alan Cox
Quoted 6 lines
This reminds me of a possible issue with the patch:
Previously the polling code assumes that the interrupt handler won't interfere with it and the polling code runs without holdingap->lock. However, with the above patch, the interrupt handler might read the Status register when the polling code is transfering data, etc. from the port. Would this cause trouble to the ATA/ATAPI devices?Oh yes, and also some controllers will corrupt their data stream if you do this during a PIO transfer of a block of ATA data.
I also believe that to be true, but don't know exactly which hardware has that issue.
On the other hand, if we don't read the status from the IRQ handler, then how does the IRQ get cleared ????
This whole thing is quite messy.
Quoted 5 lines
Quoted 2 lines
Oh yes, and also some controllers will corrupt their data stream if you do this during a PIO transfer of a block of ATA data.
I also believe that to be true, but don't know exactly which hardware has that issue.
Some stuff which needs to block local irqs during the transfer anyway (eg ancient VIA) which therefore doesn't raise that problem much if our locking is careful (right now we block local irq but we can spin_lock happily in these as they are almost always uniprocessor anyway)
There is also some intel stuff with this problem (PIIX4 errata 15, 440MX errata 13.