On 4/2/19, 4:26 PM, "David Miller" <da...@davemloft.net> wrote:
> From: Michael Zhivich <mzhiv...@akamai.com> > Date: Mon, 1 Apr 2019 13:14:28 -0400 > >> When building C++ userspace code that includes ethtool.h >> with "-Werror -Wall", g++ complains about signed-unsigned comparison in >> ethtool_validate_speed() due to definition of SPEED_UNKNOWN as -1. >> >> Change definition of SPEED_UNKNOWN to UINT_MAX to match the type of >> ethtool_validate_speed() argument (__u32). >> >> Update storage type for link speed in drivers to use u32 instead of int >> or u16 to bring drivers up-to-date with ethtool.h which includes SPEED_* >> constants larger than range of u16. >> >> Fix usage of SPEED_* constants in >> drivers/net/ethernet/cavium/thunder/thunder_bgx.c and >> drivers/infiniband/ulp/ipoib/ipoib_ethtool.c. >> >> Signed-off-by: Michael Zhivich <mzhiv...@akamai.com> > >Waaaaay too many changes in one patch. Do one thing at a time. OK - I will refactor into a patch series. > >Also, are you absolutely sure that the things you changed from 'int' >aren't used in other ways where the signedness matters? For example >for error returns? > I will double-check, but I haven't observed that during my last review. >And maybe the drivers using u16 are legitimate because they are >operating on speeds only within that bitmap range. Type matching is >not an end to itself. So I don't want to see u16 --> u32 conversions >where they aren't even necessary. > In principle, I would agree; however, there's a problem with keeping u16 storage with the current definition of ethtool_validate_speed(). Using *current* definitions, if a u16 variable is assigned SPEED_UNKNOWN (-1), and then passed to ethtool_validate_speed(), the check will succeed, but for the wrong reasons: (__u32)((u16)-1) <= INT_MAX, rather than (__u32)((u16)-1) == -1. With *proposed* change of SPEED_UNKNOWN to UINT_MAX, using u16 storage will result in -Woverflow warnings during compilation, hence the changes to drivers. >Also: > >> /* Return lane speed in unit of 1e6 bit/sec */ >> -static inline int ib_speed_enum_to_int(int speed) >> +static inline u32 ib_speed_enum_to_int(int speed) > >This is sloppy. > >You are changing the return type, but not adjusting the name of the >function appropriately. The function name says it converts to >an 'int' but now it actually converts to and returns a u32. > Fair enough, will fix. >You need to split up, audit, and present these changes more carefully >and properly. > >Thank you. Thanks for the quick response. ~ Michael
