From: Jakub Kicinski <email@example.com> To: Jacob Keller <firstname.lastname@example.org> Cc: Jiri Pirko <email@example.com>, Moshe Shemesh <firstname.lastname@example.org>, email@example.com, firstname.lastname@example.org, "David S. Miller" <email@example.com>, Jiri Pirko <firstname.lastname@example.org>, Vasundhara Volam <email@example.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: <firstname.lastname@example.org> (raw) In-Reply-To: <email@example.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.
next prev parent 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 \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.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 \ firstname.lastname@example.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