On Thu, 24 Sep 2020 22:29:55 +0300 Moshe Shemesh wrote:
> >> @@ -3964,6 +3965,7 @@ static int mlx4_devlink_reload_down(struct devlink
> >> *devlink, bool netns_change,
> >> }
> >>
> >> static int mlx4_devlink_reload_up(struct devlink *devlink, enum
> >> devlink_reload_action action,
> >> + enum devlink_reload_action_limit_level
> >> limit_level,
> >> struct netlink_ext_ack *extack, unsigned
> >> long *actions_performed)
> >> {
> >> struct mlx4_priv *priv = devlink_priv(devlink);
> >> @@ -3985,6 +3987,7 @@ static int mlx4_devlink_reload_up(struct devlink
> >> *devlink, enum devlink_reload_a
> >> static const struct devlink_ops mlx4_devlink_ops = {
> >> .port_type_set = mlx4_devlink_port_type_set,
> >> .supported_reload_actions =
> >> BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT),
> >> + .supported_reload_action_limit_levels =
> >> BIT(DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE),
> > Please cut down the name lenghts, this is just lazy.
> >
> > 'supported_reload_limits' or 'cap_reload_limits' is perfectly
> > sufficient.
> >
> > 'reload_limits' would be even better. Cause what else would it be if
> > not a capability.
>
> Sounds good.
>
> So instead of supported_reload_actions_limit_levels will have reload_limits.
>
> Instead of supported_reload_actions will have reload_actions, OK ?
Sounds good.
> May also use reload_limit_level instead of reload_action_limit_level
> everywhere if its clear enough.
I think reload_limits is clear. I'd also cut down the length of the
defines / enum names.
> > Besides I don't think drivers should have to fill negative attributes
> > (that they don't support something). Everyone is always going to
> > support NONE, since it's "unspecified" / "pick your favorite", right?
>
> Good point, will remove it.
>
> >> .reload_down = mlx4_devlink_reload_down,
> >> .reload_up = mlx4_devlink_reload_up,
> >> };
> >> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> >> index fdba7ab58a79..0c5d942dcbd5 100644
> >> --- a/include/uapi/linux/devlink.h
> >> +++ b/include/uapi/linux/devlink.h
> >> @@ -289,6 +289,22 @@ enum devlink_reload_action {
> >> DEVLINK_RELOAD_ACTION_MAX = __DEVLINK_RELOAD_ACTION_MAX - 1
> >> };
> >>
> >> +/**
> >> + * enum devlink_reload_action_limit_level - Reload action limit level.
> >> + * @DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE: No constrains on action.
> >> Action may include
> >> + * reset or downtime as needed.
> >> + * @DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NO_RESET: No reset allowed, no down
> >> time allowed,
> >> + * no link flap and no
> >> configuration is lost.
> >> + */
> >> +enum devlink_reload_action_limit_level {
> > You reserved UNSPEC for actions but not for limit level?
>
>
> Yes, I used LIMIT_LEVEL_NONE = 0 as no limit needed, so I skipped UNSPEC.
>
> Maybe should add UNSPEC and use UNSPEC as no limit needed. But UNSPEC is
> kind of invalid.
Yeah, if we have UNSPEC then it should be invalid.
I'm mostly asking for consistency, either have UNSPEC for both actions
and limits or for neither.
> >> + DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE,
> >> + DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NO_RESET,
> >> +
> >> + /* Add new reload actions limit level above */
> >> + __DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX,
> >> + DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX =
> >> __DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX - 1
> >> +};
> >> +
> >> enum devlink_attr {
> >> /* don't change the order or add anything between, this is ABI! */
> >> DEVLINK_ATTR_UNSPEC,
> >> @@ -480,6 +496,7 @@ enum devlink_attr {
> >>
> >> DEVLINK_ATTR_RELOAD_ACTION, /* u8 */
> >> DEVLINK_ATTR_RELOAD_ACTIONS_PERFORMED, /* nested */
> >> + DEVLINK_ATTR_RELOAD_ACTION_LIMIT_LEVEL, /* u8 */
> >>
> >> /* add new attributes above here, update the policy in devlink.c */
> >>
> >> diff --git a/net/core/devlink.c b/net/core/devlink.c
> >> index 318ef29f81f2..fee6fcc7dead 100644
> >> --- a/net/core/devlink.c
> >> +++ b/net/core/devlink.c
> >> @@ -462,12 +462,45 @@ static int devlink_nl_put_handle(struct sk_buff
> >> *msg, struct devlink *devlink)
> >> return 0;
> >> }
> >>
> >> +struct devlink_reload_combination {
> >> + enum devlink_reload_action action;
> >> + enum devlink_reload_action_limit_level limit_level;
> >> +};
> >> +
> >> +static const struct devlink_reload_combination
> >> devlink_reload_invalid_combinations[] = {
> >> + {
> >> + /* can't reinitialize driver with no down time */
> >> + .action = DEVLINK_RELOAD_ACTION_DRIVER_REINIT,
> >> + .limit_level = DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NO_RESET,
> >> + },
> >> +};
> >> +
> >> +static bool
> >> +devlink_reload_combination_is_invalid(enum devlink_reload_action action,
> >> + enum devlink_reload_action_limit_level
> >> limit_level)
> >> +{
> >> + int i;
> >> +
> >> + for (i = 0 ; i < ARRAY_SIZE(devlink_reload_invalid_combinations) ;
> >> i++)
> > Whitespace. Did you checkpatch?
>
>
> Yes, checked it again now, it still pass. I think checkpatch doesn't see
> double space.
And the spaces before semicolons? It's sad if checkpatch misses such
basic stuff :(
> But anyway, I missed it, I will fix.
