Re: [ANNOUNCE]: Broadcom (Emulex) FC Target driver - efct

[Roland Dreier]

On Sun, Mar 5, 2017 at 8:35 AM, Sebastian Herbszt  wrote:
> Just like Hannes I do favour integration. I guess it could be
> comparable to qla2xxx + tcm_qla2xxx, lpfc + lpfc_scst and
> lpfc + tcm_lpfc. That approach might even help Bart with his
> target driver unification if he didn't give up on that topic.

Resurrecting this old topic - sorry for not seeing this go by initially.

For context, I have a lot of experience debugging the qla2xxx target
code - we did a lot of work to get error/exception paths correct.
Basic FC target support is pretty straightforward but handling SAN log
in / log out events and other strange things that initiators do took a
lot of effort.

Anyway, my feeling is that the integration of tcm_qla2xxx and qla2xxx
was overall a net negative.  Having the target driver grafted onto the
side of an already-complex driver that has a bunch of code not
relevant to the target (SCSI error handling, logging into and timing
out remote target ports, etc) made the code harder to debug and harder
to get right.

Of course I'm in favor of making common code really common.  So
certainly we should have a common library of SLI-4 code that both the
initiator and target driver share.  And if there is more commonality,
that's great.  But any code similar to "if (initiator) ... else ..."
is really suspect to me - grepping for "qla_ini_mode_enabled" shows
great examples like

if (fcport->scan_state == QLA_FCPORT_SCAN) {
if ((qla_dual_mode_enabled(vha) ||
qla_ini_mode_enabled(vha)) &&
atomic_read(>state) == FCS_ONLINE) {
qla2x00_mark_device_lost(vha, fcport,
ql2xplogiabsentdevice, 0);
if (fcport->loop_id != FC_NO_LOOP_ID &&
(fcport->flags & FCF_FCP2_DEVICE) == 0 &&
fcport->port_type != FCT_INITIATOR &&
fcport->port_type != FCT_BROADCAST) {
ql_dbg(ql_dbg_disc, vha, 0x,
"%s %d %8phC post del sess\n",
__func__, __LINE__,
fcport->port_name);

qlt_schedule_sess_for_deletion_lock
(fcport);
continue;
}
}
}

Handling "dual mode" (both initiator and target on the same port at
the same time) is a design challenge, but I don't think the current
qla2xxx driver is an example of a maintainable way to do that.

(I'm agnostic about what to do about SLI-3 - perhaps the cleanest
thing to do is split the driver between SLI-4 and SLI-3, and handle
the initiator and target drivers for those two cases as appropriate)

I'd love to discuss this further and come up with a design that meets
the concerns about integration but also learns the lessons from
tcm_qla2xxx.

 - R.

Re: [PATCH v2 1/8] qla2xxx: kill sessions/log out initiator on RSCN and port down events

[Roland Dreier]

On Mon, Jul 27, 2015 at 1:09 AM, Hannes Reinecke h...@suse.de wrote:
 Hmm? What happened to the original FIXME?
 Is it not required anymore?

Not sure I follow the question.  The original FIXME was /* FIXME:
Re-enable Global event handling.. */ and this patch indeed re-enables
global event handling.  I don't think it's a separate patch, it's all
part of the same work.

Unless I'm misunderstanding...
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] target/iscsi: fix digest computation for chained SGs

[Roland Dreier]

On Tue, Jul 21, 2015 at 1:57 AM, Sagi Grimberg sa...@dev.mellanox.co.il wrote:
 How were you able to get a chained SG list in the target code?

Local hack.  So this bug can't be hit in current mainline code, but
patch improves the code and removes a hidden booby-trap, so I think it
makes sense to apply.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/8] IB/srp: Add multichannel support

[Roland Dreier]

On Mon, Oct 6, 2014 at 4:16 AM, Bart Van Assche bvanass...@acm.org wrote:
 On 10/02/14 19:30, Christoph Hellwig wrote:
 Also if we want to merge scsi LLDDs that can take advantage of
 multiqueue support it would probably be best if I take this via the SCSI
 tree.

 Sending these patches to you is fine with me, at least if Roland agrees.

That's fine with me.  Christoph/Bart should I just let you guys handle
all the pending SRP patches, or is there anything I should pick up via
the InfiniBand tree?

Everything that's in flight looks reasonable to me, so I'm fine with
however you guys want to merge it.

 - R.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] Include protection information in iscsi header

[Roland Dreier]

On Sun, Jun 1, 2014 at 9:19 AM, Sagi Grimberg sa...@mellanox.com wrote:
 Although these patches involve 3 subsystems with different
 maintainers (scsi, iser, target) I would prefer seeing these
 patches included together.

Why?  Because they break wire compatibility?

I hate to say it but even if they're merged at the same time, you
can't guarantee that targets and initiators will be updated together.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] target: Fix missing length check in spc_emulate_evpd_83()

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

Commit fbfe858fea2a (target_core_spc: Include target device
descriptor in VPD page 83) added a new length variable, but (due to a
cut and paste mistake?) just checks scsi_name_len against 256 twice.
Fix this to check scsi_target_len for overflow too.

Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/target/target_core_spc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index 43c5ca9878bc..3bebc71ea033 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -440,8 +440,8 @@ check_scsi_name:
padding = ((-scsi_target_len)  3);
if (padding)
scsi_target_len += padding;
-   if (scsi_name_len  256)
-   scsi_name_len = 256;
+   if (scsi_target_len  256)
+   scsi_target_len = 256;
 
buf[off-1] = scsi_target_len;
off += scsi_target_len;
-- 
1.9.rc1

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] target: Simplify command completion by removing CMD_T_FAILED flag

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

The CMD_T_FAILED flag is set used in one place to record the result of a
trivial test, and it is only tested once, few lines later.  We might as
well make the code simpler and easier to read by directly doing the test
of success where we want to use it.

Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/target/target_core_transport.c | 5 +
 include/target/target_core_base.h  | 1 -
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index c50fd9f11aab..24b4f65d8777 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -669,9 +669,6 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
return;
}
 
-   if (!success)
-   cmd-transport_state |= CMD_T_FAILED;
-
/*
 * Check for case where an explicit ABORT_TASK has been received
 * and transport_wait_for_tasks() will be waiting for completion..
@@ -681,7 +678,7 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
spin_unlock_irqrestore(cmd-t_state_lock, flags);
complete(cmd-t_transport_stop_comp);
return;
-   } else if (cmd-transport_state  CMD_T_FAILED) {
+   } else if (!success) {
INIT_WORK(cmd-work, target_complete_failure_work);
} else {
INIT_WORK(cmd-work, target_complete_ok_work);
diff --git a/include/target/target_core_base.h 
b/include/target/target_core_base.h
index c9c791209cd1..1772fadcff62 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -525,7 +525,6 @@ struct se_cmd {
 #define CMD_T_COMPLETE (1  2)
 #define CMD_T_SENT (1  4)
 #define CMD_T_STOP (1  5)
-#define CMD_T_FAILED   (1  6)
 #define CMD_T_DEV_ACTIVE   (1  7)
 #define CMD_T_REQUEST_STOP (1  8)
 #define CMD_T_BUSY (1  9)
-- 
1.9.rc1

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] qla2xxx: Remove last vestiges of qla_tgt_cmd.cmd_list

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

The only place this struct member is touched is in one INIT_LIST_HEAD.

Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/scsi/qla2xxx/qla_target.c | 2 --
 drivers/scsi/qla2xxx/qla_target.h | 1 -
 2 files changed, 3 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c 
b/drivers/scsi/qla2xxx/qla_target.c
index 38a1257e76e1..2f42b650367c 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -2593,8 +2593,6 @@ static int qlt_handle_cmd_for_atio(struct scsi_qla_host 
*vha,
return -ENOMEM;
}
 
-   INIT_LIST_HEAD(cmd-cmd_list);
-
memcpy(cmd-atio, atio, sizeof(*atio));
cmd-state = QLA_TGT_STATE_NEW;
cmd-tgt = ha-tgt.qla_tgt;
diff --git a/drivers/scsi/qla2xxx/qla_target.h 
b/drivers/scsi/qla2xxx/qla_target.h
index b33e411f28a0..f4a4beee2b96 100644
--- a/drivers/scsi/qla2xxx/qla_target.h
+++ b/drivers/scsi/qla2xxx/qla_target.h
@@ -855,7 +855,6 @@ struct qla_tgt_cmd {
uint16_t loop_id;   /* to save extra sess dereferences */
struct qla_tgt *tgt;/* to save extra sess dereferences */
struct scsi_qla_host *vha;
-   struct list_head cmd_list;
 
struct atio_from_isp atio;
 };
-- 
1.9.rc1

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] target: Return an error for WRITE SAME with ANCHOR==1

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

Per SBC-3, since we report ANC_SUP==0 in VPD page B2h, we need to return
an error (ILLEGAL REQUEST/INVALID FIELD IN CDB) for all WRITE SAME
requests with ANCHOR==1.

Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/target/target_core_sbc.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 4714c6f8da4b..d9b92b2c524d 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -263,6 +263,11 @@ sbc_setup_write_same(struct se_cmd *cmd, unsigned char 
*flags, struct sbc_ops *o
sectors, cmd-se_dev-dev_attrib.max_write_same_len);
return TCM_INVALID_CDB_FIELD;
}
+   /* We always have ANC_SUP == 0 so setting ANCHOR is always an error */
+   if (flags[0]  0x10) {
+   pr_warn(WRITE SAME with ANCHOR not supported\n);
+   return TCM_INVALID_CDB_FIELD;
+   }
/*
 * Special case for WRITE_SAME w/ UNMAP=1 that ends up getting
 * translated into block discard requests within backend code.
-- 
1.8.3.2

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction

[Roland Dreier]

On Tue, Aug 20, 2013 at 1:08 PM, Nicholas A. Bellinger
n...@daterainc.com wrote:
 Add a special case for COMPARE_AND_WRITE for the reverse data direction
 mapping used for pci_map_sg() + friends.

I don't understand this.  In fact the whole patch series looks quite
confused.  COMPARE AND WRITE is a normal Data-Out command, with no
requirement for special bidirectional handling or anything like that.
The only slightly unusual thing is that a CAW command with a NUMBER OF
LOGICAL BLOCKS equal to N will actually transfer 2*N worth of data --
one set of data for the compare operation and a second set to write if
the compare succeeds.  But just to be clear, the transfer of those 2*N
blocks happens as a single transfer during the Data-Out phase.

 - R.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction

[Roland Dreier]

On Wed, Aug 21, 2013 at 7:38 AM, Roland Dreier rol...@purestorage.com wrote:
 I don't understand this.  In fact the whole patch series looks quite
 confused.  COMPARE AND WRITE is a normal Data-Out command, with no
 requirement for special bidirectional handling or anything like that.
 The only slightly unusual thing is that a CAW command with a NUMBER OF
 LOGICAL BLOCKS equal to N will actually transfer 2*N worth of data --
 one set of data for the compare operation and a second set to write if
 the compare succeeds.  But just to be clear, the transfer of those 2*N
 blocks happens as a single transfer during the Data-Out phase.

OK, I understand the patch set a bit better.  You're using the bidi
infrastructure to have a place to stick the data that you internally
read to implement the compare, but then you end up having places like
this where you have to say, oh it's not really a bidi command, it's
just a compare and write.

Shouldn't there be a way to confine the COMPARE AND WRITE handling to
the actual implementation of that command?  Or maybe make the bidi
handling more generic so that this becomes clearer?

 - R.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] [SCSI] sg: Fix user memory corruption when SG_IO is interrupted by a signal

[Roland Dreier]

Jens / James, do you guys plan to send this to Linus for 3.11?
Triggering this bug is a bit esoteric but the impact is pretty nasty
(corrupting an unrelated process).

Thanks,
  Roland
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] [SCSI] sg: Fix user memory corruption when SG_IO is interrupted by a signal

[Roland Dreier]

On Wed, Aug 7, 2013 at 9:31 AM, Douglas Gilbert dgilb...@interlog.com wrote:
 So what kind of signal was leading to your stomping on the memory?
 Was it user generated or something like SIGIO, SIGPIPE or a RT signal?

It was sometimes SIGHUP (for reopening log files) and sometimes
SIGALARM (for various periodic things).

 To get around the SG_IO ioctl restart problem (for non idempotent
 SCSI commands) could we replace a -ERESTARTSYS return value
 with -EINTR ?

 As I noted in a previous post, for robust user space code using the
 SG_IO ioctl, masking signals during the IO may help.

Yes, absolutely.  But process A should be able to keep its memory
uncorrupted even if process B is coded wrong :)

 And what about bsg? Is it any better or worse than sg in the case
 of interrupted SG_IO ioctls? Apart from the interface (sg_io_hdr
 v3 versus v4) it should be a drop in replacement for sg.

As far as I can tell bsg looks much better w.r.t. signals -- I don't
see anywhere that it schedules work onto a workqueue or other kernel
thread, and it looks like the SG_IO ioctl there actually has nowhere
that checks for signals.  All sleeps will be uninterruptible, which I
guess may be better or worse depending on your perspective.

 - R.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] [SCSI] sg: Fix user memory corruption when SG_IO is interrupted by a signal

[Roland Dreier]

On Wed, Aug 7, 2013 at 7:38 AM, David Milburn dmilb...@redhat.com wrote:
 I was able to succesfully test this patch overnight, I had been experimenting 
 with the
 sg driver setting the BIO_NULL_MAPPED flag in sg_rq_end_io_usercontext for a 
 orphan process
 which prevented the corruption, but your solution seems much better.

Very cool, thanks for the testing.

I actually looked at using BIO_NULL_MAPPED as well, but it seemed a
bit too fragile to me -- it had the right effect of skipping
__bio_copy_iov(), and skipping the __free_pages() stuff in there is OK
because sg owns its pages rather than the bio layer, but all that
seemed vulnerable to being broken by an unrelated change.

Out of curiousity, were you already working on this bug?  Because if
you had fixed it a few weeks earlier we might not have spent so long
wondering WTF was stomping on the memory of one of our processes :)

 - R.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] [SCSI] sg: Fix user memory corruption when SG_IO is interrupted by a signal

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

There is a nasty bug in the SCSI SG_IO ioctl that in some circumstances
leads to one process writing data into the address space of some other
random unrelated process if the ioctl is interrupted by a signal.
What happens is the following:

 - A process issues an SG_IO ioctl with direction DXFER_FROM_DEV (ie the
   underlying SCSI command will transfer data from the SCSI device to
   the buffer provided in the ioctl)

 - Before the command finishes, a signal is sent to the process waiting
   in the ioctl.  This will end up waking up the sg_ioctl() code:

result = wait_event_interruptible(sfp-read_wait,
(srp_done(sfp, srp) || sdp-detached));

   but neither srp_done() nor sdp-detached is true, so we end up just
   setting srp-orphan and returning to userspace:

srp-orphan = 1;
write_unlock_irq(sfp-rq_list_lock);
return result;  /* -ERESTARTSYS because signal hit process */

   At this point the original process is done with the ioctl and
   blithely goes ahead handling the signal, reissuing the ioctl, etc.

 - Eventually, the SCSI command issued by the first ioctl finishes and
   ends up in sg_rq_end_io().  At the end of that function, we run through:

write_lock_irqsave(sfp-rq_list_lock, iflags);
if (unlikely(srp-orphan)) {
if (sfp-keep_orphan)
srp-sg_io_owned = 0;
else
done = 0;
}
srp-done = done;
write_unlock_irqrestore(sfp-rq_list_lock, iflags);

if (likely(done)) {
/* Now wake up any sg_read() that is waiting for this
 * packet.
 */
wake_up_interruptible(sfp-read_wait);
kill_fasync(sfp-async_qp, SIGPOLL, POLL_IN);
kref_put(sfp-f_ref, sg_remove_sfp);
} else {
INIT_WORK(srp-ew.work, sg_rq_end_io_usercontext);
schedule_work(srp-ew.work);
}

   Since srp-orphan *is* set, we set done to 0 (assuming the
   userspace app has not set keep_orphan via an SG_SET_KEEP_ORPHAN
   ioctl), and therefore we end up scheduling sg_rq_end_io_usercontext()
   to run in a workqueue.

 - In workqueue context we go through sg_rq_end_io_usercontext() -
   sg_finish_rem_req() - blk_rq_unmap_user() - ... -
   bio_uncopy_user() - __bio_copy_iov() - copy_to_user().

   The key point here is that we are doing copy_to_user() on a
   workqueue -- that is, we're on a kernel thread with current-mm
   equal to whatever random previous user process was scheduled before
   this kernel thread.  So we end up copying whatever data the SCSI
   command returned to the virtual address of the buffer passed into
   the original ioctl, but it's quite likely we do this copying into a
   different address space!

Fix this by telling sg_finish_rem_req() whether we're on a workqueue
or not, and if we are, calling a new function blk_rq_unmap_user_nocopy()
that does everything the original blk_rq_unmap_user() does except
calling copy_{to,from}_user().  This requires a few levels of plumbing
through a copy flag in the bio layer.

I also considered fixing this by having the sg code just set
BIO_NULL_MAPPED for bios that are unmapped from a workqueue, which
happens to work because the __free_page() part of __bio_copy_iov()
isn't needed for sg (because sg handles its own pages).  However, this
seems coincidental and fragile, so I preferred making the fix
explicit, at the cost of minor tweaks to the bio code.

Huge thanks to Costa Sapuntzakis co...@purestorage.com for the
original pointer to this bug in the sg code.

Signed-off-by: Roland Dreier rol...@purestorage.com
Cc: sta...@vger.kernel.org
---
 block/blk-map.c| 15 ---
 drivers/scsi/sg.c  | 19 ++-
 fs/bio.c   | 22 +++---
 include/linux/bio.h|  2 +-
 include/linux/blkdev.h | 11 ++-
 5 files changed, 44 insertions(+), 25 deletions(-)

diff --git a/block/blk-map.c b/block/blk-map.c
index 623e1cd..bd63201 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -25,7 +25,7 @@ int blk_rq_append_bio(struct request_queue *q, struct request 
*rq,
return 0;
 }
 
-static int __blk_rq_unmap_user(struct bio *bio)
+static int __blk_rq_unmap_user(struct bio *bio, bool copy)
 {
int ret = 0;
 
@@ -33,7 +33,7 @@ static int __blk_rq_unmap_user(struct bio *bio)
if (bio_flagged(bio, BIO_USER_MAPPED))
bio_unmap_user(bio);
else
-   ret = bio_uncopy_user(bio);
+   ret = bio_uncopy_user(bio, copy);
}
 
return ret;
@@ -80,7 +80,7 @@ static int __blk_rq_map_user(struct request_queue *q, struct 
request *rq,
 
/* if it was boucned we must call the end io function */
bio_endio(bio, 0);
-   __blk_rq_unmap_user

Re: [PATCH] [SCSI] sg: Fix user memory corruption when SG_IO is interrupted by a signal

[Roland Dreier]

On Mon, Aug 5, 2013 at 4:31 PM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
 I agree with the analysis.  The fix is a bit draconian, though.  A
 workqueue actually runs in a kernel thread and there's a simple test for
 that (!current-mm), so how about this instead (which is much less
 intrusive)

 ---

 diff --git a/fs/bio.c b/fs/bio.c
 index 94bbc04..e2ab39c 100644
 --- a/fs/bio.c
 +++ b/fs/bio.c
 @@ -1045,12 +1045,22 @@ static int __bio_copy_iov(struct bio *bio, struct 
 bio_vec *iovecs,
  int bio_uncopy_user(struct bio *bio)
  {
 struct bio_map_data *bmd = bio-bi_private;
 -   int ret = 0;
 +   struct bio_vec *bvec;
 +   int ret = 0, i;

 -   if (!bio_flagged(bio, BIO_NULL_MAPPED))
 -   ret = __bio_copy_iov(bio, bmd-iovecs, bmd-sgvecs,
 -bmd-nr_sgvecs, bio_data_dir(bio) == 
 READ,
 -0, bmd-is_our_pages);
 +   if (!bio_flagged(bio, BIO_NULL_MAPPED)) {
 +   /*
 +* if we're in a workqueue, the request is orphaned, so
 +* don't copy into the kernel address space, just free
 +*/
 +   if (current-mm)
 +   ret = __bio_copy_iov(bio, bmd-iovecs, bmd-sgvecs,
 +bmd-nr_sgvecs, 
 bio_data_dir(bio) == READ,
 +0, bmd-is_our_pages);
 +   else if (bmd-is_our_pages)
 +   bio_for_each_segment_all(bvec, bio, i)
 +   __free_page(bvec-bv_page);
 +   }
 bio_free_map_data(bmd);
 bio_put(bio);
 return ret;

Yes, looks reasonable -- I can't think of any reason why anyone would
ever want the bio code to copy to a random userspace address space.

Acked-by: Roland Dreier rol...@purestorage.com
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] [SCSI] sg: Fix user memory corruption when SG_IO is interrupted by a signal

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

There is a nasty bug in the SCSI SG_IO ioctl that in some circumstances
leads to one process writing data into the address space of some other
random unrelated process if the ioctl is interrupted by a signal.
What happens is the following:

 - A process issues an SG_IO ioctl with direction DXFER_FROM_DEV (ie the
   underlying SCSI command will transfer data from the SCSI device to
   the buffer provided in the ioctl)

 - Before the command finishes, a signal is sent to the process waiting
   in the ioctl.  This will end up waking up the sg_ioctl() code:

result = wait_event_interruptible(sfp-read_wait,
(srp_done(sfp, srp) || sdp-detached));

   but neither srp_done() nor sdp-detached is true, so we end up just
   setting srp-orphan and returning to userspace:

srp-orphan = 1;
write_unlock_irq(sfp-rq_list_lock);
return result;  /* -ERESTARTSYS because signal hit process */

   At this point the original process is done with the ioctl and
   blithely goes ahead handling the signal, reissuing the ioctl, etc.

 - Eventually, the SCSI command issued by the first ioctl finishes and
   ends up in sg_rq_end_io().  At the end of that function, we run through:

write_lock_irqsave(sfp-rq_list_lock, iflags);
if (unlikely(srp-orphan)) {
if (sfp-keep_orphan)
srp-sg_io_owned = 0;
else
done = 0;
}
srp-done = done;
write_unlock_irqrestore(sfp-rq_list_lock, iflags);

if (likely(done)) {
/* Now wake up any sg_read() that is waiting for this
 * packet.
 */
wake_up_interruptible(sfp-read_wait);
kill_fasync(sfp-async_qp, SIGPOLL, POLL_IN);
kref_put(sfp-f_ref, sg_remove_sfp);
} else {
INIT_WORK(srp-ew.work, sg_rq_end_io_usercontext);
schedule_work(srp-ew.work);
}

   Since srp-orphan *is* set, we set done to 0 (assuming the
   userspace app has not set keep_orphan via an SG_SET_KEEP_ORPHAN
   ioctl), and therefore we end up scheduling sg_rq_end_io_usercontext()
   to run in a workqueue.

 - In workqueue context we go through sg_rq_end_io_usercontext() -
   sg_finish_rem_req() - blk_rq_unmap_user() - ... -
   bio_uncopy_user() - __bio_copy_iov() - copy_to_user().

   The key point here is that we are doing copy_to_user() on a
   workqueue -- that is, we're on a kernel thread with current-mm
   equal to whatever random previous user process was scheduled before
   this kernel thread.  So we end up copying whatever data the SCSI
   command returned to the virtual address of the buffer passed into
   the original ioctl, but it's quite likely we do this copying into a
   different address space!

As suggested by James Bottomley james.bottom...@hansenpartnership.com,
add a check for current-mm (which is NULL if we're on a kernel thread
without a real userspace address space) in bio_uncopy_user(), and skip
the copy if we're on a kernel thread.

There's no reason that I can think of for any caller of bio_uncopy_user()
to want to do copying on a kernel thread with a random active userspace
address space.

Huge thanks to Costa Sapuntzakis co...@purestorage.com for the
original pointer to this bug in the sg code.

Signed-off-by: Roland Dreier rol...@purestorage.com
Cc: sta...@vger.kernel.org
---
 fs/bio.c | 20 +++-
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/fs/bio.c b/fs/bio.c
index 94bbc04..c5eae72 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -1045,12 +1045,22 @@ static int __bio_copy_iov(struct bio *bio, struct 
bio_vec *iovecs,
 int bio_uncopy_user(struct bio *bio)
 {
struct bio_map_data *bmd = bio-bi_private;
-   int ret = 0;
+   struct bio_vec *bvec;
+   int ret = 0, i;
 
-   if (!bio_flagged(bio, BIO_NULL_MAPPED))
-   ret = __bio_copy_iov(bio, bmd-iovecs, bmd-sgvecs,
-bmd-nr_sgvecs, bio_data_dir(bio) == READ,
-0, bmd-is_our_pages);
+   if (!bio_flagged(bio, BIO_NULL_MAPPED)) {
+   /*
+* if we're in a workqueue, the request is orphaned, so
+* don't copy into a random user address space, just free.
+*/
+   if (current-mm)
+   ret = __bio_copy_iov(bio, bmd-iovecs, bmd-sgvecs,
+bmd-nr_sgvecs, bio_data_dir(bio) 
== READ,
+0, bmd-is_our_pages);
+   else if (bmd-is_our_pages)
+   bio_for_each_segment_all(bvec, bio, i)
+   __free_page(bvec-bv_page);
+   }
bio_free_map_data(bmd);
bio_put(bio);
return ret;
-- 
1.8.3.2

--
To unsubscribe from

[PATCH] tcm_qla2xxx: Fix residual for underrun commands that fail

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

Suppose an initiator sends a DATA IN command with an allocation length
shorter than the FC transfer length -- we get a target message like

TARGET_CORE[qla2xxx]: Expected Transfer Length: 256 does not match SCSI CDB 
Length: 0 for SAM Opcode: 0x12

In that case, the target core adjusts the data_length and sets
se_cmd-residual_count for the underrun.  But now suppose that command
fails and we end up in tcm_qla2xxx_queue_status() -- that function
unconditionally overwrites residual_count with the already adjusted
data_length, and the initiator will burp with a message like

qla2xxx [:00:06.0]-301d:0: Dropped frame(s) detected (0x100 of 0x100 
bytes).

Fix this by adding on to the existing underflow residual count instead.

Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/scsi/qla2xxx/tcm_qla2xxx.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 7a3870f..66b0b26 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -688,8 +688,12 @@ static int tcm_qla2xxx_queue_status(struct se_cmd *se_cmd)
 * For FCP_READ with CHECK_CONDITION status, clear cmd-bufflen
 * for qla_tgt_xmit_response LLD code
 */
+   if (se_cmd-se_cmd_flags  SCF_OVERFLOW_BIT) {
+   se_cmd-se_cmd_flags = ~SCF_OVERFLOW_BIT;
+   se_cmd-residual_count = 0;
+   }
se_cmd-se_cmd_flags |= SCF_UNDERFLOW_BIT;
-   se_cmd-residual_count = se_cmd-data_length;
+   se_cmd-residual_count += se_cmd-data_length;
 
cmd-bufflen = 0;
}
-- 
1.8.1.2

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

SCSI error handling -- one error blocks the whole SCSI host

[Roland Dreier]

At LSF this year, we had a discussion about error handling and in
particular the problem that SCSI midlayer error handling waits for the
entire SCSI host (HBA) to quiesce before it starts to abort commands
etc.

James made the suggestion that FC should handle things the way SAS
does, because SAS has a strategy handler that does things the right
way.  However, now that I finally sit down and look at the code, I
don't see how this is the case.  It seems inherent in the way that
scsi_eh_scmd_add() and the thread in scsi_error_handler() work (in
particular the strategy handler can't even be called until host_failed
== host_busy; we don't bump host_failed without SHOST_RECOVERY set,
which stops queueing commands to any devices attached to the whole
HBA).

James, am I understanding your suggestion properly?  If so can you
explain what you meant about the libsas code -- I see that it has its
own strategy handler but as I said before we've already stopped every
device attached to the HBA before we ever get there.

To recapitulate the problem here, we might have a whole fabric
attached to an HBA via SAS or FC, and be doing 500K IOPS happily to 50
devices.  Then a single LUN goes wonky and all the IO stops while we
try to recover that single device, which might take minutes.

I know this has been discussed before, but can we find a way forward
here?  Is there some way we can start with per-device error recovery
and avoid disrupting IO that we can see is working fine?

Thanks,
  Roland
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [SCSI] Wake blockdev queue in scsi_internal_device_unblock() for SDEV_RUNNING

[Roland Dreier]

On Mon, Mar 11, 2013 at 11:08 AM, Mike Christie micha...@cs.wisc.edu wrote:
 If at the same time a SAS fabric event goes to the HBA, what can
 happen is the following:

  - mpt2sas calls _scsih_block_io_all_device() - 
 scsi_internal_device_block(sdev)

(In response to some HBA firmware event like 
 MPI2_EVENT_SAS_BROADCAST_PRIMITIVE)
Now sdev state is SDEV_BLOCK and blockdev queue has 
 QUEUE_FLAG_STOPPED set.

  - Someone like scsi_add_lun() calls scsi_device_set_state(sdev, 
 SDEV_RUNNING)

(SCSI bus scanning runs asynchronously to firmware event handling)
Now sdev state is SDEV_RUNNING but blockdev queue still has 
 QUEUE_FLAG_STOPPED set

 Is this a valid state change? Should it be failed in
 scsi_device_set_state when we try to go from SDEV_BLOCKED -
 SDEV_RUNNING? I am not sure if it make senses to ever have a device in
 SDEV_RUNNING but have the stopped queue bit set.

 It used to be that scsi_internal_device_unblock called
 scsi_device_set_state so the transition from SDEV_BLOCKED to
 SDEV_RUNNUNG had to be a valid transition. scsi_internal_device_unblock
 then started the queue. I am not sure if the sequence you were hitting
 was actually a transition that was intended or just worked by accident.

 Should we be failing the above call to scsi_device_set_state, and then
 the call to scsi_internal_device_unblock would work as expected.

Dunno ... I was a bit scared to go too deep into this sdev_state
stuff, since it looks very old and fragile.  With that said the big
question to me is who really owns the state -- is it the low-level
driver or the midlayer?  I don't quite understand what it means for
the midlayer to try and start a device if the low-level driver has
temporarily paused things.  However since we have the blockdev queue
stopped state also, it seems to make things work out OK.

 - R.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 01/11] iscsi-target: Add iscsit_transport API template

[Roland Dreier]

On Thu, Mar 7, 2013 at 10:02 PM, Nicholas A. Bellinger
n...@linux-iscsi.org wrote:
 Or and I discussed this point in the last status call, and given what
 the initiator did originally (eg: export iscsi_transport) he asked to
 keep it under drivers/infiniband/ulp/isert/ with the extra include bits.

 I'd have a slight preference to move iser-target code under
 drivers/target/iscsi/, and not put anything into include/target/iscsi/
 if there won't be another module that uses it..

 Do you have a preference here..?

It's not really something that matters to me.

 - R.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 01/11] iscsi-target: Add iscsit_transport API template

[Roland Dreier]

On Thu, Mar 7, 2013 at 5:45 PM, Nicholas A. Bellinger
n...@linux-iscsi.org wrote:
 +EXPORT_SYMBOL(iscsit_get_transport);

It's not clear to me why this needs to be exported.  Who would use it
outside the core iscsi target module?
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] [SCSI] Wake blockdev queue in scsi_internal_device_unblock() for SDEV_RUNNING

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

