netdev - Re: [PATCH net 2/3] ice: use int for n_per

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 9 Nov 2022 12:19:03 -0800
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Alexander Lobakin <alexandr.lobakin@...el.com>
CC:     <davem@...emloft.net>, <kuba@...nel.org>, <pabeni@...hat.com>,
        <edumazet@...gle.com>, Tony Nguyen <anthony.l.nguyen@...el.com>,
        <netdev@...r.kernel.org>, <richardcochran@...il.com>,
        Gurucharan G <gurucharanx.g@...el.com>
Subject: Re: [PATCH net 2/3] ice: use int for n_per_out loop



On 11/9/2022 8:37 AM, Alexander Lobakin wrote:
> From: Tony Nguyen <anthony.l.nguyen@...el.com>
> Date: Tue,  8 Nov 2022 15:51:15 -0800
> 
>> From: Jacob Keller <jacob.e.keller@...el.com>
>>
>> In ice_ptp_enable_all_clkout and ice_ptp_disable_all_clkout we use a uint
>> for a for loop iterating over the n_per_out value from the struct
>> ptp_clock_info. The struct member is a signed int, and the use of uint
>> generates a -Wsign-compare warning:
>>
>>    drivers/net/ethernet/intel/ice/ice_ptp.c: In function ‘ice_ptp_enable_all_clkout’:
>>    drivers/net/ethernet/intel/ice/ice_ptp.c:1710:23: error: comparison of integer expressions of different signedness: ‘uint’ {aka ‘unsigned int’} and ‘int’ [-Werror=sign-compare]
>>     1710 |         for (i = 0; i < pf->ptp.info.n_per_out; i++)
>>          |                       ^
>>    cc1: all warnings being treated as errors
>>
>> While we don't generally compile with -Wsign-compare, its still a good idea
> 
> -Wsign-compare is disabled even on W=2.
> 
>> not to mix types. Fix the two functions to use a plain signed integer.
> 
> It's still a good idea to not use ints when values below zero are
> not used. Here both @i's are used as iterators from zero and above.
> The change is just pointless. I would even understand if you casted
> ::n_per_out to `u32` in the loop condition and -Wsign-compare was
> enabled on W=1 or 2, but not this.
> 

Fair. I wonder if the struct ptp_clock_info structure can be switched to 
use uints? It doesn't make sense to have a negative number for these n_* 
fields...

Either way, we can drop this patch. I am investigating where I saw the 
report for this and am going to change that build to not use -Wsign-compare.

Thanks,
Jake