Re: [PATCH 9/9] IB/iser: Convert to CQ abstraction

[Or Gerlitz]

On 11/13/2015 3:46 PM, Christoph Hellwig wrote:

From: Sagi Grimberg


Care to sparse some text here to assist a reviewer and future bisections?!

I have asked multiple times to avoid empty change-logs for patches in 
this driver.




Signed-off-by: Sagi Grimberg
Signed-off-by: Christoph Hellwig
---
  drivers/infiniband/ulp/iser/iscsi_iser.h |  68 ---
  drivers/infiniband/ulp/iser/iser_initiator.c | 142 ++-
  drivers/infiniband/ulp/iser/iser_memory.c|  21 ++-
  drivers/infiniband/ulp/iser/iser_verbs.c | 258 ++-
  4 files changed, 209 insertions(+), 280 deletions(-)


--
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 1/9] move blk_iopoll to limit and make it generally available

[Or Gerlitz]

On Sun, Nov 15, 2015 at 10:48 AM, Sagi Grimberg
 wrote:
> Or is correct,
>
> I have attempted to convert iser to use blk_iopoll in the past, however
> I've seen inconsistent performance and latency skews (comparing to
> tasklets iser is using today). This was manifested in IOPs test cases
> where I ran multiple threads with higher queue-depth and not in
> sanitized pure latency (QD=1) test cases. Unfortunately I didn't have
> the time to pick it up since.
>
> I do have every intention of testing it again with this. If it still
> exist we will need to find the root-cause of it before converting
> drivers to use it.

Good, this way (inconsistent performance and latency skews) or another
(all shines up) -- please
let us know your findings, best through commenting within V > 0 the
cover letter posts of this series
--
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/9] IB: add a proper completion queue abstraction

[Sagi Grimberg]

+/**
+ * ib_process_direct_cq - process a CQ in caller context
+ * @cq:CQ to process
+ *
+ * This function is used to process all outstanding CQ entries on a
+ * %IB_POLL_DIRECT CQ.  It does not offload CQ processing to a different
+ * context and does not ask from completion interrupts from the HCA.
+ */
+void ib_process_cq_direct(struct ib_cq *cq)
+{
+   WARN_ON_ONCE(cq->poll_ctx != IB_POLL_DIRECT);
+
+   __ib_process_cq(cq, INT_MAX);
+}


I doubt INT_MAX is useful as a budget in any use-case. it can easily
hog the CPU. If the consumer is given access to poll a CQ, it must be
able to provide some way to budget it. Why not expose a budget argument
to the consumer?
--
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 1/9] move blk_iopoll to limit and make it generally available

[Sagi Grimberg]

On Fri, Nov 13, 2015 at 3:46 PM, Christoph Hellwig  wrote:

The new name is irq_poll as iopoll is already taken.  Better suggestions
welcome.


Sagi (or Christoph if you can address that),

@ some pointer over the last 18 months there was a port done at
mellanox for iser to use blk-iopoll and AFAIR it didn't work well or
didn't work at all. Can you tell now what was the problem and how did
you address it at your generalization?


Hi Or,

Sagi mentioned last time he tried a similar approach in iSER he saw
some large latency sparks.  We've seen nothing worse than the original
approach.  The Flash memory summit slide set has some numbers:

http://www.flashmemorysummit.com/English/Collaterals/Proceedings/2015/20150811_FA11_Bandic.pdf

they aren't quite up to date, but the latency distribution hasn't
really changed.


Or is correct,

I have attempted to convert iser to use blk_iopoll in the past, however
I've seen inconsistent performance and latency skews (comparing to
tasklets iser is using today). This was manifested in IOPs test cases
where I ran multiple threads with higher queue-depth and not in
sanitized pure latency (QD=1) test cases. Unfortunately I didn't have
the time to pick it up since.

I do have every intention of testing it again with this. If it still
exist we will need to find the root-cause of it before converting
drivers to use it.

Sagi.
--
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 7/9] IB/iser: Use a dedicated descriptor for login

[Or Gerlitz]

On 11/13/2015 3:46 PM, Christoph Hellwig wrote:

