Message ID | 20200519141813.28167-3-dmurphy@ti.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | DP83869 Enhancements | expand |
On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote: > If the op-mode for the device is not set in the device tree then set > the strapped op-mode and store it for later configuration. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> ../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode: ../drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits] 171 | if (val < 0) | ^
Hi Dan, I love your patch! Perhaps something to improve: [auto build test WARNING on net-next/master] [also build test WARNING on robh/for-next sparc-next/master net/master linus/master v5.7-rc6 next-20200518] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Dan-Murphy/DP83869-Enhancements/20200519-222047 base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 5cdfe8306631b2224e3f81fc5a1e2721c7a1948b config: sh-allmodconfig (attached as .config) compiler: sh4-linux-gcc (GCC) 9.3.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh If you fix the issue, kindly add following tag as appropriate Reported-by: kbuild test robot <lkp@intel.com> All warnings (new ones prefixed by >>, old ones prefixed by <<): drivers/net/phy/dp83869.c: In function 'dp83869_set_strapped_mode': >> drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits] 171 | if (val < 0) | ^ vim +171 drivers/net/phy/dp83869.c 164 165 static int dp83869_set_strapped_mode(struct phy_device *phydev) 166 { 167 struct dp83869_private *dp83869 = phydev->priv; 168 u16 val; 169 170 val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1); > 171 if (val < 0) 172 return val; 173 174 dp83869->mode = val & DP83869_STRAP_OP_MODE_MASK; 175 176 return 0; 177 } 178 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Jakub On 5/19/20 11:58 AM, Jakub Kicinski wrote: > On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote: >> If the op-mode for the device is not set in the device tree then set >> the strapped op-mode and store it for later configuration. >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> > ../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode: > ../drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits] > 171 | if (val < 0) > | ^ This looks to be a false positive. phy_read_mmd will return an errno or a value from 0->15 So if errno is returned then this will be true. Unless I have to do IS_ERR. I did not get that warning. But I am using 9.2-gcc. What compiler are you using? Dan
kbuild On 5/19/20 12:19 PM, kbuild test robot wrote: > Hi Dan, > > I love your patch! Perhaps something to improve: > > [auto build test WARNING on net-next/master] > [also build test WARNING on robh/for-next sparc-next/master net/master linus/master v5.7-rc6 next-20200518] > [if your patch is applied to the wrong git tree, please drop us a note to help > improve the system. BTW, we also suggest to use '--base' option to specify the > base tree in git format-patch, please see https://stackoverflow.com/a/37406982] > > url: https://github.com/0day-ci/linux/commits/Dan-Murphy/DP83869-Enhancements/20200519-222047 > base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 5cdfe8306631b2224e3f81fc5a1e2721c7a1948b > config: sh-allmodconfig (attached as .config) > compiler: sh4-linux-gcc (GCC) 9.3.0 > reproduce: > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kbuild test robot <lkp@intel.com> > > All warnings (new ones prefixed by >>, old ones prefixed by <<): > > drivers/net/phy/dp83869.c: In function 'dp83869_set_strapped_mode': >>> drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits] > 171 | if (val < 0) This looks to be a false positive. phy_read_mmd will return an errno or a value from 0->15 So if errno is returned then this will be true. Unless I have to do IS_ERR. Dan
Jakub On 5/19/20 12:40 PM, Dan Murphy wrote: > Jakub > > On 5/19/20 11:58 AM, Jakub Kicinski wrote: >> On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote: >>> If the op-mode for the device is not set in the device tree then set >>> the strapped op-mode and store it for later configuration. >>> >>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> ../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode: >> ../drivers/net/phy/dp83869.c:171:10: warning: comparison is always >> false due to limited range of data type [-Wtype-limits] >> 171 | if (val < 0) >> | ^ > > This looks to be a false positive. > > phy_read_mmd will return an errno or a value from 0->15 > > So if errno is returned then this will be true. > > Unless I have to do IS_ERR. > > I did not get that warning. But I am using 9.2-gcc. > > What compiler are you using? > I see what the issue is val needs to be an int not a u16 I will fix it Dan > Dan >
On Tue, May 19, 2020 at 09:58:18AM -0700, Jakub Kicinski wrote: > On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote: > > If the op-mode for the device is not set in the device tree then set > > the strapped op-mode and store it for later configuration. > > > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > > ../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode: > ../drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits] > 171 | if (val < 0) > | ^ Hi Jakub This happens a lot with PHY drivers. The register being read is a u16, so that is what people use. Is this now a standard GCC warning? Or have you turned on extra checking? Andrew
Andrew On 5/19/20 1:29 PM, Andrew Lunn wrote: > On Tue, May 19, 2020 at 09:58:18AM -0700, Jakub Kicinski wrote: >> On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote: >>> If the op-mode for the device is not set in the device tree then set >>> the strapped op-mode and store it for later configuration. >>> >>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> ../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode: >> ../drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits] >> 171 | if (val < 0) >> | ^ > Hi Jakub > > This happens a lot with PHY drivers. The register being read is a u16, > so that is what people use. Yes this is what happened but phy_read_mmd returns an int so the declaration of val should be an int. I will update that in v2 > Is this now a standard GCC warning? Or have you turned on extra > checking? I still was not able to reproduce this warning with gcc-9.2. I would like to know the same Dan
On Tue, 19 May 2020 13:41:40 -0500 Dan Murphy wrote: > > Is this now a standard GCC warning? Or have you turned on extra > > checking? > I still was not able to reproduce this warning with gcc-9.2. I would > like to know the same W=1 + gcc-10 here, also curious to know which one of the two makes the difference :)
Jakub On 5/19/20 1:48 PM, Jakub Kicinski wrote: > On Tue, 19 May 2020 13:41:40 -0500 Dan Murphy wrote: >>> Is this now a standard GCC warning? Or have you turned on extra >>> checking? >> I still was not able to reproduce this warning with gcc-9.2. I would >> like to know the same > W=1 + gcc-10 here, also curious to know which one of the two makes > the difference :) W=1 made the difference I got the warning with gcc-9.2 Dan
On Tue, May 19, 2020 at 01:59:16PM -0500, Dan Murphy wrote: > Jakub > > On 5/19/20 1:48 PM, Jakub Kicinski wrote: > > On Tue, 19 May 2020 13:41:40 -0500 Dan Murphy wrote: > > > > Is this now a standard GCC warning? Or have you turned on extra > > > > checking? > > > I still was not able to reproduce this warning with gcc-9.2. I would > > > like to know the same > > W=1 + gcc-10 here, also curious to know which one of the two makes > > the difference :) > > W=1 made the difference I got the warning with gcc-9.2 I wonder if we should turn on this specific warning by default in drivers/net/phy? I keep making the same mistake, and it would be nice if GCC actually told me. Andrew
Hi Dan, I love your patch! Perhaps something to improve: [auto build test WARNING on net-next/master] [also build test WARNING on robh/for-next sparc-next/master net/master linus/master v5.7-rc6 next-20200519] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Dan-Murphy/DP83869-Enhancements/20200519-222047 base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 5cdfe8306631b2224e3f81fc5a1e2721c7a1948b config: sparc64-randconfig-s001-20200519 (attached as .config) reproduce: # apt-get install sparse # sparse version: v0.6.1-193-gb8fad4bc-dirty # save the attached .config to linux build tree make C=1 ARCH=sparc64 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' If you fix the issue, kindly add following tag as appropriate Reported-by: kbuild test robot <lkp@intel.com> All warnings (new ones prefixed by >>, old ones prefixed by <<): In file included from include/linux/build_bug.h:5, from include/linux/bits.h:23, from include/linux/bitops.h:5, from include/linux/bitmap.h:8, from include/linux/ethtool.h:16, from drivers/net/phy/dp83869.c:6: drivers/net/phy/dp83869.c: In function 'dp83869_set_strapped_mode': drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits] 171 | if (val < 0) | ^ include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var' 58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) | ^~~~ >> drivers/net/phy/dp83869.c:171:2: note: in expansion of macro 'if' 171 | if (val < 0) | ^~ drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits] 171 | if (val < 0) | ^ include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var' 58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) | ^~~~ >> drivers/net/phy/dp83869.c:171:2: note: in expansion of macro 'if' 171 | if (val < 0) | ^~ drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits] 171 | if (val < 0) | ^ include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value' 69 | (cond) ? | ^~~~ include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var' 56 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) ) | ^~~~~~~~~~~~~~ >> drivers/net/phy/dp83869.c:171:2: note: in expansion of macro 'if' 171 | if (val < 0) | ^~ vim +/if +171 drivers/net/phy/dp83869.c 164 165 static int dp83869_set_strapped_mode(struct phy_device *phydev) 166 { 167 struct dp83869_private *dp83869 = phydev->priv; 168 u16 val; 169 170 val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1); > 171 if (val < 0) 172 return val; 173 174 dp83869->mode = val & DP83869_STRAP_OP_MODE_MASK; 175 176 return 0; 177 } 178 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Tue, May 19, 2020 at 12:40:37PM -0500, Dan Murphy wrote: > kbuild > > On 5/19/20 12:19 PM, kbuild test robot wrote: > > Hi Dan, > > > > I love your patch! Perhaps something to improve: > > > > [auto build test WARNING on net-next/master] > > [also build test WARNING on robh/for-next sparc-next/master net/master linus/master v5.7-rc6 next-20200518] > > [if your patch is applied to the wrong git tree, please drop us a note to help > > improve the system. BTW, we also suggest to use '--base' option to specify the > > base tree in git format-patch, please see https://stackoverflow.com/a/37406982] > > > > url: https://github.com/0day-ci/linux/commits/Dan-Murphy/DP83869-Enhancements/20200519-222047 > > base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 5cdfe8306631b2224e3f81fc5a1e2721c7a1948b > > config: sh-allmodconfig (attached as .config) > > compiler: sh4-linux-gcc (GCC) 9.3.0 > > reproduce: > > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > > chmod +x ~/bin/make.cross > > # save the attached .config to linux build tree > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh > > > > If you fix the issue, kindly add following tag as appropriate > > Reported-by: kbuild test robot <lkp@intel.com> > > > > All warnings (new ones prefixed by >>, old ones prefixed by <<): > > > > drivers/net/phy/dp83869.c: In function 'dp83869_set_strapped_mode': > > > > drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits] > > 171 | if (val < 0) > > This looks to be a false positive. > > phy_read_mmd will return an errno or a value from 0->15 thanks, here because val is defined as "u16 val", the comparison to < 0 can not work as expected. Any err returned from phy_read_mmd, which is int, will be converted to u16. > > So if errno is returned then this will be true. > > Unless I have to do IS_ERR. > > Dan > _______________________________________________ > kbuild-all mailing list -- kbuild-all@lists.01.org > To unsubscribe send an email to kbuild-all-leave@lists.01.org
diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c index 073a0f7754a5..64fa2d911074 100644 --- a/drivers/net/phy/dp83869.c +++ b/drivers/net/phy/dp83869.c @@ -65,6 +65,7 @@ #define DP83869_RGMII_RX_CLK_DELAY_EN BIT(0) /* STRAP_STS1 bits */ +#define DP83869_STRAP_OP_MODE_MASK GENMASK(2, 0) #define DP83869_STRAP_STS1_RESERVED BIT(11) #define DP83869_STRAP_MIRROR_ENABLED BIT(12) @@ -161,6 +162,20 @@ static int dp83869_config_port_mirroring(struct phy_device *phydev) DP83869_CFG3_PORT_MIRROR_EN); } +static int dp83869_set_strapped_mode(struct phy_device *phydev) +{ + struct dp83869_private *dp83869 = phydev->priv; + u16 val; + + val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1); + if (val < 0) + return val; + + dp83869->mode = val & DP83869_STRAP_OP_MODE_MASK; + + return 0; +} + #ifdef CONFIG_OF_MDIO static int dp83869_of_init(struct phy_device *phydev) { @@ -185,6 +200,10 @@ static int dp83869_of_init(struct phy_device *phydev) if (dp83869->mode < DP83869_RGMII_COPPER_ETHERNET || dp83869->mode > DP83869_SGMII_COPPER_ETHERNET) return -EINVAL; + } else { + ret = dp83869_set_strapped_mode(phydev); + if (ret) + return ret; } if (of_property_read_bool(of_node, "ti,max-output-impedance")) @@ -218,7 +237,7 @@ static int dp83869_of_init(struct phy_device *phydev) #else static int dp83869_of_init(struct phy_device *phydev) { - return 0; + return dp83869_set_strapped_mode(phydev); } #endif /* CONFIG_OF_MDIO */
If the op-mode for the device is not set in the device tree then set the strapped op-mode and store it for later configuration. Signed-off-by: Dan Murphy <dmurphy@ti.com> --- drivers/net/phy/dp83869.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)