RE: [PATCH v4 05/23] ice: Add devlink params support

Subject: RE: [PATCH v4 05/23] ice: Add devlink params support

quoted

From: Saleem, Shiraz <redacted>
Sent: Tuesday, April 13, 2021 8:11 PM

[..]

quoted

quoted

quoted

quoted

Parav is talking about generic ways to customize the aux devices
created and that would seem to serve the same function as this.

Is there an RFC or something posted for us to look at?

I do not have polished RFC content ready yet.
But coping the full config sequence snippet from the internal draft
(changed for ice
example) here as I like to discuss with you in this context.

Thanks Parav! Some comments below.

quoted

# (1) show auxiliary device types supported by a given devlink device.
# applies to pci pf,vf,sf. (in general at devlink instance).
$ devlink dev auxdev show pci/0000:06.00.0
pci/0000:06.00.0:
  current:
    roce eth
  new:
  supported:
    roce eth iwarp

# (2) enable iwarp and ethernet type of aux devices and disable roce.
$ devlink dev auxdev set pci/0000:06:00.0 roce off iwarp on

# (3) now see which aux devices will be enable on next reload.
$ devlink dev auxdev show pci/0000:06:00.0
pci/0000:06:00.0:
  current:
    roce eth
  new:
    eth iwarp
  supported:
    roce eth iwarp

# (4) now reload the device and see which aux devices are created.
At this point driver undergoes reconfig for removal of roce and
adding

iwarp.

quoted

$ devlink reload pci/0000:06:00.0

I see this is modeled like devlink resource.

Do we really to need a PCI driver re-init to switch the type of the
auxdev hanging off the PCI dev?

I don't see a need to re-init the whole PCI driver. Since only aux device config is
changed only that piece to get reloaded.

But that is what mlx5 and other implementations does on reload no? i.e. a PCI driver reinit.
I can see an ice implementation of reload morphing to similar over time to support a new config
that requires a true reinit of PCI driver entities.

quoted

Why not just allow the setting to apply dynamically during a 'set'
itself with an unplug/plug of the auxdev with correct type.

This suggestion came up in the internal discussion too.
However such task needs to synchronize with devlink reload command and also
with driver remove() sequence.
So locking wise and depending on amount of config change, it is close to what
reload will do.

Holding this mutex across the auxiliary device unplug/plug in "set" wont cut it?
https://elixir.bootlin.com/linux/v5.12-rc7/source/drivers/net/ethernet/mellanox/mlx5/core/main.c#L1304

For example other resource config or other params setting also to take effect.
So to avoid defining multiple config sequence, doing as part of already existing
devlink reload, it brings simple sequence to user.

For example,
1. enable/disable desired aux devices
2. configure device resources
3. set some device params
4. do devlink reload and apply settings done in #1 to #3

Sure. But a user might also just want to operate on just an auxiliary device configuration change. As in #1.
And he ends up having everything hanging off the PF to get blown out, including potentially
the VFs. That feels like too big a hammer.

Shiraz