If a SCSI device's old state is already SDEV_RUNNING and we're moving
to the same SDEV_RUNNING state, still wake the blockdev queue in
scsi_internal_device_unblock().  This fixes a case where we silently
hang SCSI commands forever during device discovery.  One way this can
happen is when mpt2sas is discovering a reasonably big SAS topology,
and the sd driver has queued up a bunch of sd_probe_async() instances
that are queueing SCSI commands to various devices.

If at the same time a SAS fabric event goes to the HBA, what can
happen is the following:

 - mpt2sas calls _scsih_block_io_all_device() - 
scsi_internal_device_block(sdev)

   (In response to some HBA firmware event like 
MPI2_EVENT_SAS_BROADCAST_PRIMITIVE)
   Now sdev state is SDEV_BLOCK and blockdev queue has QUEUE_FLAG_STOPPED 
set.

 - Someone like scsi_add_lun() calls scsi_device_set_state(sdev, 
SDEV_RUNNING)

   (SCSI bus scanning runs asynchronously to firmware event handling)
   Now sdev state is SDEV_RUNNING but blockdev queue still has 
QUEUE_FLAG_STOPPED set

 - mpt2sas calls _scsih_ublock_io_all_device() - 
scsi_internal_device_unblock(sdev, SDEV_RUNNING)

   (Finishes handling the firmware event)

With the old scsi_lib code, scsi_internal_device_unblock() will return
an error at this point because the sdev state is already SDEV_RUNNING.
This means we skip the call to blk_start_queue() and never actually
start executing commands again.

Fix this by still going ahead and finishing scsi_internal_device_unblock()
even if the sdev state is already SDEV_RUNNING.

Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/scsi/scsi_lib.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 765398c..75108ea 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2495,7 +2495,9 @@ scsi_internal_device_unblock(struct scsi_device *sdev,
else
sdev-sdev_state = SDEV_CREATED;
} else if (sdev-sdev_state != SDEV_CANCEL 
-sdev-sdev_state != SDEV_OFFLINE)
+  sdev-sdev_state != SDEV_OFFLINE 
+  (sdev-sdev_state != SDEV_RUNNING ||
+   new_state != SDEV_RUNNING))
return -EINVAL;
 
spin_lock_irqsave(q-queue_lock, flags);
-- 
1.8.1.2

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] target: Fix error checking for UNMAP commands

[Roland Dreier]

On Sat, Feb 9, 2013 at 1:23 AM, Chris Boot bo...@bootc.net wrote:
 +   case TCM_PARAMETER_LIST_LENGTH_ERROR:
 +   /* CURRENT ERROR */
 +   buffer[0] = 0x70;
 +   buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
 +   /* ILLEGAL REQUEST */
 +   buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
 +   /* INVALID FIELD IN PARAMETER LIST */
 +   buffer[SPC_ASC_KEY_OFFSET] = 0x1a;
 +   break;
 case TCM_UNEXPECTED_UNSOLICITED_DATA:
 /* CURRENT ERROR */
 buffer[0] = 0x70;


 Nitpick: I suspect a simple copy  paste error; INVALID FIELD IN PARAMETER
 LIST in your comment should probably read PARAMETER LIST LENGTH ERROR
 instead.

Yes, the comment is a copy and paste error.  The ASC of 1Ah is correct for
PARAMETER LIST LENGTH ERROR.  Thanks  good catch.

Nic, want me to resend?

 - R.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] target: Fix sense data for out-of-bounds IO operations

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

We're supposed to return LOGICAL BLOCK ADDRESS OUT OF RANGE, not
INVALID FIELD IN CDB.

Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/target/target_core_sbc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index a664c66..170f1f7 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -486,7 +486,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 */
if (cmd-t_task_lba || sectors) {
if (sbc_check_valid_sectors(cmd)  0)
-   return TCM_INVALID_CDB_FIELD;
+   return TCM_ADDRESS_OUT_OF_RANGE;
}
cmd-execute_cmd = ops-execute_sync_cache;
break;
-- 
1.8.1.2

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] target: Fix error checking for UNMAP commands

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

SBC-3 (revision 35) says:

The PARAMETER LIST LENGTH field specifies the length in bytes of the
UNMAP parameter list that is available to be transferred from the
Data-Out Buffer. If the parameter list length is greater than zero
and less than 0008h (i.e., eight), then the device server shall
terminate the command with CHECK CONDITION status with the sense key
set to ILLEGAL REQUEST and the additional sense code set to
PARAMETER LIST LENGTH ERROR. A PARAMETER LIST LENGTH set to zero
specifies that no data shall be sent.

so our sense code for too-short descriptors was wrong, and we were
incorrectly failing commands that didn't transfer any descriptors.

While we're at it, also handle the UNMAP check:

If the ANCHOR bit is set to one, and the ANC_SUP bit in the Logical
Block Provisioning VPD page (see 6.6.4) is set to zero, then the
device server shall terminate the command with CHECK CONDITION
status with the sense key set to ILLEGAL REQUEST and the additional
sense code set to INVALID FIELD IN CDB.

Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/target/target_core_iblock.c| 11 ++-
 drivers/target/target_core_transport.c | 10 ++
 include/target/target_core_base.h  |  1 +
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_iblock.c 
b/drivers/target/target_core_iblock.c
index b526d23..b72ca5b 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -390,10 +390,19 @@ iblock_execute_unmap(struct se_cmd *cmd)
sense_reason_t ret = 0;
int dl, bd_dl, err;
 
+   /* We never set ANC_SUP */
+   if (cdb[1])
+   return TCM_INVALID_CDB_FIELD;
+
+   if (cmd-data_length == 0) {
+   target_complete_cmd(cmd, SAM_STAT_GOOD);
+   return 0;
+   }
+
if (cmd-data_length  8) {
pr_warn(UNMAP parameter list length %u too small\n,
cmd-data_length);
-   return TCM_INVALID_PARAMETER_LIST;
+   return TCM_PARAMETER_LIST_LENGTH_ERROR;
}
 
