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

["huangguangbin (A)" <huangguangbin2@huawei.com>]

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@hisilicon.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@hisilicon.com>
>> Signed-off-by: Guangbin Huang <huangguangbin2@huawei.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.

>