netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V2 net-next 0/6] mlxfw: Improve error reporting
@ 2019-11-22 22:41 Saeed Mahameed
  2019-11-22 22:41 ` [PATCH V2 net-next 1/6] netlink: Convert extack msg to a formattable buffer Saeed Mahameed
                   ` (6 more replies)
  0 siblings, 7 replies; 14+ messages in thread
From: Saeed Mahameed @ 2019-11-22 22:41 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev, Jiri Pirko, Saeed Mahameed

Hi Dave,

This patchset improves mlxfw error reporting of mlxfw to netlink and
kernel log.

1) patch #1, Convert extack msg to a formattable buffer

2) patch #2, Make mlxfw/mlxsw fw flash devlink status notify generic,
   and enable it for mlx5.

3) rest of the patches are improving mlxfw flash error messages by
reporting detailed mlxfw FSM error messages to netlink and more detailed
kernel log.

v2:
- Avoid long switch case and replace it with one call to NL_SET_ERR_MSG_MOD
  to format the message directly from the mlxfw_fsm_state_err_str table.

merge conflict with net:
@ drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c

++<<<<<<< (net-next) (keep)
+
+       if (fsm_state_err != MLXFW_FSM_STATE_ERR_OK)
+               return mlxfw_fsm_state_err(mlxfw_dev, extack, fsm_state_err);
+
++=======  
+ 
+       if (fsm_state_err != MLXFW_FSM_STATE_ERR_OK) {
+               fsm_state_err = min_t(enum mlxfw_fsm_state_err,
+                                     fsm_state_err, MLXFW_FSM_STATE_ERR_MAX);
+               pr_err("Firmware flash failed: %s\n",
+                      mlxfw_fsm_state_err_str[fsm_state_err]);
+               NL_SET_ERR_MSG_MOD(extack, "Firmware flash failed");
+               return -EINVAL;
++>>>>>>> (net) (delete) 

To resolve just use the 1st hunk, from net-next.

Thanks,
Saeed.

---

Saeed Mahameed (6):
  netlink: Convert extack msg to a formattable buffer
  net/mlxfw: Generic mlx FW flash status notify
  net/mlxfw: Improve FSM err message reporting and return codes
  net/mlxfw: Convert pr_* to dev_* in mlxfw_fsm.c
  net/mlxfw: More error messages coverage
  net/mlxfw: Macro for error reporting

 drivers/net/ethernet/mellanox/mlx5/core/fw.c  |   1 +
 drivers/net/ethernet/mellanox/mlxfw/Kconfig   |   1 +
 drivers/net/ethernet/mellanox/mlxfw/mlxfw.h   |  36 +++--
 .../net/ethernet/mellanox/mlxfw/mlxfw_fsm.c   | 149 ++++++++++++------
 .../net/ethernet/mellanox/mlxsw/spectrum.c    |  17 +-
 include/linux/netlink.h                       |  27 ++--
 lib/nlattr.c                                  |   2 +-
 7 files changed, 145 insertions(+), 88 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH V2 net-next 1/6] netlink: Convert extack msg to a formattable buffer
  2019-11-22 22:41 [PATCH V2 net-next 0/6] mlxfw: Improve error reporting Saeed Mahameed
@ 2019-11-22 22:41 ` Saeed Mahameed
  2019-11-24  0:56   ` Jakub Kicinski
  2019-11-22 22:41 ` [PATCH V2 net-next 2/6] net/mlxfw: Generic mlx FW flash status notify Saeed Mahameed
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Saeed Mahameed @ 2019-11-22 22:41 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev, Jiri Pirko, Saeed Mahameed

Before this patch the extack msg had to be a const char and didn't leave
much room for developers to report formattable and flexible messages.

All current usages are oneliner messages, hence, a buffer of size 128B
should be sufficient to replace the const char pointer.

This will allow future usages to provide formattable messages and more
flexible error reporting, without any impact on current users.

Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
 include/linux/netlink.h | 27 ++++++++++++---------------
 lib/nlattr.c            |  2 +-
 2 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 205fa7b1f07a..de9004da0288 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -62,6 +62,7 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
 
 /* this can be increased when necessary - don't expose to userland */
 #define NETLINK_MAX_COOKIE_LEN	20
+#define NL_EXTACK_MAX_MSG_SZ	128
 
 /**
  * struct netlink_ext_ack - netlink extended ACK report struct
@@ -72,40 +73,36 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
  * @cookie_len: actual cookie data length
  */
 struct netlink_ext_ack {
-	const char *_msg;
+	char _msg[NL_EXTACK_MAX_MSG_SZ];
 	const struct nlattr *bad_attr;
 	u8 cookie[NETLINK_MAX_COOKIE_LEN];
 	u8 cookie_len;
 };
 
-/* Always use this macro, this allows later putting the
- * message into a separate section or such for things
- * like translation or listing all possible messages.
- * Currently string formatting is not supported (due
- * to the lack of an output buffer.)
- */
-#define NL_SET_ERR_MSG(extack, msg) do {		\
-	static const char __msg[] = msg;		\
+#define NL_MSG_FMT(extack, fmt, ...) \
+	WARN_ON(snprintf(extack->_msg, NL_EXTACK_MAX_MSG_SZ, fmt, ## __VA_ARGS__) \
+		>= NL_EXTACK_MAX_MSG_SZ)
+
+#define NL_SET_ERR_MSG(extack, fmt, ...) do {		\
 	struct netlink_ext_ack *__extack = (extack);	\
 							\
 	if (__extack)					\
-		__extack->_msg = __msg;			\
+		NL_MSG_FMT(__extack, fmt, ## __VA_ARGS__); \
 } while (0)
 
-#define NL_SET_ERR_MSG_MOD(extack, msg)			\
-	NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg)
+#define NL_SET_ERR_MSG_MOD(extack, fmt, ...)			\
+	NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " fmt, ## __VA_ARGS__)
 
 #define NL_SET_BAD_ATTR(extack, attr) do {		\
 	if ((extack))					\
 		(extack)->bad_attr = (attr);		\
 } while (0)
 
-#define NL_SET_ERR_MSG_ATTR(extack, attr, msg) do {	\
-	static const char __msg[] = msg;		\
+#define NL_SET_ERR_MSG_ATTR(extack, attr, fmt, ...) do {	\
 	struct netlink_ext_ack *__extack = (extack);	\
 							\
 	if (__extack) {					\
-		__extack->_msg = __msg;			\
+		NL_MSG_FMT(__extack, fmt, ## __VA_ARGS__); \
 		__extack->bad_attr = (attr);		\
 	}						\
 } while (0)
diff --git a/lib/nlattr.c b/lib/nlattr.c
index cace9b307781..2ce1d6b68ce8 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -208,7 +208,7 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 	case NLA_REJECT:
 		if (extack && pt->validation_data) {
 			NL_SET_BAD_ATTR(extack, nla);
-			extack->_msg = pt->validation_data;
+			NL_MSG_FMT(extack, pt->validation_data);
 			return -EINVAL;
 		}
 		err = -EINVAL;
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH V2 net-next 2/6] net/mlxfw: Generic mlx FW flash status notify
  2019-11-22 22:41 [PATCH V2 net-next 0/6] mlxfw: Improve error reporting Saeed Mahameed
  2019-11-22 22:41 ` [PATCH V2 net-next 1/6] netlink: Convert extack msg to a formattable buffer Saeed Mahameed
@ 2019-11-22 22:41 ` Saeed Mahameed
  2019-11-22 22:41 ` [PATCH V2 net-next 3/6] net/mlxfw: Improve FSM err message reporting and return codes Saeed Mahameed
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Saeed Mahameed @ 2019-11-22 22:41 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev, Jiri Pirko, Saeed Mahameed, Ido Schimmel

FW flash status notify is currently implemented via a callback to the
caller mlx module, and all it is doing is to call
devlink_flash_update_status_notify with the specific module devlink
instance.

Instead of repeating the whole process for all mlx modules and
re-implement the status_notify callback again and again. Just provide the
devlink instance as part of mlxfw_dev when calling mlxfw_firmware_flash
and let mlxfw do the devlink status updates directly.

This will be very useful for adding status notify support to mlx5, as
already done in this patch, with a simple one line of just providing the
devlink instance to mlxfw_firmware_flash.

mlxfw now depends on NET_DEVLINK as all other mlx modules.

Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/fw.c  |  1 +
 drivers/net/ethernet/mellanox/mlxfw/Kconfig   |  1 +
 drivers/net/ethernet/mellanox/mlxfw/mlxfw.h   |  6 ++----
 .../net/ethernet/mellanox/mlxfw/mlxfw_fsm.c   | 21 ++++++++++---------
 .../net/ethernet/mellanox/mlxsw/spectrum.c    | 17 +--------------
 5 files changed, 16 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fw.c b/drivers/net/ethernet/mellanox/mlx5/core/fw.c