buf = transport_kmap_data_sg(cmd);
diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index bd587b7..82c4204 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1514,6 +1514,7 @@ void transport_generic_request_failure(struct se_cmd *cmd,
case TCM_UNSUPPORTED_SCSI_OPCODE:
case TCM_INVALID_CDB_FIELD:
case TCM_INVALID_PARAMETER_LIST:
+   case TCM_PARAMETER_LIST_LENGTH_ERROR:
case TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE:
case TCM_UNKNOWN_MODE_PAGE:
case TCM_WRITE_PROTECTED:
@@ -2674,6 +2675,15 @@ transport_send_check_condition_and_sense(struct se_cmd 
*cmd,
/* INVALID FIELD IN PARAMETER LIST */
buffer[SPC_ASC_KEY_OFFSET] = 0x26;
break;
+   case TCM_PARAMETER_LIST_LENGTH_ERROR:
+   /* CURRENT ERROR */
+   buffer[0] = 0x70;
+   buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
+   /* ILLEGAL REQUEST */
+   buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
+   /* INVALID FIELD IN PARAMETER LIST */
+   buffer[SPC_ASC_KEY_OFFSET] = 0x1a;
+   break;
case TCM_UNEXPECTED_UNSOLICITED_DATA:
/* CURRENT ERROR */
buffer[0] = 0x70;
diff --git a/include/target/target_core_base.h 
b/include/target/target_core_base.h
index 4fa0f10..37e3baa 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -193,6 +193,7 @@ enum tcm_sense_reason_table {
TCM_RESERVATION_CONFLICT= R(0x10),
TCM_ADDRESS_OUT_OF_RANGE= R(0x11),
TCM_OUT_OF_RESOURCES= R(0x12),
+   TCM_PARAMETER_LIST_LENGTH_ERROR = R(0x13),
 #undef R
 };
 
-- 
1.8.1.2

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] target: Fix parameter list length checking in MODE SELECT

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

An empty parameter list (length == 0) is not an error, so succeed MODE
SELECT in this case.  If the parameter list length is too small,
return the correct sense code of PARAMETER LIST LENGTH ERROR.

Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/target/target_core_spc.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index 2d88f08..73c5d53 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -983,6 +983,14 @@ static sense_reason_t spc_emulate_modeselect(struct se_cmd 
*cmd)
int ret = 0;
int i;
 
+   if (!cmd-data_length) {
+   target_complete_cmd(cmd, GOOD);
+   return 0;
+   }
+
+   if (cmd-data_length  off + 2)
+   return TCM_PARAMETER_LIST_LENGTH_ERROR;
+
buf = transport_kmap_data_sg(cmd);
if (!buf)
return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
@@ -1007,6 +1015,11 @@ static sense_reason_t spc_emulate_modeselect(struct 
se_cmd *cmd)
goto out;
 
 check_contents:
+   if (cmd-data_length  off + length) {
+   ret = TCM_PARAMETER_LIST_LENGTH_ERROR;
+   goto out;
+   }
+
if (memcmp(buf + off, tbuf, length))
ret = TCM_INVALID_PARAMETER_LIST;
 
-- 
1.8.1.2

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [LSF/MM TOPIC] Thin provisioning SOFT_THRESHOLD error handling

[Roland Dreier]

On Tue, Jan 29, 2013 at 12:14 AM, Hannes Reinecke h...@suse.de wrote:
 I would like to discuss at LSF the possible implementations
 and handling mechanism for this kind of failure scenarios.

I'd be interested in that discussion.  With my Pure hat on, our array
can generate these thin provisioning threshold unit attentions, and it
would be nice if Linux did something intelligent with them.

 - R.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [LSF/MM TOPIC] I_T Nexus loss SCSI error handling

[Roland Dreier]

On Mon, Jan 21, 2013 at 3:18 AM, Hannes Reinecke h...@suse.de wrote:
 The current error handler still uses a 'target reset' (or, rather, bus
 reset) strategy, although the respective TMF  has been obsoleted since
 SAM-3. SAM-5 defines an I_T nexus loss event instead, which so far has only
 been implemented in libsas.

 There has been some discussions on the mailing list, but so far there hasn't
 been any conclusion.

 So I would like to discuss a possible implementation of a I_T Nexus loss
 strategy and how to keep compability with SAM-2 targets.

Would this handle the case of a SAS fabric with lots of target
devices, where we start to get errors from a single device while IO is
proceeding fine to everything else on the fabric?  It would be really
nice if the Linux stack handled this without escalating to a target
reset and host reset, since that disrupts all the IO that is
proceeding fine (and which won't do much to fix a target drive that
might really be dead).

 - R.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iscsi-target: Fix bug in handling of ExpStatSN ACK during u32 wrap-around

[Roland Dreier]

On Wed, Dec 26, 2012 at 1:33 PM, Ben Hutchings b...@decadent.org.uk wrote:
   if (!(cmd-cmd_flags  ICF_OOO_CMDSN)  !cmd-immediate_cmd 
 -  (cmd-cmd_sn = conn-sess-exp_cmd_sn)) {
 +  iscsi_sna_gte(cmd-stat_sn, conn-sess-exp_cmd_sn)) {
   list_del(cmd-i_conn_node);
   spin_unlock_bh(conn-cmd_lock);
   iscsit_free_cmd(cmd);
 [...]

 This changes cmd-cmd_sn to cmd-stat_sn, but the commit message only
 describes fixes to wrap-around.  Is that another fix or a bug?

Great catch, thanks!  This is indeed a bug introduced by the patch.
I'll send out a fix; I guess you could roll it up into this patch, but
I'm not sure any of this stuff is worth getting into 3.2.

 - R.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] iscsi-target: Fix CmdSN comparison (use cmd-cmd_sn instead of cmd-stat_sn)

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

Commit 64c13330a389 (iscsi-target: Fix bug in handling of ExpStatSN
ACK during u32 wrap-around) introduced a bug where we compare the
wrong SN against our ExpCmdSN.

Reported-by: Ben Hutchings b...@decadent.org.uk
Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/target/iscsi/iscsi_target_erl2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/target/iscsi/iscsi_target_erl2.c 
b/drivers/target/iscsi/iscsi_target_erl2.c
index 9ac4c151..ba6091b 100644
--- a/drivers/target/iscsi/iscsi_target_erl2.c
+++ b/drivers/target/iscsi/iscsi_target_erl2.c
@@ -372,7 +372,7 @@ int iscsit_prepare_cmds_for_realligance(struct iscsi_conn 
*conn)
 * made generic here.
 */
if (!(cmd-cmd_flags  ICF_OOO_CMDSN)  !cmd-immediate_cmd 
-iscsi_sna_gte(cmd-stat_sn, conn-sess-exp_cmd_sn)) {
+iscsi_sna_gte(cmd-cmd_sn, conn-sess-exp_cmd_sn)) {
list_del(cmd-i_conn_node);
spin_unlock_bh(conn-cmd_lock);
iscsit_free_cmd(cmd);
-- 
1.8.0

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/5] target task management fixes and cleanups

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

Hi Nic,

A few fixes for TMR handling (fix crashes when a backend is really
slow, fix a reference leak if we get a TMR for a non-existent LUN) and
a couple of trivial cleanups in related code.

Roland Dreier (5):
  target: Don't let abort handling free pending write commands too soon
  target: Fix use-after-free in LUN RESET handling
  target: Release se_cmd when LUN lookup fails for TMR
  target: Remove useless if statement
  target: Remove never-used TMR_FABRIC_TMR enum value

 drivers/target/target_core_tmr.c   | 12 
 drivers/target/target_core_transport.c |  8 +---
 include/target/target_core_base.h  |  1 -
 3 files changed, 5 insertions(+), 16 deletions(-)

-- 
1.8.0

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] target: Release se_cmd when LUN lookup fails for TMR

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

When transport_lookup_tmr_lun() fails and we return a task management
response from target_complete_tmr_failure(), we need to call
transport_cmd_check_stop_to_fabric() to release the last ref to the
cmd after calling se_tfo-queue_tm_rsp(), or else we will never remove
the failed TMR from the session command list (and we'll end up waiting
forever when trying to tear down the session).

Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/target/target_core_transport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 49390d8..fe6208f 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1393,6 +1393,8 @@ static void target_complete_tmr_failure(struct 
work_struct *work)
 
se_cmd-se_tmr_req-response = TMR_LUN_DOES_NOT_EXIST;
se_cmd-se_tfo-queue_tm_rsp(se_cmd);
+
+   transport_cmd_check_stop_to_fabric(cmd);
 }
 
 /**
-- 
1.8.0

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] target: Don't let abort handling free pending write commands too soon

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

The following sequence happens for write commands (or any other
commands with a data out phase):

 - The transport calls target_submit_cmd(), which sets CMD_T_ACTIVE in
   cmd-transport_state and sets cmd-t_state to TRANSPORT_NEW_CMD.
 - Things go on transport_generic_new_cmd(), which notices that the
   command needs to transfer data, so it sets cmd-t_state to
   TRANSPORT_WRITE_PENDING and calls transport_cmd_check_stop().
 - transport_cmd_check_stop() clears CMD_T_ACTIVE in cmd-transport_state
   and returns in the normal case.
 - Then we continue on to call -se_tfo-write_pending().
 - The data comes back from the initiator, and the transport calls
   target_execute_cmd(), which sets cmd-t_state to TRANSPORT_PROCESSING
   and calls into the backend to actually write the data.

At this point, the backend might take a long time to complete the
command, since it has to do real IO.  If an abort request comes in for
this command at this point, it will not wait for the command to finish
since CMD_T_ACTIVE is not set.  Then when the command does finally
finish, we blow up with use-after-free.

Avoid this by setting CMD_T_ACTIVE in target_execute_cmd() so that
transport_wait_for_tasks() waits for the command to finish executing.
This matches the behavior from before commit 1389533ef944 (target:
remove transport_generic_handle_data), when data was signaled via
transport_generic_handle_data(), which set CMD_T_ACTIVE because it
called transport_add_cmd_to_queue().

Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/target/target_core_transport.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index c23c76c..1dd9d66 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1688,6 +1688,7 @@ void target_execute_cmd(struct se_cmd *cmd)
}
 
cmd-t_state = TRANSPORT_PROCESSING;
+   cmd-transport_state |= CMD_T_ACTIVE;
spin_unlock_irq(cmd-t_state_lock);
 
if (!target_handle_task_attr(cmd))
-- 
1.8.0

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5] target: Remove never-used TMR_FABRIC_TMR enum value

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

Signed-off-by: Roland Dreier rol...@purestorage.com
---
 include/target/target_core_base.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/target/target_core_base.h 
b/include/target/target_core_base.h
index 7cae236..02ed017 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -210,7 +210,6 @@ enum tcm_tmreq_table {
TMR_LUN_RESET   = 5,
TMR_TARGET_WARM_RESET   = 6,
TMR_TARGET_COLD_RESET   = 7,
-   TMR_FABRIC_TMR  = 255,
 };
 
 /* fabric independent task management response values */
-- 
1.8.0

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] target: Remove useless if statement

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

We do the same thing no matter which way the test goes, so just remove
the test and do what we're going to do.

The debug messages printed the wrong value of CMD_T_ACTIVE and don't
seem particularly useful, remove them too.

Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/target/target_core_tmr.c | 12 
 1 file changed, 12 deletions(-)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index c6e0293..d0b4dd9 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -331,18 +331,6 @@ static void core_tmr_drain_state_list(
 
fe_count = atomic_read(cmd-t_fe_count);
 
-   if (!(cmd-transport_state  CMD_T_ACTIVE)) {
-   pr_debug(LUN_RESET: got CMD_T_ACTIVE for
-cdb: %p, t_fe_count: %d dev: %p\n, cmd,
-   fe_count, dev);
-   cmd-transport_state |= CMD_T_ABORTED;
-   spin_unlock_irqrestore(cmd-t_state_lock, flags);
-
-   core_tmr_handle_tas_abort(tmr_nacl, cmd, tas, fe_count);
-   continue;
-   }
-   pr_debug(LUN_RESET: Got !CMD_T_ACTIVE for cdb: %p,
-t_fe_count: %d dev: %p\n, cmd, fe_count, dev);
cmd-transport_state |= CMD_T_ABORTED;
spin_unlock_irqrestore(cmd-t_state_lock, flags);
 
-- 
1.8.0

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/11] First pass at merging Bart's HA work

[Roland Dreier]

On Mon, Nov 26, 2012 at 8:04 PM, David Dillow dillo...@ornl.gov wrote:
 We can push it through James's tree if need be, but Bart's code is
 pretty self-contained, and going through the SCSI tree will introduce
 merge dependencies. It'd be much easier to push it all through the RDMA
 tree, especially if we want to get this landed for 3.8.

OK, I guess all the srp_transport stuff looks quite simple and good to
me, so I'm OK merging it.

Is there some subset of patches that you and Bart agree are good,
which I can pick up now?  I'd love to get at least some of the SRP
stuff into 3.8, and that window is opening pretty soon.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/11] First pass at merging Bart's HA work

[Roland Dreier]

  This series compiles, but is otherwise UNTESTED. I'll be working on that
  over the next few days, with an eye on getting as much of Bart's work
  into 3.8 as possible.

Hi Dave,

Great to have you back.  Certainly I'd like to get this stuff into 3.8 too.

A couple of comments:
 - I think the srp_transport stuff should go through linux-scsi / James B.
   instead of my tree, esp. since it's shared with the IBM vscsi stuff (I think)
 - I see Bart had a few comments about a few of your patches, I'll wait
   for you guys to hash that out.

Otherwise definitely happy to merge this for 3.8!

Thanks,
  Roland
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] UAPI: (Scripted) Disintegrate include/rdma

[Roland Dreier]

Thanks, applied.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/11] First pass at merging Bart's HA work

[Roland Dreier]

 I'm amenable to that, but we do need an agreed patch set, as Roland
 says.  I also hate to apply the pressure, but I suspect -rc7 was the
 last -rc, so I'm expecting the merge window to open on 2/12.