From: Sagi Grimberg

Makes better sense and we'll need it later with CQ abstraction.
iser switch login bufs to void


Sagi, few quick comments on this patch, please address for next version..

The 2nd sentence of the change-log needs better phrasing.

also multiple checkpatch hits on the patch, please fix

CHECK: Please don't use multiple blank lines
#26: FILE: drivers/infiniband/ulp/iser/iscsi_iser.h:329:

+

WARNING: __packed is preferred over __attribute__((packed))
#42: FILE: drivers/infiniband/ulp/iser/iscsi_iser.h:345:
+} __attribute__((packed));

CHECK: Please don't use multiple blank lines
#44: FILE: drivers/infiniband/ulp/iser/iscsi_iser.h:347:
+
+

CHECK: Alignment should match open parenthesis
#161: FILE: drivers/infiniband/ulp/iser/iser_initiator.c:209:
+   if (ib_dma_mapping_error(device->ib_device,
+   desc->req_dma))

CHECK: Alignment should match open parenthesis
#172: FILE: drivers/infiniband/ulp/iser/iser_initiator.c:220:
+   if (ib_dma_mapping_error(device->ib_device,
+   desc->rsp_dma))






--
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 1/1] iscsi: fix regression caused by session lock patch

[Or Gerlitz]

On Fri, Nov 13, 2015 at 6:51 PM, Mike Christie  wrote:
> On 11/13/2015 09:06 AM, Or Gerlitz wrote:
>>> The patch has caused multiple regressions, did not even compile when
>>> > sent to me, and was poorly reviewed and I have not heard from you guys
>>> > in a week. Given the issues the patch has had and the current time, I do
>>> > not feel comfortable with it anymore. I want to re-review it and fix it
>>> > up when there is more time.
>> Mike (Hi),
>>
>> It's a complex patch that touches all the iscsi transports, and yes,
>> when it was send to you the 1st time, there was build error on one of
>> the offload transports (bad! but happens) and yes, as you pointed, one
>> static checker fix + one bug fix for it went upstream after this has
>> been merged, happens too.
>
> A patch should not cause this many issues.
>
>> What makes you say it was poorly reviewed?
>
> I just did not do a good job at looking at the patch. I should have
> caught all of these issues.
>
> - The bnx2i cleanup_task bug should have been obvious, especially for me
> because I had commented about the back lock and the abort path.
>
> - This oops, was so basic. Incorrect locking around a linked list being
> accessed from 2 threads is really one of those 1st year kernel
> programmer things.

Mike, Chris

After the locking change, adding a task to any of the connection
mgmtqueue, cmdqueue, or requeue lists is under the session forward lock.

Removing tasks from any of these lists in iscsi_data_xmit is under
the session forward lock and **before** calling down to the transport
to handle the task.

The iscsi_complete_task helper was added by Mike's commit
3bbaaad95fd38ded "[SCSI] libiscsi: handle cleanup task races"
and is indeed typically called under the backward lock && has this section

+   if (!list_empty(>running))
+   list_del_init(>running);

which per my reading of the code never comes into play, can you comment?

Lets address this area before we move to the others claims made on the patch.

Again, the patch is around for ~18 months, since 3.15, and no deep
complaints so far, lets
not jump to conclusions.

Or.
--
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 3/9] IB: add a helper to safely drain a QP

[Sagi Grimberg]

+
+struct ib_stop_cqe {
+   struct ib_cqe   cqe;
+   struct completion done;
+};
+
+static void ib_stop_done(struct ib_cq *cq, struct ib_wc *wc)
+{
+   struct ib_stop_cqe *stop =
+   container_of(wc->wr_cqe, struct ib_stop_cqe, cqe);
+
+   complete(>done);
+}
+
+/*
+ * Change a queue pair into the error state and wait until all receive
+ * completions have been processed before destroying it. This avoids that
+ * the receive completion handler can access the queue pair while it is
+ * being destroyed.
+ */
+void ib_drain_qp(struct ib_qp *qp)
+{
+   struct ib_qp_attr attr = { .qp_state = IB_QPS_ERR };
+   struct ib_stop_cqe stop = { };
+   struct ib_recv_wr wr, *bad_wr;
+   int ret;
+
+   wr.wr_cqe = 
+   stop.cqe.done = ib_stop_done;
+   init_completion();
+
+   ret = ib_modify_qp(qp, , IB_QP_STATE);
+   if (ret) {
+   WARN_ONCE(ret, "failed to drain QP: %d\n", ret);
+   return;
+   }
+
+   ret = ib_post_recv(qp, , _wr);
+   if (ret) {
+   WARN_ONCE(ret, "failed to drain QP: %d\n", ret);
+   return;
+   }
+
+   wait_for_completion();
+}