index a19790dee7b2..9c8956c51169 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fw.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fw.c
@@ -624,6 +624,7 @@ int mlx5_firmware_flash(struct mlx5_core_dev *dev,
 			.ops = &mlx5_mlxfw_dev_ops,
 			.psid = dev->board_id,
 			.psid_size = strlen(dev->board_id),
+			.devlink = priv_to_devlink(dev),
 		},
 		.mlx5_core_dev = dev
 	};
diff --git a/drivers/net/ethernet/mellanox/mlxfw/Kconfig b/drivers/net/ethernet/mellanox/mlxfw/Kconfig
index 0367f835a846..5b604501f33e 100644
--- a/drivers/net/ethernet/mellanox/mlxfw/Kconfig
+++ b/drivers/net/ethernet/mellanox/mlxfw/Kconfig
@@ -12,3 +12,4 @@ config MLXFW
 	  To compile this driver as a module, choose M here: the
 	  module will be called mlxfw.
 	select XZ_DEC
+	select NET_DEVLINK
diff --git a/drivers/net/ethernet/mellanox/mlxfw/mlxfw.h b/drivers/net/ethernet/mellanox/mlxfw/mlxfw.h
index c50e74ab02c4..cd88fd257501 100644
--- a/drivers/net/ethernet/mellanox/mlxfw/mlxfw.h
+++ b/drivers/net/ethernet/mellanox/mlxfw/mlxfw.h
@@ -6,6 +6,7 @@
 
 #include <linux/firmware.h>
 #include <linux/netlink.h>
