On Thu, 2015-04-30 at 10:58 +0200, Bart Van Assche wrote:
> Reception of a DREQ message only causes the state of a single
> channel to change. Modify the SRP initiator such that channel
> and target connection state are tracked separately. This patch
> avoids that following false positive warning can be reported
> by srp_destroy_qp():
>
> WARNING: at drivers/infiniband/ulp/srp/ib_srp.c:617 srp_destroy_qp+0xa6/0x120 [ib_srp]()
> Call Trace:
> [<ffffffff8106e10f>] warn_slowpath_common+0x7f/0xc0
> [<ffffffff8106e16a>] warn_slowpath_null+0x1a/0x20
> [<ffffffffa0440226>] srp_destroy_qp+0xa6/0x120 [ib_srp]
> [<ffffffffa0440322>] srp_free_ch_ib+0x82/0x1e0 [ib_srp]
> [<ffffffffa044408b>] srp_create_target+0x7ab/0x998 [ib_srp]
> [<ffffffff81346f60>] dev_attr_store+0x20/0x30
> [<ffffffff811dd90f>] sysfs_write_file+0xef/0x170
> [<ffffffff8116d248>] vfs_write+0xc8/0x190
> [<ffffffff8116d411>] sys_write+0x51/0x90
>
> Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
> Cc: Sagi Grimberg <sagig@xxxxxxxxxxxx>
> Cc: Sebastian Parschauer <sebastian.riemer@xxxxxxxxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> #v3.19
> ---
> drivers/infiniband/ulp/srp/ib_srp.c | 7 ++++---
> drivers/infiniband/ulp/srp/ib_srp.h | 1 +
> 2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 5ce6cfd..0eb07d3 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -465,14 +465,13 @@ static struct srp_fr_pool *srp_alloc_fr_pool(struct srp_target_port *target)
> */
> static void srp_destroy_qp(struct srp_rdma_ch *ch)
> {
> - struct srp_target_port *target = ch->target;
> static struct ib_qp_attr attr = { .qp_state = IB_QPS_ERR };
> static struct ib_recv_wr wr = { .wr_id = SRP_LAST_WR_ID };
> struct ib_recv_wr *bad_wr;
> int ret;
>
> /* Destroying a QP and reusing ch->done is only safe if not connected */
> - WARN_ON_ONCE(target->connected);
> + WARN_ON_ONCE(ch->connected);
>
> ret = ib_modify_qp(ch->qp, &attr, IB_QP_STATE);
> WARN_ONCE(ret, "ib_cm_init_qp_attr() returned %d\n", ret);
> @@ -836,6 +835,7 @@ static void srp_disconnect_target(struct srp_target_port *target)
>
> for (i = 0; i < target->ch_count; i++) {
> ch = &target->ch[i];
> + ch->connected = false;
> if (ch->cm_id && ib_send_cm_dreq(ch->cm_id, NULL, 0)) {
> shost_printk(KERN_DEBUG, target->scsi_host,
> PFX "Sending CM DREQ failed\n");
> @@ -1017,6 +1017,7 @@ static int srp_connect_ch(struct srp_rdma_ch *ch, bool multich)
> switch (ch->status) {
> case 0:
> srp_change_conn_state(target, true);
> + ch->connected = true;
> return 0;
>
> case SRP_PORT_REDIRECT:
> @@ -2367,7 +2368,7 @@ static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
> case IB_CM_DREQ_RECEIVED:
> shost_printk(KERN_WARNING, target->scsi_host,
> PFX "DREQ received - connection closed\n");
> - srp_change_conn_state(target, false);
> + ch->connected = false;
So, in this patch, you modify disconnect to set srp_change_conn_state()
to false for the target, then loop through all the channels sending
cm_dreq's, and on the receiving side, you modify the cm_dreq handler to
set each channel to false. However, once you get to 0 channels open,
shouldn't you then set the target state to false too just to keep things
consistent?
> if (ib_send_cm_drep(cm_id, NULL, 0))
> shost_printk(KERN_ERR, target->scsi_host,
> PFX "Sending CM DREP failed\n");
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
> index a611556..95a4471 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.h
> +++ b/drivers/infiniband/ulp/srp/ib_srp.h
> @@ -170,6 +170,7 @@ struct srp_rdma_ch {
>
> struct completion tsk_mgmt_done;
> u8 tsk_mgmt_status;
> + bool connected;
> };
>
> /**
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG KeyID: 0E572FDD
Attachment:
signature.asc
Description: This is a digitally signed message part
