Hi Jacob,
> From: Jacob Keller <jacob.e.kel...@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 <pa...@nvidia.com>
> > Reviewed-by: Jiri Pirko <j...@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);
> >
