[PATCH] ata_port_wait_eh(): Change irqsave into unconditional closing of interrupts

[PATCH] ata_port_wait_eh(): Change irqsave into unconditional closing of interrupts

[Kirill Tkhai]

Hi,

ata_port_wait_eh() uses spin_lock_irqsave() and this can confuse in fact
that it is suitable to use in irqs_disabled() context. But we can't.

(schedule() returns with interrupts enabled so it's possible
ata_port_wait_eh() enters with disabled interrupts but returns with enabled)

So, replace irqsave to unconditional closing of interrupts.

I propose to consider to add patch like this. (If you don't have a magic
with flags which is not obvious for me :)

Signed-off-by: Kirill Tkhai <tkhai@yandex.ru>
---
 drivers/ata/libata-eh.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index c69fcce..502c1f9 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -841,21 +841,20 @@ EXPORT_SYMBOL_GPL(ata_scsi_port_error_handler);
  */
 void ata_port_wait_eh(struct ata_port *ap)
 {
-	unsigned long flags;
 	DEFINE_WAIT(wait);
 
  retry:
-	spin_lock_irqsave(ap->lock, flags);
+	spin_lock_irq(ap->lock);
 
 	while (ap->pflags & (ATA_PFLAG_EH_PENDING | ATA_PFLAG_EH_IN_PROGRESS)) {
 		prepare_to_wait(&ap->eh_wait_q, &wait, TASK_UNINTERRUPTIBLE);
-		spin_unlock_irqrestore(ap->lock, flags);
+		spin_unlock_irq(ap->lock);
 		schedule();
-		spin_lock_irqsave(ap->lock, flags);
+		spin_lock_irq(ap->lock);
 	}
 	finish_wait(&ap->eh_wait_q, &wait);
 
-	spin_unlock_irqrestore(ap->lock, flags);
+	spin_unlock_irq(ap->lock);
 
 	/* make sure SCSI EH is complete */
 	if (scsi_host_in_recovery(ap->scsi_host)) {

Re: [PATCH] ata_port_wait_eh(): Change irqsave into unconditional closing of interrupts

[Tejun Heo]

Hello, Kirill.

On Thu, Sep 26, 2013 at 05:21:19PM +0400, Kirill Tkhai wrote:
> Hi,
> 
> ata_port_wait_eh() uses spin_lock_irqsave() and this can confuse in fact
> that it is suitable to use in irqs_disabled() context. But we can't.
> 
> (schedule() returns with interrupts enabled so it's possible
> ata_port_wait_eh() enters with disabled interrupts but returns with enabled)
> 
> So, replace irqsave to unconditional closing of interrupts.
> 
> I propose to consider to add patch like this. (If you don't have a magic
> with flags which is not obvious for me :)

Hmmm... yeah, this was Jeff's preference, at least way back, so libata
has a lot of spin_lock_irqsave()'s where spin_lock_irq() should do.
At this point, I don't really mind either way but if you wanna change
it can you please do a full sweep through libata?  While I don't mind
either state too much, I do want them to be mostly consistent one way
or the other.

Thanks.

-- 
tejun

Re: [PATCH] ata_port_wait_eh(): Change irqsave into unconditional closing of interrupts

[Kirill Tkhai]

26.09.2013, 17:32, "Tejun Heo" <tj@kernel.org>:
> Hello, Kirill.
>
> On Thu, Sep 26, 2013 at 05:21:19PM +0400, Kirill Tkhai wrote:
>
>>  Hi,
>>
>>  ata_port_wait_eh() uses spin_lock_irqsave() and this can confuse in fact
>>  that it is suitable to use in irqs_disabled() context. But we can't.
>>
>>  (schedule() returns with interrupts enabled so it's possible
>>  ata_port_wait_eh() enters with disabled interrupts but returns with enabled)
>>
>>  So, replace irqsave to unconditional closing of interrupts.
>>
>>  I propose to consider to add patch like this. (If you don't have a magic
>>  with flags which is not obvious for me :)
>
> Hmmm... yeah, this was Jeff's preference, at least way back, so libata
> has a lot of spin_lock_irqsave()'s where spin_lock_irq() should do.
> At this point, I don't really mind either way but if you wanna change
> it can you please do a full sweep through libata?  While I don't mind
> either state too much, I do want them to be mostly consistent one way
> or the other.

I don't exclude this, maybe, I do this in my spare time ;)

Kirill