(2014/06/05 21:55), David Laight wrote: > From: Toshiaki Makita >> (2014/06/05 20:03), David Laight wrote: >>> From: Toshiaki Makita >>>> br_manage_promisc() incorrectly expects br_auto_port() to return only 0 >>>> or 1, while it actually returns flags, i.e., a subset of BR_AUTO_MASK. >>>> >>>> Signed-off-by: Toshiaki Makita <[email protected]> >>>> --- >>>> net/bridge/br_if.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c >>>> index a08d2b8..6a07a40 100644 >>>> --- a/net/bridge/br_if.c >>>> +++ b/net/bridge/br_if.c >>>> @@ -153,7 +153,7 @@ void br_manage_promisc(struct net_bridge *br) >>>> * This lets us disable promiscuous mode and write >>>> * this config to hw. >>>> */ >>>> - if (br->auto_cnt <= br_auto_port(p)) >>>> + if (br->auto_cnt <= !!br_auto_port(p)) >>>> br_port_clear_promisc(p); >>>> else >>>> br_port_set_promisc(p); >>> >>> Why not the less confusing: >>> if (br->auto_cnt || br_auto_port(p)) >>> and reverse the then/else lines? >> >> I'm respecting the original style, but I'm not particular about this style. >> I'll make less confusing one, thanks :) >> >> (Your suggested condition is not exactly the same as current one, even >> if reversing if/else. v2 will be different than it. Anyway, thanks.) > > A quick truth table: > auto_cnt auto_port set/clear > 0 0 clear > 0 1 clear > 1 0 set > 1 1 clear > 2+ 0/1 clear
The last line should be set. Thanks, Toshiaki Makita > > So you want: > if (br->auto_cnt && !br_auto_port(p)) > br_port_set_promisc(p); > else > br_port_clear_promisc(p); > > Does seem like a strange condition. > > David
