Re: [EXT] Re: [net-next PATCH 1/2] octeontx2-pf: Add devlink param to init and de-init serdes

Hi Ido,

On Thu, Oct 28, 2021 at 3:55 AM Ido Schimmel [off-list ref] wrote:

On Wed, Oct 27, 2021 at 06:43:00PM +0000, Sunil Kovvuri Goutham wrote:

quoted

quoted

________________________________
From: Ido Schimmel <redacted>
Sent: Wednesday, October 27, 2021 11:41 PM
To: Jakub Kicinski <kuba@kernel.org>
Cc: sundeep subbaraya <redacted>; David Miller <davem@davemloft.net>; netdev@vger.kernel.org <redacted>; Hariprasad Kelam <redacted>; Geethasowjanya Akula <gakula@marvell.com>; Sunil Kovvuri Goutham <sgoutham@marvell.com>; Subbaraya Sundeep Bhatta <sbhatta@marvell.com>; Rakesh Babu Saladi <redacted>; Saeed Mahameed <saeed@kernel.org>; anthony.l.nguyen@intel.com <anthony.l.nguyen@intel.com>; Jesse Brandeburg <redacted>; Andrew Lunn <andrew@lunn.ch>
Subject: Re: [EXT] Re: [net-next PATCH 1/2] octeontx2-pf: Add devlink param to init and de-init serdes

On Wed, Oct 27, 2021 at 10:08:57AM -0700, Jakub Kicinski wrote:

quoted

On Wed, 27 Oct 2021 22:13:32 +0530 sundeep subbaraya wrote:

quoted

quoted

On Wed, 27 Oct 2021 16:01:14 +0530 Subbaraya Sundeep wrote:

quoted

From: Rakesh Babu <redacted>

The physical/SerDes link of an netdev interface is not
toggled on interface bring up and bring down. This is
because the same link is shared between PFs and its VFs.
This patch adds devlink param to toggle physical link so
that it is useful in cases where a physical link needs to
be re-initialized.

So it's a reset? Or are there cases where user wants the link
to stay down?

There are cases where the user wants the link to stay down and debug.
We are adding this to help customers to debug issues wrt physical links.

Intel has a similar thing, they keep adding a ethtool priv flag called
"link-down-on-close" to all their drivers.

This is the list I compiled the previous time we discussed it:

https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_commit_-3Fid-3Dc3880bd159d431d06b687b0b5ab22e24e6ef0070&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=q3VKxXQKiboRw_F01ggTzHuhwawxR1P9_tMCN2FODU4&m=4xGR8HuIRKUriC93QV4GmQBJ6KVwRgGZ05Syzpq2CAM&s=fvl1aLwL55CWIsG2NT5i3QsP4o_GTEsGhA6Epjz7ZAk&e=
https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_commit_-3Fid-3Dd5ec9e2ce41ac198de2ee18e0e529b7ebbc67408&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=q3VKxXQKiboRw_F01ggTzHuhwawxR1P9_tMCN2FODU4&m=4xGR8HuIRKUriC93QV4GmQBJ6KVwRgGZ05Syzpq2CAM&s=kH50Qq3h75xREveyWvCUn35wXagtt4uv1QRK0wMBEdk&e=
https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_commit_-3Fid-3Dab4ab73fc1ec6dec548fa36c5e383ef5faa7b4c1&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=q3VKxXQKiboRw_F01ggTzHuhwawxR1P9_tMCN2FODU4&m=4xGR8HuIRKUriC93QV4GmQBJ6KVwRgGZ05Syzpq2CAM&s=Uc3yY-5HjS7TgRBl4DPLsJ19XiHDD_PvF8hA38K4XwI&e=

It seems that various drivers default to not shutting down the link upon
ndo_stop(), but some users want to override it. I hit that too as it
breaks ECMP (switch thinks the link is up).

quoted

Maybe others do this, too.  It's time we added a standard API for
this.

The new parameter sounds like a reset, but it can also be achieved by:

# ethtool --set-priv-flags eth0 link-down-on-close on
# ip link set dev eth0 down
# ip link set dev eth0 up

Where the first command is replaced by a more standard ethtool API.

The intention here is provide an option to the user to toggle the serdes configuration
as and when he wants to.

But why? What is the motivation? The commit message basically says that
you are adding a param to toggle the physical link because it is useful
to toggle the physical link.

quoted

There is no dependency with logical interface's status.

But there is and the commit message explains why you are not doing it as
part of ndo_{stop,open}(): "because the same link is shared between PFs
and its VFs"

Such constraints also apply to other drivers and you can see that in the
"link-down-on-close" private flag. I'm also aware of propriety tools to
toggle device bits which prevent the physical link from going down upon
ndo_stop().

quoted

Having a standard API to select bringing down physical interface upon logical interface's close call
is a good idea. But this patch is not for that.

IIUC, your default behavior is not to take the physical link down upon
ndo_stop() and now you want to toggle the link. If you have a standard
API to change the default behavior, then the commands I showed will
toggle the link, no?

Actually we also need a case where debugging is required when the
logical link is
up (so that packets flow from kernel to SerDes continuously) but the
physical link
is down. We will change the commit description since it is giving the
wrong impression.
A command to change physical link up/down with no relation to ifconfig
is needed.

Thanks,
Sundeep