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

[Aya Levin]

On 9/8/2020 5:04 PM, Jiri Pirko wrote:
> Sun, Sep 06, 2020 at 05:44:28PM CEST, idosch@xxxxxxxxxx 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.
Thanks for your input
I'm preparing V2
> [...]