Hello Tomislav Požega,
The patch dab902fe1d29: "wifi: rt2x00: add TX LOFT calibration for
MT7620" from Sep 17, 2022, leads to the following Smatch static
checker warning:
drivers/net/wireless/ralink/rt2x00/rt2800lib.c:9681 rt2800_loft_iq_calibration()
warn: unsigned 'vga_gain[ch_idx]' is never less than zero.
drivers/net/wireless/ralink/rt2x00/rt2800lib.c
9540 static void rt2800_loft_iq_calibration(struct rt2x00_dev *rt2x00dev)
9541 {
9542 struct rf_reg_pair rf_store[CHAIN_NUM][13];
9543 u32 macorg1 = 0;
9544 u32 macorg2 = 0;
9545 u32 macorg3 = 0;
9546 u32 macorg4 = 0;
9547 u32 macorg5 = 0;
9548 u32 orig528 = 0;
9549 u32 orig52c = 0;
9550
9551 u32 savemacsysctrl = 0;
9552 u32 macvalue = 0;
9553 u32 mac13b8 = 0;
9554 u32 p0 = 0, p1 = 0;
9555 u32 p0_idx10 = 0, p1_idx10 = 0;
9556
9557 u8 rfvalue;
9558 u8 loft_dc_search_result[CHAIN_NUM][RF_ALC_NUM][2];
9559 u8 ger[CHAIN_NUM], per[CHAIN_NUM];
9560
9561 u8 vga_gain[] = {14, 14};
^^^^^^^^^^^^^
9562 u8 bbp = 0, ch_idx = 0, rf_alc_idx = 0, idx = 0;
9563 u8 bbpr30, rfb0r39, rfb0r42;
9564 u8 bbpr1;
9565 u8 bbpr4;
9566 u8 bbpr241, bbpr242;
9567 u8 count_step;
9568
9569 static const u8 rf_gain[] = {0x00, 0x01, 0x02, 0x04, 0x08, 0x0c};
9570 static const u8 rfvga_gain_table[] = {0x24, 0x25, 0x26, 0x27, 0x28, 0x2c, 0x2d, 0x2e, 0x2f, 0x30,
9571 0x31, 0x38, 0x39, 0x3a, 0x3b, 0x3c, 0x3d, 0x3e, 0x3F};
9572 static const u8 bbp_2324gain[] = {0x16, 0x14, 0x12, 0x10, 0x0c, 0x08};
9573
9574 savemacsysctrl = rt2800_register_read(rt2x00dev, MAC_SYS_CTRL);
9575 macorg1 = rt2800_register_read(rt2x00dev, TX_PIN_CFG);
9576 macorg2 = rt2800_register_read(rt2x00dev, RF_CONTROL0);
9577 macorg3 = rt2800_register_read(rt2x00dev, RF_BYPASS0);
9578 macorg4 = rt2800_register_read(rt2x00dev, RF_CONTROL3);
9579 macorg5 = rt2800_register_read(rt2x00dev, RF_BYPASS3);
9580 mac13b8 = rt2800_register_read(rt2x00dev, 0x13b8);
9581 orig528 = rt2800_register_read(rt2x00dev, RF_CONTROL2);
9582 orig52c = rt2800_register_read(rt2x00dev, RF_BYPASS2);
9583
9584 macvalue = rt2800_register_read(rt2x00dev, MAC_SYS_CTRL);
9585 macvalue &= (~0x04);
9586 rt2800_register_write(rt2x00dev, MAC_SYS_CTRL, macvalue);
9587
9588 if (unlikely(rt2800_wait_bbp_rf_ready(rt2x00dev, MAC_STATUS_CFG_BBP_RF_BUSY_TX)))
9589 rt2x00_warn(rt2x00dev, "RF TX busy in LOFT IQ calibration\n");
9590
9591 macvalue = rt2800_register_read(rt2x00dev, MAC_SYS_CTRL);
9592 macvalue &= (~0x08);
9593 rt2800_register_write(rt2x00dev, MAC_SYS_CTRL, macvalue);
9594
9595 if (unlikely(rt2800_wait_bbp_rf_ready(rt2x00dev, MAC_STATUS_CFG_BBP_RF_BUSY_RX)))
9596 rt2x00_warn(rt2x00dev, "RF RX busy in LOFT IQ calibration\n");
9597
9598 for (ch_idx = 0; ch_idx < 2; ch_idx++)
9599 rt2800_rf_configstore(rt2x00dev, rf_store, ch_idx);
9600
9601 bbpr30 = rt2800_bbp_read(rt2x00dev, 30);
9602 rfb0r39 = rt2800_rfcsr_read_bank(rt2x00dev, 0, 39);
9603 rfb0r42 = rt2800_rfcsr_read_bank(rt2x00dev, 0, 42);
9604
9605 rt2800_bbp_write(rt2x00dev, 30, 0x1F);
9606 rt2800_rfcsr_write_bank(rt2x00dev, 0, 39, 0x80);
9607 rt2800_rfcsr_write_bank(rt2x00dev, 0, 42, 0x5B);
9608
9609 rt2800_bbp_write(rt2x00dev, 23, 0x00);
9610 rt2800_bbp_write(rt2x00dev, 24, 0x00);
9611
9612 rt2800_setbbptonegenerator(rt2x00dev);
9613
9614 for (ch_idx = 0; ch_idx < 2; ch_idx++) {
9615 rt2800_bbp_write(rt2x00dev, 23, 0x00);
9616 rt2800_bbp_write(rt2x00dev, 24, 0x00);
9617 rt2800_register_write(rt2x00dev, MAC_SYS_CTRL, 0x00);
9618 rt2800_register_write(rt2x00dev, TX_PIN_CFG, 0x0000000F);
9619 rt2800_register_write(rt2x00dev, RF_CONTROL0, 0x00000004);
9620 rt2800_register_write(rt2x00dev, RF_BYPASS0, 0x00003306);
9621 rt2800_register_write(rt2x00dev, 0x13b8, 0x10);
9622 udelay(1);
9623
9624 if (ch_idx == 0)
9625 rt2800_rf_aux_tx0_loopback(rt2x00dev);
9626 else
9627 rt2800_rf_aux_tx1_loopback(rt2x00dev);
9628
9629 udelay(1);
9630
9631 if (ch_idx == 0)
9632 rt2800_register_write(rt2x00dev, RF_CONTROL0, 0x00001004);
9633 else
9634 rt2800_register_write(rt2x00dev, RF_CONTROL0, 0x00002004);
9635
9636 rt2800_bbp_write(rt2x00dev, 158, 0x05);
9637 rt2800_bbp_write(rt2x00dev, 159, 0x00);
9638
9639 rt2800_bbp_write(rt2x00dev, 158, 0x01);
9640 if (ch_idx == 0)
9641 rt2800_bbp_write(rt2x00dev, 159, 0x00);
9642 else
9643 rt2800_bbp_write(rt2x00dev, 159, 0x01);
9644
9645 vga_gain[ch_idx] = 18;
^^
9646 for (rf_alc_idx = 0; rf_alc_idx < 3; rf_alc_idx++) {
9647 rt2800_bbp_write(rt2x00dev, 23, bbp_2324gain[rf_alc_idx]);
9648 rt2800_bbp_write(rt2x00dev, 24, bbp_2324gain[rf_alc_idx]);
9649
9650 macvalue = rt2800_register_read(rt2x00dev, RF_CONTROL3);
9651 macvalue &= (~0x0000F1F1);
9652 macvalue |= (rf_gain[rf_alc_idx] << 4);
9653 macvalue |= (rf_gain[rf_alc_idx] << 12);
9654 rt2800_register_write(rt2x00dev, RF_CONTROL3, macvalue);
9655 macvalue = (0x0000F1F1);
9656 rt2800_register_write(rt2x00dev, RF_BYPASS3, macvalue);
9657
9658 if (rf_alc_idx == 0) {
9659 rt2800_write_dc(rt2x00dev, ch_idx, 0, 1, 0x21);
9660 for (; vga_gain[ch_idx] > 0;
9661 vga_gain[ch_idx] = vga_gain[ch_idx] - 2) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
18 is an even number so this can't actually underflow.
9662 rfvalue = rfvga_gain_table[vga_gain[ch_idx]];
9663 rt2800_rfcsr_write_dccal(rt2x00dev, 3, rfvalue);
9664 rt2800_rfcsr_write_dccal(rt2x00dev, 4, rfvalue);
9665 rt2800_write_dc(rt2x00dev, ch_idx, 0, 1, 0x00);
9666 rt2800_write_dc(rt2x00dev, ch_idx, 0, 0, 0x00);
9667 p0 = rt2800_do_fft_accumulation(rt2x00dev, 0x0A, 0);
9668 rt2800_write_dc(rt2x00dev, ch_idx, 0, 0, 0x21);
9669 p1 = rt2800_do_fft_accumulation(rt2x00dev, 0x0A, 0);
9670 rt2x00_dbg(rt2x00dev, "LOFT AGC %d %d\n", p0, p1);
9671 if ((p0 < 7000 * 7000) && (p1 < (7000 * 7000)))
9672 break;
9673 }
9674
9675 rt2800_write_dc(rt2x00dev, ch_idx, 0, 0, 0x00);
9676 rt2800_write_dc(rt2x00dev, ch_idx, 0, 1, 0x00);
9677
9678 rt2x00_dbg(rt2x00dev, "Used VGA %d %x\n", vga_gain[ch_idx],
9679 rfvga_gain_table[vga_gain[ch_idx]]);
9680
--> 9681 if (vga_gain[ch_idx] < 0)
^^^^^^^^^^^^^^^^^^^^
But if it could then this test would not work because the type is u8.
9682 vga_gain[ch_idx] = 0;
9683 }
9684
9685 rfvalue = rfvga_gain_table[vga_gain[ch_idx]];
9686
9687 rt2800_rfcsr_write_dccal(rt2x00dev, 3, rfvalue);
9688 rt2800_rfcsr_write_dccal(rt2x00dev, 4, rfvalue);
9689
9690 rt2800_loft_search(rt2x00dev, ch_idx, rf_alc_idx, loft_dc_search_result);
9691 }
9692 }
regards,
dan carpenter
Hello
This was all taken from reference driver.
i did not go through detailed debugging to see if the value is
overwritten in vendor driver.
Maybe ask devs from openwrt who upstreamed these patches
On 20/05/2023, Dan Carpenter <[email protected]> wrote:
> Hello Tomislav Požega,
>
> The patch dab902fe1d29: "wifi: rt2x00: add TX LOFT calibration for
> MT7620" from Sep 17, 2022, leads to the following Smatch static
> checker warning:
>
> drivers/net/wireless/ralink/rt2x00/rt2800lib.c:9681
> rt2800_loft_iq_calibration()
> warn: unsigned 'vga_gain[ch_idx]' is never less than zero.
>
> drivers/net/wireless/ralink/rt2x00/rt2800lib.c
> 9540 static void rt2800_loft_iq_calibration(struct rt2x00_dev
> *rt2x00dev)
> 9541 {
> 9542 struct rf_reg_pair rf_store[CHAIN_NUM][13];
> 9543 u32 macorg1 = 0;
> 9544 u32 macorg2 = 0;
> 9545 u32 macorg3 = 0;
> 9546 u32 macorg4 = 0;
> 9547 u32 macorg5 = 0;
> 9548 u32 orig528 = 0;
> 9549 u32 orig52c = 0;
> 9550
> 9551 u32 savemacsysctrl = 0;
> 9552 u32 macvalue = 0;
> 9553 u32 mac13b8 = 0;
> 9554 u32 p0 = 0, p1 = 0;
> 9555 u32 p0_idx10 = 0, p1_idx10 = 0;
> 9556
> 9557 u8 rfvalue;
> 9558 u8 loft_dc_search_result[CHAIN_NUM][RF_ALC_NUM][2];
> 9559 u8 ger[CHAIN_NUM], per[CHAIN_NUM];
> 9560
> 9561 u8 vga_gain[] = {14, 14};
> ^^^^^^^^^^^^^
>
> 9562 u8 bbp = 0, ch_idx = 0, rf_alc_idx = 0, idx = 0;
> 9563 u8 bbpr30, rfb0r39, rfb0r42;
> 9564 u8 bbpr1;
> 9565 u8 bbpr4;
> 9566 u8 bbpr241, bbpr242;
> 9567 u8 count_step;
> 9568
> 9569 static const u8 rf_gain[] = {0x00, 0x01, 0x02, 0x04, 0x08,
> 0x0c};
> 9570 static const u8 rfvga_gain_table[] = {0x24, 0x25, 0x26,
> 0x27, 0x28, 0x2c, 0x2d, 0x2e, 0x2f, 0x30,
> 9571 0x31, 0x38, 0x39,
> 0x3a, 0x3b, 0x3c, 0x3d, 0x3e, 0x3F};
> 9572 static const u8 bbp_2324gain[] = {0x16, 0x14, 0x12, 0x10,
> 0x0c, 0x08};
> 9573
> 9574 savemacsysctrl = rt2800_register_read(rt2x00dev,
> MAC_SYS_CTRL);
> 9575 macorg1 = rt2800_register_read(rt2x00dev, TX_PIN_CFG);
> 9576 macorg2 = rt2800_register_read(rt2x00dev, RF_CONTROL0);
> 9577 macorg3 = rt2800_register_read(rt2x00dev, RF_BYPASS0);
> 9578 macorg4 = rt2800_register_read(rt2x00dev, RF_CONTROL3);
> 9579 macorg5 = rt2800_register_read(rt2x00dev, RF_BYPASS3);
> 9580 mac13b8 = rt2800_register_read(rt2x00dev, 0x13b8);
> 9581 orig528 = rt2800_register_read(rt2x00dev, RF_CONTROL2);
> 9582 orig52c = rt2800_register_read(rt2x00dev, RF_BYPASS2);
> 9583
> 9584 macvalue = rt2800_register_read(rt2x00dev, MAC_SYS_CTRL);
> 9585 macvalue &= (~0x04);
> 9586 rt2800_register_write(rt2x00dev, MAC_SYS_CTRL, macvalue);
> 9587
> 9588 if (unlikely(rt2800_wait_bbp_rf_ready(rt2x00dev,
> MAC_STATUS_CFG_BBP_RF_BUSY_TX)))
> 9589 rt2x00_warn(rt2x00dev, "RF TX busy in LOFT IQ
> calibration\n");
> 9590
> 9591 macvalue = rt2800_register_read(rt2x00dev, MAC_SYS_CTRL);
> 9592 macvalue &= (~0x08);
> 9593 rt2800_register_write(rt2x00dev, MAC_SYS_CTRL, macvalue);
> 9594
> 9595 if (unlikely(rt2800_wait_bbp_rf_ready(rt2x00dev,
> MAC_STATUS_CFG_BBP_RF_BUSY_RX)))
> 9596 rt2x00_warn(rt2x00dev, "RF RX busy in LOFT IQ
> calibration\n");
> 9597
> 9598 for (ch_idx = 0; ch_idx < 2; ch_idx++)
> 9599 rt2800_rf_configstore(rt2x00dev, rf_store,
> ch_idx);
> 9600
> 9601 bbpr30 = rt2800_bbp_read(rt2x00dev, 30);
> 9602 rfb0r39 = rt2800_rfcsr_read_bank(rt2x00dev, 0, 39);
> 9603 rfb0r42 = rt2800_rfcsr_read_bank(rt2x00dev, 0, 42);
> 9604
> 9605 rt2800_bbp_write(rt2x00dev, 30, 0x1F);
> 9606 rt2800_rfcsr_write_bank(rt2x00dev, 0, 39, 0x80);
> 9607 rt2800_rfcsr_write_bank(rt2x00dev, 0, 42, 0x5B);
> 9608
> 9609 rt2800_bbp_write(rt2x00dev, 23, 0x00);
> 9610 rt2800_bbp_write(rt2x00dev, 24, 0x00);
> 9611
> 9612 rt2800_setbbptonegenerator(rt2x00dev);
> 9613
> 9614 for (ch_idx = 0; ch_idx < 2; ch_idx++) {
> 9615 rt2800_bbp_write(rt2x00dev, 23, 0x00);
> 9616 rt2800_bbp_write(rt2x00dev, 24, 0x00);
> 9617 rt2800_register_write(rt2x00dev, MAC_SYS_CTRL,
> 0x00);
> 9618 rt2800_register_write(rt2x00dev, TX_PIN_CFG,
> 0x0000000F);
> 9619 rt2800_register_write(rt2x00dev, RF_CONTROL0,
> 0x00000004);
> 9620 rt2800_register_write(rt2x00dev, RF_BYPASS0,
> 0x00003306);
> 9621 rt2800_register_write(rt2x00dev, 0x13b8, 0x10);
> 9622 udelay(1);
> 9623
> 9624 if (ch_idx == 0)
> 9625 rt2800_rf_aux_tx0_loopback(rt2x00dev);
> 9626 else
> 9627 rt2800_rf_aux_tx1_loopback(rt2x00dev);
> 9628
> 9629 udelay(1);
> 9630
> 9631 if (ch_idx == 0)
> 9632 rt2800_register_write(rt2x00dev,
> RF_CONTROL0, 0x00001004);
> 9633 else
> 9634 rt2800_register_write(rt2x00dev,
> RF_CONTROL0, 0x00002004);
> 9635
> 9636 rt2800_bbp_write(rt2x00dev, 158, 0x05);
> 9637 rt2800_bbp_write(rt2x00dev, 159, 0x00);
> 9638
> 9639 rt2800_bbp_write(rt2x00dev, 158, 0x01);
> 9640 if (ch_idx == 0)
> 9641 rt2800_bbp_write(rt2x00dev, 159, 0x00);
> 9642 else
> 9643 rt2800_bbp_write(rt2x00dev, 159, 0x01);
> 9644
> 9645 vga_gain[ch_idx] = 18;
> ^^
>
> 9646 for (rf_alc_idx = 0; rf_alc_idx < 3; rf_alc_idx++)
> {
> 9647 rt2800_bbp_write(rt2x00dev, 23,
> bbp_2324gain[rf_alc_idx]);
> 9648 rt2800_bbp_write(rt2x00dev, 24,
> bbp_2324gain[rf_alc_idx]);
> 9649
> 9650 macvalue = rt2800_register_read(rt2x00dev,
> RF_CONTROL3);
> 9651 macvalue &= (~0x0000F1F1);
> 9652 macvalue |= (rf_gain[rf_alc_idx] << 4);
> 9653 macvalue |= (rf_gain[rf_alc_idx] << 12);
> 9654 rt2800_register_write(rt2x00dev,
> RF_CONTROL3, macvalue);
> 9655 macvalue = (0x0000F1F1);
> 9656 rt2800_register_write(rt2x00dev,
> RF_BYPASS3, macvalue);
> 9657
> 9658 if (rf_alc_idx == 0) {
> 9659 rt2800_write_dc(rt2x00dev, ch_idx,
> 0, 1, 0x21);
> 9660 for (; vga_gain[ch_idx] > 0;
> 9661 vga_gain[ch_idx] =
> vga_gain[ch_idx] - 2) {
>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 18 is an even number so this can't actually underflow.
>
>
> 9662 rfvalue =
> rfvga_gain_table[vga_gain[ch_idx]];
> 9663
> rt2800_rfcsr_write_dccal(rt2x00dev, 3, rfvalue);
> 9664
> rt2800_rfcsr_write_dccal(rt2x00dev, 4, rfvalue);
> 9665 rt2800_write_dc(rt2x00dev,
> ch_idx, 0, 1, 0x00);
> 9666 rt2800_write_dc(rt2x00dev,
> ch_idx, 0, 0, 0x00);
> 9667 p0 =
> rt2800_do_fft_accumulation(rt2x00dev, 0x0A, 0);
> 9668 rt2800_write_dc(rt2x00dev,
> ch_idx, 0, 0, 0x21);
> 9669 p1 =
> rt2800_do_fft_accumulation(rt2x00dev, 0x0A, 0);
> 9670 rt2x00_dbg(rt2x00dev, "LOFT
> AGC %d %d\n", p0, p1);
> 9671 if ((p0 < 7000 * 7000) &&
> (p1 < (7000 * 7000)))
> 9672 break;
> 9673 }
> 9674
> 9675 rt2800_write_dc(rt2x00dev, ch_idx,
> 0, 0, 0x00);
> 9676 rt2800_write_dc(rt2x00dev, ch_idx,
> 0, 1, 0x00);
> 9677
> 9678 rt2x00_dbg(rt2x00dev, "Used VGA %d
> %x\n", vga_gain[ch_idx],
> 9679
> rfvga_gain_table[vga_gain[ch_idx]]);
> 9680
> --> 9681 if (vga_gain[ch_idx] < 0)
> ^^^^^^^^^^^^^^^^^^^^
> But if it could then this test would not work because the type is u8.
>
> 9682 vga_gain[ch_idx] = 0;
> 9683 }
> 9684
> 9685 rfvalue =
> rfvga_gain_table[vga_gain[ch_idx]];
> 9686
> 9687 rt2800_rfcsr_write_dccal(rt2x00dev, 3,
> rfvalue);
> 9688 rt2800_rfcsr_write_dccal(rt2x00dev, 4,
> rfvalue);
> 9689
> 9690 rt2800_loft_search(rt2x00dev, ch_idx,
> rf_alc_idx, loft_dc_search_result);
> 9691 }
> 9692 }
>
> regards,
> dan carpenter
>
On Sun, May 21, 2023 at 08:23:53PM +0200, Tom Psyborg wrote:
> Hello
>
> This was all taken from reference driver.
> i did not go through detailed debugging to see if the value is
> overwritten in vendor driver.
>
> Maybe ask devs from openwrt who upstreamed these patches
>
The code is clear enough. We could either just delete the underflow
check or change the array to an array of ints instead of char. It's
more of a philosophical question.
regards,
dan carpenter