diff mbox series

[net-next,RFC,v2,03/11] devlink: Add support for reload

Message ID 20171114161852.6633-4-jiri@resnulli.us
State RFC, archived
Delegated to: David Miller
Headers show
Series Add support for resource abstraction | expand

Commit Message

Jiri Pirko Nov. 14, 2017, 4:18 p.m. UTC 
From: Arkadi Sharshevsky <arkadis@mellanox.com>

Add support for performing driver hot reload.

Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/net/devlink.h        |  1 +
 include/uapi/linux/devlink.h |  5 ++++
 net/core/devlink.c           | 56 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+)

Comments

Jakub Kicinski Nov. 15, 2017, 8:03 a.m. UTC | #1 
On Tue, 14 Nov 2017 17:18:44 +0100, Jiri Pirko wrote:
> +static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct devlink *devlink = info->user_ptr[0];
> +	int err;
> +
> +	if (!devlink->ops->reload)
> +		return -EOPNOTSUPP;
> +
> +	err = devlink_resources_validate(devlink, NULL, info);
> +	if (err)
> +		return err;
> +
> +	mutex_unlock(&devlink->lock);
> +	err = devlink->ops->reload(devlink);
> +	mutex_lock(&devlink->lock);
> +
> +	return err;
> +}

I'm a bit confused with the locking, why is devlink->lock not held
around the validation?
Jiri Pirko Nov. 15, 2017, 8:14 a.m. UTC | #2 
Wed, Nov 15, 2017 at 09:03:59AM CET, jakub.kicinski@netronome.com wrote:
>On Tue, 14 Nov 2017 17:18:44 +0100, Jiri Pirko wrote:
>> +static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
>> +{
>> +	struct devlink *devlink = info->user_ptr[0];
>> +	int err;
>> +
>> +	if (!devlink->ops->reload)
>> +		return -EOPNOTSUPP;
>> +
>> +	err = devlink_resources_validate(devlink, NULL, info);
>> +	if (err)
>> +		return err;
>> +
>> +	mutex_unlock(&devlink->lock);
>> +	err = devlink->ops->reload(devlink);
>> +	mutex_lock(&devlink->lock);
>> +
>> +	return err;
>> +}
>
>I'm a bit confused with the locking, why is devlink->lock not held
>around the validation?

It is.
Arkadi Sharshevsky Nov. 15, 2017, 11:33 a.m. UTC | #3 
On 11/15/2017 10:03 AM, Jakub Kicinski wrote:
> On Tue, 14 Nov 2017 17:18:44 +0100, Jiri Pirko wrote:
>> +static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
>> +{
>> +	struct devlink *devlink = info->user_ptr[0];
>> +	int err;
>> +
>> +	if (!devlink->ops->reload)
>> +		return -EOPNOTSUPP;
>> +
>> +	err = devlink_resources_validate(devlink, NULL, info);
>> +	if (err)
>> +		return err;
>> +
>> +	mutex_unlock(&devlink->lock);
>> +	err = devlink->ops->reload(devlink);
>> +	mutex_lock(&devlink->lock);
>> +
>> +	return err;
>> +}
> 
> I'm a bit confused with the locking, why is devlink->lock not held
> around the validation?
> 

As Jiri mentioned it is held. The per devlink instance lock is taken
by default for each doit operation in the pre_doit(), because it operates
on a specific devlink instance.

The lock is released before performing the reload itself because during
the reload the driver register/unregisters devlink objects like sb/dpipe
/ports, which require the lock again, so this is done in order to avoid
recursive locking.
diff mbox series

Patch

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 960e80a..a33bda4 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -265,6 +265,7 @@  struct devlink_resource {
 #define DEVLINK_RESOURCE_ID_PARENT_TOP 0
 
 struct devlink_ops {
+	int (*reload)(struct devlink *devlink);
 	int (*port_type_set)(struct devlink_port *devlink_port,
 			     enum devlink_port_type port_type);
 	int (*port_split)(struct devlink *devlink, unsigned int port_index,
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 1ee1c72..ea4fa25 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -73,6 +73,11 @@  enum devlink_command {
 	DEVLINK_CMD_RESOURCE_SET,
 	DEVLINK_CMD_RESOURCE_DUMP,
 
+	/* Hot driver reload, used to query updated resources. The devlink
+	 * instance is not released during the process
+	 */
+	DEVLINK_CMD_RELOAD,
+
 	/* add new commands above here */
 	__DEVLINK_CMD_MAX,
 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 6ae644f..d93f176 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2468,6 +2468,55 @@  static int devlink_nl_cmd_resource_dump(struct sk_buff *skb,
 	return devlink_resource_fill(info, DEVLINK_CMD_RESOURCE_DUMP, 0);
 }
 
+static int
+devlink_resources_validate(struct devlink *devlink,
+			   struct devlink_resource *resource,
+			   struct genl_info *info)
+{
+	struct list_head *resource_list;
+	int err = 0;
+
+	if (resource)
+		resource_list = &resource->resource_list;
+	else
+		resource_list = &devlink->resource_list;
+
+	list_for_each_entry(resource, resource_list, list) {
+		if (resource->resource_ops &&
+		    resource->resource_ops->size_validate) {
+			err = resource->resource_ops->size_validate(devlink,
+								    resource->size,
+								    &resource->resource_list,
+								    info->extack);
+			if (err)
+				return err;
+		}
+		err = devlink_resources_validate(devlink, resource, info);
+		if (err)
+			return err;
+	}
+	return err;
+}
+
+static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
+{
+	struct devlink *devlink = info->user_ptr[0];
+	int err;
+
+	if (!devlink->ops->reload)
+		return -EOPNOTSUPP;
+
+	err = devlink_resources_validate(devlink, NULL, info);
+	if (err)
+		return err;
+
+	mutex_unlock(&devlink->lock);
+	err = devlink->ops->reload(devlink);
+	mutex_lock(&devlink->lock);
+
+	return err;
+}
+
 static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_BUS_NAME] = { .type = NLA_NUL_STRING },
 	[DEVLINK_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING },
@@ -2658,6 +2707,13 @@  static const struct genl_ops devlink_nl_ops[] = {
 		.flags = GENL_ADMIN_PERM,
 		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
 	},
+	{
+		.cmd = DEVLINK_CMD_RELOAD,
+		.doit = devlink_nl_cmd_reload,
+		.policy = devlink_nl_policy,
+		.flags = GENL_ADMIN_PERM,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
+	},
 };
 
 static struct genl_family devlink_nl_family __ro_after_init = {