[net-next,v2,3/8] devlink: Prepare code to fill multiple port function attributes

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(-)

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