+#include <net/devlink.h>
 
 enum mlxfw_fsm_state {
 	MLXFW_FSM_STATE_IDLE,
@@ -58,16 +59,13 @@ struct mlxfw_dev_ops {
 	void (*fsm_cancel)(struct mlxfw_dev *mlxfw_dev, u32 fwhandle);
 
 	void (*fsm_release)(struct mlxfw_dev *mlxfw_dev, u32 fwhandle);
-
-	void (*status_notify)(struct mlxfw_dev *mlxfw_dev,
-			      const char *msg, const char *comp_name,
-			      u32 done_bytes, u32 total_bytes);
 };
 
 struct mlxfw_dev {
 	const struct mlxfw_dev_ops *ops;
 	const char *psid;
 	u16 psid_size;
+	struct devlink *devlink;
 };
 
 #if IS_REACHABLE(CONFIG_MLXFW)
diff --git a/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c b/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
index 67990406cba2..663eac994a5c 100644
--- a/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
+++ b/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
@@ -39,16 +39,6 @@ static const char * const mlxfw_fsm_state_err_str[] = {
 		"unknown error"
 };
 
-static void mlxfw_status_notify(struct mlxfw_dev *mlxfw_dev,
-				const char *msg, const char *comp_name,
-				u32 done_bytes, u32 total_bytes)
-{
-	if (!mlxfw_dev->ops->status_notify)
-		return;
-	mlxfw_dev->ops->status_notify(mlxfw_dev, msg, comp_name,
-				      done_bytes, total_bytes);
-}
-
 static int mlxfw_fsm_state_wait(struct mlxfw_dev *mlxfw_dev, u32 fwhandle,
 				enum mlxfw_fsm_state fsm_state,
 				struct netlink_ext_ack *extack)
@@ -83,6 +73,14 @@ static int mlxfw_fsm_state_wait(struct mlxfw_dev *mlxfw_dev, u32 fwhandle,
 	return 0;
 }
 
+static void mlxfw_status_notify(struct mlxfw_dev *mlxfw_dev,
+				const char *msg, const char *comp_name,
+				u32 done_bytes, u32 total_bytes)
+{
+	devlink_flash_update_status_notify(mlxfw_dev->devlink, msg, comp_name,
+					   done_bytes, total_bytes);
+}
+
 #define MLXFW_ALIGN_DOWN(x, align_bits) ((x) & ~((1 << (align_bits)) - 1))
 #define MLXFW_ALIGN_UP(x, align_bits) \
 		MLXFW_ALIGN_DOWN((x) + ((1 << (align_bits)) - 1), (align_bits))
@@ -223,6 +221,7 @@ int mlxfw_firmware_flash(struct mlxfw_dev *mlxfw_dev,
 		return PTR_ERR(mfa2_file);
 
 	pr_info("Initialize firmware flash process\n");
+	devlink_flash_update_begin_notify(mlxfw_dev->devlink);
 	mlxfw_status_notify(mlxfw_dev, "Initializing firmware flash process",
 			    NULL, 0, 0);
 	err = mlxfw_dev->ops->fsm_lock(mlxfw_dev, &fwhandle);
@@ -261,6 +260,7 @@ int mlxfw_firmware_flash(struct mlxfw_dev *mlxfw_dev,
 	pr_info("Firmware flash done.\n");
 	mlxfw_status_notify(mlxfw_dev, "Firmware flash done", NULL, 0, 0);
 	mlxfw_mfa2_file_fini(mfa2_file);
+	devlink_flash_update_end_notify(mlxfw_dev->devlink);
 	return 0;
 
 err_state_wait_activate_to_locked:
@@ -270,6 +270,7 @@ int mlxfw_firmware_flash(struct mlxfw_dev *mlxfw_dev,
 	mlxfw_dev->ops->fsm_release(mlxfw_dev, fwhandle);
 err_fsm_lock:
 	mlxfw_mfa2_file_fini(mfa2_file);
+	devlink_flash_update_end_notify(mlxfw_dev->devlink);
 	return err;
 }
 EXPORT_SYMBOL(mlxfw_firmware_flash);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 556dca328bb5..4df6f01fe5ca 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -345,19 +345,6 @@ static void mlxsw_sp_fsm_release(struct mlxfw_dev *mlxfw_dev, u32 fwhandle)
 	mlxsw_reg_write(mlxsw_sp->core, MLXSW_REG(mcc), mcc_pl);
 }
 
-static void mlxsw_sp_status_notify(struct mlxfw_dev *mlxfw_dev,
-				   const char *msg, const char *comp_name,
-				   u32 done_bytes, u32 total_bytes)
-{
-	struct mlxsw_sp_mlxfw_dev *mlxsw_sp_mlxfw_dev =
-		container_of(mlxfw_dev, struct mlxsw_sp_mlxfw_dev, mlxfw_dev);
-	struct mlxsw_sp *mlxsw_sp = mlxsw_sp_mlxfw_dev->mlxsw_sp;
-
-	devlink_flash_update_status_notify(priv_to_devlink(mlxsw_sp->core),
-					   msg, comp_name,
-					   done_bytes, total_bytes);
-}
-
 static const struct mlxfw_dev_ops mlxsw_sp_mlxfw_dev_ops = {
 	.component_query	= mlxsw_sp_component_query,
 	.fsm_lock		= mlxsw_sp_fsm_lock,
@@ -368,7 +355,6 @@ static const struct mlxfw_dev_ops mlxsw_sp_mlxfw_dev_ops = {
 	.fsm_query_state	= mlxsw_sp_fsm_query_state,
 	.fsm_cancel		= mlxsw_sp_fsm_cancel,
 	.fsm_release		= mlxsw_sp_fsm_release,
-	.status_notify		= mlxsw_sp_status_notify,
 };
 
 static int mlxsw_sp_firmware_flash(struct mlxsw_sp *mlxsw_sp,
@@ -380,16 +366,15 @@ static int mlxsw_sp_firmware_flash(struct mlxsw_sp *mlxsw_sp,
 			.ops = &mlxsw_sp_mlxfw_dev_ops,
 			.psid = mlxsw_sp->bus_info->psid,
 			.psid_size = strlen(mlxsw_sp->bus_info->psid),
+			.devlink = priv_to_devlink(mlxsw_sp->core),
 		},
 		.mlxsw_sp = mlxsw_sp
 	};
 	int err;
 
 	mlxsw_core_fw_flash_start(mlxsw_sp->core);
-	devlink_flash_update_begin_notify(priv_to_devlink(mlxsw_sp->core));
 	err = mlxfw_firmware_flash(&mlxsw_sp_mlxfw_dev.mlxfw_dev,
 				   firmware, extack);
-	devlink_flash_update_end_notify(priv_to_devlink(mlxsw_sp->core));
 	mlxsw_core_fw_flash_end(mlxsw_sp->core);
 
 	return err;
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH V2 net-next 3/6] net/mlxfw: Improve FSM err message reporting and return codes
  2019-11-22 22:41 [PATCH V2 net-next 0/6] mlxfw: Improve error reporting Saeed Mahameed
  2019-11-22 22:41 ` [PATCH V2 net-next 1/6] netlink: Convert extack msg to a formattable buffer Saeed Mahameed
  2019-11-22 22:41 ` [PATCH V2 net-next 2/6] net/mlxfw: Generic mlx FW flash status notify Saeed Mahameed
@ 2019-11-22 22:41 ` Saeed Mahameed
  2019-11-24  1:00   ` Jakub Kicinski
  2019-11-22 22:41 ` [PATCH V2 net-next 4/6] net/mlxfw: Convert pr_* to dev_* in mlxfw_fsm.c Saeed Mahameed
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Saeed Mahameed @ 2019-11-22 22:41 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev, Jiri Pirko, Saeed Mahameed, Ido Schimmel

Report unique and standard error codes corresponding to the specific
FW flash error and report more detailed error messages to netlink.

Before:
$ devlink dev flash pci/0000:05:00.0 file ...
Error: mlxfw: Firmware flash failed.
devlink answers: Invalid argument

After:
$ devlink dev flash pci/0000:05:00.0 file ...
Error: mlxfw: Firmware flash failed: pending reset.
devlink answers: Operation already in progress

Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 .../net/ethernet/mellanox/mlxfw/mlxfw_fsm.c   | 35 +++++++++++++++----
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c b/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
index 663eac994a5c..803152ab6914 100644
--- a/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
+++ b/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
@@ -39,6 +39,32 @@ static const char * const mlxfw_fsm_state_err_str[] = {
 		"unknown error"
 };
 
+static const int mlxfw_fsm_state_errno[] = {
+	[MLXFW_FSM_STATE_ERR_ERROR] = -EREMOTEIO,
+	[MLXFW_FSM_STATE_ERR_REJECTED_DIGEST_ERR] = -EBADMSG,
+	[MLXFW_FSM_STATE_ERR_REJECTED_NOT_APPLICABLE] = -ENOENT,
+	[MLXFW_FSM_STATE_ERR_REJECTED_UNKNOWN_KEY] = -ENOKEY,
+	[MLXFW_FSM_STATE_ERR_REJECTED_AUTH_FAILED] = -EACCES,
+	[MLXFW_FSM_STATE_ERR_REJECTED_UNSIGNED] = -EKEYREVOKED,
+	[MLXFW_FSM_STATE_ERR_REJECTED_KEY_NOT_APPLICABLE] = -EKEYREJECTED,
+	[MLXFW_FSM_STATE_ERR_REJECTED_BAD_FORMAT] = -ENOEXEC,
+	[MLXFW_FSM_STATE_ERR_BLOCKED_PENDING_RESET] = -EALREADY,
+	[MLXFW_FSM_STATE_ERR_MAX] = -EINVAL
+};
+
+static int mlxfw_fsm_state_err(struct netlink_ext_ack *extack,
+			       enum mlxfw_fsm_state_err fsm_state_err)
+{
+#define MLXFW_ERR_PRFX "Firmware flash failed: "
+
+	fsm_state_err = min_t(enum mlxfw_fsm_state_err, fsm_state_err,
+			      MLXFW_FSM_STATE_ERR_MAX);
+	pr_err(MLXFW_ERR_PRFX "%s\n", mlxfw_fsm_state_err_str[fsm_state_err]);
+	NL_SET_ERR_MSG_MOD(extack, MLXFW_ERR_PRFX "%s",
+			   mlxfw_fsm_state_err_str[fsm_state_err]);
+	return mlxfw_fsm_state_errno[fsm_state_err];
+};
+
 static int mlxfw_fsm_state_wait(struct mlxfw_dev *mlxfw_dev, u32 fwhandle,
 				enum mlxfw_fsm_state fsm_state,
 				struct netlink_ext_ack *extack)
@@ -55,12 +81,9 @@ static int mlxfw_fsm_state_wait(struct mlxfw_dev *mlxfw_dev, u32 fwhandle,
 	if (err)
 		return err;
 
-	if (fsm_state_err != MLXFW_FSM_STATE_ERR_OK) {
-		pr_err("Firmware flash failed: %s\n",
-		       mlxfw_fsm_state_err_str[fsm_state_err]);
-		NL_SET_ERR_MSG_MOD(extack, "Firmware flash failed");
-		return -EINVAL;
-	}
+	if (fsm_state_err != MLXFW_FSM_STATE_ERR_OK)
+		return mlxfw_fsm_state_err(extack, fsm_state_err);
+
 	if (curr_fsm_state != fsm_state) {
 		if (--times == 0) {
 			pr_err("Timeout reached on FSM state change");
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH V2 net-next 4/6] net/mlxfw: Convert pr_* to dev_* in mlxfw_fsm.c
  2019-11-22 22:41 [PATCH V2 net-next 0/6] mlxfw: Improve error reporting Saeed Mahameed
                   ` (2 preceding siblings ...)
  2019-11-22 22:41 ` [PATCH V2 net-next 3/6] net/mlxfw: Improve FSM err message reporting and return codes Saeed Mahameed
@ 2019-11-22 22:41 ` Saeed Mahameed
  2019-11-22 22:41 ` [PATCH V2 net-next 5/6] net/mlxfw: More error messages coverage Saeed Mahameed
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Saeed Mahameed @ 2019-11-22 22:41 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev, Jiri Pirko, Saeed Mahameed, Ido Schimmel

Introduce mlxfw_{info, err, dbg} macros and make them call corresponding
dev_* macros, then convert all instances of pr_* to mlxfw_*.

This will allow printing the device name mlxfw is operating on.

Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxfw/mlxfw.h   | 32 ++++++++++-----
 .../net/ethernet/mellanox/mlxfw/mlxfw_fsm.c   | 40 ++++++++++---------
 2 files changed, 45 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxfw/mlxfw.h b/drivers/net/ethernet/mellanox/mlxfw/mlxfw.h
index cd88fd257501..a0a63e0c5aca 100644
--- a/drivers/net/ethernet/mellanox/mlxfw/mlxfw.h
+++ b/drivers/net/ethernet/mellanox/mlxfw/mlxfw.h
@@ -6,8 +6,31 @@
 
 #include <linux/firmware.h>
 #include <linux/netlink.h>
+#include <linux/device.h>
 #include <net/devlink.h>
 
+struct mlxfw_dev {
+	const struct mlxfw_dev_ops *ops;
+	const char *psid;
+	u16 psid_size;
+	struct devlink *devlink;
+};
+
+static inline
+struct device *mlxfw_dev_dev(struct mlxfw_dev *mlxfw_dev)
+{
+	return mlxfw_dev->devlink->dev;
+}
+
+#define MLXFW_PRFX "mlxfw: "
+
+#define mlxfw_info(mlxfw_dev, fmt, ...) \
+	dev_info(mlxfw_dev_dev(mlxfw_dev), MLXFW_PRFX fmt, ## __VA_ARGS__)
+#define mlxfw_err(mlxfw_dev, fmt, ...) \
+	dev_err(mlxfw_dev_dev(mlxfw_dev), MLXFW_PRFX fmt, ## __VA_ARGS__)
+#define mlxfw_dbg(mlxfw_dev, fmt, ...) \
+	dev_dbg(mlxfw_dev_dev(mlxfw_dev), MLXFW_PRFX fmt, ## __VA_ARGS__)
+
 enum mlxfw_fsm_state {
 	MLXFW_FSM_STATE_IDLE,
 	MLXFW_FSM_STATE_LOCKED,
@@ -32,8 +55,6 @@ enum mlxfw_fsm_state_err {
 	MLXFW_FSM_STATE_ERR_MAX,
 };
 
-struct mlxfw_dev;
-
 struct mlxfw_dev_ops {
 	int (*component_query)(struct mlxfw_dev *mlxfw_dev, u16 component_index,
 			       u32 *p_max_size, u8 *p_align_bits,
@@ -61,13 +82,6 @@ struct mlxfw_dev_ops {
 	void (*fsm_release)(struct mlxfw_dev *mlxfw_dev, u32 fwhandle);
 };
 
-struct mlxfw_dev {
-	const struct mlxfw_dev_ops *ops;
-	const char *psid;
-	u16 psid_size;
-	struct devlink *devlink;
-};
-
 #if IS_REACHABLE(CONFIG_MLXFW)
 int mlxfw_firmware_flash(struct mlxfw_dev *mlxfw_dev,
 			 const struct firmware *firmware,
diff --git a/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c b/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
index 803152ab6914..3ac0459aeac7 100644
--- a/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
+++ b/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
@@ -52,14 +52,17 @@ static const int mlxfw_fsm_state_errno[] = {
 	[MLXFW_FSM_STATE_ERR_MAX] = -EINVAL
 };
 
-static int mlxfw_fsm_state_err(struct netlink_ext_ack *extack,
+static int mlxfw_fsm_state_err(struct mlxfw_dev *mlxfw_dev,
+			       struct netlink_ext_ack *extack,
 			       enum mlxfw_fsm_state_err fsm_state_err)
 {
 #define MLXFW_ERR_PRFX "Firmware flash failed: "
 
 	fsm_state_err = min_t(enum mlxfw_fsm_state_err, fsm_state_err,
 			      MLXFW_FSM_STATE_ERR_MAX);
-	pr_err(MLXFW_ERR_PRFX "%s\n", mlxfw_fsm_state_err_str[fsm_state_err]);
+
+	mlxfw_err(mlxfw_dev, MLXFW_ERR_PRFX "%s\n",
+		  mlxfw_fsm_state_err_str[fsm_state_err]);
 	NL_SET_ERR_MSG_MOD(extack, MLXFW_ERR_PRFX "%s",
 			   mlxfw_fsm_state_err_str[fsm_state_err]);
 	return mlxfw_fsm_state_errno[fsm_state_err];
@@ -82,11 +85,11 @@ static int mlxfw_fsm_state_wait(struct mlxfw_dev *mlxfw_dev, u32 fwhandle,
 		return err;
 
 	if (fsm_state_err != MLXFW_FSM_STATE_ERR_OK)
-		return mlxfw_fsm_state_err(extack, fsm_state_err);
+		return mlxfw_fsm_state_err(mlxfw_dev, extack, fsm_state_err);
 
 	if (curr_fsm_state != fsm_state) {
 		if (--times == 0) {
-			pr_err("Timeout reached on FSM state change");
+			mlxfw_err(mlxfw_dev, "Timeout reached on FSM state change\n");
 			NL_SET_ERR_MSG_MOD(extack, "Timeout reached on FSM state change");
 			return -ETIMEDOUT;
 		}
@@ -132,8 +135,8 @@ static int mlxfw_flash_component(struct mlxfw_dev *mlxfw_dev,
 
 	comp_max_size = min_t(u32, comp_max_size, MLXFW_FSM_MAX_COMPONENT_SIZE);
 	if (comp->data_size > comp_max_size) {
-		pr_err("Component %d is of size %d which is bigger than limit %d\n",
-		       comp->index, comp->data_size, comp_max_size);
+		mlxfw_err(mlxfw_dev, "Component %d is of size %d which is bigger than limit %d\n",
+			  comp->index, comp->data_size, comp_max_size);
 		NL_SET_ERR_MSG_MOD(extack, "Component is bigger than limit");
 		return -EINVAL;
 	}
@@ -141,7 +144,7 @@ static int mlxfw_flash_component(struct mlxfw_dev *mlxfw_dev,
 	comp_max_write_size = MLXFW_ALIGN_DOWN(comp_max_write_size,
 					       comp_align_bits);
 
-	pr_debug("Component update\n");
+	mlxfw_dbg(mlxfw_dev, "Component update\n");
 	mlxfw_status_notify(mlxfw_dev, "Updating component", comp_name, 0, 0);
 	err = mlxfw_dev->ops->fsm_component_update(mlxfw_dev, fwhandle,
 						   comp->index,
@@ -154,7 +157,7 @@ static int mlxfw_flash_component(struct mlxfw_dev *mlxfw_dev,
 	if (err)
 		goto err_out;
 
-	pr_debug("Component download\n");
+	mlxfw_dbg(mlxfw_dev, "Component download\n");
 	mlxfw_status_notify(mlxfw_dev, "Downloading component",
 			    comp_name, 0, comp->data_size);
 	for (offset = 0;
@@ -173,7 +176,7 @@ static int mlxfw_flash_component(struct mlxfw_dev *mlxfw_dev,
 				    comp->data_size);
 	}
 
-	pr_debug("Component verify\n");
+	mlxfw_dbg(mlxfw_dev, "Component verify\n");
 	mlxfw_status_notify(mlxfw_dev, "Verifying component", comp_name, 0, 0);
 	err = mlxfw_dev->ops->fsm_component_verify(mlxfw_dev, fwhandle,
 						   comp->index);
@@ -203,7 +206,7 @@ static int mlxfw_flash_components(struct mlxfw_dev *mlxfw_dev, u32 fwhandle,
 					      mlxfw_dev->psid_size,
 					      &component_count);
 	if (err) {
-		pr_err("Could not find device PSID in MFA2 file\n");
+		mlxfw_err(mlxfw_dev, "Could not find device PSID in MFA2 file\n");
 		NL_SET_ERR_MSG_MOD(extack, "Could not find device PSID in MFA2 file");
 		return err;
 	}
@@ -216,7 +219,8 @@ static int mlxfw_flash_components(struct mlxfw_dev *mlxfw_dev, u32 fwhandle,
 		if (IS_ERR(comp))
 			return PTR_ERR(comp);
 
-		pr_info("Flashing component type %d\n", comp->index);
+		mlxfw_info(mlxfw_dev, "Flashing component type %d\n",
+			   comp->index);
 		err = mlxfw_flash_component(mlxfw_dev, fwhandle, comp, extack);
 		mlxfw_mfa2_file_component_put(comp);
 		if (err)
@@ -234,7 +238,7 @@ int mlxfw_firmware_flash(struct mlxfw_dev *mlxfw_dev,
 	int err;
 
 	if (!mlxfw_mfa2_check(firmware)) {
-		pr_err("Firmware file is not MFA2\n");
+		mlxfw_err(mlxfw_dev, "Firmware file is not MFA2\n");
 		NL_SET_ERR_MSG_MOD(extack, "Firmware file is not MFA2");
 		return -EINVAL;
 	}
@@ -243,13 +247,13 @@ int mlxfw_firmware_flash(struct mlxfw_dev *mlxfw_dev,
 	if (IS_ERR(mfa2_file))
 		return PTR_ERR(mfa2_file);
 
-	pr_info("Initialize firmware flash process\n");
+	mlxfw_info(mlxfw_dev, "Initialize firmware flash process\n");
 	devlink_flash_update_begin_notify(mlxfw_dev->devlink);
 	mlxfw_status_notify(mlxfw_dev, "Initializing firmware flash process",
 			    NULL, 0, 0);
 	err = mlxfw_dev->ops->fsm_lock(mlxfw_dev, &fwhandle);
 	if (err) {
-		pr_err("Could not lock the firmware FSM\n");
+		mlxfw_err(mlxfw_dev, "Could not lock the firmware FSM\n");
 		NL_SET_ERR_MSG_MOD(extack, "Could not lock the firmware FSM");
 		goto err_fsm_lock;
 	}
@@ -263,11 +267,11 @@ int mlxfw_firmware_flash(struct mlxfw_dev *mlxfw_dev,
 	if (err)
 		goto err_flash_components;
 
-	pr_debug("Activate image\n");
+	mlxfw_dbg(mlxfw_dev, "Activate image\n");
 	mlxfw_status_notify(mlxfw_dev, "Activating image", NULL, 0, 0);
 	err = mlxfw_dev->ops->fsm_activate(mlxfw_dev, fwhandle);
 	if (err) {
-		pr_err("Could not activate the downloaded image\n");
+		mlxfw_err(mlxfw_dev, "Could not activate the downloaded image\n");
 		NL_SET_ERR_MSG_MOD(extack, "Could not activate the downloaded image");
 		goto err_fsm_activate;
 	}
@@ -277,10 +281,10 @@ int mlxfw_firmware_flash(struct mlxfw_dev *mlxfw_dev,
 	if (err)
 		goto err_state_wait_activate_to_locked;
 
-	pr_debug("Handle release\n");
+	mlxfw_dbg(mlxfw_dev, "Handle release\n");
 	mlxfw_dev->ops->fsm_release(mlxfw_dev, fwhandle);
 
-	pr_info("Firmware flash done.\n");
+	mlxfw_info(mlxfw_dev, "Firmware flash done\n");
 	mlxfw_status_notify(mlxfw_dev, "Firmware flash done", NULL, 0, 0);
 	mlxfw_mfa2_file_fini(mfa2_file);
 	devlink_flash_update_end_notify(mlxfw_dev->devlink);
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH V2 net-next 5/6] net/mlxfw: More error messages coverage
  2019-11-22 22:41 [PATCH V2 net-next 0/6] mlxfw: Improve error reporting Saeed Mahameed
                   ` (3 preceding siblings ...)
  2019-11-22 22:41 ` [PATCH V2 net-next 4/6] net/mlxfw: Convert pr_* to dev_* in mlxfw_fsm.c Saeed Mahameed
@ 2019-11-22 22:41 ` Saeed Mahameed
  2019-11-22 22:41 ` [PATCH V2 net-next 6/6] net/mlxfw: Macro for error reporting Saeed Mahameed
  2019-11-24  0:48 ` [PATCH V2 net-next 0/6] mlxfw: Improve " Jakub Kicinski
  6 siblings, 0 replies; 14+ messages in thread
From: Saeed Mahameed @ 2019-11-22 22:41 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev, Jiri Pirko, Saeed Mahameed

Make sure mlxfw_firmware_flash reports a detailed user readable error
message in every possible error path, basically every time
mlxfw_dev->ops->*() is called and an error is returned, or when image
initialization is failed.

Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 .../net/ethernet/mellanox/mlxfw/mlxfw_fsm.c   | 47 +++++++++++++++----
 1 file changed, 38 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c b/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
index 3ac0459aeac7..2726f17a68fe 100644
--- a/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
+++ b/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
@@ -81,8 +81,11 @@ static int mlxfw_fsm_state_wait(struct mlxfw_dev *mlxfw_dev, u32 fwhandle,
 retry:
 	err = mlxfw_dev->ops->fsm_query_state(mlxfw_dev, fwhandle,
 					      &curr_fsm_state, &fsm_state_err);
-	if (err)
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "FSM state query failed, err (%d)",
+				   err);
 		return err;
+	}
 
 	if (fsm_state_err != MLXFW_FSM_STATE_ERR_OK)
 		return mlxfw_fsm_state_err(mlxfw_dev, extack, fsm_state_err);
@@ -130,8 +133,12 @@ static int mlxfw_flash_component(struct mlxfw_dev *mlxfw_dev,
 	err = mlxfw_dev->ops->component_query(mlxfw_dev, comp->index,
 					      &comp_max_size, &comp_align_bits,
 					      &comp_max_write_size);
-	if (err)
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "FSM component query failed, comp_name(%s) err (%d)",
+				   comp_name, err);
 		return err;
+	}
 
 	comp_max_size = min_t(u32, comp_max_size, MLXFW_FSM_MAX_COMPONENT_SIZE);
 	if (comp->data_size > comp_max_size) {
@@ -149,8 +156,12 @@ static int mlxfw_flash_component(struct mlxfw_dev *mlxfw_dev,
 	err = mlxfw_dev->ops->fsm_component_update(mlxfw_dev, fwhandle,
 						   comp->index,
 						   comp->data_size);
-	if (err)
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "FSM component update failed, comp_name(%s) err (%d)",
+				   comp_name, err);
 		return err;
+	}
 
 	err = mlxfw_fsm_state_wait(mlxfw_dev, fwhandle,
 				   MLXFW_FSM_STATE_DOWNLOAD, extack);
