Re: [PATCH net-next RFC v1 2/4] devlink: Add devlink traps under devlink_ports context

Sun, Sep 06, 2020 at 05:44:28PM CEST, idosch@idosch.org wrote:

On Wed, Sep 02, 2020 at 06:32:12PM +0300, Aya Levin wrote:

[...]

I understand how this struct allows you to re-use a lot of code between
per-device and per-port traps, but it's mainly enabled by the fact that
you use the same netlink commands for both per-device and per-port
traps. Is this OK?

I see this is already done for health reporters, but it's inconsistent
with the devlink-param API:

DEVLINK_CMD_PARAM_GET
DEVLINK_CMD_PARAM_SET
DEVLINK_CMD_PARAM_NEW
DEVLINK_CMD_PARAM_DEL

DEVLINK_CMD_PORT_PARAM_GET
DEVLINK_CMD_PORT_PARAM_SET
DEVLINK_CMD_PORT_PARAM_NEW
DEVLINK_CMD_PORT_PARAM_DEL

And also with the general device/port commands:

DEVLINK_CMD_GET
DEVLINK_CMD_SET
DEVLINK_CMD_NEW
DEVLINK_CMD_DEL

DEVLINK_CMD_PORT_GET
DEVLINK_CMD_PORT_SET
DEVLINK_CMD_PORT_NEW
DEVLINK_CMD_PORT_DEL

Wouldn't it be cleaner to add new commands?

DEVLINK_CMD_PORT_TRAP_GET
DEVLINK_CMD_PORT_TRAP_SET
DEVLINK_CMD_PORT_TRAP_NEW
DEVLINK_CMD_PORT_TRAP_DEL

I think the health API is the exception in this case and therefore might
not be the best thing to mimic. IIUC, existing per-port health reporters
were exposed as per-device and later moved to be exposed as per-port
[1]:

"This patchset comes to fix a design issue as some health reporters
report on errors and run recovery on device level while the actual
functionality is on port level. As for the current implemented devlink
health reporters it is relevant only to Tx and Rx reporters of mlx5,
which has only one port, so no real effect on functionality, but this
should be fixed before more drivers will use devlink health reporters."

Yeah, this slipped trough my fingers unfortunatelly :/ But with
introduction of per-port health reporters, we could introduce new
commands, that would be no problem. Pity :/

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ac4cd4781eacd1fd185c85522e869bd5d3254b96

Since we still don't have per-port traps, we can design it better from
the start.

I agree. Let's have a separate commands for per-port.


[...]