[PATCH] staging: brcm80211: return false if not a broadcom board

[PATCH] staging: brcm80211: return false if not a broadcom board

[Greg Dietsche]

This code looks wrong to me. I think it meant to return false
if the board's vendor id isn't Broadcom's.

Compile tested only.

Signed-off-by: Greg Dietsche <Gregory.Dietsche@cuw.edu>
---
 drivers/staging/brcm80211/brcmsmac/wlc_bmac.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c b/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
index 4534926..ee13238 100644
--- a/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
+++ b/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
@@ -1943,7 +1943,7 @@ static bool wlc_validboardtype(struct wlc_hw_info *wlc_hw)
 	}
 
 	if (wlc_hw->sih->boardvendor != PCI_VENDOR_ID_BROADCOM)
-		return goodboard;
+		goodboard = false; 
 
 	return goodboard;
 }
-- 
1.7.2.5

Re: [PATCH] staging: brcm80211: return false if not a broadcom board

[Henry Ptasinski]

On Wed, Jun 15, 2011 at 03:25:09PM -0700, Greg Dietsche wrote:
> This code looks wrong to me. I think it meant to return false
> if the board's vendor id isn't Broadcom's.
> 
> Compile tested only.
> 
> Signed-off-by: Greg Dietsche <Gregory.Dietsche@cuw.edu>
> ---
>  drivers/staging/brcm80211/brcmsmac/wlc_bmac.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c b/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
> index 4534926..ee13238 100644
> --- a/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
> +++ b/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
> @@ -1943,7 +1943,7 @@ static bool wlc_validboardtype(struct wlc_hw_info *wlc_hw)
>  	}
>  
>  	if (wlc_hw->sih->boardvendor != PCI_VENDOR_ID_BROADCOM)
> -		return goodboard;
> +		goodboard = false; 
>  
>  	return goodboard;
>  }

Actually, the original code is correct.

For boards with the Broadcom ID, this function checks that the boardrev is
sane. Dev boards that aren't properly configured may have invalid info, so this
check is mainly to catch that problem.

For boards with any other vendor, we don't have any sanity checks that we know
should be done, so the board info is always considered good.

The device ID is of course always checked.

- Henry

Re: [PATCH] staging: brcm80211: return false if not a broadcom board

[Greg Dietsche]

On 06/15/2011 08:36 PM, Henry Ptasinski wrote:
> On Wed, Jun 15, 2011 at 03:25:09PM -0700, Greg Dietsche wrote:
>    
>> This code looks wrong to me. I think it meant to return false
>> if the board's vendor id isn't Broadcom's.
>>
>> Compile tested only.
>>
>> Signed-off-by: Greg Dietsche<Gregory.Dietsche@cuw.edu>
>> ---
>>   drivers/staging/brcm80211/brcmsmac/wlc_bmac.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c b/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
>> index 4534926..ee13238 100644
>> --- a/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
>> +++ b/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
>> @@ -1943,7 +1943,7 @@ static bool wlc_validboardtype(struct wlc_hw_info *wlc_hw)
>>   	}
>>
>>   	if (wlc_hw->sih->boardvendor != PCI_VENDOR_ID_BROADCOM)
>> -		return goodboard;
>> +		goodboard = false;
>>
>>   	return goodboard;
>>   }
>>      
> Actually, the original code is correct.
>
> For boards with the Broadcom ID, this function checks that the boardrev is
> sane. Dev boards that aren't properly configured may have invalid info, so this
> check is mainly to catch that problem.
>
> For boards with any other vendor, we don't have any sanity checks that we know
> should be done, so the board info is always considered good.
>
> The device ID is of course always checked.
>
> - Henry
>
>    
Hi Henry,
Thanks for the explanation - I'm still a little confused on this... 
probably more because of how the code reads. Let me explain:

The part that threw me off in this code when I originally read it is 
that it always returns goodboard. It doesn't actually matter what the 
vendor id is. So my first thought was "what did they really mean to code 
here...?" That's why I sent the patch. So if the patch isn't necessary, 
then great! :) But it seems like we could/should just drop the check for 
vendor id?

thanks,
Greg

Re: [PATCH] staging: brcm80211: return false if not a broadcom board

[Henry Ptasinski]

