| Message ID | 1491380201679@web5j.yandex.ru |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)) {
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(-) -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html