netdev - RE: [net-next 3/4] i40e: Add support for a new feature: Total Port Shutdown

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <61CC2BC414934749BD9F5BF3D5D9404498670105@ORSMSX112.amr.corp.intel.com>
Date:   Tue, 21 Apr 2020 22:36:21 +0000
From:   "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "nhorman@...hat.com" <nhorman@...hat.com>,
        "sassmann@...hat.com" <sassmann@...hat.com>,
        "Kwapulinski, Piotr" <piotr.kwapulinski@...el.com>,
        "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>,
        "Bowers, AndrewX" <andrewx.bowers@...el.com>
Subject: RE: [net-next 3/4] i40e: Add support for a new feature: Total Port
 Shutdown

> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Tuesday, April 21, 2020 10:56
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@...el.com>
> Cc: davem@...emloft.net; Kubalewski, Arkadiusz
> <arkadiusz.kubalewski@...el.com>; netdev@...r.kernel.org;
> nhorman@...hat.com; sassmann@...hat.com; Kwapulinski, Piotr
> <piotr.kwapulinski@...el.com>; Loktionov, Aleksandr
> <aleksandr.loktionov@...el.com>; Bowers, AndrewX
> <andrewx.bowers@...el.com>
> Subject: Re: [net-next 3/4] i40e: Add support for a new feature: Total Port
> Shutdown
> 
> On Mon, 20 Apr 2020 18:49:31 -0700 Jeff Kirsher wrote:
> > From: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
> >
> > Currently after requesting to down a link on a physical network port,
> > the traffic is no longer being processed but the physical link with a
> > link partner is still established.
> >
> > Total Port Shutdown allows to completely shutdown the port on the
> > link-down procedure by physically removing the link from the port.
> >
> > Introduced changes:
> > - probe NVM if the feature was enabled at initialization of the port
> > - special handling on link-down procedure to let FW physically
> > shutdown the port if the feature was enabled
> 
> How is this different than link-down-on-close?
[Kirsher, Jeffrey T] 

First of all total-port-shutdown is a read only flag, the user cannot set it
from the OS.  It is possible to set it in bios, but only if the motherboard
supports it and the NIC has that capability.  Also, the behavior on both
slightly differs, link-down-on-close brings the link down by sending
(to firmware) phy_type=0, while total-port-shutdown does not, the
phy_type is not changed, instead firmware is using
I40E_AQ_PHY_ENABLE_LINK flag. 

> 
> Perhaps it'd be good to start documenting the private flags in Documentation/
[Kirsher, Jeffrey T] 

We could look at adding that information into our kernel documentation, I am
planning on updating the driver documentation in a follow-up patch set.  Would
a descriptive code comment help in this case? 

> 
> > Testing Hints (required if no HSD):
> > Link up/down, link-down-on-close
> >
> > Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
> > Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@...el.com>
> > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
> > Tested-by: Andrew Bowers <andrewx.bowers@...el.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
> 
> > @@ -12012,6 +12085,16 @@ static int i40e_sw_init(struct i40e_pf *pf)
> >
> >  	pf->tx_timeout_recovery_level = 1;
> >
> > +	if (pf->hw.mac.type != I40E_MAC_X722 &&
> > +	    i40e_is_total_port_shutdown_enabled(pf)) {
> > +		/* Link down on close must be on when total port shutdown
> > +		 * is enabled for a given port
> > +		 */
> > +		pf->flags |= (I40E_FLAG_TOTAL_PORT_SHUTDOWN
> > +			  | I40E_FLAG_LINK_DOWN_ON_CLOSE_ENABLED);
> 
> FWIW this is the correct code style in the kernel:
> 
> flags = BLA |
>         BLA2;
[Kirsher, Jeffrey T] 

This will get fixed in v2.

> 
> > +		dev_info(&pf->pdev->dev,
> > +			 "Total Port Shutdown is enabled, link-down-on-close
> forced on\n");
> > +	}
> >  	mutex_init(&pf->switch_mutex);
> >
> >  sw_init_done: