LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Jacob Keller <jacob.e.keller@intel.com>
Cc: Jiri Pirko <jiri@resnulli.us>, Moshe Shemesh <moshe@mellanox.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	"David S. Miller" <davem@davemloft.net>,
	Jiri Pirko <jiri@mellanox.com>,
	Vasundhara Volam <vasundhara-v.volam@broadcom.com>
Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command
Date: Tue, 28 Jul 2020 13:06:53 -0700
Message-ID: <20200728130653.7ce2f013@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <d6fbfedd-9022-ff67-23ed-418607beecc2@intel.com>

On Tue, 28 Jul 2020 12:18:30 -0700 Jacob Keller wrote:
> On 7/28/2020 11:44 AM, Jakub Kicinski wrote:
> > From user perspective what's important is what the reset achieves (and
> > perhaps how destructive it is). We can define the reset levels as:
> > 
> > $ devlink dev reload pci/0000:82:00.0 net-ns-respawn
> > $ devlink dev reload pci/0000:82:00.0 driver-param-init
> > $ devlink dev reload pci/0000:82:00.0 fw-activate
> > 
> > combining should be possible when user wants multiple things to happen:
> > 
> > $ devlink dev reload pci/0000:82:00.0 fw-activate driver-param-init
> 
> Where today "driver-param-init" is the default behavior. But didn't we
> just say that mlxsw also does the equivalent of fw-activate?

Actually the default should probably be the combination of
driver-param-init and net-ns-respawn.

My expectations would be that the driver must perform the lowest reset 
level possible that satisfies the requested functional change. 
IOW driver may do more, in fact it should be acceptable for the driver
to always for a full HW reset (unless --live or other constraint is
specified).

> > We can also add the "reset level" specifier - for the cases where
> > device is misbehaving:
> > 
> > $ devlink dev reload pci/0000:82:00.0 level [driver|fw|hardware]
> 
> I guess I don't quite see how level fits in? This is orthogonal to the
> other settings?

Yup, it is, it's already orthogonal to what reload does today, hence the
need for the "driver default" hack.

> > But I don't think that we can go from the current reload command
> > cleanly to just a level reset. The driver-specific default is a bad
> > smell which indicates we're changing semantics from what user wants 
> > to what the reset depth is. Our semantics with the patch as it stands
> > are in fact:
> >  - if you want to load new params or change netns, don't pass the level
> >    - the "driver default" workaround dictates the right reset level for
> >    param init;
> >  - if you want to activate new firmware - select the reset level you'd
> >    like from the reset level options.
> >   
> 
> I think I agree, having the "what gets reloaded" as a separate vector
> makes sense and avoids confusion. For example for ice hardware,
> "fw-activate" really does mean "Do an EMP reset" rather than just a
> "device reset" which could be interpreted differently. ice can do
> function level reset, or core device reset also. Neither of those two
> resets activates firmware.
> 
> Additionally the current function load process in ice doesn't support
> driver-init at all. That's something I'd like to see happen but is quite
> a significant refactor for how the driver loads. We need to refactor
> everything out so that devlink is created early on and factor out
> load/unload into handlers that can be called by the devlink reload. As I
> have primarily been focused on flash update I sort of left that for the
> future because it was a huge task to solve.

Cool! That was what I was concerned about, but I didn't know any
existing driver already has the problem. "FW reset" is not nearly
a clear enough operation. We'd end up with drivers differing and 
users having to refer to vendor documentation to find out which 
"reset level" maps to what.

I think the components in ethtool-reset try to address the same
problem, and they have the notion of per-port, and per-device.
In the modern world we lack the per-host notion, but that's still 
strictly clearer than the limited API proposed here.

  reply index

