On Thu, 2014-10-02 at 10:34 +0300, Sagi Grimberg wrote: > 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... > Er, yes of course. > 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); > Applied. Thanks Sagi! -- 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
