[PATCH] qla2xxx: spin_unlock_irq in IRQ context

[PATCH] qla2xxx: spin_unlock_irq in IRQ context

[Hisashi Hifumi]

Hi.

  The function qla2x00_ramp_up_queue_depth()  can be called in IRQ context.
  So spin_unlock_irq in qla2x00_ramp_up_queue_depth() is not appropriate.

Thanks.

Signed-off-by :Hisashi Hifumi <hifumi.hisashi@oss.ntt.co.jp>

--- linux-2.6.20-rc4.org/drivers/scsi/qla2xxx/qla_isr.c	2006-11-30 
06:57:37.000000000 +0900
+++ linux-2.6.20-rc4/drivers/scsi/qla2xxx/qla_isr.c	2007-01-09 
11:26:45.000000000 +0900
@@ -650,10 +650,18 @@ qla2x00_ramp_up_queue_depth(scsi_qla_hos
  	    fcport->last_queue_full + ql2xqfullrampup * HZ))
  		return;

-	spin_unlock_irq(&ha->hardware_lock);
+	if (in_irq())
+		spin_unlock(&ha->hardware_lock);
+	else
+		spin_unlock_irq(&ha->hardware_lock);
+
  	starget_for_each_device(sdev->sdev_target, fcport,
  	    qla2x00_adjust_sdev_qdepth_up);
-	spin_lock_irq(&ha->hardware_lock);
+
+	if (in_irq())
+		spin_lock(&ha->hardware_lock);
+	else
+		spin_lock_irq(&ha->hardware_lock);
  }

  /** 

Re: [PATCH] qla2xxx: spin_unlock_irq in IRQ context

[James Bottomley]

On Tue, 2007-01-09 at 14:35 +0900, Hisashi Hifumi wrote:
> -       spin_unlock_irq(&ha->hardware_lock);
> +       if (in_irq())
> +               spin_unlock(&ha->hardware_lock);
> +       else
> +               spin_unlock_irq(&ha->hardware_lock);
> + 

Really, no!

If the function can be called with interrupts disabled as well as
enabled, then the spin_lock_irq needs to become a spin_lock_irqsave()
which is the correct API for this case.

James

Re: [PATCH] qla2xxx: spin_unlock_irq in IRQ context

[Hisashi Hifumi]

Hi

 >If the function can be called with interrupts disabled as well as
 >enabled, then the spin_lock_irq needs to become a spin_lock_irqsave()
 >which is the correct API for this case.

I know that spin_lock_irqsave()/spin_unlock_irqrestore() is correct .
But in this case, many places need to be modified to use
spin_lock_irqsave()/spin_unlock_irqrestore().Because
spin_lock_irqsave()/spin_unlock_irqrestore() is not in the same function,
and it is needed to pass eflags.

So I use in_irq() to distinguish between IRQ and process context and
minimize modifications.

Thanks. 

Re: [PATCH] qla2xxx: spin_unlock_irq in IRQ context

[Matthew Wilcox]

On Tue, Jan 09, 2007 at 04:46:34PM +0900, Hisashi Hifumi wrote:
> Hi
> 
> >If the function can be called with interrupts disabled as well as
> >enabled, then the spin_lock_irq needs to become a spin_lock_irqsave()
> >which is the correct API for this case.
> 
> I know that spin_lock_irqsave()/spin_unlock_irqrestore() is correct .
> But in this case, many places need to be modified to use
> spin_lock_irqsave()/spin_unlock_irqrestore().Because
> spin_lock_irqsave()/spin_unlock_irqrestore() is not in the same function,
> and it is needed to pass eflags.
> 
> So I use in_irq() to distinguish between IRQ and process context and
> minimize modifications.

Why do we need to re-enable interrupts here?  Can't we just
spin_unlock() / spin_lock()?  Actually, I don't see why we need to drop
the lock here ... starget_for_each_device can be called under a
spinlock.  qla2x00_adjust_sdev_qdepth_up() only calls
scsi_adjust_queue_depth().  Is there an AB-BA issue with hardware_lock
and request_queue->queue_lock?