This is taken from srp, and srp drains using a recv wr due to a race
causing a use-after-free condition in srp which re-posts a recv buffer
in the recv completion handler. srp does not really care if there are
pending send flushes.

I'm not sure if there are ordering rules for send/recv queues in
terms of flush completions, meaning that even if all recv flushes
were consumed maybe there are send flushes still pending.

I think that for a general drain helper it would be useful to
make sure that both the recv _and_ send flushes were drained.

So, something like:

void ib_drain_qp(struct ib_qp *qp)
{
struct ib_qp_attr attr = { .qp_state = IB_QPS_ERR };
struct ib_stop_cqe rstop, sstop;
struct ib_recv_wr rwr = {}, *bad_rwr;
struct ib_send_wr swr = {}, *bad_swr;
int ret;

rwr.wr_cqe = 
rstop.cqe.done = ib_stop_done;
init_completion();

swr.wr_cqe = 
sstop.cqe.done = ib_stop_done;
init_completion();

ret = ib_modify_qp(qp, , IB_QP_STATE);
if (ret) {
WARN_ONCE(ret, "failed to drain QP: %d\n", ret);
return;
}

ret = ib_post_recv(qp, , _rwr);
if (ret) {
WARN_ONCE(ret, "failed to drain recv queue: %d\n", ret);
return;
}

ret = ib_post_send(qp, , _swr);
if (ret) {
WARN_ONCE(ret, "failed to drain send queue: %d\n", ret);
return;
}

wait_for_completion();
wait_for_completion();
}

Thoughts?
--
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/9] IB: add a proper completion queue abstraction

[Sagi Grimberg]

On 15/11/2015 14:55, Christoph Hellwig wrote:

On Sun, Nov 15, 2015 at 11:40:02AM +0200, Sagi Grimberg wrote:

I doubt INT_MAX is useful as a budget in any use-case. it can easily
hog the CPU. If the consumer is given access to poll a CQ, it must be
able to provide some way to budget it. Why not expose a budget argument
to the consumer?


Because in theory we could have a lot of sends completing before
we finally need to reap them.  I think that's more of a theoretical
than real issue.


Still, processing a CQ possibly forever is not something we'd want to
enable in an API, if a caller wants to do that anyway, it should loop
this call...



My preference would be to simply kill this mode though.  Allocate a IU
to each block request in SRP and only use the free_tx list for task
management and AEN/req_limit calls.  Then we can use a single CQ
and mark the regular I/O requests as unsignalled.


It might be better. I'd say that we keep this API and let Bart decide
if he wants to do that in srp. If he wants to convert srp, we can
always drop it.


AFAICS no other driver wants a similar polling mode as the SRP initiator
does for it's send queue.


iser worked in this mode in the past. But we changed that.
--
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 1/9] move blk_iopoll to limit and make it generally available

[Christoph Hellwig]

On Sun, Nov 15, 2015 at 10:48:41AM +0200, Sagi Grimberg wrote:
> I have attempted to convert iser to use blk_iopoll in the past, however
> I've seen inconsistent performance and latency skews (comparing to
> tasklets iser is using today). This was manifested in IOPs test cases
> where I ran multiple threads with higher queue-depth and not in
> sanitized pure latency (QD=1) test cases. Unfortunately I didn't have
> the time to pick it up since.
>
> I do have every intention of testing it again with this. If it still
> exist we will need to find the root-cause of it before converting
> drivers to use it.

Thanks.  If you see issue like that with high iops and high loads
the next thing to check are:

 a) increasing the budget (both in the main poll loop and the IB
code)
 b) try without the single jiffie time limit in the main softirq handler
--
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] st: Remove obsolete scsi_tape.max_pfn

[Geert Uytterhoeven]

Its last user was removed 10 years ago, in commit
8b05b773b6030de5 ("[SCSI] convert st to use scsi_execute_async").

Signed-off-by: Geert Uytterhoeven 
---
 drivers/scsi/st.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/scsi/st.h b/drivers/scsi/st.h
index b6486b5d86811227..8c732c8de0153eb1 100644
--- a/drivers/scsi/st.h
+++ b/drivers/scsi/st.h
@@ -148,8 +148,6 @@ struct scsi_tape {
int tape_type;
int long_timeout;   /* timeout for commands known to take long time 
*/
 
-   unsigned long max_pfn;  /* the maximum page number reachable by the HBA 
*/
-
/* Mode characteristics */
struct st_modedef modes[ST_NBR_MODES];
int current_mode;
-- 
1.9.1

--
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 1/9] move blk_iopoll to limit and make it generally available

[Sagi Grimberg]

On 15/11/2015 11:04, Or Gerlitz wrote:

On Sun, Nov 15, 2015 at 10:48 AM, Sagi Grimberg
 wrote:

Or is correct,

I have attempted to convert iser to use blk_iopoll in the past, however
I've seen inconsistent performance and latency skews (comparing to
tasklets iser is using today). This was manifested in IOPs test cases
where I ran multiple threads with higher queue-depth and not in
sanitized pure latency (QD=1) test cases. Unfortunately I didn't have
the time to pick it up since.

I do have every intention of testing it again with this. If it still
exist we will need to find the root-cause of it before converting
drivers to use it.


Good, this way (inconsistent performance and latency skews) or another
(all shines up) -- please
let us know your findings, best through commenting within V > 0 the
cover letter posts of this series



Hi Or & Co,

I ran some tests on the iser code with this patchset applied.
I can confirm that I did not see any performance degradations.
summary (on my test servers):
1  LUN:   ~530K  (IOPs)
2  LUNs:  ~1080K (IOPs)
4  LUNs:  ~1350K (IOPs)
8  LUNs:  ~1930K (IOPs)
16 LUns:  ~2250K (IOPs)

These results are true both for tasklet and iopoll.

So, I don't have anything smart to say here, the IO
stack (block, scsi) has gone through major changes since
the last time I looked into this, so it'll be pretty hard to figure
out what was the root cause back then...

Sagi.
--
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/9] IB: add a proper completion queue abstraction

[Christoph Hellwig]

On Sun, Nov 15, 2015 at 11:40:02AM +0200, Sagi Grimberg wrote:
> I doubt INT_MAX is useful as a budget in any use-case. it can easily
> hog the CPU. If the consumer is given access to poll a CQ, it must be
> able to provide some way to budget it. Why not expose a budget argument
> to the consumer?

Because in theory we could have a lot of sends completing before
we finally need to reap them.  I think that's more of a theoretical
than real issue.

My preference would be to simply kill this mode though.  Allocate a IU
to each block request in SRP and only use the free_tx list for task
management and AEN/req_limit calls.  Then we can use a single CQ
and mark the regular I/O requests as unsignalled.

AFAICS no other driver wants a similar polling mode as the SRP initiator
does for it's send queue.
--
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] drivers/message/fusion: constify mpt_pci_driver structures

[Julia Lawall]

The mpt_pci_driver structures are never modified, so declare them as const.

Done with the help of Coccinelle.

Signed-off-by: Julia Lawall 

---
 drivers/message/fusion/mptbase.c |8 +---
 drivers/message/fusion/mptbase.h |3 ++-
 drivers/message/fusion/mptctl.c  |2 +-
 drivers/message/fusion/mptlan.c  |2 +-
 4 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index 5dcc031..451e73c 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -140,7 +140,9 @@ static int   
MptDriverClass[MPT_MAX_PROTOCOL_DRIVERS];
 static MPT_EVHANDLERMptEvHandlers[MPT_MAX_PROTOCOL_DRIVERS];
