Re: [PATCH net-next v2 1/5] devlink: Reduce struct devlink exposure

From: Leon Romanovsky <leon@kernel.org>
To: Jakub Kicinski <kuba@kernel.org>
Cc: "David S . Miller" <davem@davemloft.net>,
	Ido Schimmel <idosch@nvidia.com>, Ingo Molnar <mingo@redhat.com>,
	Jiri Pirko <jiri@nvidia.com>,
	linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org,
	mlxsw@nvidia.com, Moshe Shemesh <moshe@nvidia.com>,
	netdev@vger.kernel.org, Saeed Mahameed <saeedm@nvidia.com>,
	Salil Mehta <salil.mehta@huawei.com>,
	Shay Drory <shayd@nvidia.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Tariq Toukan <tariqt@nvidia.com>,
	Yisen Zhuang <yisen.zhuang@huawei.com>
Subject: Re: [PATCH net-next v2 1/5] devlink: Reduce struct devlink exposure
Date: Tue, 5 Oct 2021 09:13:18 +0300	[thread overview]
Message-ID: <YVvs/kNqxumdoVjR@unreal> (raw)
In-Reply-To: <20211004163808.437ea8f9@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>

On Mon, Oct 04, 2021 at 04:38:08PM -0700, Jakub Kicinski wrote:
> On Sun,  3 Oct 2021 21:12:02 +0300 Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > The declaration of struct devlink in general header provokes the
> > situation where internal fields can be accidentally used by the driver
> > authors. In order to reduce such possible situations, let's reduce the
> > namespace exposure of struct devlink.
> > 
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> 
> 100% subjective but every time I decided to hide a structure definition
> like this I came to regret it later. The fact there is only one minor
> infraction in drivers poking at members seems to prove this is not in
> fact needed.

Yes, it is subjective, my experience is completely opposite :). Every
time the internals were exposed, they were abused.

IMHO, the one user that poked into the struct devlink internals is a pure
luck together with lack of devlink adoption outside of the netdev which
limited number of devlink API users. The more devlink will be used, the
more creative usage will be.

For example, ionic had internal logic based on internal devlink_port state:
 * c2255ff47768 ("ionic: cleanly release devlink instance")
 * d7907a2b1a3b ("devlink: Remove duplicated registration check")

However, this patch was written not because of having right software
abstraction, but because of the next patch, where I needed to have
declaration of "struct devlink_ops" before struct devlink itself.

Without this patch, I would need to heavily reshuffle include/net/devlink.h
to have structs declarations written in different order. So a lot of
churn for something that needs to be fixed anyway (in my opinion).

Thanks