I think the srp_transport bits are all simple and non-controversial.
So at least from my perspective, OK to merge right now, except
that Dave mentioned he screwed up the attribution on one email,
but that should be easy to fix with a quick resend.

 - R.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/4] Make aborts work on tcm_qla2xxx, other cleanups

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

Hi Nic,

Here's a series that makes aborts actually work on qla2xxx.  Stopping
and releasing commands is quite convoluted so I'm not sure the first
patch is totally correct, but without it I can easily reproduce task
hangs or list corruption by having an initiator flood a tcm_qla2xxx
target with aborts.  With those fixes, Steve's patch is pretty
straightforward.

The last two patches are just cleanups I noticed while debugging this.

Just to be clear: to the extent that this is copyrightable work, it is
released exclusively under the GPL.  No permission is granted to
redistribute this under any other terms.

 - R.

Roland Dreier (3):
  target: Fix handling of aborted commands
  target: Clean up logic in transport_put_cmd()
  target: Clean up flow in transport_check_aborted_status()

Steve Hodgson (1):
  qla2xxx: Look up LUN for abort requests

 drivers/scsi/qla2xxx/qla_target.c  |   19 ++-
 drivers/target/target_core_transport.c |   40 ++--
 2 files changed, 36 insertions(+), 23 deletions(-)

-- 
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] target: Fix handling of aborted commands

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

- If we stop processing an already-aborted command in
  target_execute_cmd(), then we need to complete t_transport_stop_comp
  to wake up the the TMR handling thread, or else it will end up
  waiting forever.

- If we've a already sent an aborted status for a command in
  transport_check_aborted_status() then we should bail out of
  transport_send_task_abort() to avoid freeing the command twice.

Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/target/target_core_transport.c |6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 9097155..dcecbfb 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1819,8 +1819,10 @@ void target_execute_cmd(struct se_cmd *cmd)
/*
 * If the received CDB has aleady been aborted stop processing it here.
 */
-   if (transport_check_aborted_status(cmd, 1))
+   if (transport_check_aborted_status(cmd, 1)) {
+   complete(cmd-t_transport_stop_comp);
return;
+   }
 
/*
 * Determine if IOCTL context caller in requesting the stopping of this
@@ -3067,7 +3069,7 @@ void transport_send_task_abort(struct se_cmd *cmd)
unsigned long flags;
 
spin_lock_irqsave(cmd-t_state_lock, flags);
-   if (cmd-se_cmd_flags  SCF_SENT_CHECK_CONDITION) {
+   if (cmd-se_cmd_flags  (SCF_SENT_CHECK_CONDITION | 
SCF_SENT_DELAYED_TAS)) {
spin_unlock_irqrestore(cmd-t_state_lock, flags);
return;
}
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] qla2xxx: Look up LUN for abort requests

[Roland Dreier]

From: Steve Hodgson st...@purestorage.com

Search through the list of pending commands on the session list to find
the command the initiator is actually aborting, so that we can pass the
correct LUN to the core TMR handling code.

Signed-off-by: Steve Hodgson st...@purestorage.com
Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/scsi/qla2xxx/qla_target.c |   19 ++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c 
b/drivers/scsi/qla2xxx/qla_target.c
index 62aa558..79fbdd7 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -1264,9 +1264,26 @@ static int __qlt_24xx_handle_abts(struct scsi_qla_host 
*vha,
struct abts_recv_from_24xx *abts, struct qla_tgt_sess *sess)
 {
struct qla_hw_data *ha = vha-hw;
+   struct se_session *se_sess = sess-se_sess;
struct qla_tgt_mgmt_cmd *mcmd;
+   struct se_cmd *se_cmd;
+   u32 lun = 0;
int rc;
 
+   spin_lock(se_sess-sess_cmd_lock);
+   list_for_each_entry(se_cmd, se_sess-sess_cmd_list, se_cmd_list) {
+   struct qla_tgt_cmd *cmd =
+   container_of(se_cmd, struct qla_tgt_cmd, se_cmd);
+   if (cmd-tag == abts-exchange_addr_to_abort) {
+   lun = cmd-unpacked_lun;
+   break;
+   }
+   }
+   spin_unlock(se_sess-sess_cmd_lock);
+
+   if (!lun)
+   return -ENOENT;
+
ql_dbg(ql_dbg_tgt_mgt, vha, 0xf00f,
qla_target(%d): task abort (tag=%d)\n,
vha-vp_idx, abts-exchange_addr_to_abort);
@@ -1283,7 +1300,7 @@ static int __qlt_24xx_handle_abts(struct scsi_qla_host 
*vha,
mcmd-sess = sess;
memcpy(mcmd-orig_iocb.abts, abts, sizeof(mcmd-orig_iocb.abts));
 
-   rc = ha-tgt.tgt_ops-handle_tmr(mcmd, 0, TMR_ABORT_TASK,
+   rc = ha-tgt.tgt_ops-handle_tmr(mcmd, lun, TMR_ABORT_TASK,
abts-exchange_addr_to_abort);
if (rc != 0) {
ql_dbg(ql_dbg_tgt_mgt, vha, 0xf052,
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] target: Clean up logic in transport_put_cmd()

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

No need to have a goto where a return is clearer.

Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/target/target_core_transport.c |   10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index dcecbfb..0f29d70 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2183,9 +2183,10 @@ static void transport_put_cmd(struct se_cmd *cmd)
unsigned long flags;
 
spin_lock_irqsave(cmd-t_state_lock, flags);
-   if (atomic_read(cmd-t_fe_count)) {
-   if (!atomic_dec_and_test(cmd-t_fe_count))
-   goto out_busy;
+   if (atomic_read(cmd-t_fe_count) 
+   !atomic_dec_and_test(cmd-t_fe_count)) {
+   spin_unlock_irqrestore(cmd-t_state_lock, flags);
+   return;
}
 
if (cmd-transport_state  CMD_T_DEV_ACTIVE) {
@@ -2196,9 +2197,6 @@ static void transport_put_cmd(struct se_cmd *cmd)
 
transport_free_pages(cmd);
transport_release_cmd(cmd);
-   return;
-out_busy:
-   spin_unlock_irqrestore(cmd-t_state_lock, flags);
 }
 
 /*
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] target: Clean up flow in transport_check_aborted_status()

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/target/target_core_transport.c |   24 ++--
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 0f29d70..f225f90 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -3042,23 +3042,19 @@ EXPORT_SYMBOL(transport_send_check_condition_and_sense);
 
 int transport_check_aborted_status(struct se_cmd *cmd, int send_status)
 {
-   int ret = 0;
+   if (!(cmd-transport_state  CMD_T_ABORTED))
+   return 0;
 
-   if (cmd-transport_state  CMD_T_ABORTED) {
-   if (!send_status ||
-(cmd-se_cmd_flags  SCF_SENT_DELAYED_TAS))
-   return 1;
+   if (!send_status || (cmd-se_cmd_flags  SCF_SENT_DELAYED_TAS))
+   return 1;
 
-   pr_debug(Sending delayed SAM_STAT_TASK_ABORTED
-status for CDB: 0x%02x ITT: 0x%08x\n,
-   cmd-t_task_cdb[0],
-   cmd-se_tfo-get_task_tag(cmd));
+   pr_debug(Sending delayed SAM_STAT_TASK_ABORTED status for CDB: 0x%02x 
ITT: 0x%08x\n,
+cmd-t_task_cdb[0], cmd-se_tfo-get_task_tag(cmd));
 
-   cmd-se_cmd_flags |= SCF_SENT_DELAYED_TAS;
-   cmd-se_tfo-queue_status(cmd);
-   ret = 1;
-   }
-   return ret;
+   cmd-se_cmd_flags |= SCF_SENT_DELAYED_TAS;
+   cmd-se_tfo-queue_status(cmd);
+
+   return 1;
 }
 EXPORT_SYMBOL(transport_check_aborted_status);
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] Fix the qla2xxx loopback selftests

[Roland Dreier]

On Tue, Aug 14, 2012 at 2:15 PM, Chad Dupuis chad.dup...@qlogic.com wrote:


 On Tue, 14 Aug 2012, st...@purestorage.com wrote:

 From: Steve Hodgson st...@purestorage.com

 A few months ago our 2.6.39 based kernel started crashing almost 100%
 of the time when running the selftests, after seeminly unrelated kernel
 changes. In the end it was traced down to this use after free.

 Whilst here fix an error path memory leak.

 Thanks,

 Steve

 Steve Hodgson (2):
  qla2xxx: Fix use after free in qla2x000_process_loopback.
  qla2xxx: Free rsp_data even on error in qla2x00_process_loopback()

 drivers/scsi/qla2xxx/qla_bsg.c |   17 +
 1 file changed, 9 insertions(+), 8 deletions(-)



 Hi Steve, thanks for the patches. We'll look into them and let you know if
 there is anything extra needed.

Looking at the 3.7-rc code, it looks like the use-after-free is still there.
Any plans on merging these patches?

 - R.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] qla2xxx target fixes

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

Hi everyone, a couple of qla2xxx target fixes that we've been shipping
for a while at Pure and that I've finally got around to cleaning up
and merging to the latest kernel tree...

Roland Dreier (2):
  tcm_qla2xxx: Format VPD page 83h SCSI name string according to SPC
  qla2xxx: Update target lookup session tables when a target session
changes

 drivers/scsi/qla2xxx/qla_target.c  |   22 +--
 drivers/scsi/qla2xxx/qla_target.h  |1 +
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |   77 +++-
 drivers/scsi/qla2xxx/tcm_qla2xxx.h |2 +
 4 files changed, 89 insertions(+), 13 deletions(-)

-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] tcm_qla2xxx: Format VPD page 83h SCSI name string according to SPC

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

My draft of SPC-4 says the following about the SCSI name string in
inquiry VPD page 83h:

The SCSI NAME STRING field starts with either:

a) the four UTF-8 characters 'eui.' concatenated with 16, 24, or
   32 hexadecimal digits (i.e., the UTF-8 characters 0 through 9
   and A through F) for an EUI-64 based identifier (see
   7.8.6.5). The first hexadecimal digit shall be the most
   significant four bits of the first byte (i.e., most significant
   byte) of the EUI-64 based identifier;
b) the four UTF-8 characters 'naa.' concatenated with 16 or 32
   hexadecimal digits for an NAA identifier (see 7.8.6.6). The
   first hexadecimal digit shall be the most significant four bits
   of the first byte (i.e., most significant byte) of the NAA
   identifier; or
c) the four UTF-8 characters 'iqn.' concatenated with an iSCSI
   Name for an iSCSI-name based identifier (see iSCSI).

However, the .tpg_get_wwn method for tcm_qla2xxx formats the WWN so
the SCSI name string looks like 52:4a:93:7d:24:5f:b2:12,t,0x0001.
This patch corrects the code so that VPD 83h gives a SPC-compliant
SCSI name string like naa.524a937d245fb212,t,0x0001 while leavig
other uses alone (so configfs will still work with ':' separated WWNs).

Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |4 +++-
 drivers/scsi/qla2xxx/tcm_qla2xxx.h |2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 2358c16..d8211cc 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -237,7 +237,7 @@ static char *tcm_qla2xxx_get_fabric_wwn(struct 
se_portal_group *se_tpg)
struct tcm_qla2xxx_tpg, se_tpg);
struct tcm_qla2xxx_lport *lport = tpg-lport;
 
-   return lport-lport_name[0];
+   return lport-lport_naa_name;
 }
 
 static char *tcm_qla2xxx_npiv_get_fabric_wwn(struct se_portal_group *se_tpg)
@@ -1534,6 +1534,7 @@ static struct se_wwn *tcm_qla2xxx_make_lport(
lport-lport_wwpn = wwpn;
tcm_qla2xxx_format_wwn(lport-lport_name[0], TCM_QLA2XXX_NAMELEN,
wwpn);
+   sprintf(lport-lport_naa_name, naa.%016llx, (unsigned long long) 
wwpn);
 
ret = tcm_qla2xxx_init_lport(lport);
if (ret != 0)
@@ -1601,6 +1602,7 @@ static struct se_wwn *tcm_qla2xxx_npiv_make_lport(
lport-lport_npiv_wwnn = npiv_wwnn;
tcm_qla2xxx_npiv_format_wwn(lport-lport_npiv_name[0],
TCM_QLA2XXX_NAMELEN, npiv_wwpn, npiv_wwnn);
+   sprintf(lport-lport_naa_name, naa.%016llx, (unsigned long long) 
npiv_wwpn);
 
 /* FIXME: tcm_qla2xxx_npiv_make_lport */
ret = -ENOSYS;
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.h 
b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
index 8254981..9ba075f 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.h
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
@@ -61,6 +61,8 @@ struct tcm_qla2xxx_lport {
u64 lport_npiv_wwnn;
/* ASCII formatted WWPN for FC Target Lport */
char lport_name[TCM_QLA2XXX_NAMELEN];
+   /* ASCII formatted naa WWPN for VPD page 83 etc */
+   char lport_naa_name[TCM_QLA2XXX_NAMELEN];
/* ASCII formatted WWPN+WWNN for NPIV FC Target Lport */
char lport_npiv_name[TCM_QLA2XXX_NPIV_NAMELEN];
/* map for fc_port pointers in 24-bit FC Port ID space */
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] qla2xxx: Update target lookup session tables when a target session changes

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

