[PATCH net] devlink: Delay health recover notification until devlink registered

[PATCH net] devlink: Delay health recover notification until devlink registered

[Shay Drory]

Currently, invoking health recover before devlink_register() triggers
a WARN_ON. However, it is possible for a device to have health errors
during its probing flow, before the device driver will call to
devlink_register(). e.g.: it is valid to invoke health recover before
devlink_register().

Hence, apply delay notification mechanism to health reporters.

Fixes: cf530217408e ("devlink: Notify users when objects are accessible")
Signed-off-by: Shay Drory <shayd@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
 net/devlink/devl_internal.h | 21 +++++++++++++++++++++
 net/devlink/health.c        | 29 +++++++++--------------------
 net/devlink/leftover.c      |  5 +++++
 3 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index 62921b2eb0d3..9269dbe1b047 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -53,6 +53,25 @@ struct devlink {
 	char priv[] __aligned(NETDEV_ALIGN);
 };
 
+struct devlink_health_reporter {
+	struct list_head list;
+	void *priv;
+	const struct devlink_health_reporter_ops *ops;
+	struct devlink *devlink;
+	struct devlink_port *devlink_port;
+	struct devlink_fmsg *dump_fmsg;
+	struct mutex dump_lock; /* lock parallel read/write from dump buffers */
+	u64 graceful_period;
+	bool auto_recover;
+	bool auto_dump;
+	u8 health_state;
+	u64 dump_ts;
+	u64 dump_real_ts;
+	u64 error_count;
+	u64 recovery_count;
+	u64 last_recovery_ts;
+};
+
 extern struct xarray devlinks;
 extern struct genl_family devlink_nl_family;
 
@@ -168,6 +187,8 @@ extern const struct devlink_cmd devl_cmd_selftests_get;
 
 /* Notify */
 void devlink_notify(struct devlink *devlink, enum devlink_command cmd);
+void devlink_recover_notify_check(struct devlink_health_reporter *reporter,
+				  enum devlink_command cmd);
 
 /* Ports */
 int devlink_port_netdevice_event(struct notifier_block *nb,
diff --git a/net/devlink/health.c b/net/devlink/health.c
index 194340a8bb86..b1ceea733926 100644
--- a/net/devlink/health.c
+++ b/net/devlink/health.c
@@ -51,25 +51,6 @@ static void devlink_fmsg_free(struct devlink_fmsg *fmsg)
 	kfree(fmsg);
 }
 
-struct devlink_health_reporter {
-	struct list_head list;
-	void *priv;
-	const struct devlink_health_reporter_ops *ops;
-	struct devlink *devlink;
-	struct devlink_port *devlink_port;
-	struct devlink_fmsg *dump_fmsg;
-	struct mutex dump_lock; /* lock parallel read/write from dump buffers */
-	u64 graceful_period;
-	bool auto_recover;
-	bool auto_dump;
-	u8 health_state;
-	u64 dump_ts;
-	u64 dump_real_ts;
-	u64 error_count;
-	u64 recovery_count;
-	u64 last_recovery_ts;
-};
-
 void *
 devlink_health_reporter_priv(struct devlink_health_reporter *reporter)
 {
@@ -480,7 +461,8 @@ static void devlink_recover_notify(struct devlink_health_reporter *reporter,
 	int err;
 
 	WARN_ON(cmd != DEVLINK_CMD_HEALTH_REPORTER_RECOVER);
-	ASSERT_DEVLINK_REGISTERED(devlink);
+	if (!devl_is_registered(devlink))
+		return;
 
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!msg)
@@ -496,6 +478,13 @@ static void devlink_recover_notify(struct devlink_health_reporter *reporter,
 				0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
 }
 
+void devlink_recover_notify_check(struct devlink_health_reporter *reporter,
+				  enum devlink_command cmd)
+{
+	if (reporter->error_count)
+		devlink_recover_notify(reporter, cmd);
+}
+
 void
 devlink_health_reporter_recovery_done(struct devlink_health_reporter *reporter)
 {
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index 1f00f874471f..6d07fd55c75b 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -6659,6 +6659,7 @@ void devlink_notify_register(struct devlink *devlink)
 {
 	struct devlink_trap_policer_item *policer_item;
 	struct devlink_trap_group_item *group_item;
+	struct devlink_health_reporter *reporter;
 	struct devlink_param_item *param_item;
 	struct devlink_trap_item *trap_item;
 	struct devlink_port *devlink_port;
@@ -6695,6 +6696,10 @@ void devlink_notify_register(struct devlink *devlink)
 	xa_for_each(&devlink->params, param_id, param_item)
 		devlink_param_notify(devlink, 0, param_item,
 				     DEVLINK_CMD_PARAM_NEW);
+
+	list_for_each_entry(reporter, &devlink->reporter_list, list)
+		devlink_recover_notify_check(reporter,
+					     DEVLINK_CMD_HEALTH_REPORTER_RECOVER);
 }
 
 void devlink_notify_unregister(struct devlink *devlink)
-- 
2.38.1

Re: [PATCH net] devlink: Delay health recover notification until devlink registered

[Jakub Kicinski]

On Tue, 8 Aug 2023 16:37:20 +0300 Shay Drory wrote:
> Currently, invoking health recover before devlink_register() triggers
> a WARN_ON. However, it is possible for a device to have health errors
> during its probing flow, before the device driver will call to
> devlink_register(). e.g.: it is valid to invoke health recover before
> devlink_register().
> 
> Hence, apply delay notification mechanism to health reporters.

devlink_register() is dead, long live devl_register().

At the very least the commit message should enlighten us as to why 
the devlink instance can't be registered first, before the health
reporter.