diff mbox series

[net-next,v2,4/8] devlink: Support get and set state of port function

Message ID 20200917172020.26484-5-parav@nvidia.com
State Superseded
Headers show
Series devlink: Add SF add/delete devlink ops | expand

Commit Message

Parav Pandit Sept. 17, 2020, 5:20 p.m. UTC 
devlink port function can be in active or inactive state.
Allow users to get and set port function's state.

Example of a PCI SF port which supports a port function:
Create a device with ID=10 and one physical port.
$ echo "10 1" > /sys/bus/netdevsim/new_device
$ devlink port show
netdevsim/netdevsim10/0: type eth netdev eth0 flavour physical port 1 splittable false

$ devlink port add netdevsim/netdevsim10/10 flavour pcipf pfnum 0
$ devlink port add netdevsim/netdevsim10/11 flavour pcisf pfnum 0 sfnum 44
$ devlink port show netdevsim/netdevsim10/11
netdevsim/netdevsim10/11: type eth netdev eni10npf0sf44 flavour pcisf controller 0 pfnum 0 sfnum 44 external false splittable false
  function:
    hw_addr 00:00:00:00:00:00 state inactive

$ devlink port function set netdevsim/netdevsim10/11 hw_addr 00:11:22:33:44:55 state active

$ devlink port show netdevsim/netdevsim10/11 -jp
{
    "port": {
        "netdevsim/netdevsim10/11": {
            "type": "eth",
            "netdev": "eni10npf0sf44",
            "flavour": "pcisf",
            "controller": 0,
            "pfnum": 0,
            "sfnum": 44,
            "external": false,
            "splittable": false,
            "function": {
                "hw_addr": "00:11:22:33:44:55",
                "state": "active"
            }
        }
    }
}

Signed-off-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
 include/net/devlink.h        | 20 ++++++++++
 include/uapi/linux/devlink.h |  6 +++
 net/core/devlink.c           | 77 +++++++++++++++++++++++++++++++++++-
 3 files changed, 101 insertions(+), 2 deletions(-)

Comments

David Ahern Sept. 17, 2020, 8:23 p.m. UTC | #1 
On 9/17/20 11:20 AM, Parav Pandit wrote:
> diff --git a/net/core/devlink.c b/net/core/devlink.c

> index d152489e48da..c82098cb75da 100644

> --- a/net/core/devlink.c

> +++ b/net/core/devlink.c

> @@ -595,6 +598,40 @@ devlink_port_function_hw_addr_fill(struct devlink *devlink, const struct devlink

>  	return 0;

>  }

>  

> +static bool devlink_port_function_state_valid(u8 state)


you have a named enum so why not 'enum devlink_port_function_state state'?


> +{

> +	return state == DEVLINK_PORT_FUNCTION_STATE_INACTIVE ||

> +	       state == DEVLINK_PORT_FUNCTION_STATE_ACTIVE;

> +}

> +

> +static int devlink_port_function_state_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)

> +{

> +	enum devlink_port_function_state state;

> +	int err;

> +

> +	if (!ops->port_function_state_get)

> +		return 0;

> +

> +	err = ops->port_function_state_get(devlink, port, &state, extack);

> +	if (err) {

> +		if (err == -EOPNOTSUPP)

> +			return 0;

> +		return err;

> +	}

> +	if (!devlink_port_function_state_valid(state)) {

> +		WARN_ON(1);


WARN_ON_ONCE at most.

> +		NL_SET_ERR_MSG_MOD(extack, "Invalid state value read from driver");

> +		return -EINVAL;

> +	}

> +	err = nla_put_u8(msg, DEVLINK_PORT_FUNCTION_ATTR_STATE, state);

> +	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)
Parav Pandit Sept. 18, 2020, 3:30 a.m. UTC | #2 
> From: David Ahern <dsahern@gmail.com>

> Sent: Friday, September 18, 2020 1:54 AM

> 

> On 9/17/20 11:20 AM, Parav Pandit wrote:

> > diff --git a/net/core/devlink.c b/net/core/devlink.c index

> > d152489e48da..c82098cb75da 100644

> > --- a/net/core/devlink.c

> > +++ b/net/core/devlink.c

> > @@ -595,6 +598,40 @@ devlink_port_function_hw_addr_fill(struct devlink

> *devlink, const struct devlink

> >  	return 0;

> >  }

> >

> > +static bool devlink_port_function_state_valid(u8 state)

> 

> you have a named enum so why not 'enum devlink_port_function_state state'?

>

Right. I should. I missed it.
Will do.
 
> 

> > +{

> > +	return state == DEVLINK_PORT_FUNCTION_STATE_INACTIVE ||

> > +	       state == DEVLINK_PORT_FUNCTION_STATE_ACTIVE;

> > +}

> > +

> > +static int devlink_port_function_state_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) {

> > +	enum devlink_port_function_state state;

> > +	int err;

> > +

> > +	if (!ops->port_function_state_get)

> > +		return 0;

> > +

> > +	err = ops->port_function_state_get(devlink, port, &state, extack);

> > +	if (err) {

> > +		if (err == -EOPNOTSUPP)

> > +			return 0;

> > +		return err;

> > +	}

> > +	if (!devlink_port_function_state_valid(state)) {

> > +		WARN_ON(1);

> 

> WARN_ON_ONCE at most.

> 

Yep. I will change to WARN_ON_ONCE.
diff mbox series

Patch

diff --git a/include/net/devlink.h b/include/net/devlink.h
index ebab2c0360d0..500c22835686 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1200,6 +1200,26 @@  struct devlink_ops {
 	int (*port_function_hw_addr_set)(struct devlink *devlink, struct devlink_port *port,
 					 const u8 *hw_addr, int hw_addr_len,
 					 struct netlink_ext_ack *extack);
+	/**
+	 * @port_function_state_get: Port function's state get function.
+	 *
+	 * Should be used by device drivers to report the state of a function managed
+	 * by the devlink port. Driver should return -EOPNOTSUPP if it doesn't support port
+	 * function handling for a particular port.
+	 */
+	int (*port_function_state_get)(struct devlink *devlink, struct devlink_port *port,
+				       enum devlink_port_function_state *state,
+				       struct netlink_ext_ack *extack);
+	/**
+	 * @port_function_state_set: Port function's state set function.
+	 *
+	 * Should be used by device drivers to set the state of a function managed
+	 * by the devlink port. Driver should return -EOPNOTSUPP if it doesn't support port
+	 * function handling for a particular port.
+	 */
+	int (*port_function_state_set)(struct devlink *devlink, struct devlink_port *port,
+				       enum devlink_port_function_state state,
+				       struct netlink_ext_ack *extack);
 	/**
 	 * @port_new: Port add function.
 	 *
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 09c41b9ce407..8e513f1cd638 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -518,9 +518,15 @@  enum devlink_resource_unit {
 enum devlink_port_function_attr {
 	DEVLINK_PORT_FUNCTION_ATTR_UNSPEC,
 	DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR,	/* binary */
+	DEVLINK_PORT_FUNCTION_ATTR_STATE,	/* u8 */
 
 	__DEVLINK_PORT_FUNCTION_ATTR_MAX,
 	DEVLINK_PORT_FUNCTION_ATTR_MAX = __DEVLINK_PORT_FUNCTION_ATTR_MAX - 1
 };
 
+enum devlink_port_function_state {
+	DEVLINK_PORT_FUNCTION_STATE_INACTIVE,
+	DEVLINK_PORT_FUNCTION_STATE_ACTIVE,
+};
+
 #endif /* _UAPI_LINUX_DEVLINK_H_ */
diff --git a/net/core/devlink.c b/net/core/devlink.c
index d152489e48da..c82098cb75da 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -87,6 +87,9 @@  EXPORT_TRACEPOINT_SYMBOL_GPL(devlink_hwerr);
 
 static const struct nla_policy devlink_function_nl_policy[DEVLINK_PORT_FUNCTION_ATTR_MAX + 1] = {
 	[DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR] = { .type = NLA_BINARY },
+	[DEVLINK_PORT_FUNCTION_ATTR_STATE] =
+		NLA_POLICY_RANGE(NLA_U8, DEVLINK_PORT_FUNCTION_STATE_INACTIVE,
+				 DEVLINK_PORT_FUNCTION_STATE_ACTIVE),
 };
 
 static LIST_HEAD(devlink_list);
@@ -595,6 +598,40 @@  devlink_port_function_hw_addr_fill(struct devlink *devlink, const struct devlink
 	return 0;
 }
 
+static bool devlink_port_function_state_valid(u8 state)
+{
+	return state == DEVLINK_PORT_FUNCTION_STATE_INACTIVE ||
+	       state == DEVLINK_PORT_FUNCTION_STATE_ACTIVE;
+}
+
+static int devlink_port_function_state_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)
+{
+	enum devlink_port_function_state state;
+	int err;
+
+	if (!ops->port_function_state_get)
+		return 0;
+
+	err = ops->port_function_state_get(devlink, port, &state, extack);
+	if (err) {
+		if (err == -EOPNOTSUPP)
+			return 0;
+		return err;
+	}
+	if (!devlink_port_function_state_valid(state)) {
+		WARN_ON(1);
+		NL_SET_ERR_MSG_MOD(extack, "Invalid state value read from driver");
+		return -EINVAL;
+	}
+	err = nla_put_u8(msg, DEVLINK_PORT_FUNCTION_ATTR_STATE, state);
+	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)
@@ -611,6 +648,11 @@  devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct devlink_port *por
 
 	ops = devlink->ops;
 	err = devlink_port_function_hw_addr_fill(devlink, ops, port, msg, extack, &msg_updated);
+	if (err)
+		goto out;
+	err = devlink_port_function_state_fill(devlink, ops, port, msg, extack, &msg_updated);
+
+out:
 	if (err || !msg_updated)
 		nla_nest_cancel(msg, function_attr);
 	else
@@ -879,6 +921,28 @@  devlink_port_function_hw_addr_set(struct devlink *devlink, struct devlink_port *
 	return 0;
 }
 
+static int
+devlink_port_function_state_set(struct devlink *devlink, struct devlink_port *port,
+				const struct nlattr *attr, struct netlink_ext_ack *extack)
+{
+	enum devlink_port_function_state state;
+	const struct devlink_ops *ops;
+	int err;
+
+	state = nla_get_u8(attr);
+	ops = devlink->ops;
+	if (!ops->port_function_state_set) {
+		NL_SET_ERR_MSG_MOD(extack, "Port function does not support state setting");
+		return -EOPNOTSUPP;
+	}
+	err = ops->port_function_state_set(devlink, port, state, extack);
+	if (err)
+		return err;
+
+	devlink_port_notify(port, DEVLINK_CMD_PORT_NEW);
+	return 0;
+}
+
 static int
 devlink_port_function_set(struct devlink *devlink, struct devlink_port *port,
 			  const struct nlattr *attr, struct netlink_ext_ack *extack)
@@ -894,9 +958,18 @@  devlink_port_function_set(struct devlink *devlink, struct devlink_port *port,
 	}
 
 	attr = tb[DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR];
-	if (attr)
+	if (attr) {
 		err = devlink_port_function_hw_addr_set(devlink, port, attr, extack);
-
+		if (err)
+			return err;
+	}
+	/* Keep this as the last function attribute set, so that when
+	 * multiple port function attributes are set along with state,
+	 * Those can be applied first before activating the state.
+	 */
+	attr = tb[DEVLINK_PORT_FUNCTION_ATTR_STATE];
+	if (attr)
+		err = devlink_port_function_state_set(devlink, port, attr, extack);
 	return err;
 }