It is possible for the target code to change the loop_id or s_id of a
target session in reaction to an FC fabric change.  However, the
session structures are stored in tables that are indexed by these two
keys, and if we just change the session structure but leave the
pointers to it in the old places in the table, havoc can ensue.  For
example, a new session might come along that should go in the old slot
in the table and overwrite the old session pointer.

To handle this, add a new tgt_ops-update_sess() method that also
updates the by loop_id and by s_id lookup tables when a session
changes, so that the keys where a session pointer is stored in these
tables always matches the keys in the session structure itself.

Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/scsi/qla2xxx/qla_target.c  |   22 +--
 drivers/scsi/qla2xxx/qla_target.h  |1 +
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |   73 
 3 files changed, 84 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c 
b/drivers/scsi/qla2xxx/qla_target.c
index 0e09d8f..556e941 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -557,6 +557,7 @@ static bool qlt_check_fcport_exist(struct scsi_qla_host 
*vha,
int pmap_len;
fc_port_t *fcport;
int global_resets;
+   unsigned long flags;
 
 retry:
global_resets = atomic_read(ha-tgt.qla_tgt-tgt_global_resets_count);
@@ -625,10 +626,10 @@ retry:
sess-s_id.b.area, sess-loop_id, fcport-d_id.b.domain,
fcport-d_id.b.al_pa, fcport-d_id.b.area, fcport-loop_id);
 
-   sess-s_id = fcport-d_id;
-   sess-loop_id = fcport-loop_id;
-   sess-conf_compl_supported = !!(fcport-flags 
-   FCF_CONF_COMP_SUPPORTED);
+   spin_lock_irqsave(ha-hardware_lock, flags);
+   ha-tgt.tgt_ops-update_sess(sess, fcport-d_id, fcport-loop_id,
+   !!(fcport-flags  FCF_CONF_COMP_SUPPORTED));
+   spin_unlock_irqrestore(ha-hardware_lock, flags);
 
res = true;
 
@@ -740,10 +741,9 @@ static struct qla_tgt_sess *qlt_create_sess(
qlt_undelete_sess(sess);
 
kref_get(sess-se_sess-sess_kref);
-   sess-s_id = fcport-d_id;
-   sess-loop_id = fcport-loop_id;
-   sess-conf_compl_supported = !!(fcport-flags 
-   FCF_CONF_COMP_SUPPORTED);
+   ha-tgt.tgt_ops-update_sess(sess, fcport-d_id, 
fcport-loop_id,
+   !!(fcport-flags  
FCF_CONF_COMP_SUPPORTED));
+
if (sess-local  !local)
sess-local = 0;
spin_unlock_irqrestore(ha-hardware_lock, flags);
@@ -869,10 +869,8 @@ void qlt_fc_port_added(struct scsi_qla_host *vha, 
fc_port_t *fcport)
ql_dbg(ql_dbg_tgt_mgt, vha, 0xf007,
Reappeared sess %p\n, sess);
}
-   sess-s_id = fcport-d_id;
-   sess-loop_id = fcport-loop_id;
-   sess-conf_compl_supported = !!(fcport-flags 
-   FCF_CONF_COMP_SUPPORTED);
+   ha-tgt.tgt_ops-update_sess(sess, fcport-d_id, 
fcport-loop_id,
+   !!(fcport-flags  
FCF_CONF_COMP_SUPPORTED));
}
 
if (sess  sess-local) {
diff --git a/drivers/scsi/qla2xxx/qla_target.h 
b/drivers/scsi/qla2xxx/qla_target.h
index 170af15..bad7495 100644
--- a/drivers/scsi/qla2xxx/qla_target.h
+++ b/drivers/scsi/qla2xxx/qla_target.h
@@ -648,6 +648,7 @@ struct qla_tgt_func_tmpl {
 
int (*check_initiator_node_acl)(struct scsi_qla_host *, unsigned char *,
void *, uint8_t *, uint16_t);
+   void (*update_sess)(struct qla_tgt_sess *, port_id_t, uint16_t, bool);
struct qla_tgt_sess *(*find_sess_by_loop_id)(struct scsi_qla_host *,
const uint16_t);
struct qla_tgt_sess *(*find_sess_by_s_id)(struct scsi_qla_host *,
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index d8211cc..3d74f2f 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -1457,6 +1457,78 @@ static int tcm_qla2xxx_check_initiator_node_acl(
return 0;
 }
 
+static void tcm_qla2xxx_update_sess(struct qla_tgt_sess *sess, port_id_t s_id,
+   uint16_t loop_id, bool conf_compl_supported)
+{
+   struct qla_tgt *tgt = sess-tgt;
+   struct qla_hw_data *ha = tgt-ha;
+   struct tcm_qla2xxx_lport *lport = ha-tgt.target_lport_ptr;
+   struct se_node_acl *se_nacl = sess-se_sess-se_node_acl;
+   struct tcm_qla2xxx_nacl *nacl = container_of(se_nacl,
+   struct tcm_qla2xxx_nacl

Re: [PATCH] qla2xxx: Fix endianness of task management response code

[Roland Dreier]

On Fri, Sep 21, 2012 at 1:02 AM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
 The data in status1 appears to get used a word at a time ... what about
 the other three bytes you don't set; are they guaranteed to be zero? (in
 which case this works, it just looks wrong from the way the thing is
 used in the rest of the code).

Yes, the structure comes from

ctio = (struct ctio7_to_24xx *)qla2x00_alloc_iocbs(ha, NULL);

a few lines earlier, and that does:

/* Prep packet */
memset(pkt, 0, REQUEST_ENTRY_SIZE);

before

return pkt;
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] qla2xxx: Fix endianness of task management response code

[Roland Dreier]

On Wed, Sep 19, 2012 at 12:59 AM, James Bottomley
james.bottom...@hansenpartnership.com wrote:
 Is this also true on Big Endian Hardware?  Because the fix you have
 assumes that the TIO IOCB with SCSI status mode 1 should be CPU
 endian ... that doesn't look right since this is passed directly over
 the PCI bus (and the PCI bus is little endian), so shouldn't the correct
 fix be to replace cpu_to_be32 with cpu_to_le32?

After my patch the assignment is to a u8, and that byte is in the right
place in memory.  So I don't think there's any endianness bug.

 - R.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] qla2xxx: Fix endianness of task management response code

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

The qla2xxx firmware actually expects the task management response
code in a CTIO IOCB with SCSI status mode 1 to be in little-endian
byte order, ie the response code should be the first byte in the
sense_data[] array.  The old code erroneously byte-swapped the
response code, which puts it in the wrong place on the wire and leads
to initiators thinking every task management request succeeds (since
they see 0 in the byte where they look for the response code).

Cc: Chad Dupuis chad.dup...@qlogic.com
Cc: Arun Easi arun.e...@qlogic.com
Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/scsi/qla2xxx/qla_target.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c 
b/drivers/scsi/qla2xxx/qla_target.c
index 5b30132..41b74ba 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -1403,7 +1403,7 @@ static void qlt_24xx_send_task_mgmt_ctio(struct 
scsi_qla_host *ha,
ctio-u.status1.scsi_status =
__constant_cpu_to_le16(SS_RESPONSE_INFO_LEN_VALID);
ctio-u.status1.response_len = __constant_cpu_to_le16(8);
-   ((uint32_t *)ctio-u.status1.sense_data)[0] = cpu_to_be32(resp_code);
+   ctio-u.status1.sense_data[0] = resp_code;
 
qla2x00_start_iocbs(ha, ha-req);
 }
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] target: Remove unused se_cmd.cmd_spdtl

[Roland Dreier]

On Fri, Aug 17, 2012 at 6:02 PM, Nicholas A. Bellinger
n...@linux-iscsi.org wrote:
 No, or at least that is not what happens anymore with current target
 core + iscsi-target code..

 The se_cmd-data_length re-assignment here is what will be used by
 iscsi-target fabric code for all iSCSI descriptor related allocations
 +ransfer, instead of the original fabric 'expected transfer length' that
 declares the size of the SCSI initiator's available buffer at the other
 end.

Not sure I follow this.  Isn't cmd-data_length also what we use to
allocate the buffer used to store the data we're going to transfer?

I guess my example with READ(10) actually works, because
iblock_execute_rw() just uses the buffer allocated, rather than
paying attention to the sector count in the command.

But what if an initiator sends, say, REPORT LUNS or PR OUT
with an allocation length of 8192, but a transport-level length
of only 4096?  If the REPORT LUNS or PR OUT response is
bigger than 4096 bytes, we'll overflow the allocated buffer with
your patch, right?

 - R.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()

[Roland Dreier]

On Mon, Jul 16, 2012 at 6:56 PM, Nicholas A. Bellinger
n...@linux-iscsi.org wrote:
 Do you have a plan for how to handle this?  Do we really want to plumb
 through another callback to tell the fabric driver to free the command
 in this case?

 I need to think more about this ahead of changing it back again for-3.6
 now that other fabrics have the assumption of target_submit_cmd() would
 not fail.

 There is a clear case with qla2xxx for just changing it back (we might
 end up doing that just yet) but I wanted to get the other important bits
 into for-next into place first..

Sleeping on things, I now feel pretty strongly that having target_submit_cmd
return an error value for immediate errors where the command does not
make it into the target core is the right approach.

Freeing commands is already one of the most confusing and complex parts
of the target code, with check_stop_free, cmd_wait_comp and
SCF_ACK_KREF.  Adding another code flow back to the fabric driver
with yet another set of semantics around freeing a command seems like
it's making things even harder to maintain.

On the other hand, every caller of target_submit_cmd() in the tree already
has an error path where it handles problems it detects right before calling
target_submit_cmd(), and so it's trivial to share that for problems detected
inside target_submit_cmd().  If you look at patch 4/7, pretty much the
only changes are like going from

target_submit_cmd(...);

to

if (target_submit_cmd(...))
goto err;

and in fact the -handle_cmd() wrapper that qla_target uses to call
target_submit_cmd via tcm_qla2xxx already has a return value that
qla_target handled properly!

I guess another approach would be to split target_submit_cmd() into
the target_get_sess_cmd() part and the rest of it, and have fabric
drivers handle errors queueing commands but not have to worry about
the submit cmd part failing.  But I'm not sure that's any better.

Andy, any feelings about this one way or another?  Christoph?

Thanks,
  Roland
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/7] series to fix qla2xxx use-after-free

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

Hi Nic,

Here's a series that's fundamentally about fixing a use-after-free in
qla_target code.  It ends up being seven patches because I wanted to
make each step easy to review, and several of these are just cleanups
that stand on their own.

We have a few more qla2xxx-related fixes that I need to clean up and
merge with the latest.  I'll send them soon.

Roland Dreier (7):
  qla2xxx: Get rid of redundant qla_tgt_sess.tearing_down
  target: Un-export target_get_sess_cmd()
  sbp-target: Consolidate duplicated error path code in
sbp_handle_command()
  target: Allow for target_submit_cmd() returning errors
  target: Check sess_tearing_down in target_get_sess_cmd()
  qla2xxx: Remove racy, now-redundant check of sess_tearing_down
  target: Remove se_session.sess_wait_list

 drivers/scsi/qla2xxx/qla_target.c  |   16 ++
 drivers/scsi/qla2xxx/qla_target.h  |1 -
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |   10 +++---
 drivers/target/sbp/sbp_target.c|   36 ++---
 drivers/target/target_core_transport.c |   55 
 drivers/target/tcm_fc/tfc_cmd.c|8 +++--
 drivers/usb/gadget/tcm_usb_gadget.c|   20 +---
 include/target/target_core_base.h  |1 -
 include/target/target_core_fabric.h|5 ++-
 9 files changed, 73 insertions(+), 79 deletions(-)

-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/7] qla2xxx: Get rid of redundant qla_tgt_sess.tearing_down

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

The only place that sets qla_tgt_sess.tearing_down calls
target_splice_sess_cmd_list() immediately afterwards, without dropping
the lock it holds.  That function sets se_session.sess_tearing_down,
so we can get rid of the qla_target-specific flag, and in the one
place that looks at the qla_tgt_sess.tearing_down flag just test
se_session.sess_tearing_down instead.

Cc: Chad Dupuis chad.dup...@qlogic.com
Cc: Arun Easi arun.e...@qlogic.com
Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/scsi/qla2xxx/qla_target.c  |2 +-
 drivers/scsi/qla2xxx/qla_target.h  |1 -
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |1 -
 3 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c 
b/drivers/scsi/qla2xxx/qla_target.c
index 77759c7..87b5a330 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -2644,7 +2644,7 @@ static void qlt_do_work(struct work_struct *work)
sess = ha-tgt.tgt_ops-find_sess_by_s_id(vha,
atio-u.isp24.fcp_hdr.s_id);
if (sess) {
-   if (unlikely(sess-tearing_down)) {
+   if (unlikely(sess-se_sess-sess_tearing_down)) {
sess = NULL;
spin_unlock_irqrestore(ha-hardware_lock, flags);
goto out_term;
diff --git a/drivers/scsi/qla2xxx/qla_target.h 
b/drivers/scsi/qla2xxx/qla_target.h
index 9f9ef16..07aa265 100644
--- a/drivers/scsi/qla2xxx/qla_target.h
+++ b/drivers/scsi/qla2xxx/qla_target.h
@@ -813,7 +813,6 @@ struct qla_tgt_sess {
unsigned int conf_compl_supported:1;
unsigned int deleted:1;
unsigned int local:1;
-   unsigned int tearing_down:1;
 
struct se_session *se_sess;
struct scsi_qla_host *vha;
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 6e64314..3cb2b97 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -466,7 +466,6 @@ static int tcm_qla2xxx_shutdown_session(struct se_session 
*se_sess)
vha = sess-vha;
 
spin_lock_irqsave(vha-hw-hardware_lock, flags);
-   sess-tearing_down = 1;
target_splice_sess_cmd_list(se_sess);
spin_unlock_irqrestore(vha-hw-hardware_lock, flags);
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/7] target: Un-export target_get_sess_cmd()

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

There are no in-tree users of target_get_sess_cmd() outside of
target_core_transport.c.  Any new code should use the higher-level
target_submit_cmd() interface.  So let's un-export target_get_sess_cmd()
and make it static to the one file where it's actually used.

Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/target/target_core_transport.c |6 +++---
 include/target/target_core_fabric.h|1 -
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 634d0f3..82e40e1 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -73,6 +73,7 @@ static void transport_complete_task_attr(struct se_cmd *cmd);
 static void transport_handle_queue_full(struct se_cmd *cmd,
struct se_device *dev);
 static int transport_generic_get_mem(struct se_cmd *cmd);
+static void target_get_sess_cmd(struct se_session *, struct se_cmd *, bool);
 static void transport_put_cmd(struct se_cmd *cmd);
 static void transport_remove_cmd_from_queue(struct se_cmd *cmd);
 static int transport_set_sense_codes(struct se_cmd *cmd, u8 asc, u8 ascq);
@@ -3648,8 +3649,8 @@ EXPORT_SYMBOL(transport_generic_free_cmd);
  * @se_cmd:command descriptor to add
  * @ack_kref:  Signal that fabric will perform an ack target_put_sess_cmd()
  */
-void target_get_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cmd,
-   bool ack_kref)
+static void target_get_sess_cmd(struct se_session *se_sess, struct se_cmd 
*se_cmd,
+   bool ack_kref)
 {
unsigned long flags;
 
@@ -3669,7 +3670,6 @@ void target_get_sess_cmd(struct se_session *se_sess, 
struct se_cmd *se_cmd,
se_cmd-check_release = 1;
spin_unlock_irqrestore(se_sess-sess_cmd_lock, flags);
 }
-EXPORT_SYMBOL(target_get_sess_cmd);
 
 static void target_release_cmd_kref(struct kref *kref)
 {
diff --git a/include/target/target_core_fabric.h 
b/include/target/target_core_fabric.h
index c78a233..1e68882 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -129,7 +129,6 @@ booltransport_wait_for_tasks(struct se_cmd *);
 inttransport_check_aborted_status(struct se_cmd *, int);
 inttransport_send_check_condition_and_sense(struct se_cmd *, u8, int);
 
-void   target_get_sess_cmd(struct se_session *, struct se_cmd *, bool);
 inttarget_put_sess_cmd(struct se_session *, struct se_cmd *);
 void   target_splice_sess_cmd_list(struct se_session *);
 void   target_wait_for_sess_cmds(struct se_session *, int);
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/7] sbp-target: Consolidate duplicated error path code in sbp_handle_command()

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

Cc: Chris Boot bo...@bootc.net
Cc: Stefan Richter stef...@s5r6.in-berlin.de
Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/target/sbp/sbp_target.c |   28 
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
index 7e6136e..2425ca4 100644
--- a/drivers/target/sbp/sbp_target.c
+++ b/drivers/target/sbp/sbp_target.c
@@ -1219,28 +1219,14 @@ static void sbp_handle_command(struct 
sbp_target_request *req)
ret = sbp_fetch_command(req);
if (ret) {
pr_debug(sbp_handle_command: fetch command failed: %d\n, ret);
-   req-status.status |= cpu_to_be32(
-   STATUS_BLOCK_RESP(STATUS_RESP_TRANSPORT_FAILURE) |
-   STATUS_BLOCK_DEAD(0) |
-   STATUS_BLOCK_LEN(1) |
-   STATUS_BLOCK_SBP_STATUS(SBP_STATUS_UNSPECIFIED_ERROR));
-   sbp_send_status(req);
-   sbp_free_request(req);
-   return;
+   goto err;
}
 
ret = sbp_fetch_page_table(req);
if (ret) {
pr_debug(sbp_handle_command: fetch page table failed: %d\n,
ret);
-   req-status.status |= cpu_to_be32(
-   STATUS_BLOCK_RESP(STATUS_RESP_TRANSPORT_FAILURE) |
-   STATUS_BLOCK_DEAD(0) |
-   STATUS_BLOCK_LEN(1) |
-   STATUS_BLOCK_SBP_STATUS(SBP_STATUS_UNSPECIFIED_ERROR));
-   sbp_send_status(req);
-   sbp_free_request(req);
-   return;
+   goto err;
}
 
unpacked_lun = req-login-lun-unpacked_lun;
@@ -1252,6 +1238,16 @@ static void sbp_handle_command(struct sbp_target_request 
*req)
target_submit_cmd(req-se_cmd, sess-se_sess, req-cmd_buf,
req-sense_buf, unpacked_lun, data_length,
MSG_SIMPLE_TAG, data_dir, 0);
+   return;
+
+err:
+   req-status.status |= cpu_to_be32(
+   STATUS_BLOCK_RESP(STATUS_RESP_TRANSPORT_FAILURE) |
+   STATUS_BLOCK_DEAD(0) |
+   STATUS_BLOCK_LEN(1) |
+   STATUS_BLOCK_SBP_STATUS(SBP_STATUS_UNSPECIFIED_ERROR));
+   sbp_send_status(req);
+   sbp_free_request(req);
 }
 
 /*
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/7] qla2xxx: Remove racy, now-redundant check of sess_tearing_down

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

Now that target_submit_cmd() / target_get_sess_cmd() check
sess_tearing_down before adding commands to the list, we no longer
need the check in qlt_do_work().  In fact this check is racy anyway
(and that race is what inspired the change to add the check of
sess_tearing_down to the target core).

Cc: Chad Dupuis chad.dup...@qlogic.com
Cc: Arun Easi arun.e...@qlogic.com
Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/scsi/qla2xxx/qla_target.c |   16 +++-
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c 
b/drivers/scsi/qla2xxx/qla_target.c
index 87b5a330..5b30132 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -2643,19 +2643,9 @@ static void qlt_do_work(struct work_struct *work)
spin_lock_irqsave(ha-hardware_lock, flags);
sess = ha-tgt.tgt_ops-find_sess_by_s_id(vha,
atio-u.isp24.fcp_hdr.s_id);
-   if (sess) {
-   if (unlikely(sess-se_sess-sess_tearing_down)) {
-   sess = NULL;
-   spin_unlock_irqrestore(ha-hardware_lock, flags);
-   goto out_term;
-   } else {
-   /*
-* Do the extra kref_get() before dropping
-* qla_hw_data-hardware_lock.
-*/
-   kref_get(sess-se_sess-sess_kref);
-   }
-   }
+   /* Do kref_get() before dropping qla_hw_data-hardware_lock. */
+   if (sess)
+   kref_get(sess-se_sess-sess_kref);
spin_unlock_irqrestore(ha-hardware_lock, flags);
 
if (unlikely(!sess)) {
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/7] target: Allow for target_submit_cmd() returning errors

[Roland Dreier]

From: Roland Dreier rol...@purestorage.com

We want it to be possible for target_submit_cmd() to return errors up
to its fabric module callers.  For now just update the prototype to
return an int, and update all callers to handle non-zero return values
as an error.

Cc: Chad Dupuis chad.dup...@qlogic.com
Cc: Arun Easi arun.e...@qlogic.com
Cc: Chris Boot bo...@bootc.net
Cc: Stefan Richter stef...@s5r6.in-berlin.de
Cc: Mark Rustad mark.d.rus...@intel.com
Cc: Signed-off-by: Sebastian Andrzej Siewior bige...@linutronix.de
Cc: Felipe Balbi ba...@ti.com
Signed-off-by: Roland Dreier rol...@purestorage.com
---
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |7 +++
 drivers/target/sbp/sbp_target.c|8 +---
 drivers/target/target_core_transport.c |9 +
 drivers/target/tcm_fc/tfc_cmd.c|8 +---
 drivers/usb/gadget/tcm_usb_gadget.c|   20 
 include/target/target_core_fabric.h|2 +-
 6 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 3cb2b97..7db7ca7 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -599,10 +599,9 @@ static int tcm_qla2xxx_handle_cmd(scsi_qla_host_t *vha, 
struct qla_tgt_cmd *cmd,
return -EINVAL;
}
 
-   target_submit_cmd(se_cmd, se_sess, cdb, cmd-sense_buffer[0],
-   cmd-unpacked_lun, data_length, fcp_task_attr,
-   data_dir, flags);
-   return 0;
+   return target_submit_cmd(se_cmd, se_sess, cdb, cmd-sense_buffer[0],
+cmd-unpacked_lun, data_length, fcp_task_attr,
+data_dir, flags);
 }
 
 static void tcm_qla2xxx_do_rsp(struct work_struct *work)
diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
index 2425ca4..fb75d05 100644
--- a/drivers/target/sbp/sbp_target.c
+++ b/drivers/target/sbp/sbp_target.c
@@ -1235,9 +1235,11 @@ static void sbp_handle_command(struct sbp_target_request 
*req)
pr_debug(sbp_handle_command ORB:0x%llx unpacked_lun:%d data_len:%d 
data_dir:%d\n,
req-orb_pointer, unpacked_lun, data_length, data_dir);
 
-   target_submit_cmd(req-se_cmd, sess-se_sess, req-cmd_buf,
-   req-sense_buf, unpacked_lun, data_length,
-   MSG_SIMPLE_TAG, data_dir, 0);
+   if (target_submit_cmd(req-se_cmd, sess-se_sess, req-cmd_buf,
+ req-sense_buf, unpacked_lun, data_length,
+ MSG_SIMPLE_TAG, data_dir, 0))
+   goto err;
+
return;
 
 err:
diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 82e40e1..34ca821 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1544,7 +1544,7 @@ EXPORT_SYMBOL(transport_handle_cdb_direct);
  * This may only be called from process context, and also currently
  * assumes internal allocation of fabric payload buffer by target-core.
  **/
-void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
+int target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
unsigned char *cdb, unsigned char *sense, u32 unpacked_lun,
u32 data_length, int task_attr, int data_dir, int flags)
 {
@@ -1583,7 +1583,7 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct 
se_session *se_sess,
transport_send_check_condition_and_sense(se_cmd,
se_cmd-scsi_sense_reason, 0);
target_put_sess_cmd(se_sess, se_cmd);
-   return;
+   return 0;
}
/*
 * Sanitize CDBs via transport_generic_cmd_sequencer() and
@@ -1592,7 +1592,7 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct 
se_session *se_sess,
rc = target_setup_cmd_from_cdb(se_cmd, cdb);
if (rc != 0) {
transport_generic_request_failure(se_cmd);
-   return;
+   return 0;
}
 
/*
@@ -1608,7 +1608,8 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct 
se_session *se_sess,
 * when fabric has filled the incoming buffer.
 */
transport_handle_cdb_direct(se_cmd);
-   return;
+
+   return 0;
 }
 EXPORT_SYMBOL(target_submit_cmd);
 
diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
index f03fb97..c7c90aa 100644
--- a/drivers/target/tcm_fc/tfc_cmd.c
+++ b/drivers/target/tcm_fc/tfc_cmd.c
@@ -541,9 +541,11 @@ static void ft_send_work(struct work_struct *work)
 * Use a single se_cmd-cmd_kref as we expect to release se_cmd
 * directly from ft_check_stop_free callback in response path.
 */