/* Reset handler lookup table */
 static MPT_RESETHANDLER 
MptResetHandlers[MPT_MAX_PROTOCOL_DRIVERS];
-static struct mpt_pci_driver   
*MptDeviceDriverHandlers[MPT_MAX_PROTOCOL_DRIVERS];
+
+static const
+struct mpt_pci_driver *MptDeviceDriverHandlers[MPT_MAX_PROTOCOL_DRIVERS];
 
 #ifdef CONFIG_PROC_FS
 static struct proc_dir_entry   *mpt_proc_root_dir;
@@ -826,7 +828,7 @@ mpt_reset_deregister(u8 cb_idx)
  * @cb_idx: MPT protocol driver index
  */
 int
-mpt_device_driver_register(struct mpt_pci_driver * dd_cbfunc, u8 cb_idx)
+mpt_device_driver_register(const struct mpt_pci_driver *dd_cbfunc, u8 cb_idx)
 {
MPT_ADAPTER *ioc;
const struct pci_device_id *id;
@@ -855,7 +857,7 @@ mpt_device_driver_register(struct mpt_pci_driver * 
dd_cbfunc, u8 cb_idx)
 void
 mpt_device_driver_deregister(u8 cb_idx)
 {
-   struct mpt_pci_driver *dd_cbfunc;
+   const struct mpt_pci_driver *dd_cbfunc;
MPT_ADAPTER *ioc;
 
if (!cb_idx || cb_idx >= MPT_MAX_PROTOCOL_DRIVERS)
diff --git a/drivers/message/fusion/mptbase.h b/drivers/message/fusion/mptbase.h
index 813d463..e29e4be 100644
--- a/drivers/message/fusion/mptbase.h
+++ b/drivers/message/fusion/mptbase.h
@@ -920,7 +920,8 @@ extern int   mpt_event_register(u8 cb_idx, MPT_EVHANDLER 
ev_cbfunc);
 extern void mpt_event_deregister(u8 cb_idx);
 extern int  mpt_reset_register(u8 cb_idx, MPT_RESETHANDLER reset_func);
 extern void mpt_reset_deregister(u8 cb_idx);
-extern int  mpt_device_driver_register(struct mpt_pci_driver * dd_cbfunc, 
u8 cb_idx);
+extern int  mpt_device_driver_register(const struct mpt_pci_driver 
*dd_cbfunc,
+ u8 cb_idx);
 extern void mpt_device_driver_deregister(u8 cb_idx);
 extern MPT_FRAME_HDR   *mpt_get_msg_frame(u8 cb_idx, MPT_ADAPTER *ioc);
 extern void mpt_free_msg_frame(MPT_ADAPTER *ioc, MPT_FRAME_HDR *mf);
diff --git a/drivers/message/fusion/mptctl.c b/drivers/message/fusion/mptctl.c
index 02b5f69..7d051af 100644
--- a/drivers/message/fusion/mptctl.c
+++ b/drivers/message/fusion/mptctl.c
@@ -2990,7 +2990,7 @@ mptctl_remove(struct pci_dev *pdev)
 {
 }
 
-static struct mpt_pci_driver mptctl_driver = {
+static const struct mpt_pci_driver mptctl_driver = {
   .probe   = mptctl_probe,
   .remove  = mptctl_remove,
 };
diff --git a/drivers/message/fusion/mptlan.c b/drivers/message/fusion/mptlan.c
index cbe9607..4b20af4 100644
--- a/drivers/message/fusion/mptlan.c
+++ b/drivers/message/fusion/mptlan.c
@@ -1443,7 +1443,7 @@ mptlan_remove(struct pci_dev *pdev)
}
 }
 
-static struct mpt_pci_driver mptlan_driver = {
+static const struct mpt_pci_driver mptlan_driver = {
.probe  = mptlan_probe,
.remove = mptlan_remove,
 };

--
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] ib_srp: initialize dma_length in srp_map_idb

[Sagi Grimberg]

On 15/11/2015 19:59, Christoph Hellwig wrote:

Without this sg_dma_len will return 0 on architectures tha have
the dma_length field.

Signed-off-by: Christoph Hellwig 
---
  drivers/infiniband/ulp/srp/ib_srp.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c 
b/drivers/infiniband/ulp/srp/ib_srp.c
index 32f7962..445c0a6 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1520,6 +1520,9 @@ static int srp_map_idb(struct srp_rdma_ch *ch, struct 
srp_request *req,
state.sg_nents = 1;
sg_set_buf(idb_sg, req->indirect_desc, idb_len);
idb_sg->dma_address = req->indirect_dma_addr; /* hack! */
+#ifdef CONFIG_NEED_SG_DMA_LENGTH
+   idb_sg->dma_length = idb_sg->length;/* hack^2 */
+#endif


:)

We should really get this properly map/unmap per IO at some point.
Probably do it in both code paths...

Having said that,

Looks fine,

Reviewed-by: Sagi Grimberg 
--
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] ib_srp: initialize dma_length in srp_map_idb

