[PATCH] cnic: fix double initalization of bnx2 based cards

[PATCH] cnic: fix double initalization of bnx2 based cards

[Neil Horman]

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

Re: [PATCH] cnic: fix double initalization of bnx2 based cards

[Ben Hutchings]

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.

Re: [PATCH] cnic: fix double initalization of bnx2 based cards

[Michael Chan]

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);
> 

Re: [PATCH] cnic: fix double initalization of bnx2 based cards

[Neil Horman]

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
> 

Re: [PATCH] cnic: fix double initalization of bnx2 based cards

[Ben Hutchings]

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.

Re: [PATCH] cnic: fix double initalization of bnx2 based cards

[Michael Chan]

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
> > 
> 

Re: [PATCH] cnic: fix double initalization of bnx2 based cards

[Neil Horman]

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
> 

Re: [PATCH] cnic: fix double initalization of bnx2 based cards

[Michael Chan]

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.