Message ID | 20200917172020.26484-4-parav@nvidia.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | devlink: Add SF add/delete devlink ops | expand |
Hi Jacob, > From: Jacob Keller <jacob.e.keller@intel.com> > Sent: Friday, September 18, 2020 12:29 AM > > > On 9/17/2020 10:20 AM, Parav Pandit wrote: > > Prepare code to fill zero or more port function optional attributes. > > Subsequent patch makes use of this to fill more port function > > attributes. > > > > Signed-off-by: Parav Pandit <parav@nvidia.com> > > Reviewed-by: Jiri Pirko <jiri@nvidia.com> > > --- > > net/core/devlink.c | 53 > > +++++++++++++++++++++++++--------------------- > > 1 file changed, 29 insertions(+), 24 deletions(-) > > > > diff --git a/net/core/devlink.c b/net/core/devlink.c index > > e93730065c57..d152489e48da 100644 > > --- a/net/core/devlink.c > > +++ b/net/core/devlink.c > > @@ -570,6 +570,31 @@ static int devlink_nl_port_attrs_put(struct sk_buff > *msg, > > return 0; > > } > > > > +static int > > +devlink_port_function_hw_addr_fill(struct devlink *devlink, const struct > devlink_ops *ops, > > + struct devlink_port *port, struct sk_buff > *msg, > > + struct netlink_ext_ack *extack, bool > *msg_updated) { > > + u8 hw_addr[MAX_ADDR_LEN]; > > + int hw_addr_len; > > + int err; > > + > > + if (!ops->port_function_hw_addr_get) > > + return 0; > > + > > + err = ops->port_function_hw_addr_get(devlink, port, hw_addr, > &hw_addr_len, extack); > > + if (err) { > > + if (err == -EOPNOTSUPP) > > + return 0; > > + return err; > > + } > > + err = nla_put(msg, DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR, > hw_addr_len, hw_addr); > > + if (err) > > + return err; > > + *msg_updated = true; > > + return 0; > > +} > > + > > static int > > devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct devlink_port > *port, > > struct netlink_ext_ack *extack) @@ -577,36 > +602,16 @@ > > devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct devlink_port > *por > > struct devlink *devlink = port->devlink; > > const struct devlink_ops *ops; > > struct nlattr *function_attr; > > - bool empty_nest = true; > > - int err = 0; > > + bool msg_updated = false; > > + int err; > > > > function_attr = nla_nest_start_noflag(msg, > DEVLINK_ATTR_PORT_FUNCTION); > > if (!function_attr) > > return -EMSGSIZE; > > > > ops = devlink->ops; > > - if (ops->port_function_hw_addr_get) { > > - int hw_addr_len; > > - u8 hw_addr[MAX_ADDR_LEN]; > > - > > - err = ops->port_function_hw_addr_get(devlink, port, hw_addr, > &hw_addr_len, extack); > > - if (err == -EOPNOTSUPP) { > > - /* Port function attributes are optional for a port. If > port doesn't > > - * support function attribute, returning -EOPNOTSUPP is > not an error. > > - */ > > We lost this comment in the move it looks like. I think it's still useful to keep for > clarity of why we're converting -EOPNOTSUPP in the return. You are right. It is a useful comment. However, it is already covered in include/net/devlink.h in the standard comment of the callback function. For new driver implementation, looking there will be more useful. So I guess its ok to drop from here. > > > - err = 0; > > - goto out; > > - } else if (err) { > > - goto out; > > - } > > - err = nla_put(msg, > DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR, hw_addr_len, hw_addr); > > - if (err) > > - goto out; > > - empty_nest = false; > > - } > > - > > -out: > > - if (err || empty_nest) > > + err = devlink_port_function_hw_addr_fill(devlink, ops, port, msg, > extack, &msg_updated); > > + if (err || !msg_updated) > > nla_nest_cancel(msg, function_attr); > > else > > nla_nest_end(msg, function_attr); > >
On 9/17/2020 8:35 PM, Parav Pandit wrote: > Hi Jacob, > >> From: Jacob Keller <jacob.e.keller@intel.com> >> Sent: Friday, September 18, 2020 12:29 AM >> >> >> We lost this comment in the move it looks like. I think it's still useful to keep for >> clarity of why we're converting -EOPNOTSUPP in the return. > You are right. It is a useful comment. > However, it is already covered in include/net/devlink.h in the standard comment of the callback function. > For new driver implementation, looking there will be more useful. > So I guess its ok to drop from here. > Yea if it's still in the header I don't think it's too important to keep here. Thanks! -Jake
> From: Jacob Keller <jacob.e.keller@intel.com> > Sent: Saturday, September 19, 2020 4:24 AM > > > On 9/17/2020 8:35 PM, Parav Pandit wrote: > > Hi Jacob, > > > >> From: Jacob Keller <jacob.e.keller@intel.com> > >> Sent: Friday, September 18, 2020 12:29 AM > >> > >> > >> We lost this comment in the move it looks like. I think it's still > >> useful to keep for clarity of why we're converting -EOPNOTSUPP in the > return. > > You are right. It is a useful comment. > > However, it is already covered in include/net/devlink.h in the standard > comment of the callback function. > > For new driver implementation, looking there will be more useful. > > So I guess its ok to drop from here. > > > > Yea if it's still in the header I don't think it's too important to keep here. > Alright. Will keep in the header. Thanks. > Thanks! > -Jake
diff --git a/net/core/devlink.c b/net/core/devlink.c index e93730065c57..d152489e48da 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -570,6 +570,31 @@ static int devlink_nl_port_attrs_put(struct sk_buff *msg, return 0; } +static int +devlink_port_function_hw_addr_fill(struct devlink *devlink, const struct devlink_ops *ops, + struct devlink_port *port, struct sk_buff *msg, + struct netlink_ext_ack *extack, bool *msg_updated) +{ + u8 hw_addr[MAX_ADDR_LEN]; + int hw_addr_len; + int err; + + if (!ops->port_function_hw_addr_get) + return 0; + + err = ops->port_function_hw_addr_get(devlink, port, hw_addr, &hw_addr_len, extack); + if (err) { + if (err == -EOPNOTSUPP) + return 0; + return err; + } + err = nla_put(msg, DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR, hw_addr_len, hw_addr); + if (err) + return err; + *msg_updated = true; + return 0; +} + static int devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct devlink_port *port, struct netlink_ext_ack *extack) @@ -577,36 +602,16 @@ devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct devlink_port *por struct devlink *devlink = port->devlink; const struct devlink_ops *ops; struct nlattr *function_attr; - bool empty_nest = true; - int err = 0; + bool msg_updated = false; + int err; function_attr = nla_nest_start_noflag(msg, DEVLINK_ATTR_PORT_FUNCTION); if (!function_attr) return -EMSGSIZE; ops = devlink->ops; - if (ops->port_function_hw_addr_get) { - int hw_addr_len; - u8 hw_addr[MAX_ADDR_LEN]; - - err = ops->port_function_hw_addr_get(devlink, port, hw_addr, &hw_addr_len, extack); - if (err == -EOPNOTSUPP) { - /* Port function attributes are optional for a port. If port doesn't - * support function attribute, returning -EOPNOTSUPP is not an error. - */ - err = 0; - goto out; - } else if (err) { - goto out; - } - err = nla_put(msg, DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR, hw_addr_len, hw_addr); - if (err) - goto out; - empty_nest = false; - } - -out: - if (err || empty_nest) + err = devlink_port_function_hw_addr_fill(devlink, ops, port, msg, extack, &msg_updated); + if (err || !msg_updated) nla_nest_cancel(msg, function_attr); else nla_nest_end(msg, function_attr);