On 10/2/2014 3:49 AM, Nicholas A. Bellinger wrote:
On Wed, 2014-10-01 at 13:45 +0300, Or Gerlitz wrote:
Some static checker complaints on isert code, worth looking
Or.
# make CHECK="../tools/smatch/smatch -p=kernel --two-passes" C=2 CC=gcc
SUBDIRS=drivers/infiniband/ulp/isert -j 12 modules | tee warns.txt
CHECK drivers/infiniband/ulp/isert/ib_isert.c
drivers/infiniband/ulp/isert/ib_isert.c:701 isert_connect_request()
warn: inconsistent returns 'sem:&isert_np->np_sem'.
Locked on: line 571
line 581
line 701
Unlocked on: line 683
drivers/infiniband/ulp/isert/ib_isert.c:3155 isert_accept_np() error:
double lock 'sem:&isert_np->np_sem'
drivers/infiniband/ulp/isert/ib_isert.c:3155 isert_accept_np warn:
unused return: ret = down_interruptible()
These are false positives from using a sempahore to signal incoming
login requests between CMA generated isert_connect_request() to login
kernel thread context in isert_accept_np().
A struct completion would be a better fit here, and makes smatch happy.
How about the following..?
You'd need to reinit_completion each accept_np (which you don't
in the patch below). This was a state conditioned event before
(resembles a completion).
The problem is that we can get multiple CMA connect request in
parallel and it is not synchronized with accept_np what so ever...
IMO Semaphore is the correct usage here. And it actually survive stress
login tests...
I'd just suggest the fix below:
commit 6e228063baa3fe6c6f8d8cd356dbdf9a0f0a2650
Author: Sagi Grimberg <sagig@xxxxxxxxxxxx>
Date: Thu Oct 2 10:23:06 2014 +0300
iser-target: Fix smatch warning
Unused return value from down_interruptible
Reported-by: Or Gerlitz <ogerlitz@xxxxxxxxxxxx>
Signed-off-by: Sagi Grimberg <sagig@xxxxxxxxxxxx>
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c
b/drivers/infiniband/ulp/isert/ib_isert.c
index f751e7c..7826b9b 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -3142,7 +3142,7 @@ isert_accept_np(struct iscsi_np *np, struct
iscsi_conn *conn)
accept_wait:
ret = down_interruptible(&isert_np->np_sem);
- if (max_accept > 5)
+ if (ret || max_accept > 5)
return -ENODEV;
spin_lock_bh(&np->np_thread_lock);
--nab
iser-target: Convert isert_np->np_sem to struct completion
This patch converts isert_np->np_sem to use a proper struct completion
to eliminate the following smatch warnings related to using a semaphore
to signal completion between isert_connect_request() -> isert_accept_np().
drivers/infiniband/ulp/isert/ib_isert.c:701 isert_connect_request()
warn: inconsistent returns 'sem:&isert_np->np_sem'.
Locked on: line 571
line 581
line 701
Unlocked on: line 683
drivers/infiniband/ulp/isert/ib_isert.c:3155 isert_accept_np() error:
double lock 'sem:&isert_np->np_sem'
drivers/infiniband/ulp/isert/ib_isert.c:3155 isert_accept_np warn:
unused return: ret = down_interruptible()
Reported-by: Or Gerlitz <ogerlitz@xxxxxxxxxxxx>
Signed-off-by: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.
index d4c7928..b3d0bae 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -677,8 +677,8 @@ isert_connect_request(struct rdma_cm_id *cma_id, struct rdma_cm_event *ev
list_add_tail(&isert_conn->conn_accept_node, &isert_np->np_accept_list);
mutex_unlock(&isert_np->np_accept_mutex);
- pr_debug("isert_connect_request() up np_sem np: %p\n", np);
- up(&isert_np->np_sem);
+ pr_debug("isert_connect_request() complete np_accept_comp np: %p\n", np);
+ complete(&isert_np->np_accept_comp);
return 0;
out_conn_dev:
@@ -3011,9 +3011,9 @@ isert_setup_np(struct iscsi_np *np,
pr_err("Unable to allocate struct isert_np\n");
return -ENOMEM;
}
- sema_init(&isert_np->np_sem, 0);
mutex_init(&isert_np->np_accept_mutex);
INIT_LIST_HEAD(&isert_np->np_accept_list);
+ init_completion(&isert_np->np_accept_comp);
init_completion(&isert_np->np_login_comp);
sa = (struct sockaddr *)ksockaddr;
@@ -3151,8 +3151,8 @@ isert_accept_np(struct iscsi_np *np, struct iscsi_conn *conn)
int max_accept = 0, ret;
accept_wait:
- ret = down_interruptible(&isert_np->np_sem);
- if (max_accept > 5)
+ ret = wait_for_completion_interruptible(&isert_np->np_accept_comp);
+ if (ret || max_accept > 5)
return -ENODEV;
spin_lock_bh(&np->np_thread_lock);
diff --git a/drivers/infiniband/ulp/isert/ib_isert.h b/drivers/infiniband/ulp/isert/ib_isert.
index 04f51f7..21b018b 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.h
+++ b/drivers/infiniband/ulp/isert/ib_isert.h
@@ -182,9 +182,9 @@ struct isert_device {
};
struct isert_np {
- struct semaphore np_sem;
struct rdma_cm_id *np_cm_id;
struct mutex np_accept_mutex;
struct list_head np_accept_list;
+ struct completion np_accept_comp;
struct completion np_login_comp;
};
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
