Re: [PATCH v7 5/5] ethdev: format module EEPROM for SFF-8636

[David Marchand <david.marchand@redhat.com>]

Hello,

I see a lot of switch / case where tables could do the job.
In any case, I'll focus only on one part of the code, that triggers a
build warning witch clang 14 (+ ASan):

On Tue, May 24, 2022 at 8:31 AM Robin Zhang <robinx.zhang@intel.com> wrote:
> +       /* Channel Specific Data */
> +       for (i = 0; i < MAX_CHANNEL_NUM; i++) {
> +               uint8_t rx_power_offset, tx_bias_offset;
> +               uint8_t tx_power_offset;
> +
> +               switch (i) {
> +               case 0:
> +                       rx_power_offset = SFF_8636_RX_PWR_1_OFFSET;
> +                       tx_power_offset = SFF_8636_TX_PWR_1_OFFSET;
> +                       tx_bias_offset = SFF_8636_TX_BIAS_1_OFFSET;
> +                       break;
> +               case 1:
> +                       rx_power_offset = SFF_8636_RX_PWR_2_OFFSET;
> +                       tx_power_offset = SFF_8636_TX_PWR_2_OFFSET;
> +                       tx_bias_offset = SFF_8636_TX_BIAS_2_OFFSET;
> +                       break;
> +               case 2:
> +                       rx_power_offset = SFF_8636_RX_PWR_3_OFFSET;
> +                       tx_power_offset = SFF_8636_TX_PWR_3_OFFSET;
> +                       tx_bias_offset = SFF_8636_TX_BIAS_3_OFFSET;
> +                       break;
> +               case 3:
> +                       rx_power_offset = SFF_8636_RX_PWR_4_OFFSET;
> +                       tx_power_offset = SFF_8636_TX_PWR_4_OFFSET;
> +                       tx_bias_offset = SFF_8636_TX_BIAS_4_OFFSET;
> +                       break;
> +               }
> +               sd->scd[i].bias_cur = OFFSET_TO_U16(tx_bias_offset);
> +               sd->scd[i].rx_power = OFFSET_TO_U16(rx_power_offset);
> +               sd->scd[i].tx_power = OFFSET_TO_U16(tx_power_offset);
> +       }

[15/442] Compiling C object lib/librte_ethdev.a.p/ethdev_sff_8636.c.o
In file included from ../lib/ethdev/sff_8636.c:13:
../lib/ethdev/sff_common.h: In function ‘sff_8636_show_all’:
../lib/ethdev/sff_common.h:101:22: warning: ‘tx_power_offset’ may be
used uninitialized [-Wmaybe-uninitialized]
  101 |                 (data[offset] << 8 | data[(offset) + 1])
      |                      ^
../lib/ethdev/sff_8636.c:621:25: note: ‘tx_power_offset’ was declared here
  621 |                 uint8_t tx_power_offset;
      |                         ^~~~~~~~~~~~~~~
../lib/ethdev/sff_common.h:101:22: warning: ‘tx_bias_offset’ may be
used uninitialized [-Wmaybe-uninitialized]
  101 |                 (data[offset] << 8 | data[(offset) + 1])
      |                      ^
../lib/ethdev/sff_8636.c:620:42: note: ‘tx_bias_offset’ was declared here
  620 |                 uint8_t rx_power_offset, tx_bias_offset;
      |                                          ^~~~~~~~~~~~~~
../lib/ethdev/sff_common.h:101:22: warning: ‘rx_power_offset’ may be
used uninitialized [-Wmaybe-uninitialized]
  101 |                 (data[offset] << 8 | data[(offset) + 1])
      |                      ^
../lib/ethdev/sff_8636.c:620:25: note: ‘rx_power_offset’ was declared here
  620 |                 uint8_t rx_power_offset, tx_bias_offset;
      |                         ^~~~~~~~~~~~~~~
[268/268] Linking target app/test/dpdk-test


This is a false positive.
This can be avoided using some tables:

+               const uint8_t rx_power_offset[MAX_CHANNEL_NUM] = {
+                       SFF_8636_RX_PWR_1_OFFSET,
+                       SFF_8636_RX_PWR_2_OFFSET,
+                       SFF_8636_RX_PWR_3_OFFSET,
+                       SFF_8636_RX_PWR_4_OFFSET,
+               };
+               const uint8_t tx_power_offset[MAX_CHANNEL_NUM] = {
+                       SFF_8636_TX_PWR_1_OFFSET,
+                       SFF_8636_TX_PWR_2_OFFSET,
+                       SFF_8636_TX_PWR_3_OFFSET,
+                       SFF_8636_TX_PWR_4_OFFSET,
+               };
+               const uint8_t tx_bias_offset[MAX_CHANNEL_NUM] = {
+                       SFF_8636_TX_BIAS_1_OFFSET,
+                       SFF_8636_TX_BIAS_2_OFFSET,
+                       SFF_8636_TX_BIAS_3_OFFSET,
+                       SFF_8636_TX_BIAS_4_OFFSET,
+               };
+               sd->scd[i].bias_cur = OFFSET_TO_U16(tx_bias_offset[i]);
+               sd->scd[i].rx_power = OFFSET_TO_U16(rx_power_offset[i]);
+               sd->scd[i].tx_power = OFFSET_TO_U16(tx_power_offset[i]);


-- 
David Marchand