[Or Gerlitz]

On Sun, Nov 15, 2015, Sagi Grimberg  wrote:
> On 15/11/2015 19:59, Christoph Hellwig wrote:

>> Without this sg_dma_len will return 0 on architectures tha have
>> the dma_length field.

and what wrong with that?

Christoph, probably typo here? "tha" needs to be "that"

>> Signed-off-by: Christoph Hellwig 
>> ---
>>   drivers/infiniband/ulp/srp/ib_srp.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
>> b/drivers/infiniband/ulp/srp/ib_srp.c
>> index 32f7962..445c0a6 100644
>> --- a/drivers/infiniband/ulp/srp/ib_srp.c
>> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
>> @@ -1520,6 +1520,9 @@ static int srp_map_idb(struct srp_rdma_ch *ch,
>> struct srp_request *req,
>> state.sg_nents = 1;
>> sg_set_buf(idb_sg, req->indirect_desc, idb_len);
>> idb_sg->dma_address = req->indirect_dma_addr; /* hack! */
>> +#ifdef CONFIG_NEED_SG_DMA_LENGTH
>> +   idb_sg->dma_length = idb_sg->length;  /* hack^2 */
>> +#endif
>
>
> :)
>
> We should really get this properly map/unmap per IO at some point.
> Probably do it in both code paths...

Sagi, can you please elaborate a little further on the problem, srpt
WA, what do we do in isert and what is the proposed not WA solution?

> Having said that,
> Looks fine,
> Reviewed-by: Sagi Grimberg 
--
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 v5 00/11] exynos-ufs: add support for Exynos

[Alim Akhtar]

Hi Kishon,

Any more concern on the PHY part of this series?

Thanks!

