Hi Marco,
FYI, the error/warning still remains.
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/[email protected]/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/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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) {
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 }
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On 23-02-24, kernel test robot wrote:
> Hi Marco,
>
> FYI, the error/warning still remains.
>
> 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/[email protected]/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.
Regards,
Marco
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <[email protected]>
> | Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> 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) {
> 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 }
> 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
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests
>
Hi Marco,
On Fri, Feb 24, 2023 at 9:58 AM Marco Felsch <[email protected]> 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/[email protected]/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 <[email protected]>
> > | Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> >
> > 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).
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 }
> > 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 -- [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
Hi Geert,
On 23-02-24, Geert Uytterhoeven wrote:
> Hi Marco,
>
> On Fri, Feb 24, 2023 at 9:58 AM Marco Felsch <[email protected]> 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/[email protected]/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 <[email protected]>
> > > | Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > >
> > > 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 -- [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
>