@@ -169,8 +180,12 @@ static int mlxfw_flash_component(struct mlxfw_dev *mlxfw_dev,
 		err = mlxfw_dev->ops->fsm_block_download(mlxfw_dev, fwhandle,
 							 block_ptr, block_size,
 							 offset);
-		if (err)
+		if (err) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Component download failed, comp_name(%s) err (%d)",
+					   comp_name, err);
 			goto err_out;
+		}
 		mlxfw_status_notify(mlxfw_dev, "Downloading component",
 				    comp_name, offset + block_size,
 				    comp->data_size);
@@ -180,8 +195,12 @@ static int mlxfw_flash_component(struct mlxfw_dev *mlxfw_dev,
 	mlxfw_status_notify(mlxfw_dev, "Verifying component", comp_name, 0, 0);
 	err = mlxfw_dev->ops->fsm_component_verify(mlxfw_dev, fwhandle,
 						   comp->index);
-	if (err)
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "FSM component verify failed, comp_name(%s) err (%d)",
+				   comp_name, err);
 		goto err_out;
+	}
 
 	err = mlxfw_fsm_state_wait(mlxfw_dev, fwhandle,
 				   MLXFW_FSM_STATE_LOCKED, extack);
@@ -216,8 +235,13 @@ static int mlxfw_flash_components(struct mlxfw_dev *mlxfw_dev, u32 fwhandle,
 
 		comp = mlxfw_mfa2_file_component_get(mfa2_file, mlxfw_dev->psid,
 						     mlxfw_dev->psid_size, i);
-		if (IS_ERR(comp))
-			return PTR_ERR(comp);
+		if (IS_ERR(comp)) {
+			err = PTR_ERR(comp);
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Failed to get MFA2 component, component (%d) err (%d)",
+					   i, err);
+			return err;
+		}
 
 		mlxfw_info(mlxfw_dev, "Flashing component type %d\n",
 			   comp->index);
@@ -244,8 +268,13 @@ int mlxfw_firmware_flash(struct mlxfw_dev *mlxfw_dev,
 	}
 
 	mfa2_file = mlxfw_mfa2_file_init(firmware);
-	if (IS_ERR(mfa2_file))
-		return PTR_ERR(mfa2_file);
+	if (IS_ERR(mfa2_file)) {
+		err = PTR_ERR(mfa2_file);
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Failed to initialize MFA2 firmware file, err (%d)",
+				   err);
+		return err;
+	}
 
 	mlxfw_info(mlxfw_dev, "Initialize firmware flash process\n");
 	devlink_flash_update_begin_notify(mlxfw_dev->devlink);
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH V2 net-next 6/6] net/mlxfw: Macro for error reporting
  2019-11-22 22:41 [PATCH V2 net-next 0/6] mlxfw: Improve error reporting Saeed Mahameed
                   ` (4 preceding siblings ...)
  2019-11-22 22:41 ` [PATCH V2 net-next 5/6] net/mlxfw: More error messages coverage Saeed Mahameed
@ 2019-11-22 22:41 ` Saeed Mahameed
  2019-11-24  0:48 ` [PATCH V2 net-next 0/6] mlxfw: Improve " Jakub Kicinski
  6 siblings, 0 replies; 14+ messages in thread
From: Saeed Mahameed @ 2019-11-22 22:41 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev, Jiri Pirko, Saeed Mahameed

Add a macro to simplify error message reporting, instead of always
calling both mlxfw_err and NL_SET_ERR_MSG_MOD with the same message,
use a macro instead.

This is doable now since NL_SET_ERR_MSG_MOD can accept formattable
messages.

Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 .../net/ethernet/mellanox/mlxfw/mlxfw_fsm.c   | 78 ++++++++++---------
 1 file changed, 41 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c b/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
index 2726f17a68fe..cc333368a367 100644
--- a/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
+++ b/drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
@@ -52,6 +52,12 @@ static const int mlxfw_fsm_state_errno[] = {
 	[MLXFW_FSM_STATE_ERR_MAX] = -EINVAL
 };
 
