Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 16 Jan 2023 19:42:34 +0200
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     Jakub Kicinski <kuba@kernel.org>
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>,
        Michal Kubecek <mkubecek@suse.cz>,
        Claudiu Manoil <claudiu.manoil@nxp.com>,
        Vinicius Costa Gomes <vinicius.gomes@intel.com>,
        Xiaoliang Yang <xiaoliang.yang_1@nxp.com>,
        Kurt Kanzenbach <kurt@linutronix.de>,
        Rui Sousa <rui.sousa@nxp.com>,
        Ferenc Fejes <ferenc.fejes@ericsson.com>,
        Pranavi Somisetty <pranavi.somisetty@amd.com>,
        Harini Katakam <harini.katakam@amd.com>,
        Colin Foster <colin.foster@in-advantage.com>,
        UNGLinuxDriver@microchip.com,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>
Subject: Re: [PATCH v2 net-next 04/12] net: ethtool: netlink: retrieve stats
 from multiple sources (eMAC, pMAC)
Message-ID: <20230116174234.yzq6cnczs6fxww6q@skbuf>
References: <20230111161706.1465242-1-vladimir.oltean@nxp.com>
 <20230111161706.1465242-5-vladimir.oltean@nxp.com>
 <20230113204336.401a2062@kernel.org>
 <20230114232214.tj6bsfhmhfg3zjxw@skbuf>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230114232214.tj6bsfhmhfg3zjxw@skbuf>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?YvqULhosC9BZZq7unvjYMfUy3iU1Vai7657q7M1Wa68E/F8rJfrHe38QxF7y?=
 =?us-ascii?Q?P54U1nCpXLql8Ix+ofAumory9V3HAXIXV+95co4Ht9OhbZV7SE/ybQB1Y6s3?=
 =?us-ascii?Q?euveEtg+v8l35v03r7OYBEchHxNffBa/4R+R4+Y2OVdGZxwBsiY5yane8k3B?=
 =?us-ascii?Q?v/a1ftq//WGaxaqzAkuNIUGPLcEEL7NGXYaZspn+Hcc1iKm7QPRwcpw3j5Zd?=
 =?us-ascii?Q?qJ/QAAYH3gYJw6M6iHNcIz1weyo7SLRL4nswIc7IZMHwb4oWeIXN8eB5qfbh?=
 =?us-ascii?Q?86LU08M07x0tGTCndb9OQmOF87ah+m0Z5NEwhH+hkOxVwI07xPhBCc6ke6iz?=
 =?us-ascii?Q?qgRuuu+c3MDmv8qli731FDtBF1twLiTrfvo5awS8vX7lz7r4VO6eiiHo4aM8?=
 =?us-ascii?Q?zaklZFrS2SSY1DsQFfnHF993aQ+v2uFXWBGQNQZKKNU4PhX+ihR4kbZnjCTU?=
 =?us-ascii?Q?XbYNO/MIvXp7iffxCLwT6S3DvPHJqSkUf4vDtwk/q23RRzsJ6jZzkVG0zHYO?=
 =?us-ascii?Q?gT0HdYAHtnmwArW5Soh/1V7vO36sKasJJghld3pCo3WPvy1xHnDwwOtuvMo8?=
 =?us-ascii?Q?YpRMUSlFJgA13lcbwH1uzYe+mgqvhYIyLwUg0mbZ5e3W1p7ydzOm8pdJCFkc?=
 =?us-ascii?Q?eyHbMqjy+flpSXJO09rx9MnVbjNxT/yJPAdfMsolBBJznGvrMbIZQZykrdTu?=
 =?us-ascii?Q?QCpIii+AGsZj29HANiIFZV4eB/j11HBx8PKAfBMlrC5694a4bRSoYjEeTVXj?=
 =?us-ascii?Q?oGgP1SCjDij4avg6Bg8z4tPr36xLrtJal5ZlXpl6LNhlpfmveV0duZvbexmw?=
 =?us-ascii?Q?w3zCEhE/gYlFO0Tw86fYpDkjenIu1/yEz8VWheqh7hvxbOe2MAaJJifm3CNp?=
 =?us-ascii?Q?jvGUZ1gpZq6M1dBpo49I+QgjPjiUntjSJ81Pz4LCwEZpg7dkvxdOcVOpG6qM?=
 =?us-ascii?Q?7x9OBspRusO4mOJpKyd6ouWelQSRi0r2FP+ayXIQCMdXnZwLoRj9ThQhlTyk?=
 =?us-ascii?Q?OYVrU6XjJTp2NLH8RwWV9RrTL/FPvF+W7rkC8jPIPvjgdMFp7a+ppnGBdx0h?=
 =?us-ascii?Q?jM51uowKhqTlJnZrs6NbjNVVTuUG4i7MdjXWCFjd1AUafleBb3UCyTYGvLOT?=
 =?us-ascii?Q?74HwC63EjRwLPO0rTgj+9/wg4ATokqb6dApEq+z7EZTc8MN5LRyWXlOLAhWr?=
 =?us-ascii?Q?OX9nLBMq+n8N4iuKIVETaKurny4r0W/yvgHspYJI5840lUwJGyBbE3K0LN5x?=
 =?us-ascii?Q?s42mOGXwLu4apEqM2uj9tjr8jUy3qB4Anonx/iXApVb5uZr9FGsmquTZZOg/?=
 =?us-ascii?Q?D69754t+2+XpqDVzFoj05KX52B/Ha70k8haU728yY4upWkBXGtfaFneyoydF?=
 =?us-ascii?Q?d41cLLR0x5ArTxhJqIolmEAArRR5f1N6ZkxoxKwcxPsjVpE5Q83ESevDw+0G?=
 =?us-ascii?Q?3PCcfcT5rKclT9BxhaIOKE6/x2BT7JxqgWzGz0s7HI/+H/B1ywvI2g7gHBzs?=
 =?us-ascii?Q?/UH38Q7+cuBOMNYdk+cV9AlV9r8lizf45M2ofjvryJjhwNLDpeuw2ZaVVDUl?=
 =?us-ascii?Q?0aD9Ud5WI9K2CNyshz/WwD1xhpfGpbJoEnzr6TiMDU7WhYr9hogqocMg0w5R?=
 =?us-ascii?Q?QA=3D=3D?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b3c80069-75b3-4723-30fb-08daf7e90fb4