-   target_submit_cmd(cmd-se_cmd, cmd-sess-se_sess, fcp-fc_cdb,
-   cmd-ft_sense_buffer[0], scsilun_to_int(fcp

Re: [PATCH 4/7] target: Allow for target_submit_cmd() returning errors

[Roland Dreier]

On Mon, Jul 16, 2012 at 4:00 PM, Nicholas A. Bellinger
n...@linux-iscsi.org wrote:
 Mmmm.  The original target_submit_cmd() code had been propagating up a
 return value, but then we decided (via Agrover's patch) that it made
 more sense for target_submit_cmd() to always handle exceptions via
 normal TFO callbacks, and not have the fabric worry about the return
 here..

 Also, I'm not sure if the error paths that this patch now accesses after
 target_submit_cmd() failure are going to deal with different types of
 target_submit_cmd() failures correctly.

 So NACK for the moment, as I don't really see why this is necessary in
 the first place..?

Read on in the series to see why this is needed; in short, for qla2xxx
at least, we need a race-free way to check for sess_tearing_down
atomically with actually adding the command to sess_cmd_list.

I'm OK with returning failure via callback, but

 a) I'm not sure we can use the normal TFO callbacks in case
we can't add the command to sess_cmd_list (seems like it
opens the door to other use-after-frees in qla2xxx at least)
 b) Maybe it's OK if we say that failure to add the command to
the sess_cmd_list is the only time submit cmd fails?

The qla2xxx race/use-after-free is definitely real, we hit it in testing
here with active IO across ACL changes.

 - R.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()

[Roland Dreier]

On Mon, Jul 16, 2012 at 4:08 PM, Nicholas A. Bellinger
n...@linux-iscsi.org wrote:
 However, I'm still leaning towards a way to do this for tcm_qla2xxx that
 does not require all fabric callers to handle target_submit_cmd()
 exceptions directly..

 How about a special target_get_sess_cmd() failure callback to free
 se_cmd when target_get_sess_cmd() fails, but not actually send a SCSI
 response back out to the fabric here during session shutdown..?

I guess that's OK, although I'm not sure it ends up being cleaner.  If
you look at my 4/7 patch, you see that all target_submit_cmd callers
have an error path where they handle terminating commands that
fail processing just before calling submit_cmd anyway.

eg for qla2xxx we'll need a callback to call qlt_send_term_exchange()
in addition to the error path call that qlt_do_work() has anyway.
Similarly tcm_fc ends up with a second call to ft_send_resp_code_and_free().
etc.

But I don't really have a strong feeling about this, if the view is that
submit_cmd() should never return failure directly then I'm OK with that.

 That said, I'm going to commit this patch (slightly changed to keep
 target_submit_cmd() returning void for now) but I'd still like a better
 way of handling this particular failure without propagating up the
 return value.

OK, I'll take a look at how you handle this...

 - R.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()

[Roland Dreier]

 OK, I'll take a look at how you handle this...

So looking at commit bc187ea6c3b3 in the tree you just pushed out
(target: Check sess_tearing_down in target_get_sess_cmd()) it looks
like you just return from target_submit_cmd() if we fail to add the
command to sess_cmd_list -- doesn't this mean we just leak those
commands and possibly leave the HW sitting there with open exchanges?

Do you have a plan for how to handle this?  Do we really want to plumb
through another callback to tell the fabric driver to free the command
in this case?

 - R.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ofa-general] [PATCH 2/2] ib fmr pool: flush used clean entries

[Roland Dreier]

This looks like a really nice approach to me.  Olaf?

 - R.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Integration of SCST in the mainstream Linux kernel

[Roland Dreier]

  .   .   STGT read SCST read.STGT read
SCST read.
  .   .  performance   performance   . performance
  performance   .
  .   .  (0.5K, MB/s)  (0.5K, MB/s)  .   (1 MB, MB/s)  
   (1 MB, MB/s)  .
  . iSER (8 Gb/s network) . 250N/A   .   360   
  N/A   .
  . SRP  (8 Gb/s network) . N/A421   .   N/A   
  683   .

  On the comparable figures, which only seem to be IPoIB they're showing a
  13-18% variance, aren't they?  Which isn't an incredible difference.

Maybe I'm all wet, but I think iSER vs. SRP should be roughly
comparable.  The exact formatting of various messages etc. is
different but the data path using RDMA is pretty much identical.  So
the big difference between STGT iSER and SCST SRP hints at some big
difference in the efficiency of the two implementations.

 - R.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] DMA buffer alignment annotations

[Roland Dreier]

   +#define __dma_aligned 
   __attribute__((aligned(ARCH_MIN_DMA_ALIGNMENT)))
   +#define __dma_buffer  __dma_buffer_line(__LINE__)
   +#define __dma_buffer_line(line)   __dma_aligned;\
   +  char __dma_pad_##line[0] __dma_aligned

  You introduce __dma_buffer_line() if ARCH_MIN_DMA_ALIGNMENT is set but
  not if it isn't...

__dma_buffer_line() is just an internal implementation detail to take
care of string pasting properly.  Perhaps there should be a comment
warning people not to use it directly.

 - R.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] scsi: Use new __dma_buffer to align sense buffer in scsi_cmnd

[Roland Dreier]

  It's also incomplete as a fix because I don't see what guarantees the
  buffer size will always exceed cache line size

There's a macro trick that adds a pad member after the buffer too, so
that it gets rounded up to the cacheline size:

  +#define __dma_aligned   
  __attribute__((aligned(ARCH_MIN_DMA_ALIGNMENT)))
  +#define __dma_buffer__dma_buffer_line(__LINE__)
  +#define __dma_buffer_line(line) __dma_aligned;\
  +char __dma_pad_##line[0] __dma_aligned

So that part is OK at least.

 - R.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SCSI breakage on non-cache coherent architectures

[Roland Dreier]

  Actually, we already established on IRC that the lasi700 driver doesn't
  need this, principally because the parisc architecture doesn't do an
  invalidate for DMA_FROM_DEVICE but a flush and invalidate
  (architecturally, if you read our manuals, even pdc is entitled to write
  back dirty lines, so it's not clear there's actually an invalidate
  instruction we can use).   This is also one possible temporary fix for
  the other architectures if we can't get a different method to work
  nicely.

I think doing a writeback and invalidate is a very fragile way to deal
with DMA into the middle of a data structure.  It may work OK for now,
but you have to make sure forever into the future that no codepath
anywhere else ever touches the cacheline that you're DMAing into while
the DMA is pending.  It just leaves a hidden trap that is too easy to
step on, because the architectures that get pretty much all testing
all have cache-coherent DMA.

Reviving my ancient __dma_buffer patch seems far preferable to me.

 - R.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SCSI breakage on non-cache coherent architectures

[Roland Dreier]

  I've been debugging various issues on the PowerPC 44x embedded
  architecture which happens to have non-coherent PCI DMA.
  
  One of the problem I'm hitting is that one really need to enforce
  kmalloc alignement to cache lines or bad things will happen (among
  others with USB), for some reasons, powerpc failed to do so, I fixed it.

Heh... I hit the same problem literally 5 years ago:
http://lwn.net/Articles/1783/

I implemented the __dma_buffer annotation:
http://lwn.net/Articles/2269/

But DaveM said we should just use the PCI pool code instead:
http://lwn.net/Articles/2270/

 - R.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SCSI breakage on non-cache coherent architectures

[Roland Dreier]

  2) Add the __dma_cacheline_aligned tag.
  
  But note that with #2 it could get quite ugly because the
  alignment and size both have a minimum that needs to be
  enforced, not just the alignment alone.  So either:
  
  struct foo {
   unsigned int other_unrelated_stuff;
  
   struct object dma_thing __dma_cacheline_aligned;
  
   unsigned int more_nondma_stuff __dma_cacheline_aligned;
  };
  
  or:
  
  struct foo {
   unsigned int other_unrelated_stuff;
  
   union {
   struct object dma_thing __dma_cacheline_aligned;
   char __pad[(sizeof(object) + DMA_CACHELINE_SIZE 
  ~DMA_CACHELINE_SIZE)];
   } u;
  
   unsigned int more_nondma_stuff __dma_cacheline_aligned;
  };

I wrapped this ugliness up inside the macro back in what I posted in
2002 (http://lkml.org/lkml/2002/6/12/234):

#define __dma_buffer __dma_buffer_line(__LINE__)
#define __dma_buffer_line(line) __dma_buffer_expand_line(line)
#define __dma_buffer_expand_line(line) \
__attribute__ ((aligned(L1_CACHE_BYTES))); \
char __dma_pad_ ## line [0] __attribute__ ((aligned(L1_CACHE_BYTES)))

then you just need to tag the actual member like:

char foo[3] __dma_buffer;

 - R.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Asynchronous scsi scanning

[Roland Dreier]

  No, it does matter.  Your suggestion doesn't work, because
  /sys/module/scsi_mod/parameters/ belongs to the module code.  To create
  a new attribute there, you use the module_param() code -- and there's
  no way to have code called when your parameter is changed.

If I'm not misunderstanding what you're talking about, there is
actually a way to have code called when a module parameter is changed:
module_param_call().

 - R.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] First pass at InfiniBand SRP initiator

[Roland Dreier]

Here's a first pass at an InfiniBand SCSI RDMA Protocol (SRP)
initiator.  This allows systems to talk to storage over an IB network--
IB SRP storage is available from a number of vendors.

This obviously isn't ready for merging yet, since it implements no
error handling, etc.  However, I'd appreciate any and all comments on
the code that exists, so that I can fix things up before going too far
off into the weeds.

One design note: all discovery of storage is assumed to be pushed off
into userspace, which then tells the kernel to connect to a given
target port by doing something like:

echo 
id_ext=2104cfe7a949,ioc_guid=0005ad0015dd,dgid=fe85ad0015dd,pkey=,service_id=0066
  /sys/class/infiniband_srp/srp-mthca1-1/add_target

Thanks,
  Roland

--- /dev/null   1970-01-01 00:00:00.0 +
+++ linux-export/drivers/infiniband/ulp/srp/ib_srp.c2005-07-15 
13:02:00.278274501 -0700
@@ -0,0 +1,1344 @@
+/*
+ * Copyright (c) 2005 Cisco Systems.  All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ *  - Redistributions of source code must retain the above
+ *copyright notice, this list of conditions and the following
+ *disclaimer.
+ *
+ *  - Redistributions in binary form must reproduce the above
+ *copyright notice, this list of conditions and the following
+ *disclaimer in the documentation and/or other materials
+ *provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ * $Id: ib_srp.c 2860 2005-07-14 16:39:43Z roland $
+ */
+
+#include linux/version.h
+#include linux/module.h
+
+#include linux/init.h
+#include linux/slab.h
+#include linux/err.h
+#include linux/idr.h
+#include linux/string.h
+#include linux/parser.h
+
+#include asm/atomic.h
+
+#include scsi/scsi.h
+#include scsi/scsi_device.h
+#include scsi/scsi_dbg.h
+
+#include ib_cache.h
+
+#include ib_srp.h
+
+#define DRV_NAME   ib_srp
+#define PFXDRV_NAME : 
+#define DRV_VERSION0.01
+#define DRV_RELDATEJanuary 11, 2005
+
+MODULE_AUTHOR(Roland Dreier);
+MODULE_DESCRIPTION(InfiniBand SCSI RDMA Protocol driver);
+MODULE_LICENSE(Dual BSD/GPL);
+
+static int topspin_workarounds = 1;
+
+module_param(topspin_workarounds, int, 0444);
+MODULE_PARM_DESC(topspin_workarounds,
+Enable workarounds for Topspin/Cisco SRP target bugs if != 
0);
+
+static const u8 topspin_oui[3] = { 0x00, 0x05, 0xad };
+
+static atomic_t srp_uid;
+
+static rwlock_t idr_lock;
+static DEFINE_IDR(target_idr);
+
+static void srp_add_one(struct ib_device *device);
+static void srp_remove_one(struct ib_device *device);
+
+static struct ib_client srp_client = {
+   .name   = srp,
+   .add= srp_add_one,
+   .remove = srp_remove_one
+};
+
+static inline struct srp_target_port *host_to_target(struct Scsi_Host *host)
+{
+   return (struct srp_target_port *) host-hostdata;
+}
+
+static const char *srp_target_info(struct Scsi_Host *host)
+{
+   return host_to_target(host)-target_name;
+}
+
+static struct srp_iu *srp_alloc_iu(struct srp_host *host, size_t size,
+  unsigned int __nocast gfp_mask,
+  enum dma_data_direction direction)
+{
+   struct srp_iu *iu;
+
+   iu = kmalloc(sizeof *iu, gfp_mask);
+   if (!iu)
+   return NULL;
+
+   iu-buf = kmalloc(size, gfp_mask);
+   if (!iu-buf) {
+   kfree(iu);
+   return NULL;
+   }
+
+   memset(iu-buf, 0, size);
+
+   iu-dma = dma_map_single(host-dev-dma_device, iu-buf, size, 
direction);
+   if (dma_mapping_error(iu-dma)) {
+   kfree(iu-buf);
+   kfree(iu);
+   return NULL;
+   }
+
+   iu-size  = size;
+   iu-direction = direction;
+
+   return iu;
+}
+
+static void srp_free_iu(struct srp_host *host, struct srp_iu *iu)
+{
+   if (!iu)
+   return;
+
+   dma_unmap_single(host-dev-dma_device, iu-dma, iu-size, 
iu-direction);
+   kfree(iu-buf);
+   kfree(iu);
+}
+
+static