* [PATCH] cnic: fix double initalization of bnx2 based cards @ 2011-03-08 18:56 Neil Horman 2011-03-08 18:59 ` Ben Hutchings 0 siblings, 1 reply; 8+ messages in thread From: Neil Horman @ 2011-03-08 18:56 UTC (permalink / raw) To: netdev Cc: Neil Horman, David S. Miller, Michael Chan, Dmitry Kravkov, Eddie Wai, Eilon Greenstein bnx2 cards can work with the cnic driver, but when the cnic driver detects a bnx2 card, is_cnic_dev erroneously calls the initalization routines for both bnx2 and bnx2x (the former being a regex subset of the later). This causes initalization of bnx2 to unilaterally fail in the cnic driver, which, while not catastrophic, is definately not expected. Fix this by choosing either the bnx2 or bnx2x initalization path, not both Signed-off-by: Neil Horman <nhorman@tuxdriver.com> CC: David S. Miller <davem@davemloft.net> CC: Michael Chan <mchan@broadcom.com> CC: Dmitry Kravkov <dmitry@broadcom.com> CC: Eddie Wai <waie@broadcom.com> CC: Eilon Greenstein <eilong@broadcom.com> --- drivers/net/cnic.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/net/cnic.c b/drivers/net/cnic.c index 271a1f0..18b59ad 100644 --- a/drivers/net/cnic.c +++ b/drivers/net/cnic.c @@ -5292,7 +5292,7 @@ static struct cnic_dev *is_cnic_dev(struct net_device *dev) if (!strcmp(drvinfo.driver, "bnx2")) cdev = init_bnx2_cnic(dev); - if (!strcmp(drvinfo.driver, "bnx2x")) + else if (!strcmp(drvinfo.driver, "bnx2x")) cdev = init_bnx2x_cnic(dev); if (cdev) { write_lock(&cnic_dev_lock); -- 1.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] cnic: fix double initalization of bnx2 based cards 2011-03-08 18:56 [PATCH] cnic: fix double initalization of bnx2 based cards Neil Horman @ 2011-03-08 18:59 ` Ben Hutchings 2011-03-08 19:07 ` Michael Chan 0 siblings, 1 reply; 8+ messages in thread From: Ben Hutchings @ 2011-03-08 18:59 UTC (permalink / raw) To: Neil Horman Cc: netdev, David S. Miller, Michael Chan, Dmitry Kravkov, Eddie Wai, Eilon Greenstein On Tue, 2011-03-08 at 13:56 -0500, Neil Horman wrote: > bnx2 cards can work with the cnic driver, but when the cnic driver detects a > bnx2 card, is_cnic_dev erroneously calls the initalization routines for both > bnx2 and bnx2x (the former being a regex subset of the later). Since when does strcmp() do a regex match? And why is a function named 'is_cnic_dev' doing initialisation, anyway? Ben. > This causes > initalization of bnx2 to unilaterally fail in the cnic driver, which, while not > catastrophic, is definately not expected. Fix this by choosing either the bnx2 > or bnx2x initalization path, not both > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > CC: David S. Miller <davem@davemloft.net> > CC: Michael Chan <mchan@broadcom.com> > CC: Dmitry Kravkov <dmitry@broadcom.com> > CC: Eddie Wai <waie@broadcom.com> > CC: Eilon Greenstein <eilong@broadcom.com> > --- > drivers/net/cnic.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/net/cnic.c b/drivers/net/cnic.c > index 271a1f0..18b59ad 100644 > --- a/drivers/net/cnic.c > +++ b/drivers/net/cnic.c > @@ -5292,7 +5292,7 @@ static struct cnic_dev *is_cnic_dev(struct net_device *dev) > > if (!strcmp(drvinfo.driver, "bnx2")) > cdev = init_bnx2_cnic(dev); > - if (!strcmp(drvinfo.driver, "bnx2x")) > + else if (!strcmp(drvinfo.driver, "bnx2x")) > cdev = init_bnx2x_cnic(dev); > if (cdev) { > write_lock(&cnic_dev_lock); -- Ben Hutchings, Senior Software Engineer, Solarflare Communications Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cnic: fix double initalization of bnx2 based cards 2011-03-08 18:59 ` Ben Hutchings @ 2011-03-08 19:07 ` Michael Chan 2011-03-08 19:36 ` Neil Horman 0 siblings, 1 reply; 8+ messages in thread From: Michael Chan @ 2011-03-08 19:07 UTC (permalink / raw) To: Ben Hutchings Cc: Neil Horman, netdev@vger.kernel.org, David S. Miller, Dmitry Kravkov, Eddie Wai, Eilon Greenstein On Tue, 2011-03-08 at 10:59 -0800, Ben Hutchings wrote: > On Tue, 2011-03-08 at 13:56 -0500, Neil Horman wrote: > > bnx2 cards can work with the cnic driver, but when the cnic driver detects a > > bnx2 card, is_cnic_dev erroneously calls the initalization routines for both > > bnx2 and bnx2x (the former being a regex subset of the later). > > Since when does strcmp() do a regex match? Yeah, strcmp() does NULL-terminated string compare, right? > > And why is a function named 'is_cnic_dev' doing initialisation, anyway? Just data structure init if a supported device is detected. > > Ben. > > > This causes > > initalization of bnx2 to unilaterally fail in the cnic driver, which, while not > > catastrophic, is definately not expected. Fix this by choosing either the bnx2 > > or bnx2x initalization path, not both > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > > CC: David S. Miller <davem@davemloft.net> > > CC: Michael Chan <mchan@broadcom.com> > > CC: Dmitry Kravkov <dmitry@broadcom.com> > > CC: Eddie Wai <waie@broadcom.com> > > CC: Eilon Greenstein <eilong@broadcom.com> > > --- > > drivers/net/cnic.c | 2 +- > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/net/cnic.c b/drivers/net/cnic.c > > index 271a1f0..18b59ad 100644 > > --- a/drivers/net/cnic.c > > +++ b/drivers/net/cnic.c > > @@ -5292,7 +5292,7 @@ static struct cnic_dev *is_cnic_dev(struct net_device *dev) > > > > if (!strcmp(drvinfo.driver, "bnx2")) > > cdev = init_bnx2_cnic(dev); > > - if (!strcmp(drvinfo.driver, "bnx2x")) > > + else if (!strcmp(drvinfo.driver, "bnx2x")) > > cdev = init_bnx2x_cnic(dev); > > if (cdev) { > > write_lock(&cnic_dev_lock); > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cnic: fix double initalization of bnx2 based cards 2011-03-08 19:07 ` Michael Chan @ 2011-03-08 19:36 ` Neil Horman 2011-03-08 19:40 ` Ben Hutchings 2011-03-08 19:43 ` Michael Chan 0 siblings, 2 replies; 8+ messages in thread From: Neil Horman @ 2011-03-08 19:36 UTC (permalink / raw) To: Michael Chan Cc: Ben Hutchings, netdev@vger.kernel.org, David S. Miller, Dmitry Kravkov, Eddie Wai, Eilon Greenstein On Tue, Mar 08, 2011 at 11:07:57AM -0800, Michael Chan wrote: > > On Tue, 2011-03-08 at 10:59 -0800, Ben Hutchings wrote: > > On Tue, 2011-03-08 at 13:56 -0500, Neil Horman wrote: > > > bnx2 cards can work with the cnic driver, but when the cnic driver detects a > > > bnx2 card, is_cnic_dev erroneously calls the initalization routines for both > > > bnx2 and bnx2x (the former being a regex subset of the later). > > > > Since when does strcmp() do a regex match? > > Yeah, strcmp() does NULL-terminated string compare, right? > Sorry, poor choice of words. It doesn't do a regex match, I only ment to illustrate that bnx2 is a substring of bnx2x. strcmp does this: strcmp(const char *str1, const char *str2) { ... while (*s1 || *s2) { ... } ... } Since bnx2 is a substring of bnx2 both if clauses are a match, since you'll hit the null terminator of the shorter string in both, which means we call both initalization functions. I'll leave commentary on initalization in is_cnic_dev to the authors :) Neil > > > > > Ben. > > > > > This causes > > > initalization of bnx2 to unilaterally fail in the cnic driver, which, while not > > > catastrophic, is definately not expected. Fix this by choosing either the bnx2 > > > or bnx2x initalization path, not both > > > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > > > CC: David S. Miller <davem@davemloft.net> > > > CC: Michael Chan <mchan@broadcom.com> > > > CC: Dmitry Kravkov <dmitry@broadcom.com> > > > CC: Eddie Wai <waie@broadcom.com> > > > CC: Eilon Greenstein <eilong@broadcom.com> > > > --- > > > drivers/net/cnic.c | 2 +- > > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > > > diff --git a/drivers/net/cnic.c b/drivers/net/cnic.c > > > index 271a1f0..18b59ad 100644 > > > --- a/drivers/net/cnic.c > > > +++ b/drivers/net/cnic.c > > > @@ -5292,7 +5292,7 @@ static struct cnic_dev *is_cnic_dev(struct net_device *dev) > > > > > > if (!strcmp(drvinfo.driver, "bnx2")) > > > cdev = init_bnx2_cnic(dev); > > > - if (!strcmp(drvinfo.driver, "bnx2x")) > > > + else if (!strcmp(drvinfo.driver, "bnx2x")) > > > cdev = init_bnx2x_cnic(dev); > > > if (cdev) { > > > write_lock(&cnic_dev_lock); > > > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cnic: fix double initalization of bnx2 based cards 2011-03-08 19:36 ` Neil Horman @ 2011-03-08 19:40 ` Ben Hutchings 2011-03-08 19:43 ` Michael Chan 1 sibling, 0 replies; 8+ messages in thread From: Ben Hutchings @ 2011-03-08 19:40 UTC (permalink / raw) To: Neil Horman Cc: Michael Chan, netdev@vger.kernel.org, David S. Miller, Dmitry Kravkov, Eddie Wai, Eilon Greenstein On Tue, 2011-03-08 at 14:36 -0500, Neil Horman wrote: > On Tue, Mar 08, 2011 at 11:07:57AM -0800, Michael Chan wrote: > > > > On Tue, 2011-03-08 at 10:59 -0800, Ben Hutchings wrote: > > > On Tue, 2011-03-08 at 13:56 -0500, Neil Horman wrote: > > > > bnx2 cards can work with the cnic driver, but when the cnic driver detects a > > > > bnx2 card, is_cnic_dev erroneously calls the initalization routines for both > > > > bnx2 and bnx2x (the former being a regex subset of the later). > > > > > > Since when does strcmp() do a regex match? > > > > Yeah, strcmp() does NULL-terminated string compare, right? > > > Sorry, poor choice of words. It doesn't do a regex match, I only ment to > illustrate that bnx2 is a substring of bnx2x. > > strcmp does this: > > strcmp(const char *str1, const char *str2) { > ... > while (*s1 || *s2) { > ... > } > ... > } > > Since bnx2 is a substring of bnx2 both if clauses are a match, since you'll hit > the null terminator of the shorter string in both, which means we call both > initalization functions. [...] If you don't even know what strcmp() does, what are you doing hacking on the kernel? Ben. -- Ben Hutchings, Senior Software Engineer, Solarflare Communications Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cnic: fix double initalization of bnx2 based cards 2011-03-08 19:36 ` Neil Horman 2011-03-08 19:40 ` Ben Hutchings @ 2011-03-08 19:43 ` Michael Chan 2011-03-08 21:40 ` Neil Horman 1 sibling, 1 reply; 8+ messages in thread From: Michael Chan @ 2011-03-08 19:43 UTC (permalink / raw) To: Neil Horman Cc: Ben Hutchings, netdev@vger.kernel.org, David S. Miller, Dmitry Kravkov, Eddie Wai, Eilon Greenstein On Tue, 2011-03-08 at 11:36 -0800, Neil Horman wrote: > On Tue, Mar 08, 2011 at 11:07:57AM -0800, Michael Chan wrote: > > > > On Tue, 2011-03-08 at 10:59 -0800, Ben Hutchings wrote: > > > On Tue, 2011-03-08 at 13:56 -0500, Neil Horman wrote: > > > > bnx2 cards can work with the cnic driver, but when the cnic driver detects a > > > > bnx2 card, is_cnic_dev erroneously calls the initalization routines for both > > > > bnx2 and bnx2x (the former being a regex subset of the later). > > > > > > Since when does strcmp() do a regex match? > > > > Yeah, strcmp() does NULL-terminated string compare, right? > > > Sorry, poor choice of words. It doesn't do a regex match, I only ment to > illustrate that bnx2 is a substring of bnx2x. > > strcmp does this: > > strcmp(const char *str1, const char *str2) { > ... > while (*s1 || *s2) { > ... > } > ... > } > > Since bnx2 is a substring of bnx2 both if clauses are a match, since you'll hit > the null terminator of the shorter string in both, which means we call both > initalization functions. No, that should not happen. When we compare NULL-terminated "bnx2" with NULL-terminated "bnx2x", it won't match because the '\0' won't match the 'x', right? I think the patch is good. We can avoid the unnecessary strcmp() when we have a match with "bnx2" already. But we should not get both matches even without the patch. > > I'll leave commentary on initalization in is_cnic_dev to the authors :) > > Neil > > > > > > > > > > Ben. > > > > > > > This causes > > > > initalization of bnx2 to unilaterally fail in the cnic driver, which, while not > > > > catastrophic, is definately not expected. Fix this by choosing either the bnx2 > > > > or bnx2x initalization path, not both > > > > > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > > > > CC: David S. Miller <davem@davemloft.net> > > > > CC: Michael Chan <mchan@broadcom.com> > > > > CC: Dmitry Kravkov <dmitry@broadcom.com> > > > > CC: Eddie Wai <waie@broadcom.com> > > > > CC: Eilon Greenstein <eilong@broadcom.com> > > > > --- > > > > drivers/net/cnic.c | 2 +- > > > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > > > > > diff --git a/drivers/net/cnic.c b/drivers/net/cnic.c > > > > index 271a1f0..18b59ad 100644 > > > > --- a/drivers/net/cnic.c > > > > +++ b/drivers/net/cnic.c > > > > @@ -5292,7 +5292,7 @@ static struct cnic_dev *is_cnic_dev(struct net_device *dev) > > > > > > > > if (!strcmp(drvinfo.driver, "bnx2")) > > > > cdev = init_bnx2_cnic(dev); > > > > - if (!strcmp(drvinfo.driver, "bnx2x")) > > > > + else if (!strcmp(drvinfo.driver, "bnx2x")) > > > > cdev = init_bnx2x_cnic(dev); > > > > if (cdev) { > > > > write_lock(&cnic_dev_lock); > > > > > > > > > -- > > To unsubscribe from this list: send the line "unsubscribe netdev" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cnic: fix double initalization of bnx2 based cards 2011-03-08 19:43 ` Michael Chan @ 2011-03-08 21:40 ` Neil Horman 2011-03-08 21:41 ` Michael Chan 0 siblings, 1 reply; 8+ messages in thread From: Neil Horman @ 2011-03-08 21:40 UTC (permalink / raw) To: Michael Chan Cc: Ben Hutchings, netdev@vger.kernel.org, David S. Miller, Dmitry Kravkov, Eddie Wai, Eilon Greenstein On Tue, Mar 08, 2011 at 11:43:16AM -0800, Michael Chan wrote: > > On Tue, 2011-03-08 at 11:36 -0800, Neil Horman wrote: > > On Tue, Mar 08, 2011 at 11:07:57AM -0800, Michael Chan wrote: > > > > > > On Tue, 2011-03-08 at 10:59 -0800, Ben Hutchings wrote: > > > > On Tue, 2011-03-08 at 13:56 -0500, Neil Horman wrote: > > > > > bnx2 cards can work with the cnic driver, but when the cnic driver detects a > > > > > bnx2 card, is_cnic_dev erroneously calls the initalization routines for both > > > > > bnx2 and bnx2x (the former being a regex subset of the later). > > > > > > > > Since when does strcmp() do a regex match? > > > > > > Yeah, strcmp() does NULL-terminated string compare, right? > > > > > Sorry, poor choice of words. It doesn't do a regex match, I only ment to > > illustrate that bnx2 is a substring of bnx2x. > > > > strcmp does this: > > > > strcmp(const char *str1, const char *str2) { > > ... > > while (*s1 || *s2) { > > ... > > } > > ... > > } > > > > Since bnx2 is a substring of bnx2 both if clauses are a match, since you'll hit > > the null terminator of the shorter string in both, which means we call both > > initalization functions. > > No, that should not happen. When we compare NULL-terminated "bnx2" with > NULL-terminated "bnx2x", it won't match because the '\0' won't match the > 'x', right? > > I think the patch is good. We can avoid the unnecessary strcmp() when > we have a match with "bnx2" already. But we should not get both matches > even without the patch. > Crap, sorry, I rescind this (although you're right, it probably wouldn't hurt to roll this change in, just to save a potential second strcmp). Anywho, I'm getting this error on the ifup of my bnx2 interface: bnx2i: iSCSI not supported, dev=eth1 That message appears to only get set (from my read) on cards driven by the bnx2x driver, and when I saw the 2 ifs rather than the if/else, I jumped to a conclusion that we must be using the bnx2x init path on a bnx2 driver. Apologies. /me wanders off to see where completion_status gets set to ISCSI_KCQE_COMPLETION_STATUS_ISCSI_NOT_SUPPORTED for bnx2. Neil > > > > I'll leave commentary on initalization in is_cnic_dev to the authors :) > > > > Neil > > > > > > > > > > > > > > > Ben. > > > > > > > > > This causes > > > > > initalization of bnx2 to unilaterally fail in the cnic driver, which, while not > > > > > catastrophic, is definately not expected. Fix this by choosing either the bnx2 > > > > > or bnx2x initalization path, not both > > > > > > > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > > > > > CC: David S. Miller <davem@davemloft.net> > > > > > CC: Michael Chan <mchan@broadcom.com> > > > > > CC: Dmitry Kravkov <dmitry@broadcom.com> > > > > > CC: Eddie Wai <waie@broadcom.com> > > > > > CC: Eilon Greenstein <eilong@broadcom.com> > > > > > --- > > > > > drivers/net/cnic.c | 2 +- > > > > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > > > > > > > diff --git a/drivers/net/cnic.c b/drivers/net/cnic.c > > > > > index 271a1f0..18b59ad 100644 > > > > > --- a/drivers/net/cnic.c > > > > > +++ b/drivers/net/cnic.c > > > > > @@ -5292,7 +5292,7 @@ static struct cnic_dev *is_cnic_dev(struct net_device *dev) > > > > > > > > > > if (!strcmp(drvinfo.driver, "bnx2")) > > > > > cdev = init_bnx2_cnic(dev); > > > > > - if (!strcmp(drvinfo.driver, "bnx2x")) > > > > > + else if (!strcmp(drvinfo.driver, "bnx2x")) > > > > > cdev = init_bnx2x_cnic(dev); > > > > > if (cdev) { > > > > > write_lock(&cnic_dev_lock); > > > > > > > > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe netdev" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cnic: fix double initalization of bnx2 based cards 2011-03-08 21:40 ` Neil Horman @ 2011-03-08 21:41 ` Michael Chan 0 siblings, 0 replies; 8+ messages in thread From: Michael Chan @ 2011-03-08 21:41 UTC (permalink / raw) To: Neil Horman Cc: Ben Hutchings, netdev@vger.kernel.org, David S. Miller, Dmitry Kravkov, Eddie Wai, Eilon Greenstein On Tue, 2011-03-08 at 13:40 -0800, Neil Horman wrote: > > No, that should not happen. When we compare NULL-terminated "bnx2" with > > NULL-terminated "bnx2x", it won't match because the '\0' won't match the > > 'x', right? > > > > I think the patch is good. We can avoid the unnecessary strcmp() when > > we have a match with "bnx2" already. But we should not get both matches > > even without the patch. > > > Crap, sorry, I rescind this (although you're right, it probably wouldn't hurt to > roll this change in, just to save a potential second strcmp). Anywho, I'm > getting this error on the ifup of my bnx2 interface: > bnx2i: iSCSI not supported, dev=eth1 > > That message appears to only get set (from my read) on cards driven by the bnx2x > driver, and when I saw the 2 ifs rather than the if/else, I jumped to a > conclusion that we must be using the bnx2x init path on a bnx2 driver. > Apologies. > > /me wanders off to see where completion_status gets set to > ISCSI_KCQE_COMPLETION_STATUS_ISCSI_NOT_SUPPORTED for bnx2. On bnx2, this error code is set by the firmware if iSCSI is not configured in NVRAM. Thanks. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-03-08 21:46 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-03-08 18:56 [PATCH] cnic: fix double initalization of bnx2 based cards Neil Horman 2011-03-08 18:59 ` Ben Hutchings 2011-03-08 19:07 ` Michael Chan 2011-03-08 19:36 ` Neil Horman 2011-03-08 19:40 ` Ben Hutchings 2011-03-08 19:43 ` Michael Chan 2011-03-08 21:40 ` Neil Horman 2011-03-08 21:41 ` Michael Chan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).