Thread overview: 58+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-27 11:02 [PATCH net-next RFC 00/13] Add devlink reload level option Moshe Shemesh
2020-07-27 11:02 ` [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command Moshe Shemesh
2020-07-28  0:58   ` Jakub Kicinski
2020-07-28 13:58     ` Jiri Pirko
2020-07-28 16:47       ` Jacob Keller
2020-07-28 18:44         ` Jakub Kicinski
2020-07-28 19:18           ` Jacob Keller
2020-07-28 20:06             ` Jakub Kicinski [this message]
2020-07-29 14:54               ` Moshe Shemesh
2020-07-29 21:07                 ` Jakub Kicinski
2020-07-30 12:30                   ` Moshe Shemesh
2020-07-30 23:11                     ` Jakub Kicinski
2020-08-01 21:32                       ` Moshe Shemesh
2020-08-03 14:14                         ` Jiri Pirko
2020-08-03 20:57                           ` Jakub Kicinski
2020-08-04 10:04                             ` Jiri Pirko
2020-08-04 20:39                               ` Jakub Kicinski
2020-08-05 11:02                                 ` Jiri Pirko
2020-08-06 18:25                                   ` Jakub Kicinski
2020-08-06 22:56                                     ` Jacob Keller
2020-08-09 13:21                                     ` Moshe Shemesh
2020-08-10 16:53                                       ` Jakub Kicinski
2020-08-10 17:09                                         ` Jacob Keller
2020-08-10 18:17                                           ` Jakub Kicinski
2020-08-11  5:46                                         ` Jiri Pirko
2020-07-27 11:02 ` [PATCH net-next RFC 02/13] devlink: Add reload levels data to dev get Moshe Shemesh
2020-07-28  0:58   ` Jakub Kicinski
2020-07-29 14:37     ` Moshe Shemesh
2020-07-29 21:11       ` Jakub Kicinski
2020-07-30 12:05         ` Moshe Shemesh
2020-07-27 11:02 ` [PATCH net-next RFC 03/13] net/mlx5: Add functions to set/query MFRL register Moshe Shemesh
2020-07-27 11:02 ` [PATCH net-next RFC 04/13] net/mlx5: Set cap for pci sync for fw update event Moshe Shemesh
2020-07-27 11:02 ` [PATCH net-next RFC 05/13] net/mlx5: Handle sync reset request event Moshe Shemesh
2020-07-27 11:02 ` [PATCH net-next RFC 06/13] net/mlx5: Handle sync reset now event Moshe Shemesh
2020-07-27 11:02 ` [PATCH net-next RFC 07/13] net/mlx5: Handle sync reset abort event Moshe Shemesh
2020-07-27 11:02 ` [PATCH net-next RFC 08/13] net/mlx5: Add support for devlink reload level fw reset Moshe Shemesh
2020-07-27 11:02 ` [PATCH net-next RFC 09/13] devlink: Add enable_remote_dev_reset generic parameter Moshe Shemesh
2020-07-28  0:59   ` Jakub Kicinski
2020-07-29 14:42     ` Moshe Shemesh
2020-07-29 20:57       ` Jakub Kicinski
2020-07-30 12:08         ` Moshe Shemesh
2020-07-27 11:02 ` [PATCH net-next RFC 10/13] net/mlx5: Add devlink param enable_remote_dev_reset support Moshe Shemesh
2020-07-28  0:59   ` Jakub Kicinski
2020-07-27 11:02 ` [PATCH net-next RFC 11/13] net/mlx5: Add support for fw live patch event Moshe Shemesh
2020-07-27 11:02 ` [PATCH net-next RFC 12/13] net/mlx5: Add support for devlink reload level live patch Moshe Shemesh
2020-07-27 11:02 ` [PATCH net-next RFC 13/13] devlink: Add Documentation/networking/devlink/devlink-reload.rst Moshe Shemesh
2020-07-28  5:25 ` [PATCH net-next RFC 00/13] Add devlink reload level option Vasundhara Volam
2020-07-28 16:43   ` Jacob Keller
2020-08-03 10:24     ` Vasundhara Volam
2020-08-03 12:17       ` Moshe Shemesh
2020-08-03 12:47         ` Vasundhara Volam
2020-08-03 13:52           ` Moshe Shemesh
2020-08-04 10:13             ` Vasundhara Volam
2020-08-05  6:32               ` Moshe Shemesh
2020-08-05  6:55                 ` Vasundhara Volam
2020-08-05  8:20                   ` Moshe Shemesh
2020-08-12  9:34                     ` Vasundhara Volam
2020-07-28 16:37 ` Jacob Keller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200728130653.7ce2f013@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com \
    --to=kuba@kernel.org \
    --cc=davem@davemloft.net \
    --cc=jacob.e.keller@intel.com \
    --cc=jiri@mellanox.com \
    --cc=jiri@resnulli.us \
    --cc=linux-kernel@vger.kernel.org \
    --cc=moshe@mellanox.com \
    --cc=netdev@vger.kernel.org \
    --cc=vasundhara-v.volam@broadcom.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git