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

["Henry Ptasinski" <henryp@broadcom.com>]

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