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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CALHRZuqpaqvunTga+8OK4GSa3oRao-CBxit6UzRvN3a1-T0dhA@mail.gmail.com>
Date:   Thu, 28 Oct 2021 17:48:02 +0530
From:   sundeep subbaraya <sundeep.lkml@...il.com>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     Sunil Kovvuri Goutham <sgoutham@...vell.com>,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Hariprasad Kelam <hkelam@...vell.com>,
        Geethasowjanya Akula <gakula@...vell.com>,
        Subbaraya Sundeep Bhatta <sbhatta@...vell.com>,
        Rakesh Babu Saladi <rsaladi2@...vell.com>,
        Saeed Mahameed <saeed@...nel.org>,
        "anthony.l.nguyen@...el.com" <anthony.l.nguyen@...el.com>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Andrew Lunn <andrew@...n.ch>
Subject: 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 <idosch@...sch.org> wrote:
>
> On Wed, Oct 27, 2021 at 06:43:00PM +0000, Sunil Kovvuri Goutham wrote:
> >
> > > ________________________________
> > > From: Ido Schimmel <idosch@...sch.org>
> > > Sent: Wednesday, October 27, 2021 11:41 PM
> > > To: Jakub Kicinski <kuba@...nel.org>
> > > Cc: sundeep subbaraya <sundeep.lkml@...il.com>; David Miller <davem@...emloft.net>; netdev@...r.kernel.org <netdev@...r.kernel.org>; Hariprasad Kelam <hkelam@...vell.com>; Geethasowjanya Akula <gakula@...vell.com>; Sunil Kovvuri Goutham <sgoutham@...vell.com>; Subbaraya Sundeep Bhatta <sbhatta@...vell.com>; Rakesh Babu Saladi <rsaladi2@...vell.com>; Saeed Mahameed <saeed@...nel.org>; anthony.l.nguyen@...el.com <anthony.l.nguyen@...el.com>; Jesse Brandeburg <jesse.brandeburg@...el.com>; Andrew Lunn <andrew@...n.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:
> > > > On Wed, 27 Oct 2021 22:13:32 +0530 sundeep subbaraya wrote:
> > > > > > On Wed, 27 Oct 2021 16:01:14 +0530 Subbaraya Sundeep wrote:
> > > > > > > From: Rakesh Babu <rsaladi2@...vell.com>
> > > > > > >
> > > > > > > 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).
> > >
> > > > 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.
>
> > 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().
>
> > 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