Re: [PATCH v5 net-next 2/8] sfc: add devlink info support for ef100

Tue, Feb 07, 2023 at 03:42:45PM CET, alejandro.lucero-palau@amd.com wrote:

On 2/2/23 11:58, Jiri Pirko wrote:

quoted

Thu, Feb 02, 2023 at 12:14:17PM CET, alejandro.lucero-palau@amd.com wrote:

quoted

From: Alejandro Lucero <redacted>

Support for devlink info command.

You are quite brief for couple hundred line patch. Care to shed some
more details for the reader? Also, use imperative mood (applies to the
rest of the pathes)

[...]

OK. I'll be more talkative and imperative here.

quoted

quoted

+static int efx_devlink_info_get(struct devlink *devlink,
+				struct devlink_info_req *req,
+				struct netlink_ext_ack *extack)
+{
+	struct efx_devlink *devlink_private = devlink_priv(devlink);
+	struct efx_nic *efx = devlink_private->efx;
+	char msg[NETLINK_MAX_FMTMSG_LEN];
+	int errors_reported = 0;
+	int rc;
+
+	/* Several different MCDI commands are used. We report first error
+	 * through extack along with total number of errors. Specific error
+	 * information via system messages.
+	 */
+	rc = efx_devlink_info_board_cfg(efx, req);
+	if (rc) {
+		sprintf(msg, "Getting board info failed");
+		errors_reported++;
+	}
+	rc = efx_devlink_info_stored_versions(efx, req);
+	if (rc) {
+		if (!errors_reported)
+			sprintf(msg, "Getting stored versions failed");
+		errors_reported += rc;
+	}
+	rc = efx_devlink_info_running_versions(efx, req);
+	if (rc) {
+		if (!errors_reported)
+			sprintf(msg, "Getting board info failed");
+		errors_reported++;

Under which circumstances any of the errors above happen? Is it a common
thing? Or is it result of some fatal event?

They are not common at all. If any of those happen, it is a bad sign, 
and it is more than likely there are more than one because something is 
not working properly. That is the reason I only report first error found 
plus the total number of errors detected.

quoted

You treat it like it is quite common, which seems very odd to me.
If they are rare, just return error right away to the caller.

Well, that is done now. And as I say, I'm not reporting all but just the 
first one, mainly because the buffer limitation with NETLINK_MAX_FMTMSG_LEN.

If errors trigger, a more complete information will appear in system 
messages, so that is the reason with:

+               NL_SET_ERR_MSG_FMT(extack,
+                                  "%s. %d total errors. Check system messages",
+                                  msg, errors_reported);

I guess you are concerned with the extack report being overwhelmed, but 
I do not think that is the case.

No, I'm wondering why you just don't put error message into exack and
return -ESOMEERROR right away.

quoted

quoted

+	}
+
+	if (errors_reported)
+		NL_SET_ERR_MSG_FMT(extack,
+				   "%s. %d total errors. Check system messages",
+				   msg, errors_reported);
+	return 0;
+}
+
static const struct devlink_ops sfc_devlink_ops = {
+	.info_get			= efx_devlink_info_get,
};

[...]