Message-ID: <20250317092808.jel2au3cgfwblaxk@skbuf>
Date: Mon, 17 Mar 2025 11:28:08 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Wei Fang <wei.fang@....com>
Cc: Claudiu Manoil <claudiu.manoil@....com>,
	Clark Wang <xiaoning.wang@....com>,
	"andrew+netdev@...n.ch" <andrew+netdev@...n.ch>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>,
	"christophe.leroy@...roup.eu" <christophe.leroy@...roup.eu>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v4 net-next 01/14] net: enetc: add initial netc-lib
 driver to support NTMP

Hi Wei,

On Fri, Mar 14, 2025 at 03:48:21PM +0200, Wei Fang wrote:
> > I mean, I was just suggesting to group the macros with the macros, and
> > the struct fields with the struct fields. Mixing them together looks a
> > bit messy to me. Even worse in the definition of "union netc_cbd" which
> > IMO needs to be reconsidered a bit (a hint given below).
> 
> I think this is a matter of personal preference. I was inspired by some
> of Intel's structure definitions. I think this method allows me to quickly
> understand what fields the member consists of and what bits each field
> occupies.
(...)
> Thanks, but we have added fully NTMP support in downstream, it's a great
> challenge for me to convert it to the 'packing' method. I don't think I have
> too much time to do this conversion. And I also need some time to figure
> out how to use it and whether it is worth doing so.

Ok, I'm not forcing you to use pack_fields(), but given the fact that
bit field overlap is now something that has to be manually checked, and
a bug of this kind already exists in this set, you will have to wait for
me, or other reviewers, to go through the bit field definitions from the
entire set.

> > And I agree with you, I also don't want ntmp_private.h to be exposed to
> > the NTMP API consumers, and I wasn't suggesting that. I just want the
> > NTMP API to spare consumer drivers of the gory details, like for example
> > the buffer layout of a MAC filtering entry, reserved fields, things like
> > that. What I was saying is to keep the buffer layout private to
> > ntmp_private.h, and expose a more abstract data structure to API
> > consumers.
> 
> Sorry, I don't fully understand, for example, if we place the definition
> of "struct maft_keye_data" in ntmp_private.h, how does the debugfs
> get the information from "struct maft_keye_data"? Add a helper function
> in the NTMP driver and export it to enetc driver? And how does
> enetc4_pf_add_si_mac_exact_filter() to set the mac address to "struct
> maft_keye_data", add another helper? If so, I think it is more complicated.

Well, my complaint, which has to do with style and personal preference,
is that exposing packed data structures to NTMP API clients puts an
unnecessary burden on them. For example, NTMP users have to call cpu_to_le16()
to populate data.cfge.si_bitmap. A bug in the packed layout will
potentially have to be fixed in multiple places. The two options for a
more high-level NTMP API that I see are:
- You expose pointers to the packed data structures, but API functions
  provide getters and setters, and the exact buffer layout is only known
  to the NTMP layer.
- The API functions expose "unpacked" data structures, which are more
  abstract and don't contain reserved fields and are in CPU native
  endianness, and the NTMP layer packs them to buffers, either using
  pack_fields() or manually.

Claudiu, what do you think? I can withdraw the request to hide packed
MAFT (and other) struct definitions from include/linux/fsl/ntmp.h if you
think they're fine there.

> > Thank you for posting the downstream layout of struct ntmp_priv which I
> > was already aware of. What I was saying is that the word "private" means
> > an aspect of the implementation which is hidden from other code layers.
> > There's nothing "private" here if the NTMP layer has access to the
> > definition of this structure. I was asking whether you agree that it's
> > more adequate to name this structure "ntmp_client", since it represents
> > the data associated with a consumer of the NTMP API - a NETC SI or (in
> > the future) the switch. Or some other name. But the word "private" seems
> > misused.
> 
> Okay, it seems to make you feel confused, let me rename it later, how about
> "ntmp_user"?

"ntmp_user" works for me better than "ntmp_priv", thanks. Are you also
going to make the API functions from include/linux/fsl/ntmp.h take
"struct ntmp_user *" as first argument, rather than "struct netc_cbdrs *"?

Powered by blists - more mailing lists