Re: [patch net-next v3 01/11] devlink: remove devlink features

[Jiri Pirko <jiri@resnulli.us>]

Tue, Jan 10, 2023 at 01:55:00AM CET, kuba@kernel.org wrote:
>On Mon,  9 Jan 2023 19:31:10 +0100 Jiri Pirko wrote:
>> Note that mlx5 used this to enable devlink reload conditionally only
>> when device didn't act as multi port slave. Move the multi port check
>> into mlx5_devlink_reload_down() callback alongside with the other
>> checks preventing the device from reload in certain states.
>
>Right, but this is not 100% equivalent because we generate the
>notifications _before_ we try to reload_down:
>
>	devlink_ns_change_notify(devlink, dest_net, curr_net, false);
>	err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack);
>	if (err)
>		return err;
>
>IDK why, I haven't investigated.

Right, but that is done even in other cases where down can't be done. I
I think there's a bug here, down DEL notification is sent before calling
down which can potentially fail. I think the notification call should be
moved after reload_down() call. Then the bahaviour would stay the same
for the features case and will get fixed for the reload_down() reject
cases. What do you think?