Re: [PATCH net-next] cxgb4/chcr: nic-tls stats in ethtool

[rohit maheshwari <rohitm@chelsio.com>]

On 23/03/20 2:27 AM, Jakub Kicinski wrote:
> On Sat, 21 Mar 2020 16:53:36 +0530 Rohit Maheshwari wrote:
>> Included nic tls statistics in ethtool stats.
>>
>> Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com>
>> ---
>>   .../ethernet/chelsio/cxgb4/cxgb4_ethtool.c    | 57 +++++++++++++++++++
>>   1 file changed, 57 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
>> index 398ade42476c..4998f1d1e218 100644
>> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
>> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
>> @@ -134,6 +134,28 @@ static char loopback_stats_strings[][ETH_GSTRING_LEN] = {
>>   	"bg3_frames_trunc       ",
>>   };
>>   
>> +#ifdef CONFIG_CHELSIO_TLS_DEVICE
>> +struct chcr_tls_stats {
>> +	u64 tx_tls_encrypted_packets;
>> +	u64 tx_tls_encrypted_bytes;
>> +	u64 tx_tls_ctx;
>> +	u64 tx_tls_ooo;
>> +	u64 tx_tls_skip_no_sync_data;
>> +	u64 tx_tls_drop_no_sync_data;
>> +	u64 tx_tls_drop_bypass_req;
> I don't understand why you need to have a structure for this, and then
> you memset it to 0, unnecessarily, but I guess that's just a matter of
> taste.
Yeah, this memset is pure stupidity, I'll remove it and this new
structure as well. Thanks for the suggestion.
>> +};
>> +
>> +static char chcr_tls_stats_strings[][ETH_GSTRING_LEN] = {
>> +	"tx_tls_encrypted_pkts  ",
>> +	"tx_tls_encrypted_bytes ",
>> +	"tx_tls_ctx             ",
>> +	"tx_tls_ooo             ",
>> +	"tx_tls_skip_nosync_data",
>> +	"tx_tls_drop_nosync_data",
>> +	"tx_tls_drop_bypass_req ",
> These, however, are not correct - please remove the spaces at the end,
> otherwise your names are different than for other vendors. And there is
> an underscore in the middle of "no_sync".
>
> When you're told to adhere to API recommendation, please adhere to it
> exactly.
These spaces are used for alignment, so the statistics will be readable
for end user. As far as I understood, these are minimum set of TLS
related statistics, with or without space it will remain same for end
user. Please let me know if I am interpreting it wrong.
  However I agree about the no_sync, I'll change it in my
next patch.