Re: drivers/media/i2c/tc358746.c:817:13: warning: 'p_best' is used uninitialized

From: Marco Felsch
Date: Mon Feb 27 2023 - 04:10:57 EST

Hi Geert,

On 23-02-24, Geert Uytterhoeven wrote:
> Hi Marco,
>
> On Fri, Feb 24, 2023 at 9:58 AM Marco Felsch <m.felsch@xxxxxxxxxxxxxx> wrote:
> > On 23-02-24, kernel test robot wrote:
> > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > head: e4bc15889506723d7b93c053ad4a75cd58248d74
> > > commit: 80a21da360516fa602f3a50eb9792f9dfbfb5fdb media: tc358746: add Toshiba TC358746 Parallel to CSI-2 bridge driver
> > > date: 4 months ago
> > > config: arc-randconfig-r031-20230223 (https://download.01.org/0day-ci/archive/20230224/202302240951.roaFGUy5-lkp@xxxxxxxxx/config)
> > > compiler: arceb-elf-gcc (GCC) 12.1.0
> > > reproduce (this is a W=1 build):
> > > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > > chmod +x ~/bin/make.cross
> > > # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=80a21da360516fa602f3a50eb9792f9dfbfb5fdb
> > > git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> > > git fetch --no-tags linus master
> > > git checkout 80a21da360516fa602f3a50eb9792f9dfbfb5fdb
> > > # save the config file
> > > mkdir build_dir && cp config build_dir/.config
> > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc olddefconfig
> > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/media/i2c/
> >
> > This is still a false positive, should we initialize p_best to make the
> > compiler happy? I think Hans did this once, but he said that this will
> > be gone with gcc-13 if I remember correctly.
>
> Are you sure this is a false positive?
>
> > > If you fix the issue, kindly add following tag where applicable
> > > | Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > > | Link: https://lore.kernel.org/oe-kbuild-all/202302240951.roaFGUy5-lkp@xxxxxxxxx/
> > >
> > > All warnings (new ones prefixed by >>):
> > >
> > > drivers/media/i2c/tc358746.c: In function 'tc358746_find_pll_settings':
> > > >> drivers/media/i2c/tc358746.c:817:13: warning: 'p_best' is used uninitialized [-Wuninitialized]
> > > 817 | u16 p_best, p;
> > > | ^~~~~~
> > > >> drivers/media/i2c/tc358746.c:816:13: warning: 'm_best' is used uninitialized [-Wuninitialized]
> > > 816 | u16 m_best, mul;
> > > | ^~~~~~
> > >
> > >
> > > vim +/p_best +817 drivers/media/i2c/tc358746.c
> > >
> > > 805
> > > 806 static unsigned long tc358746_find_pll_settings(struct tc358746 *tc358746,
> > > 807 unsigned long refclk,
> > > 808 unsigned long fout)
> > > 809
> > > 810 {
> > > 811 struct device *dev = tc358746->sd.dev;
> > > 812 unsigned long best_freq = 0;
> > > 813 u32 min_delta = 0xffffffff;
> > > 814 u16 prediv_max = 17;
> > > 815 u16 prediv_min = 1;
> > > > 816 u16 m_best, mul;
> > > > 817 u16 p_best, p;
> > > 818 u8 postdiv;
> > > 819
> > > 820 if (fout > 1000 * HZ_PER_MHZ) {
> > > 821 dev_err(dev, "HS-Clock above 1 Ghz are not supported\n");
> > > 822 return 0;
> > > 823 }
> > > 824
> > > 825 if (fout >= 500 * HZ_PER_MHZ)
> > > 826 postdiv = 1;
> > > 827 else if (fout >= 250 * HZ_PER_MHZ)
> > > 828 postdiv = 2;
> > > 829 else if (fout >= 125 * HZ_PER_MHZ)
> > > 830 postdiv = 4;
> > > 831 else
> > > 832 postdiv = 8;
> > > 833
> > > 834 for (p = prediv_min; p <= prediv_max; p++) {
> > > 835 unsigned long delta, fin;
> > > 836 u64 tmp;
> > > 837
> > > 838 fin = DIV_ROUND_CLOSEST(refclk, p);
> > > 839 if (fin < 4 * HZ_PER_MHZ || fin > 40 * HZ_PER_MHZ)
> > > 840 continue;
> > > 841
> > > 842 tmp = fout * p * postdiv;
> > > 843 do_div(tmp, fin);
> > > 844 mul = tmp;
> > > 845 if (mul > 511)
> > > 846 continue;
> > > 847
> > > 848 tmp = mul * fin;
> > > 849 do_div(tmp, p * postdiv);
> > > 850
> > > 851 delta = abs(fout - tmp);
> > > 852 if (delta < min_delta) {
>
> So you assume this branch will be taken at least once.
> However, if the smallest delta is 0xffffffff, this is never true.
> Moreover, tmp is u64, while delta is unsigned long, which is
> either 32-bit or 64-bit (it is 32-bit on ARC, I think).

You're right about the u64/unsigned long problem, I will check this :)
But for 'p_best' usaage I'm sure, since we initialze best_freq to zero
and set it only within this if where we set p_best too.

> So I think the code can definitely be improved by cleaning up
> the types, possibly killing the warning as well...
>
> > > 853 p_best = p;
> > > 854 m_best = mul;
> > > 855 min_delta = delta;
> > > 856 best_freq = tmp;
> > > 857 };
> > > 858
> > > 859 if (delta == 0)
> > > 860 break;
> > > 861 };
> > > 862
> > > 863 if (!best_freq) {
> > > 864 dev_err(dev, "Failed find PLL frequency\n");
> > > 865 return 0;
> > > 866 }

If the above if wasn't reached, we will never pass the above if.

Regards,
Marco

> > > 867
> > > 868 tc358746->pll_post_div = postdiv;
> > > 869 tc358746->pll_pre_div = p_best;
> > > 870 tc358746->pll_mul = m_best;
> > > 871
> > > 872 if (best_freq != fout)
> > > 873 dev_warn(dev, "Request PLL freq:%lu, found PLL freq:%lu\n",
> > > 874 fout, best_freq);
> > > 875
> > > 876 dev_dbg(dev, "Found PLL settings: freq:%lu prediv:%u multi:%u postdiv:%u\n",
> > > 877 best_freq, p_best, m_best, postdiv);
> > > 878
> > > 879 return best_freq;
> > > 880 }
> > > 881
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
>
> 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
>