netdev - Re: [PATCH net-next 3/5] ethtool: add support to set/get rx buf len

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 for Android: free password hash cracker in your pocket

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

Message-ID: <004dc46f-da33-3dbf-14db-006249c18fb1@huawei.com>
Date:   Thu, 26 Aug 2021 15:05:07 +0800
From:   "huangguangbin (A)" <huangguangbin2@...wei.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     <davem@...emloft.net>, <mkubecek@...e.cz>, <andrew@...n.ch>,
        <amitc@...lanox.com>, <idosch@...sch.org>, <danieller@...dia.com>,
        <netdev@...r.kernel.org>, <lipeng321@...wei.com>,
        <chenhao288@...ilicon.com>
Subject: Re: [PATCH net-next 3/5] ethtool: add support to set/get rx buf len



On 2021/8/25 23:09, Jakub Kicinski wrote:
> On Wed, 25 Aug 2021 14:40:53 +0800 Guangbin Huang wrote:
>> From: Hao Chen <chenhao288@...ilicon.com>
>>
>> Add support to set rx buf len via ethtool -G parameter and get
>> rx buf len via ethtool -g parameter.
>>
>> Signed-off-by: Hao Chen <chenhao288@...ilicon.com>
>> Signed-off-by: Guangbin Huang <huangguangbin2@...wei.com>
> 
>> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
>> index 266e95e4fb33..6e26586274b3 100644
>> --- a/include/uapi/linux/ethtool.h
>> +++ b/include/uapi/linux/ethtool.h
>> @@ -516,6 +516,7 @@ struct ethtool_coalesce {
>>    *	jumbo ring
>>    * @tx_pending: Current maximum supported number of pending entries
>>    *	per TX ring
>> + * @rx_buf_len: Current supported size of rx ring BD buffer.
> 
> How about "Current length of buffers on the rx ring"?
> 
Yes, thanks.

>>    * If the interface does not have separate RX mini and/or jumbo rings,
>>    * @rx_mini_max_pending and/or @rx_jumbo_max_pending will be 0.
>> @@ -533,6 +534,7 @@ struct ethtool_ringparam {
>>   	__u32	rx_mini_pending;
>>   	__u32	rx_jumbo_pending;
>>   	__u32	tx_pending;
>> +	__u32	rx_buf_len;
>>   };
> 
> You can't extend this structure, because it's used by the IOCTL
> interface directly. You need to pass the new value to the drivers
> in a different way. You can look at what Yufeng Mo did recently
> for an example approach.
> 
Ok, thanks your advice, we will modify this patch.

>> @@ -105,6 +109,7 @@ const struct nla_policy ethnl_rings_set_policy[] = {
>>   	[ETHTOOL_A_RINGS_RX_MINI]		= { .type = NLA_U32 },
>>   	[ETHTOOL_A_RINGS_RX_JUMBO]		= { .type = NLA_U32 },
>>   	[ETHTOOL_A_RINGS_TX]			= { .type = NLA_U32 },
>> +	[ETHTOOL_A_RINGS_RX_BUF_LEN]            = { .type = NLA_U32 },
> 
> We should prevent user space for passing 0 as input, so we can use it
> as a special value in the kernel. NLA_POLICY_MIN()
> 
Ok.

>>   };
>>   
>>   int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info)
>> @@ -142,6 +147,8 @@ int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info)
>>   	ethnl_update_u32(&ringparam.rx_jumbo_pending,
>>   			 tb[ETHTOOL_A_RINGS_RX_JUMBO], &mod);
>>   	ethnl_update_u32(&ringparam.tx_pending, tb[ETHTOOL_A_RINGS_TX], &mod);
>> +	ethnl_update_u32(&ringparam.rx_buf_len,
>> +			 tb[ETHTOOL_A_RINGS_RX_BUF_LEN], &mod);
>>   	ret = 0;
>>   	if (!mod)
>>   		goto out_ops;
> 
> We need a way of preventing drivers which don't support the new option
> from just silently ignoring it. Please add a capability like
> cap_link_lanes_supported and reject non-0 ETHTOOL_A_RINGS_RX_BUF_LEN
> if it's not set.
> .
Ok.

>