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

[Tejun Heo <htejun@gmail.com>]

Hello, Albert.

Just a few comments.

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

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

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

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

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

Thanks.

-- 
tejun