On Mon, 2011-06-27 at 09:31 -0700, Roland Dreier wrote:
> From: Roland Dreier <roland@xxxxxxxxxxxxxxx>
>
> Trying to drop hardware_lock in qla24xx_send_cmd_to_target() is not
> safe, because sometimes we are in process context and sometimes we are
> called from the qla2xxx interrupt handler, and so we don't know
> whether to enable irqs or not. This leads to lockdep warnings among
> other problems.
>
> The simplest fix seems to be to just keep hardware_lock held when
> calling ->handle_cmd(). Then when tcm_qla2xxx_handle_cmd() knows that
> it is going to call back into qla2xxx, it can simply clear locked_rsp
> unconditionally, getting rid of the unsafe spin_is_locked() test
> (unsafe because spin_is_locked() could return true when _some other_
> context is holding the lock).
>
> This also makes the locking for ->handle_cmd the same between
> qla24xx_send_cmd_to_target() and qla2xxx_send_cmd_to_target() (since
> the qla2xxx_ version never dropped the lock).
>
> Signed-off-by: Roland Dreier <roland@xxxxxxxxxxxxxxx>
> ---
> Nick, I've been running this for a few days without any problems, and
> in addition to working in practice it seems to be the sanest way to
> handle the locking here.
>
Hey Roland,
This one was merged as well into master and lio-4.1-2.6.39.. I've still
be running w/o this patch on lio-4.0 and backports for some time without
problem in order to address one particular raw block flash driver that
seems to have problem with this held..
I will be getting this hardware back before long to debug, but I think
this particular patch is going to at least require some more target core
changes IIRC for their special driver..
Andrew, do you have any thoughts on this..?
Thanks,
--nab
> drivers/scsi/qla2xxx/qla_target.c | 2 --
> drivers/target/tcm_qla2xxx/tcm_qla2xxx_fabric.c | 7 ++++---
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> index e332e59..37b50d3 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -3170,10 +3170,8 @@ static int qla24xx_send_cmd_to_target(struct scsi_qla_host *vha, struct qla_tgt_
> /*
> * Dispatch command to tcm_qla2xxx fabric module code
> */
> - spin_unlock_irq(&vha->hw->hardware_lock);
> ret = vha->hw->qla2x_tmpl->handle_cmd(vha, cmd, unpacked_lun, data_length,
> fcp_task_attr, data_dir, bidi);
> - spin_lock_irq(&vha->hw->hardware_lock);
> return ret;
> }
>
> diff --git a/drivers/target/tcm_qla2xxx/tcm_qla2xxx_fabric.c b/drivers/target/tcm_qla2xxx/tcm_qla2xxx_fabric.c
> index 5f4e1bf..b711361 100644
> --- a/drivers/target/tcm_qla2xxx/tcm_qla2xxx_fabric.c
> +++ b/drivers/target/tcm_qla2xxx/tcm_qla2xxx_fabric.c
> @@ -609,10 +609,11 @@ int tcm_qla2xxx_handle_cmd(scsi_qla_host_t *vha, struct qla_tgt_cmd *cmd,
> if (transport_lookup_cmd_lun(se_cmd, lun) < 0) {
> /*
> * Clear qla_tgt_cmd->locked_rsp as ha->hardware_lock
> - * is already held here..
> + * is already held here, and we'll end up calling back
> + * into ->queue_status (tcm_qla2xxx_queue_status())
> + * and hence qla2xxx_xmit_response().
> */
> - if (spin_is_locked(&cmd->vha->hw->hardware_lock))
> - cmd->locked_rsp = 0;
> + cmd->locked_rsp = 0;
>
> /* NON_EXISTENT_LUN */
> transport_send_check_condition_and_sense(se_cmd,
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
