Re: [PATCH for-next 0/9] mlx4 changes in virtual GID management
From: Roland Dreier Date: Sun, 5 Apr 2015 07:51:08 -0700 > On Sat, Apr 4, 2015 at 10:15 PM, Or Gerlitz wrote: >> Indeed. No maintainer voice makes it kind of impossible for >> discussions to converge. What happens over the last years is that when >> there's no easy consensus on matter Y, everyone stops breathing and >> wait to see what happens on the rc1 night, b/c Roland doesn't spell >> his view/preference (nor exposes his for-next branch till the last >> minute, see now) many times it seems more as coin flipping. > > To me this attitude shows a failure of the community. If I need to > make every decision, then that doesn't scale. People can ask > questions a lot more easily than I can answer them. No, you need to step in and be the benevolent dictator when people have their thumbs up their asses and can't make up their minds, otherwise things grind to a halt. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/16] ib_srpt: fix error return code
On 04/05/15 14:06, Julia Lawall wrote:
Return a negative error code on failure.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
//
@@
identifier ret; expression e1,e2;
@@
(
if (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
//
In the first case, the printk is changed to use the ret value, rather
than duplicating PTR_ERR(ch->thread). This requires changing the format
from %ld to %d, to account for the type of ret.
Signed-off-by: Julia Lawall
---
drivers/infiniband/ulp/srpt/ib_srpt.c |6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 6e0a477..5ec98f5 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2143,8 +2143,8 @@ retry:
ch->thread = kthread_run(srpt_compl_thread, ch, "ib_srpt_compl");
if (IS_ERR(ch->thread)) {
- printk(KERN_ERR "failed to create kernel thread %ld\n",
- PTR_ERR(ch->thread));
+ ret = PTR_ERR(ch->thread);
+ printk(KERN_ERR "failed to create kernel thread %d\n", ret);
ch->thread = NULL;
goto err_destroy_qp;
}
@@ -2590,6 +2590,7 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
" configured yet for initiator %s.\n", ch->sess_name);
rej->reason = __constant_cpu_to_be32(
SRP_LOGIN_REJ_CHANNEL_LIMIT_REACHED);
+ ret = -EINVAL;
goto destroy_ib;
}
@@ -2598,6 +2599,7 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
rej->reason = __constant_cpu_to_be32(
SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES);
pr_debug("Failed to create session\n");
+ ret = PTR_ERR(ch->sess);
goto deregister_session;
}
ch->sess->se_node_acl = &nacl->nacl;
This is not the proper way to fix srpt_cm_req_recv(). The kthread_run()
call must be moved from srpt_create_ch_ib() to after the session
registration code in srpt_cm_req_recv(), and the session registration
call + the kthread_run() call must be protected via a mutex against
concurrent HCA hot-plug removal events (see also srpt_remove_one() and
srpt_remove_sdev()).
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH for-next 0/9] mlx4 changes in virtual GID management
On Sat, Apr 4, 2015 at 10:15 PM, Or Gerlitz wrote: > Indeed. No maintainer voice makes it kind of impossible for > discussions to converge. What happens over the last years is that when > there's no easy consensus on matter Y, everyone stops breathing and > wait to see what happens on the rc1 night, b/c Roland doesn't spell > his view/preference (nor exposes his for-next branch till the last > minute, see now) many times it seems more as coin flipping. To me this attitude shows a failure of the community. If I need to make every decision, then that doesn't scale. People can ask questions a lot more easily than I can answer them. In general if a consensus emerges, I'm pretty likely to trust it. In particular, as Sean mentioned, I tend to trust vendors about low-level drivers, although of course I sometimes catch mistakes even then. But for changes that touch the core, when there's a disagreement, you can't expect me to be the one who always solves it. I might have an opinion, but in a lot of cases, both sides might have a point and the only way forward is to come up with a new idea that works for everyone. And I'm not smart enough to come up with that solution every time. - R. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/16] fix error return code
The complate semantic patch that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
//
@ok exists@
identifier f,ret,i;
expression e;
constant c;
@@
// identify a function that returns a negative return value at least once.
f(...) {
... when any
(
return -c@i;
|
ret = -c@i;
... when != ret = e
return ret;
|
if (ret < 0) { ... return ret; }
)
... when any
}
@r exists@
identifier ret,ok.f,fn;
expression e1,e2,e3,e4,e5,e6,x;
statement S,S1;
position p1,p2,p3;
@@
// identify a case where the return variable is set to a non-negative value
// and then returned in error-handling code
f(...) {
... when any
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != \(ret = e1\|ret++\|ret--\|ret+=e1\|ret-=e1\)
when != &ret
when any
(
if (<+... ret = e5 ...+>) S1
|
if (<+... &ret ...+>) S1
|
if@p2(<+...x = fn(...)...+>)
{
... when != ret = e6
when forall
return@p3 ret;
}
|
break;
|
x = fn(...)
... when != \(ret = e4\|ret++\|ret--\|ret+=e4\|ret-=e4\)
when != &ret
(
if (<+... ret = e3 ...+>) S
|
if (<+... &ret ...+>) S
|
if@p2(<+...\(x != 0\|x < 0\|x == NULL\|IS_ERR(x)\)...+>)
{
... when != ret = e2
when forall
return@p3 ret;
}
)
)
... when any
}
@printer depends on r@
position p;
identifier ok.f,pr;
constant char [] c;
@@
f(...) { <...pr@p(...,c,...)...> }
@bad0 exists@
identifier r.ret,ok.f,g != {ERR_PTR,IS_ERR};
position p != printer.p;
@@
f(...) { ... when any
g@p(...,ret,...)
... when any
}
@bad depends on !bad0 exists@
position r.p1,r.p2;
statement S1,S2;
identifier r.ret;
expression e1;
@@
// ignore the above if there is some path where the variable is set to
// something else
(
if@p1 (\(ret < 0\|ret != 0\)) S1
|
ret@p1 = 0
)
... when any
\(ret = e1\|ret++\|ret--\|ret+=e1\|ret-=e1\|&ret\)
... when any
if@p2(...) S2
@bad1 depends on !bad0 && !bad exists@
position r.p2;
statement S2;
identifier r.ret;
expression e1;
constant c;
@@
ret = -c
... when != \(ret = e1\|ret++\|ret--\|ret+=e1\|ret-=e1\)
when != &ret
when any
if@p2(...) S2
@bad2 depends on !bad0 && !bad && !bad1 exists@
position r.p1,r.p2;
identifier r.ret;
expression e1;
statement S2;
constant c;
@@
// likewise ignore it if there has been an intervening return
ret@p1 = 0
... when != if (...) { ... ret = e1 ... return ret; }
when != if (...) { ... return -c; }
when any
if@p2(...) S2
@script:python depends on !bad0 && !bad && !bad1 && !bad2@
p1 << r.p1;
p2 << r.p2;
p3 << r.p3;
@@
cocci.print_main("",p1)
cocci.print_secs("",p2)
cocci.print_secs("",p3)
//
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 12/16] ib_srpt: fix error return code
Return a negative error code on failure.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
//
@@
identifier ret; expression e1,e2;
@@
(
if (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
//
In the first case, the printk is changed to use the ret value, rather
than duplicating PTR_ERR(ch->thread). This requires changing the format
from %ld to %d, to account for the type of ret.
Signed-off-by: Julia Lawall
---
drivers/infiniband/ulp/srpt/ib_srpt.c |6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 6e0a477..5ec98f5 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2143,8 +2143,8 @@ retry:
ch->thread = kthread_run(srpt_compl_thread, ch, "ib_srpt_compl");
if (IS_ERR(ch->thread)) {
- printk(KERN_ERR "failed to create kernel thread %ld\n",
- PTR_ERR(ch->thread));
+ ret = PTR_ERR(ch->thread);
+ printk(KERN_ERR "failed to create kernel thread %d\n", ret);
ch->thread = NULL;
goto err_destroy_qp;
}
@@ -2590,6 +2590,7 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
" configured yet for initiator %s.\n", ch->sess_name);
rej->reason = __constant_cpu_to_be32(
SRP_LOGIN_REJ_CHANNEL_LIMIT_REACHED);
+ ret = -EINVAL;
goto destroy_ib;
}
@@ -2598,6 +2599,7 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
rej->reason = __constant_cpu_to_be32(
SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES);
pr_debug("Failed to create session\n");
+ ret = PTR_ERR(ch->sess);
goto deregister_session;
}
ch->sess->se_node_acl = &nacl->nacl;
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
