Hi Tal,
With gcc-4.1.2:
drivers/net/ethernet/broadcom/bcmsysport.c: In function ‘bcm_sysport_poll’:
include/linux/net_dim.h:354: warning: ‘curr_stats.ppms’ may be
used uninitialized in this function
include/linux/net_dim.h:354: warning: ‘curr_stats.bpms’ may be
used uninitialized in this function
include/linux/net_dim.h:354: warning: ‘curr_stats.epms’ may be
used uninitialized in this function
Indeed, ...
| static inline void net_dim_calc_stats(struct net_dim_sample *start,
| struct net_dim_sample *end,
| struct net_dim_stats *curr_stats)
| {
| /* u32 holds up to 71 minutes, should be enough */
| u32 delta_us = ktime_us_delta(end->time, start->time);
| u32 npkts = BIT_GAP(BITS_PER_TYPE(u32), end->pkt_ctr, start->pkt_ctr);
| u32 nbytes = BIT_GAP(BITS_PER_TYPE(u32), end->byte_ctr,
| start->byte_ctr);
|
| if (!delta_us)
| return;
... if delta_us is zero, none of the below will be initialized ...
| curr_stats->ppms = DIV_ROUND_UP(npkts * USEC_PER_MSEC, delta_us);
| curr_stats->bpms = DIV_ROUND_UP(nbytes * USEC_PER_MSEC, delta_us);
| curr_stats->epms = DIV_ROUND_UP(NET_DIM_NEVENTS * USEC_PER_MSEC,
| delta_us);
| }
|
| static inline void net_dim(struct net_dim *dim,
| struct net_dim_sample end_sample)
| {
| struct net_dim_stats curr_stats;
| u16 nevents;
|
| switch (dim->state) {
| case NET_DIM_MEASURE_IN_PROGRESS:
| nevents = BIT_GAP(BITS_PER_TYPE(u16),
| end_sample.event_ctr,
| dim->start_sample.event_ctr);
| if (nevents < NET_DIM_NEVENTS)
| break;
| net_dim_calc_stats(&dim->start_sample, &end_sample,
| &curr_stats);
... in the output parameter curr_stats ...
| if (net_dim_decision(&curr_stats, dim)) {
... and net_dim_decision will make some funky decisions based on
uninitialized data.
What are the proper values to initialize curr_stats with?
Alternatively, perhaps the call to net_dim_decision() should be made
dependent on delta_us being non-zero?
| dim->state = NET_DIM_APPLY_NEW_PROFILE;
| schedule_work(&dim->work);
| break;
| }
| /* fall through */
| case NET_DIM_START_MEASURE:
| dim->state = NET_DIM_MEASURE_IN_PROGRESS;
| break;
| case NET_DIM_APPLY_NEW_PROFILE:
| break;
| }
| }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 4/5/2018 4:13 PM, Geert Uytterhoeven wrote:
> Hi Tal,
>
> With gcc-4.1.2:
>
> drivers/net/ethernet/broadcom/bcmsysport.c: In function ‘bcm_sysport_poll’:
> include/linux/net_dim.h:354: warning: ‘curr_stats.ppms’ may be
> used uninitialized in this function
> include/linux/net_dim.h:354: warning: ‘curr_stats.bpms’ may be
> used uninitialized in this function
> include/linux/net_dim.h:354: warning: ‘curr_stats.epms’ may be
> used uninitialized in this function
>
> Indeed, ...
>
> | static inline void net_dim_calc_stats(struct net_dim_sample *start,
> | struct net_dim_sample *end,
> | struct net_dim_stats *curr_stats)
> | {
> | /* u32 holds up to 71 minutes, should be enough */
> | u32 delta_us = ktime_us_delta(end->time, start->time);
> | u32 npkts = BIT_GAP(BITS_PER_TYPE(u32), end->pkt_ctr, start->pkt_ctr);
> | u32 nbytes = BIT_GAP(BITS_PER_TYPE(u32), end->byte_ctr,
> | start->byte_ctr);
> |
> | if (!delta_us)
> | return;
>
> ... if delta_us is zero, none of the below will be initialized ...
>
> | curr_stats->ppms = DIV_ROUND_UP(npkts * USEC_PER_MSEC, delta_us);
> | curr_stats->bpms = DIV_ROUND_UP(nbytes * USEC_PER_MSEC, delta_us);
> | curr_stats->epms = DIV_ROUND_UP(NET_DIM_NEVENTS * USEC_PER_MSEC,
> | delta_us);
> | }
> |
> | static inline void net_dim(struct net_dim *dim,
> | struct net_dim_sample end_sample)
> | {
> | struct net_dim_stats curr_stats;
> | u16 nevents;
> |
> | switch (dim->state) {
> | case NET_DIM_MEASURE_IN_PROGRESS:
> | nevents = BIT_GAP(BITS_PER_TYPE(u16),
> | end_sample.event_ctr,
> | dim->start_sample.event_ctr);
> | if (nevents < NET_DIM_NEVENTS)
> | break;
> | net_dim_calc_stats(&dim->start_sample, &end_sample,
> | &curr_stats);
>
> ... in the output parameter curr_stats ...
>
> | if (net_dim_decision(&curr_stats, dim)) {
>
> ... and net_dim_decision will make some funky decisions based on
> uninitialized data.
>
> What are the proper values to initialize curr_stats with?
> Alternatively, perhaps the call to net_dim_decision() should be made
> dependent on delta_us being non-zero?
First, thanks a lot for pointing this out. There are no valid values for
initializing curr_stats. If we consider the most straightforward (all
0s) this may result in a (big) negative delta between current and
previous stats and a wrong decision. Any other value would make very
little sense.
The case of !delta_us is an error flow (0 time passed or more probably
issues when setting start and/or end times). I suggest adding a return
value to net_dim_calc_stats() and abort the net_dim cycle if an error
occurs.
>
> | dim->state = NET_DIM_APPLY_NEW_PROFILE;
> | schedule_work(&dim->work);
> | break;
> | }
> | /* fall through */
> | case NET_DIM_START_MEASURE:
> | dim->state = NET_DIM_MEASURE_IN_PROGRESS;
> | break;
> | case NET_DIM_APPLY_NEW_PROFILE:
> | break;
> | }
> | }
>
> Gr{oetje,eeting}s,
>
> Geert
>