On Wed, Jun 15, 2011 at 07:49:06PM -0700, Greg Dietsche wrote:
> 
> 
> On 06/15/2011 08:36 PM, Henry Ptasinski wrote:
> > On Wed, Jun 15, 2011 at 03:25:09PM -0700, Greg Dietsche wrote:
> >    
> >> This code looks wrong to me. I think it meant to return false
> >> if the board's vendor id isn't Broadcom's.
> >>
> >> Compile tested only.
> >>
> >> Signed-off-by: Greg Dietsche<Gregory.Dietsche@cuw.edu>
> >> ---
> >>   drivers/staging/brcm80211/brcmsmac/wlc_bmac.c |    2 +-
> >>   1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c b/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
> >> index 4534926..ee13238 100644
> >> --- a/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
> >> +++ b/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
> >> @@ -1943,7 +1943,7 @@ static bool wlc_validboardtype(struct wlc_hw_info *wlc_hw)
> >>   	}
> >>
> >>   	if (wlc_hw->sih->boardvendor != PCI_VENDOR_ID_BROADCOM)
> >> -		return goodboard;
> >> +		goodboard = false;
> >>
> >>   	return goodboard;
> >>   }
> >>      
> > Actually, the original code is correct.
> >
> > For boards with the Broadcom ID, this function checks that the boardrev is
> > sane. Dev boards that aren't properly configured may have invalid info, so this
> > check is mainly to catch that problem.
> >
> > For boards with any other vendor, we don't have any sanity checks that we know
> > should be done, so the board info is always considered good.
> >
> > The device ID is of course always checked.
> >
> > - Henry
> >
> >    
> Hi Henry,
> Thanks for the explanation - I'm still a little confused on this... 
> probably more because of how the code reads. Let me explain:
> 
> The part that threw me off in this code when I originally read it is 
> that it always returns goodboard. It doesn't actually matter what the 
> vendor id is. So my first thought was "what did they really mean to code 
> here...?" That's why I sent the patch. So if the patch isn't necessary, 
> then great! :) But it seems like we could/should just drop the check for 
> vendor id?

How's this for a somewhat clearer implementation:

static bool brcms_c_validboardtype(struct brcms_c_hw_info *wlc_hw)
{
        bool goodboard = true;
        uint boardrev = wlc_hw->boardrev;

        if (wlc_hw->sih->boardvendor == PCI_VENDOR_ID_BROADCOM) {
                /* validate boardrev */
                if (boardrev == 0)
                        goodboard = false;
                else if (boardrev > 0xff) {
                        /* 4 bits each for board type, major, minor, and tiny
                        version numbers */
                        uint brt = (boardrev & 0xf000) >> 12;
                        uint b0 = (boardrev & 0xf00) >> 8;
                        uint b1 = (boardrev & 0xf0) >> 4;
                        uint b2 = boardrev & 0xf;

                        if ((brt > 2) || (brt == 0) || (b0 > 9) || (b0 == 0)
                                || (b1 > 9) || (b2 > 9))
                                goodboard = false;
                }
        }

        return goodboard;
}


If that's agreeable, I'll add it to a series of cleanup patches that I've got
in progress.

- Henry

Re: [PATCH] staging: brcm80211: return false if not a broadcom board

[Julian Calaby]

Henry,

On Fri, Jun 17, 2011 at 09:37, Henry Ptasinski <henryp@broadcom.com> wrote:
> How's this for a somewhat clearer implementation:
>
> static bool brcms_c_validboardtype(struct brcms_c_hw_info *wlc_hw)
> {
>        bool goodboard = true;
>        uint boardrev = wlc_hw->boardrev;
>
>        if (wlc_hw->sih->boardvendor == PCI_VENDOR_ID_BROADCOM) {

You could reduce indentation by having this be:

if (wlc_hw->sih->boardvendor != PCI_VENDOR_ID_BROADCOM)
    return true;

>                /* validate boardrev */
>                if (boardrev == 0)
>                        goodboard = false;

and remove the goodboard variable by having this return false immediately.

>                else if (boardrev > 0xff) {

You could also drop the else and have this as

if (boardrev <= 0xff)
    return true;

>                        /* 4 bits each for board type, major, minor, and tiny
>                        version numbers */
>                        uint brt = (boardrev & 0xf000) >> 12;
>                        uint b0 = (boardrev & 0xf00) >> 8;
>                        uint b1 = (boardrev & 0xf0) >> 4;
>                        uint b2 = boardrev & 0xf;
>
>                        if ((brt > 2) || (brt == 0) || (b0 > 9) || (b0 == 0)
>                                || (b1 > 9) || (b2 > 9))
>                                goodboard = false;

and return false here too.

>                }
>        }
>
>        return goodboard;

then just return true here.

> }

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

Re: [PATCH] staging: brcm80211: return false if not a broadcom board

[Henry Ptasinski]

On Thu, Jun 16, 2011 at 04:45:21PM -0700, Julian Calaby wrote:
> Henry,
> 
> On Fri, Jun 17, 2011 at 09:37, Henry Ptasinski <henryp@broadcom.com> wrote:
> > How's this for a somewhat clearer implementation:
> >
> > static bool brcms_c_validboardtype(struct brcms_c_hw_info *wlc_hw)
> > {
> >        bool goodboard = true;
> >        uint boardrev = wlc_hw->boardrev;
> >
> >        if (wlc_hw->sih->boardvendor == PCI_VENDOR_ID_BROADCOM) {
> 
> You could reduce indentation by having this be:
> 
> if (wlc_hw->sih->boardvendor != PCI_VENDOR_ID_BROADCOM)
>     return true;
> 
> >                /* validate boardrev */
> >                if (boardrev == 0)
> >                        goodboard = false;
> 
> and remove the goodboard variable by having this return false immediately.
> 
> >                else if (boardrev > 0xff) {
> 
> You could also drop the else and have this as
> 
> if (boardrev <= 0xff)
>     return true;
> 
> >                        /* 4 bits each for board type, major, minor, and tiny
> >                        version numbers */
> >                        uint brt = (boardrev & 0xf000) >> 12;
> >                        uint b0 = (boardrev & 0xf00) >> 8;
> >                        uint b1 = (boardrev & 0xf0) >> 4;
> >                        uint b2 = boardrev & 0xf;
> >
> >                        if ((brt > 2) || (brt == 0) || (b0 > 9) || (b0 == 0)
> >                                || (b1 > 9) || (b2 > 9))
> >                                goodboard = false;
> 
> and return false here too.
> 
> >                }
> >        }
> >
> >        return goodboard;
> 
> then just return true here.
> 
> > }
> 
> Thanks,
> 
> -- 
> Julian Calaby

