Re: [PATCH 2/3] libata: add support for blk-iopoll

[Jens Axboe]

On Mon, Aug 10 2009, Jonathan Corbet wrote:
> Hey, Jens,
> 
> I'm a little slow in looking at this, hopefully it's not completely
> noise...
> 
> > @@ -2157,7 +2194,12 @@ static irqreturn_t ahci_interrupt(int irq, void *dev_instance)
> >  
> >  		ap = host->ports[i];
> >  		if (ap) {
> > -			ahci_port_intr(ap);
> > +			if (!blk_iopoll_enabled)
> > +				ahci_port_intr(ap);
> > +			else if (blk_iopoll_sched_prep(&ap->iopoll)) {
> > +				ap_irq_disable(ap);
> > +				blk_iopoll_sched(&ap->iopoll);
> > +			}
> >  			VPRINTK("port %u\n", i);
> >  		} else {
> >  			VPRINTK("port %u (no irq)\n", i);
> 
> It seems to me that, if blk_iopoll_sched_prep() fails, the interrupt
> will be dropped on the floor; would you not need an explicit
> ahci_port_intr() call in that case too?  Unless I've misunderstood as
> usual...

If that happens, it is probably a spurious IRQ since it's already
scheduled to run (and hasn't yet). So it should be fine, in reality it
should not happen since the IRQ should have been acked and the iopoll
handler scheduled.

> Documenting the "zero means failure" nature of blk_iopoll_sched_prep()
> might also be a good idea; I predict confusion otherwise.

There's no real failure case, it zero just means "already scheduled".
But we do usually use 0 as the "normal" case, so good point anyway. I'll
change it.

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/