+#define MLXFW_ERR_MSG(mlxfw_dev, extack, fmt, ...)	\
+({							\
+	mlxfw_err(mlxfw_dev, fmt "\n", ## __VA_ARGS__);	\
+	NL_SET_ERR_MSG_MOD(extack, fmt, ## __VA_ARGS__);\
+})
+
 static int mlxfw_fsm_state_err(struct mlxfw_dev *mlxfw_dev,
 			       struct netlink_ext_ack *extack,
 			       enum mlxfw_fsm_state_err fsm_state_err)
@@ -61,10 +67,8 @@ static int mlxfw_fsm_state_err(struct mlxfw_dev *mlxfw_dev,
 	fsm_state_err = min_t(enum mlxfw_fsm_state_err, fsm_state_err,
 			      MLXFW_FSM_STATE_ERR_MAX);
 
-	mlxfw_err(mlxfw_dev, MLXFW_ERR_PRFX "%s\n",
-		  mlxfw_fsm_state_err_str[fsm_state_err]);
-	NL_SET_ERR_MSG_MOD(extack, MLXFW_ERR_PRFX "%s",
-			   mlxfw_fsm_state_err_str[fsm_state_err]);
+	MLXFW_ERR_MSG(mlxfw_dev, extack, MLXFW_ERR_PRFX "%s",
+		      mlxfw_fsm_state_err_str[fsm_state_err]);
 	return mlxfw_fsm_state_errno[fsm_state_err];
 };
 
@@ -82,8 +86,8 @@ static int mlxfw_fsm_state_wait(struct mlxfw_dev *mlxfw_dev, u32 fwhandle,
 	err = mlxfw_dev->ops->fsm_query_state(mlxfw_dev, fwhandle,
 					      &curr_fsm_state, &fsm_state_err);
 	if (err) {
-		NL_SET_ERR_MSG_MOD(extack, "FSM state query failed, err (%d)",
-				   err);
+		MLXFW_ERR_MSG(mlxfw_dev, extack, "FSM state query failed, err (%d)",
+			      err);
 		return err;
 	}
 
@@ -92,8 +96,8 @@ static int mlxfw_fsm_state_wait(struct mlxfw_dev *mlxfw_dev, u32 fwhandle,
 
 	if (curr_fsm_state != fsm_state) {
 		if (--times == 0) {
-			mlxfw_err(mlxfw_dev, "Timeout reached on FSM state change\n");
-			NL_SET_ERR_MSG_MOD(extack, "Timeout reached on FSM state change");
+			MLXFW_ERR_MSG(mlxfw_dev, extack,
+				      "Timeout reached on FSM state change");
 			return -ETIMEDOUT;
 		}
 		msleep(MLXFW_FSM_STATE_WAIT_CYCLE_MS);
@@ -134,17 +138,17 @@ static int mlxfw_flash_component(struct mlxfw_dev *mlxfw_dev,
 					      &comp_max_size, &comp_align_bits,
 					      &comp_max_write_size);
 	if (err) {
-		NL_SET_ERR_MSG_MOD(extack,
-				   "FSM component query failed, comp_name(%s) err (%d)",
-				   comp_name, err);
+		MLXFW_ERR_MSG(mlxfw_dev, extack,
+			      "FSM component query failed, comp_name(%s) err (%d)",
+			      comp_name, err);
 		return err;
 	}
 
 	comp_max_size = min_t(u32, comp_max_size, MLXFW_FSM_MAX_COMPONENT_SIZE);
 	if (comp->data_size > comp_max_size) {
-		mlxfw_err(mlxfw_dev, "Component %d is of size %d which is bigger than limit %d\n",
-			  comp->index, comp->data_size, comp_max_size);
-		NL_SET_ERR_MSG_MOD(extack, "Component is bigger than limit");
+		MLXFW_ERR_MSG(mlxfw_dev, extack,
+			      "Component size is bigger than limit, Component %d size %d limit %d",
+			      comp->index, comp->data_size, comp_max_size);
 		return -EINVAL;
 	}
 
@@ -157,9 +161,9 @@ static int mlxfw_flash_component(struct mlxfw_dev *mlxfw_dev,
 						   comp->index,
 						   comp->data_size);
 	if (err) {
-		NL_SET_ERR_MSG_MOD(extack,
-				   "FSM component update failed, comp_name(%s) err (%d)",
-				   comp_name, err);
+		MLXFW_ERR_MSG(mlxfw_dev, extack,
+			      "FSM component update failed, comp_name(%s) err (%d)",
+			      comp_name, err);
 		return err;
 	}
 
@@ -181,9 +185,9 @@ static int mlxfw_flash_component(struct mlxfw_dev *mlxfw_dev,
 							 block_ptr, block_size,
 							 offset);
 		if (err) {
-			NL_SET_ERR_MSG_MOD(extack,
-					   "Component download failed, comp_name(%s) err (%d)",
-					   comp_name, err);
+			MLXFW_ERR_MSG(mlxfw_dev, extack,
+				      "Component download failed, comp_name(%s) err (%d)",
+				      comp_name, err);
 			goto err_out;
 		}
 		mlxfw_status_notify(mlxfw_dev, "Downloading component",
@@ -196,9 +200,9 @@ static int mlxfw_flash_component(struct mlxfw_dev *mlxfw_dev,
 	err = mlxfw_dev->ops->fsm_component_verify(mlxfw_dev, fwhandle,
 						   comp->index);
 	if (err) {
-		NL_SET_ERR_MSG_MOD(extack,
-				   "FSM component verify failed, comp_name(%s) err (%d)",
-				   comp_name, err);
+		MLXFW_ERR_MSG(mlxfw_dev, extack,
+			      "FSM component verify failed, comp_name(%s) err (%d)",
+			      comp_name, err);
 		goto err_out;
 	}
 
@@ -225,8 +229,9 @@ static int mlxfw_flash_components(struct mlxfw_dev *mlxfw_dev, u32 fwhandle,
 					      mlxfw_dev->psid_size,
 					      &component_count);
 	if (err) {
-		mlxfw_err(mlxfw_dev, "Could not find device PSID in MFA2 file\n");
-		NL_SET_ERR_MSG_MOD(extack, "Could not find device PSID in MFA2 file");
+		MLXFW_ERR_MSG(mlxfw_dev, extack,
+			      "Could not find device PSID in MFA2 file, err (%d)",
+			      err);
 		return err;
 	}
 
@@ -237,9 +242,9 @@ static int mlxfw_flash_components(struct mlxfw_dev *mlxfw_dev, u32 fwhandle,
 						     mlxfw_dev->psid_size, i);
 		if (IS_ERR(comp)) {
 			err = PTR_ERR(comp);
-			NL_SET_ERR_MSG_MOD(extack,
-					   "Failed to get MFA2 component, component (%d) err (%d)",
-					   i, err);
+			MLXFW_ERR_MSG(mlxfw_dev, extack,
+				      "Failed to get MFA2 component, component (%d) err (%d)",
+				      i, err);
 			return err;
 		}
 
@@ -262,17 +267,16 @@ int mlxfw_firmware_flash(struct mlxfw_dev *mlxfw_dev,
 	int err;
 
 	if (!mlxfw_mfa2_check(firmware)) {
-		mlxfw_err(mlxfw_dev, "Firmware file is not MFA2\n");
-		NL_SET_ERR_MSG_MOD(extack, "Firmware file is not MFA2");
+		MLXFW_ERR_MSG(mlxfw_dev, extack, "Firmware file is not MFA2");
 		return -EINVAL;
 	}
 
 	mfa2_file = mlxfw_mfa2_file_init(firmware);
 	if (IS_ERR(mfa2_file)) {
 		err = PTR_ERR(mfa2_file);
-		NL_SET_ERR_MSG_MOD(extack,
-				   "Failed to initialize MFA2 firmware file, err (%d)",
-				   err);
+		MLXFW_ERR_MSG(mlxfw_dev, extack,
+			      "Failed to initialize MFA2 firmware file, err (%d)",
+			      err);
 		return err;
 	}
 
@@ -282,8 +286,8 @@ int mlxfw_firmware_flash(struct mlxfw_dev *mlxfw_dev,
 			    NULL, 0, 0);
 	err = mlxfw_dev->ops->fsm_lock(mlxfw_dev, &fwhandle);
 	if (err) {
-		mlxfw_err(mlxfw_dev, "Could not lock the firmware FSM\n");
-		NL_SET_ERR_MSG_MOD(extack, "Could not lock the firmware FSM");
+		MLXFW_ERR_MSG(mlxfw_dev, extack,
+			      "Could not lock the firmware FSM, err (%d)", err);
 		goto err_fsm_lock;
 	}
 
@@ -300,8 +304,8 @@ int mlxfw_firmware_flash(struct mlxfw_dev *mlxfw_dev,
 	mlxfw_status_notify(mlxfw_dev, "Activating image", NULL, 0, 0);
 	err = mlxfw_dev->ops->fsm_activate(mlxfw_dev, fwhandle);
 	if (err) {
-		mlxfw_err(mlxfw_dev, "Could not activate the downloaded image\n");
-		NL_SET_ERR_MSG_MOD(extack, "Could not activate the downloaded image");
+		MLXFW_ERR_MSG(mlxfw_dev, extack,
+			      "Could not activate the downloaded image, err (%d)", err);
 		goto err_fsm_activate;
 	}
 
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH V2 net-next 0/6] mlxfw: Improve error reporting
  2019-11-22 22:41 [PATCH V2 net-next 0/6] mlxfw: Improve error reporting Saeed Mahameed
                   ` (5 preceding siblings ...)
  2019-11-22 22:41 ` [PATCH V2 net-next 6/6] net/mlxfw: Macro for error reporting Saeed Mahameed
@ 2019-11-24  0:48 ` Jakub Kicinski
  6 siblings, 0 replies; 14+ messages in thread
From: Jakub Kicinski @ 2019-11-24  0:48 UTC (permalink / raw)
  To: Saeed Mahameed; +Cc: David S. Miller, netdev, Jiri Pirko

On Fri, 22 Nov 2019 22:41:45 +0000, Saeed Mahameed wrote:
> merge conflict with net:
> @ drivers/net/ethernet/mellanox/mlxfw/mlxfw_fsm.c
> 
> ++<<<<<<< (net-next) (keep)
> +
> +       if (fsm_state_err != MLXFW_FSM_STATE_ERR_OK)
> +               return mlxfw_fsm_state_err(mlxfw_dev, extack, fsm_state_err);
> +
> ++=======  
> + 
> +       if (fsm_state_err != MLXFW_FSM_STATE_ERR_OK) {
> +               fsm_state_err = min_t(enum mlxfw_fsm_state_err,
> +                                     fsm_state_err, MLXFW_FSM_STATE_ERR_MAX);
> +               pr_err("Firmware flash failed: %s\n",
> +                      mlxfw_fsm_state_err_str[fsm_state_err]);
> +               NL_SET_ERR_MSG_MOD(extack, "Firmware flash failed");
> +               return -EINVAL;
> ++>>>>>>> (net) (delete)   
> 
> To resolve just use the 1st hunk, from net-next.

Net has been merged into -next, please rebase and repost, this doesn't
apply right now.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH V2 net-next 1/6] netlink: Convert extack msg to a formattable buffer
  2019-11-22 22:41 ` [PATCH V2 net-next 1/6] netlink: Convert extack msg to a formattable buffer Saeed Mahameed
@ 2019-11-24  0:56   ` Jakub Kicinski
  2019-11-24  1:10     ` David Ahern
  2019-11-25  9:48     ` Johannes Berg
  0 siblings, 2 replies; 14+ messages in thread
From: Jakub Kicinski @ 2019-11-24  0:56 UTC (permalink / raw)
  To: Saeed Mahameed
  Cc: David S. Miller, netdev, Jiri Pirko, David Ahern, Johannes Berg

On Fri, 22 Nov 2019 22:41:47 +0000, Saeed Mahameed wrote:
> Before this patch the extack msg had to be a const char and didn't leave
> much room for developers to report formattable and flexible messages.
> 
> All current usages are oneliner messages, hence, a buffer of size 128B
> should be sufficient to replace the const char pointer.
> 
> This will allow future usages to provide formattable messages and more
> flexible error reporting, without any impact on current users.
> 
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> Reviewed-by: Jiri Pirko <jiri@mellanox.com>

Let's CC DSA and Johannes on this one

> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index 205fa7b1f07a..de9004da0288 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -62,6 +62,7 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
>  
>  /* this can be increased when necessary - don't expose to userland */
>  #define NETLINK_MAX_COOKIE_LEN	20
> +#define NL_EXTACK_MAX_MSG_SZ	128
>  
>  /**
>   * struct netlink_ext_ack - netlink extended ACK report struct
> @@ -72,40 +73,36 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
>   * @cookie_len: actual cookie data length
>   */
>  struct netlink_ext_ack {
> -	const char *_msg;
> +	char _msg[NL_EXTACK_MAX_MSG_SZ];
>  	const struct nlattr *bad_attr;
>  	u8 cookie[NETLINK_MAX_COOKIE_LEN];
>  	u8 cookie_len;
>  };
>  
> -/* Always use this macro, this allows later putting the
> - * message into a separate section or such for things
> - * like translation or listing all possible messages.
> - * Currently string formatting is not supported (due
> - * to the lack of an output buffer.)
> - */
> -#define NL_SET_ERR_MSG(extack, msg) do {		\
> -	static const char __msg[] = msg;		\
> +#define NL_MSG_FMT(extack, fmt, ...) \
> +	WARN_ON(snprintf(extack->_msg, NL_EXTACK_MAX_MSG_SZ, fmt, ## __VA_ARGS__) \
> +		>= NL_EXTACK_MAX_MSG_SZ)

I'd personally appreciate a word of analysis and reassurance in the
commit message that this snprintf + WARN_ON inlined in every location
where extack is used won't bloat the kernel :S

One could easily imagine a solution which would continue to carry the
static strings via a pointer without the unnecessary roundtrip thru
snprintf().

> +#define NL_SET_ERR_MSG(extack, fmt, ...) do {		\
>  	struct netlink_ext_ack *__extack = (extack);	\
>  							\
>  	if (__extack)					\
> -		__extack->_msg = __msg;			\
> +		NL_MSG_FMT(__extack, fmt, ## __VA_ARGS__); \
>  } while (0)
>  
> -#define NL_SET_ERR_MSG_MOD(extack, msg)			\
> -	NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg)
> +#define NL_SET_ERR_MSG_MOD(extack, fmt, ...)			\
> +	NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " fmt, ## __VA_ARGS__)
>  
>  #define NL_SET_BAD_ATTR(extack, attr) do {		\
>  	if ((extack))					\
>  		(extack)->bad_attr = (attr);		\
>  } while (0)
>  
> -#define NL_SET_ERR_MSG_ATTR(extack, attr, msg) do {	\
> -	static const char __msg[] = msg;		\
> +#define NL_SET_ERR_MSG_ATTR(extack, attr, fmt, ...) do {	\
>  	struct netlink_ext_ack *__extack = (extack);	\
>  							\
>  	if (__extack) {					\
> -		__extack->_msg = __msg;			\
> +		NL_MSG_FMT(__extack, fmt, ## __VA_ARGS__); \
>  		__extack->bad_attr = (attr);		\
>  	}						\
>  } while (0)
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index cace9b307781..2ce1d6b68ce8 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -208,7 +208,7 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
>  	case NLA_REJECT:
>  		if (extack && pt->validation_data) {
>  			NL_SET_BAD_ATTR(extack, nla);
> -			extack->_msg = pt->validation_data;
> +			NL_MSG_FMT(extack, pt->validation_data);
>  			return -EINVAL;
>  		}
>  		err = -EINVAL;


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH V2 net-next 3/6] net/mlxfw: Improve FSM err message reporting and return codes
  2019-11-22 22:41 ` [PATCH V2 net-next 3/6] net/mlxfw: Improve FSM err message reporting and return codes Saeed Mahameed
@ 2019-11-24  1:00   ` Jakub Kicinski
  0 siblings, 0 replies; 14+ messages in thread
From: Jakub Kicinski @ 2019-11-24  1:00 UTC (permalink / raw)
  To: Saeed Mahameed
  Cc: David S. Miller, netdev, Jiri Pirko, Ido Schimmel, David Ahern,
	Johannes Berg

On Fri, 22 Nov 2019 22:41:50 +0000, Saeed Mahameed wrote:
> +	NL_SET_ERR_MSG_MOD(extack, MLXFW_ERR_PRFX "%s",
> +			   mlxfw_fsm_state_err_str[fsm_state_err]);

Things like this also require a word of comment, because the intention
of wrapping all extact strings into the macro IIRC was to mark them for
possible translation. 

IDK if we still care about that (IMHO we should), we probably need at
least a new macro for cases like this so the strings in the error table
are marked for extraction for translators as well, no?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH V2 net-next 1/6] netlink: Convert extack msg to a formattable buffer
  2019-11-24  0:56   ` Jakub Kicinski
@ 2019-11-24  1:10     ` David Ahern
  2019-11-24  1:20       ` David Ahern
  2019-11-25  9:48     ` Johannes Berg
  1 sibling, 1 reply; 14+ messages in thread
From: David Ahern @ 2019-11-24  1:10 UTC (permalink / raw)
  To: Jakub Kicinski, Saeed Mahameed
  Cc: David S. Miller, netdev, Jiri Pirko, Johannes Berg

On 11/23/19 5:56 PM, Jakub Kicinski wrote:
>> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
>> index 205fa7b1f07a..de9004da0288 100644
>> --- a/include/linux/netlink.h
>> +++ b/include/linux/netlink.h
>> @@ -62,6 +62,7 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
>>  
>>  /* this can be increased when necessary - don't expose to userland */
>>  #define NETLINK_MAX_COOKIE_LEN	20
>> +#define NL_EXTACK_MAX_MSG_SZ	128

This makes extack use a lot of stack. There are a number of places that
are already emitting warnings about high stack usage.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH V2 net-next 1/6] netlink: Convert extack msg to a formattable buffer
  2019-11-24  1:10     ` David Ahern
@ 2019-11-24  1:20       ` David Ahern
  0 siblings, 0 replies; 14+ messages in thread
From: David Ahern @ 2019-11-24  1:20 UTC (permalink / raw)
  To: Jakub Kicinski, Saeed Mahameed
  Cc: David S. Miller, netdev, Jiri Pirko, Johannes Berg

On 11/23/19 6:10 PM, David Ahern wrote:
> On 11/23/19 5:56 PM, Jakub Kicinski wrote:
>>> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
>>> index 205fa7b1f07a..de9004da0288 100644
>>> --- a/include/linux/netlink.h
>>> +++ b/include/linux/netlink.h
>>> @@ -62,6 +62,7 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
>>>  
>>>  /* this can be increased when necessary - don't expose to userland */
>>>  #define NETLINK_MAX_COOKIE_LEN	20
>>> +#define NL_EXTACK_MAX_MSG_SZ	128
> 
> This makes extack use a lot of stack. There are a number of places that
> are already emitting warnings about high stack usage.
> 

It also adds unnecessary overhead since initialization 0's out the
entire buffer (most sites declare with {}; a few use memset).

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH V2 net-next 1/6] netlink: Convert extack msg to a formattable buffer
  2019-11-24  0:56   ` Jakub Kicinski
  2019-11-24  1:10     ` David Ahern
@ 2019-11-25  9:48     ` Johannes Berg
  2019-11-25 23:12       ` Saeed Mahameed
  1 sibling, 1 reply; 14+ messages in thread
From: Johannes Berg @ 2019-11-25  9:48 UTC (permalink / raw)
  To: Jakub Kicinski, Saeed Mahameed
  Cc: David S. Miller, netdev, Jiri Pirko, David Ahern

Hi Jakub,

On Sat, 2019-11-23 at 16:56 -0800, Jakub Kicinski wrote:
> 
> > -/* Always use this macro, this allows later putting the
> > - * message into a separate section or such for things
> > - * like translation or listing all possible messages.

Regarding your other email also - this here was one stated purpose -
what I had also (maybe even more) in mind back then was that we'd be
able to elide the strings entirely for really small embedded builds. If
it's not used interactively, there isn't that much value in the strings
after all.

> > - * Currently string formatting is not supported (due
> > - * to the lack of an output buffer.)
> > - */
> > -#define NL_SET_ERR_MSG(extack, msg) do {		\
> > -	static const char __msg[] = msg;		\
> > +#define NL_MSG_FMT(extack, fmt, ...) \
> > +	WARN_ON(snprintf(extack->_msg, NL_EXTACK_MAX_MSG_SZ, fmt, ## __VA_ARGS__) \
> > +		>= NL_EXTACK_MAX_MSG_SZ)
> 
> I'd personally appreciate a word of analysis and reassurance in the
> commit message that this snprintf + WARN_ON inlined in every location
> where extack is used won't bloat the kernel :S

That does seem quite excessive, indeed.

I think _if_ we want this at all then I'd say that we should move this
out-of-line, to have a helper function that will kasprintf() if
necessary, and use a bit somewhere to indicate "has been allocated" so
it can be freed later?

However, this will need some kind of "release extack" API for those
places that declare their own struct on the stack, and would need to be
reentrant (in the sense that old error messages may be overwritten, and
must be freed at that point)...

Maybe an alternative would be to have a "can sprintf" flag, and then
provide a buffer in another pointer? The caller can then decide whether
this should be permitted, e.g. netlink_rcv_skb() could provide it, but
other places maybe don't need to?


Here in the patchset though, I basically found three cases using this
capability:

+	NL_SET_ERR_MSG_MOD(extack, MLXFW_ERR_PRFX "%s",
+			   mlxfw_fsm_state_err_str[fsm_state_err]);

This one seems somewhat unnecessary - it just takes a fixed string and
adds a prefix, that may be easier this way but it wouldn't be *that*
hard to "explode" that into a bunch of NL_SET_ERR_MSG_MOD() statements I
guess.

+		NL_SET_ERR_MSG_MOD(extack, "FSM state query failed, err (%d)",
+				   err);

This (and other similar instances) is pretty useless, the error number
is almost certainly returned to userspace anyway?

+		NL_SET_ERR_MSG_MOD(extack,
+				   "FSM component query failed, comp_name(%s) err (%d)",
+				   comp_name, err);

This (and similar) one seems like the only one that's reasonable. Note
that "comp_name" is actually just a number though.

johannes


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH V2 net-next 1/6] netlink: Convert extack msg to a formattable buffer
  2019-11-25  9:48     ` Johannes Berg
@ 2019-11-25 23:12       ` Saeed Mahameed
  0 siblings, 0 replies; 14+ messages in thread
From: Saeed Mahameed @ 2019-11-25 23:12 UTC (permalink / raw)
  To: johannes, jakub.kicinski; +Cc: dsahern, Jiri Pirko, davem, netdev

On Mon, 2019-11-25 at 10:48 +0100, Johannes Berg wrote:
> Hi Jakub,
> 
> On Sat, 2019-11-23 at 16:56 -0800, Jakub Kicinski wrote:
> > > -/* Always use this macro, this allows later putting the
> > > - * message into a separate section or such for things
> > > - * like translation or listing all possible messages.
> 
> Regarding your other email also - this here was one stated purpose -
> what I had also (maybe even more) in mind back then was that we'd be
> able to elide the strings entirely for really small embedded builds.
> If
> it's not used interactively, there isn't that much value in the
> strings
> after all.
> 
> > > - * Currently string formatting is not supported (due
> > > - * to the lack of an output buffer.)
> > > - */
> > > -#define NL_SET_ERR_MSG(extack, msg) do {		\
> > > -	static const char __msg[] = msg;		\
> > > +#define NL_MSG_FMT(extack, fmt, ...) \
> > > +	WARN_ON(snprintf(extack->_msg, NL_EXTACK_MAX_MSG_SZ, fmt, ##
> > > __VA_ARGS__) \
> > > +		>= NL_EXTACK_MAX_MSG_SZ)
> > 
> > I'd personally appreciate a word of analysis and reassurance in the
> > commit message that this snprintf + WARN_ON inlined in every
> > location
> > where extack is used won't bloat the kernel :S
> 
> That does seem quite excessive, indeed.
> 
> I think _if_ we want this at all then I'd say that we should move
> this
> out-of-line, to have a helper function that will kasprintf() if
> necessary, and use a bit somewhere to indicate "has been allocated"
> so
> it can be freed later?
> 
> However, this will need some kind of "release extack" API for those
> places that declare their own struct on the stack, and would need to
> be
> reentrant (in the sense that old error messages may be overwritten,
> and
> must be freed at that point)...
> 
> Maybe an alternative would be to have a "can sprintf" flag, and then
> provide a buffer in another pointer? The caller can then decide
> whether
> this should be permitted, e.g. netlink_rcv_skb() could provide it,
> but
> other places maybe don't need to?
> 

I thought about all of these alternatives before i wrote this patch,
the idea here is to keep it simple and not introduce any state bits or
multiple buffers to choose from, also it is hard to guarantee that we
are going to hit the line of code that is going to free the allocated
extack message.

only the driver knows if it want a formattable message and it should
allocate it, and only the netlink core should free it, I personally
don't like such asymmetries in the kernel code, without a proper
contract or infrastructure that will guarantee correctness and no
memleaks.

> 
> Here in the patchset though, I basically found three cases using this
> capability:
> 
> +	NL_SET_ERR_MSG_MOD(extack, MLXFW_ERR_PRFX "%s",
> +			   mlxfw_fsm_state_err_str[fsm_state_err]);
> 
> This one seems somewhat unnecessary - it just takes a fixed string
> and
> adds a prefix, that may be easier this way but it wouldn't be *that*
> hard to "explode" that into a bunch of NL_SET_ERR_MSG_MOD()
> statements I
> guess.
> 
> +		NL_SET_ERR_MSG_MOD(extack, "FSM state query failed, err
> (%d)",
> +				   err);
> 
> This (and other similar instances) is pretty useless, the error
> number
> is almost certainly returned to userspace anyway?
> 
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "FSM component query failed,
> comp_name(%s) err (%d)",
> +				   comp_name, err);
> 
> This (and similar) one seems like the only one that's reasonable.
> Note
> that "comp_name" is actually just a number though.
> 

Yes, most of the use cases don't really require a formattable message.
I only did it after some internal code reviews that pointed out that
doing this will make my code more easy on the eye, since i could wrap
everything in one function that would dump dmesg and extack messages in
one shot.

I will drop this patch for now.

Thanks,
Saeed.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-11-25 23:12 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-22 22:41 [PATCH V2 net-next 0/6] mlxfw: Improve error reporting Saeed Mahameed
2019-11-22 22:41 ` [PATCH V2 net-next 1/6] netlink: Convert extack msg to a formattable buffer Saeed Mahameed
2019-11-24  0:56   ` Jakub Kicinski
2019-11-24  1:10     ` David Ahern
2019-11-24  1:20       ` David Ahern
2019-11-25  9:48     ` Johannes Berg
2019-11-25 23:12       ` Saeed Mahameed
2019-11-22 22:41 ` [PATCH V2 net-next 2/6] net/mlxfw: Generic mlx FW flash status notify Saeed Mahameed
2019-11-22 22:41 ` [PATCH V2 net-next 3/6] net/mlxfw: Improve FSM err message reporting and return codes Saeed Mahameed
2019-11-24  1:00   ` Jakub Kicinski
2019-11-22 22:41 ` [PATCH V2 net-next 4/6] net/mlxfw: Convert pr_* to dev_* in mlxfw_fsm.c Saeed Mahameed
2019-11-22 22:41 ` [PATCH V2 net-next 5/6] net/mlxfw: More error messages coverage Saeed Mahameed
2019-11-22 22:41 ` [PATCH V2 net-next 6/6] net/mlxfw: Macro for error reporting Saeed Mahameed
2019-11-24  0:48 ` [PATCH V2 net-next 0/6] mlxfw: Improve " Jakub Kicinski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).