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

["Zhang, RobinX" <robinx.zhang@intel.com>]

Hi, David

This is a good suggestion and I will adopt it, send v8. Thanks!

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, May 24, 2022 5:04 PM
> To: Zhang, RobinX <robinx.zhang@intel.com>
> Cc: dev <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>;
> Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Liu, KevinX
> <kevinx.liu@intel.com>
> Subject: Re: [PATCH v7 5/5] ethdev: format module EEPROM for SFF-8636
> 
> 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