incorrect shift and mask operation in drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c

incorrect shift and mask operation in drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c

[Colin King (gmail)]

Hi,

Static analysis on 
drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c has found 
an issue with a mask and shift operation in function 
wlc_phy_rxcal_radio_setup_nphy() as follows:

lines 26326-26330:

         offtune_val =
                 (pi->tx_rx_cal_radio_saveregs
                  [2] & 0xF0) >> 8;
         offtune_val =
                 (offtune_val <= 0x7) ? 0xF : 0;

and similar in lines 26376-26381 too.

The issue is that the expression pi->tx_rx_cal_radio_saveregs[2] & 0xF0
when shifted 8 places right is always zero, so this looks like a mistake 
since some value value between 0..0xf is expected in the second statement.

Since pi->tx_rx_cal_radio_saveregs[2] is a u16 value the expression 
could plausible be:

	(pi->tx_rx_cal_radio_saveregs[2] & 0xf0) >> 4
or
	(pi->tx_rx_cal_radio_saveregs[2] & 0xf00) >> 8

I don't have knowledge of the hardware so I'm not sure what a suitable 
fix is.

Regards,

Colin

Re: incorrect shift and mask operation in drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c

[Arend van Spriel]

On 10/15/2024 1:02 AM, Colin King (gmail) wrote:
> Hi,
> 
> Static analysis on drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/ 
> phy_n.c has found an issue with a mask and shift operation in function 
> wlc_phy_rxcal_radio_setup_nphy() as follows:
> 
> lines 26326-26330:
> 
>          offtune_val =
>                  (pi->tx_rx_cal_radio_saveregs
>                   [2] & 0xF0) >> 8;
>          offtune_val =
>                  (offtune_val <= 0x7) ? 0xF : 0;
> 
> and similar in lines 26376-26381 too.
> 
> The issue is that the expression pi->tx_rx_cal_radio_saveregs[2] & 0xF0
> when shifted 8 places right is always zero, so this looks like a mistake 
> since some value value between 0..0xf is expected in the second statement.
> 
> Since pi->tx_rx_cal_radio_saveregs[2] is a u16 value the expression 
> could plausible be:
> 
>      (pi->tx_rx_cal_radio_saveregs[2] & 0xf0) >> 4
> or
>      (pi->tx_rx_cal_radio_saveregs[2] & 0xf00) >> 8
> 
> I don't have knowledge of the hardware so I'm not sure what a suitable 
> fix is.

Thanks, Colin

That looks pretty redundant indeed. I look into the history of it, but 
ended up in an old end-of-life development tree and the code was pretty 
much the same. It is one of the finer dark arts of our phy/radio 
development team I guess ;-)

I see that Dmitry Antipov submitted a patch to address this. Again 
thanks for reporting.

Regards,
Arend