Re: [RFC PATCH net-next v3 15/21] ethtool: provide link settings and link modes in GET_SETTINGS request

From: Florian Fainelli <f.fainelli@gmail.com>
To: Michal Kubecek <mkubecek@suse.cz>
Cc: netdev@vger.kernel.org, David Miller <davem@davemloft.net>,
	Andrew Lunn <andrew@lunn.ch>,
	Jakub Kicinski <jakub.kicinski@netronome.com>,
	Jiri Pirko <jiri@resnulli.us>,
	linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH net-next v3 15/21] ethtool: provide link settings and link modes in GET_SETTINGS request
Date: Thu, 21 Feb 2019 09:40:48 -0800	[thread overview]
Message-ID: <4397375c-3a61-e098-80a8-126e9cfeb52e@gmail.com> (raw)
In-Reply-To: <20190221101425.GO23151@unicorn.suse.cz>

On 2/21/19 2:14 AM, Michal Kubecek wrote:
> On Wed, Feb 20, 2019 at 07:14:50PM -0800, Florian Fainelli wrote:
>> On 2/18/2019 10:22 AM, Michal Kubecek wrote:
>>> +#define ETH_SETTINGS_IM_LINKINFO		0x01
>>> +#define ETH_SETTINGS_IM_LINKMODES		0x02
>>> +
>>> +#define ETH_SETTINGS_IM_ALL			0x03
>>
>> You could define ETH_SETTINGS_IM_ALL as:
>>
>> #define ETH_SETTING_IM_ALL		\
>> 		(ETH_SETTINGS_IM_LINKINFO |
>> 		 ETH_SETTINGS_IM_LINMODES)
>>
>> that would scale better IMHO, especially given that you have to keep
>> bumping that mask with new bits in subsequent patches.
> 
> I'm considering going even further and using something similar to what
> is used for NETIF_F_* constants so that the *_ALL value would be
> calculated automatically. But I'm not sure if it's not too fancy for
> a uapi header file.

Adding new netdev features still requires defining two constants: one in
the enumeration, and one that resolves the bit to bitmask constant, so
this would amount to the same possible mistakes/errors being made here
by changing two lines.

> 
>>> +	if (tb[ETHA_SETTINGS_INFOMASK])
>>> +		req_info->req_mask = nla_get_u32(tb[ETHA_SETTINGS_INFOMASK]);
>>> +	if (tb[ETHA_SETTINGS_COMPACT])
>>> +		req_info->compact = true;
>>> +	if (req_info->req_mask == 0)
>>> +		req_info->req_mask = ETH_SETTINGS_IM_ALL;
>>
>> What if userland is newer than the kernel and specifies a req_mask with
>> bits set that you don't support? Should not you always do an &
>> ETH_SETTINGS_IM_ALL here?
> 
> In that case only known bits would be handled and the check at the end
> of prepare_info() would add a warning to extack that part of the
> information couldn't be provided (same as if some of the recognized
> parts didn't have necessary ethtool_ops handlers or if they failed).

I guess that is fair, I was just wondering if it made sense for the
kernel to report that there is one particular bitmask of settings that
it does not support at all and report that through netlink extended ack,
as opposed to silently not processing the bits it does not know.
-- 
Florian