Yea, it's a lot flatter with those changes.  Look for it in a patch set
relatively soon ...

- Henry

Re: [PATCH] staging: brcm80211: return false if not a broadcom board

[Greg Dietsche]

On 06/17/2011 05:01 PM, Henry Ptasinski wrote:
> On Thu, Jun 16, 2011 at 04:45:21PM -0700, Julian Calaby wrote:
>    
>> Henry,
>>
>> On Fri, Jun 17, 2011 at 09:37, Henry Ptasinski<henryp@broadcom.com>  wrote:
>>      
>>> How's this for a somewhat clearer implementation:
>>>
>>> static bool brcms_c_validboardtype(struct brcms_c_hw_info *wlc_hw)
>>> {
>>>         bool goodboard = true;
>>>         uint boardrev = wlc_hw->boardrev;
>>>
>>>         if (wlc_hw->sih->boardvendor == PCI_VENDOR_ID_BROADCOM) {
>>>        
>> You could reduce indentation by having this be:
>>
>> if (wlc_hw->sih->boardvendor != PCI_VENDOR_ID_BROADCOM)
>>      return true;
>>
>>      
>>>                 /* validate boardrev */
>>>                 if (boardrev == 0)
>>>                         goodboard = false;
>>>        
>> and remove the goodboard variable by having this return false immediately.
>>
>>      
>>>                 else if (boardrev>  0xff) {
>>>        
>> You could also drop the else and have this as
>>
>> if (boardrev<= 0xff)
>>      return true;
>>
>>      
>>>                         /* 4 bits each for board type, major, minor, and tiny
>>>                         version numbers */
>>>                         uint brt = (boardrev&  0xf000)>>  12;
>>>                         uint b0 = (boardrev&  0xf00)>>  8;
>>>                         uint b1 = (boardrev&  0xf0)>>  4;
>>>                         uint b2 = boardrev&  0xf;
>>>
>>>                         if ((brt>  2) || (brt == 0) || (b0>  9) || (b0 == 0)
>>>                                 || (b1>  9) || (b2>  9))
>>>                                 goodboard = false;
>>>        
>> and return false here too.
>>
>>      
>>>                 }
>>>         }
>>>
>>>         return goodboard;
>>>        
>> then just return true here.
>>
>>      
>>> }
>>>        
>> Thanks,
>>
>> -- 
>> Julian Calaby
>>      
> Yea, it's a lot flatter with those changes.  Look for it in a patch set
> relatively soon ...
>
> - Henry
>
>
>    

All good points above - also, I might add a short comment to explain why 
the code checks the vendor id so it's clear to people who read the code 
in the future too.

Greg

Re: [PATCH] staging: brcm80211: return false if not a broadcom board

[Dan Carpenter]

On Wed, Jun 15, 2011 at 06:36:16PM -0700, Henry Ptasinski wrote:
> On Wed, Jun 15, 2011 at 03:25:09PM -0700, Greg Dietsche wrote:
> > This code looks wrong to me. I think it meant to return false
> > if the board's vendor id isn't Broadcom's.
> > 
> > Compile tested only.
> > 
> > Signed-off-by: Greg Dietsche <Gregory.Dietsche@cuw.edu>
> > ---
> >  drivers/staging/brcm80211/brcmsmac/wlc_bmac.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c b/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
> > index 4534926..ee13238 100644
> > --- a/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
> > +++ b/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
> > @@ -1943,7 +1943,7 @@ static bool wlc_validboardtype(struct wlc_hw_info *wlc_hw)
> >  	}
> >  
> >  	if (wlc_hw->sih->boardvendor != PCI_VENDOR_ID_BROADCOM)
> > -		return goodboard;
> > +		goodboard = false; 
> >  
> >  	return goodboard;
> >  }
> 
> Actually, the original code is correct.
> 
> For boards with the Broadcom ID, this function checks that the boardrev is
> sane. Dev boards that aren't properly configured may have invalid info, so this
> check is mainly to catch that problem.
> 
> For boards with any other vendor, we don't have any sanity checks that we know
> should be done, so the board info is always considered good.
>

Sounds like it should be "goodboard = true;" then.

regards,
dan carpenter