Re: [PATCH 1/2] ppc/pnv: initialize 'taddr' in pnv_phb3_translate_tve()

[Daniel Henrique Barboza]

On 1/26/22 14:28, Matheus K. Ferst wrote:
> On 26/01/2022 10:41, Daniel Henrique Barboza wrote:
> > The 'taddr' variable is left unintialized, being set only inside the
> > "while ((lev--) >= 0)" loop where we get the TCE address. The 'lev' var
> > is an int32_t that is being initiliazed by the GETFIELD() macro, which
> > returns an uint64_t.
> >
> > For a human reader this means that 'lev' will always be positive or zero.
> > But some compilers may beg to differ. 'lev' being an int32_t can in theory
> > be set as negative, and the "while ((lev--) >= 0)" loop might never be
> > reached, and 'taddr' will be left unitialized.
>
> If we expect this code to execute at least once, wouldn't it be better to use a 
> do-while? E.g.:
>
> do {
>      lev--;
>
>      /* Grab the TCE address */
>      taddr = base | (((addr >> sh) & ((1ul << tbl_shift) - 1)) << 3);
>      if (dma_memory_read(&address_space_memory, taddr, &tce,
>      /* ... */
>      }
>      sh -= tbl_shift;
>      base = tce & ~0xfffull;
> } while (lev >= 0);
>
> Otherwise, I think we'll need to initialize tce too.

Initializing tce isn't necessary, at least as far as compiler warning goes,
because tce will be defaulted to zero and its current use (tce & 3, tce & 2,
tce & 1 operations) isn't offending the compiler. For now at least.

That said, I think using a do/while() loop is an idea that fixes the issue while
keeping the code flow, without having to add extra initializations, so I ended
up changing it as you suggested.


Thanks,


Daniel

> Thanks,
> Matheus K. Ferst
> Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
> Analista de Software
> Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>