On Wed, Feb 20, 2019 at 07:14:50PM -0800, Florian Fainelli wrote:

On 2/18/2019 10:22 AM, Michal Kubecek wrote:

quoted

+#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.

quoted

+	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).

Michal