On Mon, Nov 9, 2015 at 10:56 AM, Alim Akhtar  wrote:
> This patch-set introduces UFS (Universal Flash Storage) host support
> for Samsung Exynos SoC. Mostly, it consists of UFS PHY and host specific 
> driver.
> And it also contains some quirks handling for Exynos.
>
> NOTE: ** This series has a dependency on [4]. **
>
> -Changes since v4:
> * Removed platform specific PHY ops as suggested by Kishon
> * Rebased on the top of Yaniv Gardi's work [4]
> * make use of newly introduce ufshcd_{get,set}_variant
> * other small changes and improvements.
> * rebased on the top of linux next-20151109
>
> -Changes since v3:
> * Fixed compilation warrings as reported by "Kbuild Test Robot"[5].
> * Restructure the driver to make it as a platform driver, rebased on top of 
> [4].
> * Addressed review comments from Arnd Bergmann[5].
> * Other misc changes and improvements.
>
> -Changes since v2:
> * Addressed review comments from Kishon[1] and Rob Herring [2]
> * Splited ufs dt binding documetation from ufs driver patch
>
> -Changes since v1:
> * Addressed review comments from Alexey[3] and various review comments from 
> Amit.
> * Updated email id of Seungwon as his samsung id is void now.
> * Added ufs platform data
>
> [1]-> https://lkml.org/lkml/2015/9/18/29
> [2]-> https://lkml.org/lkml/2015/9/21/668
> [3]-> https://lkml.org/lkml/2015/8/23/124
> [4]-> https://lkml.org/lkml/2015/10/28/271
> [5]-> https://lkml.org/lkml/2015/10/1/402
>
> This patch set is tested on exynos7-espresso board.
>
>
> Alim Akhtar (1):
>   Documentation: samsung-phy: Add dt bindings for UFS
>
> Seungwon Jeon (10):
>   phy: exynos-ufs: add UFS PHY driver for EXYNOS SoC
>   scsi: ufs: add quirk to contain unconformable utrd field
>   scsi: ufs: add quirk to fix mishandling utrlclr/utmrlclr
>   scsi: ufs: add quirk not to allow reset of interrupt aggregation
>   scsi: ufs: add quirk to enable host controller without hce
>   scsi: ufs: add specific callback for nexus type
>   scsi: ufs: add add specific callback for hibern8
>   scsi: ufs: make ufshcd_config_pwr_mode of non-static func
>   Documentation: devicetree: ufs: Add DT bindings for exynos UFS host
> controller
>   scsi: ufs-exynos: add UFS host support for Exynos SoCs
>
>  .../devicetree/bindings/phy/samsung-phy.txt|   22 +
>  .../devicetree/bindings/ufs/ufs-exynos.txt |  104 ++
>  drivers/phy/Kconfig|7 +
>  drivers/phy/Makefile   |1 +
>  drivers/phy/phy-exynos-ufs.c   |  241 
>  drivers/phy/phy-exynos-ufs.h   |   85 ++
>  drivers/phy/phy-exynos7-ufs.h  |   89 ++
>  drivers/scsi/ufs/Kconfig   |   12 +
>  drivers/scsi/ufs/Makefile  |1 +
>  drivers/scsi/ufs/ufs-exynos-hw.c   |  131 ++
>  drivers/scsi/ufs/ufs-exynos-hw.h   |   43 +
>  drivers/scsi/ufs/ufs-exynos.c  | 1304 
> 
>  drivers/scsi/ufs/ufs-exynos.h  |  247 
>  drivers/scsi/ufs/ufshcd.c  |  168 ++-
>  drivers/scsi/ufs/ufshcd.h  |   54 +
>  drivers/scsi/ufs/ufshci.h  |   26 +-
>  drivers/scsi/ufs/unipro.h  |   47 +
>  include/linux/phy/phy-exynos-ufs.h |   85 ++
>  18 files changed, 2647 insertions(+), 20 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/ufs/ufs-exynos.txt
>  create mode 100644 drivers/phy/phy-exynos-ufs.c
>  create mode 100644 drivers/phy/phy-exynos-ufs.h
>  create mode 100644 drivers/phy/phy-exynos7-ufs.h
>  create mode 100644 drivers/scsi/ufs/ufs-exynos-hw.c
>  create mode 100644 drivers/scsi/ufs/ufs-exynos-hw.h
>  create mode 100644 drivers/scsi/ufs/ufs-exynos.c
>  create mode 100644 drivers/scsi/ufs/ufs-exynos.h
>  create mode 100644 include/linux/phy/phy-exynos-ufs.h
>
> --
> 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



-- 
Regards,
Alim
--
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] ib_srp: initialize dma_length in srp_map_idb

[Christoph Hellwig]

Without this sg_dma_len will return 0 on architectures tha have
the dma_length field.

Signed-off-by: Christoph Hellwig 
---
 drivers/infiniband/ulp/srp/ib_srp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c 
b/drivers/infiniband/ulp/srp/ib_srp.c
index 32f7962..445c0a6 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1520,6 +1520,9 @@ static int srp_map_idb(struct srp_rdma_ch *ch, struct 
srp_request *req,
state.sg_nents = 1;
sg_set_buf(idb_sg, req->indirect_desc, idb_len);
idb_sg->dma_address = req->indirect_dma_addr; /* hack! */
+#ifdef CONFIG_NEED_SG_DMA_LENGTH
+   idb_sg->dma_length = idb_sg->length;  /* hack^2 */
+#endif
ret = srp_map_finish_fr(, ch);
if (ret < 0)
return ret;
-- 
1.9.1

--
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