X-MS-Exchange-CrossTenant-AuthSource: VI1PR04MB5136.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Jan 2023 17:42:39.1129
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: LwwpHqRMNuECm3e8DtEOnZeTipwdw81icbYQnW9q5PCwpbdAwTtoqh/o/Io+0oC+6OWZFBUBFEu41V1iX4+ZLg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PR3PR04MB7353
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sun, Jan 15, 2023 at 01:22:14AM +0200, Vladimir Oltean wrote:
> On Fri, Jan 13, 2023 at 08:43:36PM -0800, Jakub Kicinski wrote:
> > On Wed, 11 Jan 2023 18:16:58 +0200 Vladimir Oltean wrote:
> > > +/**
> > > + * enum ethtool_stats_src - source of ethtool statistics
> > > + * @ETHTOOL_STATS_SRC_AGGREGATE:
> > > + *	if device supports a MAC merge layer, this retrieves the aggregate
> > > + *	statistics of the eMAC and pMAC. Otherwise, it retrieves just the
> > > + *	statistics of the single (express) MAC.
> > > + * @ETHTOOL_STATS_SRC_EMAC:
> > > + *	if device supports a MM layer, this retrieves the eMAC statistics.
> > > + *	Otherwise, it retrieves the statistics of the single (express) MAC.
> > > + * @ETHTOOL_STATS_SRC_PMAC:
> > > + *	if device supports a MM layer, this retrieves the pMAC statistics.
> > > + */
> > > +enum ethtool_stats_src {
> > > +	ETHTOOL_STATS_SRC_AGGREGATE,
> > > +	ETHTOOL_STATS_SRC_EMAC,
> > > +	ETHTOOL_STATS_SRC_PMAC,
> > > +};
> > 
> > Should we somehow call it "MAC stats"?
> > 
> > Right now its named like a generic attribute, but it's not part of 
> > the header nest (ETHTOOL_A_HEADER_*).
> > 
> > I'm not sure myself which way is better, but feels like either it
> > should be generic, in the header nest, and parsed by the common code;
> > or named more specifically and stay in the per-cmd attrs.
> 
> Considering that I currently have separate netlink attributes for
> ETHTOOL_MSG_STATS_GET (ETHTOOL_A_STATS_SRC) and for
> ETHTOOL_MSG_PAUSE_GET (ETHTOOL_A_PAUSE_STATS_SRC), I'm going to add just
> a single attribute right under ETHTOOL_A_HEADER_FLAGS for v3 and go from
> there. Is it ok if I keep naming it ETHTOOL_A_STATS_SRC, or would you
> prefer something else?

I'm already lost while trying to implement this change request.

ETHTOOL_A_STATS_HEADER uses NLA_POLICY_NESTED(ethnl_header_policy),
while ETHTOOL_A_PAUSE_HEADER uses NLA_POLICY_NESTED(ethnl_header_policy_stats).

The two header nest policies look like this:

const struct nla_policy ethnl_header_policy[] = {
	[ETHTOOL_A_HEADER_DEV_INDEX]	= { .type = NLA_U32 },
	[ETHTOOL_A_HEADER_DEV_NAME]	= { .type = NLA_NUL_STRING,
					    .len = ALTIFNAMSIZ - 1 },
	[ETHTOOL_A_HEADER_FLAGS]	= NLA_POLICY_MASK(NLA_U32,
							  ETHTOOL_FLAGS_BASIC),
};

const struct nla_policy ethnl_header_policy_stats[] = {
	[ETHTOOL_A_HEADER_DEV_INDEX]	= { .type = NLA_U32 },
	[ETHTOOL_A_HEADER_DEV_NAME]	= { .type = NLA_NUL_STRING,
					    .len = ALTIFNAMSIZ - 1 },
	[ETHTOOL_A_HEADER_FLAGS]	= NLA_POLICY_MASK(NLA_U32,
							  ETHTOOL_FLAGS_STATS),
};

The request seems to be for ETHTOOL_A_PAUSE_HEADER to use a policy like this:

 const struct nla_policy ethnl_header_policy_mac_stats[] = {
 	[ETHTOOL_A_HEADER_DEV_INDEX]	= { .type = NLA_U32 },
 	[ETHTOOL_A_HEADER_DEV_NAME]	= { .type = NLA_NUL_STRING,
 					    .len = ALTIFNAMSIZ - 1 },
 	[ETHTOOL_A_HEADER_FLAGS]	= NLA_POLICY_MASK(NLA_U32,
 							  ETHTOOL_FLAGS_STATS),
+	[ETHTOOL_A_HEADER_MAC_STATS_SRC] = NLA_POLICY_MASK(NLA_U32,
+							   ETHTOOL_MAC_STATS_SRC_PMAC),
 };

and for ETHTOOL_A_STATS_HEADER to use a policy like this:

const struct nla_policy ethnl_header_policy_mac_stats_src_basic[] = {
	[ETHTOOL_A_HEADER_DEV_INDEX]	= { .type = NLA_U32 },
	[ETHTOOL_A_HEADER_DEV_NAME]	= { .type = NLA_NUL_STRING,
					    .len = ALTIFNAMSIZ - 1 },
	[ETHTOOL_A_HEADER_FLAGS]	= NLA_POLICY_MASK(NLA_U32,
							  ETHTOOL_FLAGS_BASIC),
+	[ETHTOOL_A_HEADER_MAC_STATS_SRC] = NLA_POLICY_MASK(NLA_U32,
+							   ETHTOOL_MAC_STATS_SRC_PMAC),
};

Did I get this right?