diff mbox series

[net-next,06/14] mlx5: Use proper logging and tracing line terminations

Message ID 20200124215431.47151-7-saeedm@mellanox.com
State Superseded
Delegated to: David Miller
Headers show
Series [net-next,01/14] devlink: Force enclosing array on binary fmsg data | expand

Commit Message

Saeed Mahameed Jan. 24, 2020, 9:55 p.m. UTC 
From: Joe Perches <joe@perches.com>

netdev_err should use newline termination but mlx5_health_report
is used in a trace output function devlink_health_report where
no newline should be used.

Remove the newlines from a couple formats and add a format string
of "%s\n" to the netdev_err call to not directly output the
logging string.

Also use snprintf to avoid any possible output string overrun.

Signed-off-by: Joe Perches <joe@perches.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en/health.c    |  2 +-
 .../net/ethernet/mellanox/mlx5/core/en/reporter_rx.c   |  9 +++++----
 .../net/ethernet/mellanox/mlx5/core/en/reporter_tx.c   | 10 +++++-----
 3 files changed, 11 insertions(+), 10 deletions(-)

Comments

Joe Perches Jan. 24, 2020, 10:15 p.m. UTC | #1 
On Fri, 2020-01-24 at 21:55 +0000, Saeed Mahameed wrote:
> From: Joe Perches <joe@perches.com>

Not really from me.

> netdev_err should use newline termination but mlx5_health_report
> is used in a trace output function devlink_health_report where
> no newline should be used.
> 
> Remove the newlines from a couple formats and add a format string
> of "%s\n" to the netdev_err call to not directly output the
> logging string.
> 
> Also use snprintf to avoid any possible output string overrun.
[]
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
[]
> @@ -389,10 +389,10 @@ int mlx5e_reporter_tx_timeout(struct mlx5e_txqsq *sq)
>  	err_ctx.ctx = sq;
>  	err_ctx.recover = mlx5e_tx_reporter_timeout_recover;
>  	err_ctx.dump = mlx5e_tx_reporter_dump_sq;
> -	sprintf(err_str,
> -		"TX timeout on queue: %d, SQ: 0x%x, CQ: 0x%x, SQ Cons: 0x%x SQ Prod: 0x%x, usecs since last trans: %u\n",
> -		sq->channel->ix, sq->sqn, sq->cq.mcq.cqn, sq->cc, sq->pc,
> -		jiffies_to_usecs(jiffies - sq->txq->trans_start));
> +	snprintf(err_str, sizeof(err_str),
> +		 "TX timeout on queue: %d, SQ: 0x%x, CQ: 0x%x, SQ Cons: 0x%x SQ Prod: 0x%x, usecs since last trans: %u\n",
> +		 sq->channel->ix, sq->sqn, sq->cq.mcq.cqn, sq->cc, sq->pc,
> +		 jiffies_to_usecs(jiffies - sq->txq->trans_start));

This is not the patch I sent.
There should not be a newline here and
there was no newline in the patch I sent.

>  	return mlx5e_health_report(priv, priv->tx_reporter, err_str, &err_ctx);
>  }
Saeed Mahameed Jan. 24, 2020, 10:42 p.m. UTC | #2 
On Fri, 2020-01-24 at 14:15 -0800, Joe Perches wrote:
> On Fri, 2020-01-24 at 21:55 +0000, Saeed Mahameed wrote:
> > From: Joe Perches <joe@perches.com>
> 
> Not really from me.
> 
> > netdev_err should use newline termination but mlx5_health_report
> > is used in a trace output function devlink_health_report where
> > no newline should be used.
> > 
> > Remove the newlines from a couple formats and add a format string
> > of "%s\n" to the netdev_err call to not directly output the
> > logging string.
> > 
> > Also use snprintf to avoid any possible output string overrun.
> []
> > diff --git
> > a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
> []
> > @@ -389,10 +389,10 @@ int mlx5e_reporter_tx_timeout(struct
> > mlx5e_txqsq *sq)
> >  	err_ctx.ctx = sq;
> >  	err_ctx.recover = mlx5e_tx_reporter_timeout_recover;
> >  	err_ctx.dump = mlx5e_tx_reporter_dump_sq;
> > -	sprintf(err_str,
> > -		"TX timeout on queue: %d, SQ: 0x%x, CQ: 0x%x, SQ Cons:
> > 0x%x SQ Prod: 0x%x, usecs since last trans: %u\n",
> > -		sq->channel->ix, sq->sqn, sq->cq.mcq.cqn, sq->cc, sq-
> > >pc,
> > -		jiffies_to_usecs(jiffies - sq->txq->trans_start));
> > +	snprintf(err_str, sizeof(err_str),
> > +		 "TX timeout on queue: %d, SQ: 0x%x, CQ: 0x%x, SQ Cons:
> > 0x%x SQ Prod: 0x%x, usecs since last trans: %u\n",
> > +		 sq->channel->ix, sq->sqn, sq->cq.mcq.cqn, sq->cc, sq-
> > >pc,
> > +		 jiffies_to_usecs(jiffies - sq->txq->trans_start));
> 
> This is not the patch I sent.
> There should not be a newline here and
> there was no newline in the patch I sent.
> 

Hi Joe,

Your patch didn't apply cleanly on top of net-next-mlx5, since there
were some already accepted patches touching the same places, so i took
the liberty to make the necessary changes for it to apply..

you make the call:

1) I can re-arrange things in order to make your patch appear as it
was, Although don't see any reason of doing this, since it will be a
lot of work for nothing.. 

2) keep it as is, and fix whatever you don't like about the current
state of the patch. (remove the newline).. 

Thanks,
Saeed.
Joe Perches Jan. 24, 2020, 11:01 p.m. UTC | #3 
On Fri, 2020-01-24 at 22:42 +0000, Saeed Mahameed wrote:
> 2) keep it as is, and fix whatever you don't like about the current
> state of the patch. (remove the newline).. 

Just removing then newlines will be fine, thanks.

There were two unnecessary newlines in your posted patch,
one in rx, one in tx.

cheers, Joe
Saeed Mahameed Jan. 24, 2020, 11:22 p.m. UTC | #4 
On Fri, 2020-01-24 at 15:01 -0800, Joe Perches wrote:
> On Fri, 2020-01-24 at 22:42 +0000, Saeed Mahameed wrote:
> > 2) keep it as is, and fix whatever you don't like about the current
> > state of the patch. (remove the newline).. 
> 
> Just removing then newlines will be fine, thanks.
> 
> There were two unnecessary newlines in your posted patch,
> one in rx, one in tx.
> 

I will fix them!

Thanks!
diff mbox series

Patch

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/health.c b/drivers/net/ethernet/mellanox/mlx5/core/en/health.c
index 7178f421d2cb..68d019b27642 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/health.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/health.c
@@ -198,7 +198,7 @@  int mlx5e_health_report(struct mlx5e_priv *priv,
 			struct devlink_health_reporter *reporter, char *err_str,
 			struct mlx5e_err_ctx *err_ctx)
 {
-	netdev_err(priv->netdev, err_str);
+	netdev_err(priv->netdev, "%s\n", err_str);
 
 	if (!reporter)
 		return err_ctx->recover(&err_ctx->ctx);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_rx.c
index 9599fdd620a8..fdd81f76cd1e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_rx.c
@@ -519,8 +519,9 @@  void mlx5e_reporter_rx_timeout(struct mlx5e_rq *rq)
 	err_ctx.ctx = rq;
 	err_ctx.recover = mlx5e_rx_reporter_timeout_recover;
 	err_ctx.dump = mlx5e_rx_reporter_dump_rq;
-	sprintf(err_str, "RX timeout on channel: %d, ICOSQ: 0x%x RQ: 0x%x, CQ: 0x%x\n",
-		icosq->channel->ix, icosq->sqn, rq->rqn, rq->cq.mcq.cqn);
+	snprintf(err_str, sizeof(err_str),
+		 "RX timeout on channel: %d, ICOSQ: 0x%x RQ: 0x%x, CQ: 0x%x\n",
+		 icosq->channel->ix, icosq->sqn, rq->rqn, rq->cq.mcq.cqn);
 
 	mlx5e_health_report(priv, priv->rx_reporter, err_str, &err_ctx);
 }
@@ -534,7 +535,7 @@  void mlx5e_reporter_rq_cqe_err(struct mlx5e_rq *rq)
 	err_ctx.ctx = rq;
 	err_ctx.recover = mlx5e_rx_reporter_err_rq_cqe_recover;
 	err_ctx.dump = mlx5e_rx_reporter_dump_rq;
-	sprintf(err_str, "ERR CQE on RQ: 0x%x", rq->rqn);
+	snprintf(err_str, sizeof(err_str), "ERR CQE on RQ: 0x%x", rq->rqn);
 
 	mlx5e_health_report(priv, priv->rx_reporter, err_str, &err_ctx);
 }
@@ -548,7 +549,7 @@  void mlx5e_reporter_icosq_cqe_err(struct mlx5e_icosq *icosq)
 	err_ctx.ctx = icosq;
 	err_ctx.recover = mlx5e_rx_reporter_err_icosq_cqe_recover;
 	err_ctx.dump = mlx5e_rx_reporter_dump_icosq;
-	sprintf(err_str, "ERR CQE on ICOSQ: 0x%x", icosq->sqn);
+	snprintf(err_str, sizeof(err_str), "ERR CQE on ICOSQ: 0x%x", icosq->sqn);
 
 	mlx5e_health_report(priv, priv->rx_reporter, err_str, &err_ctx);
 }
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
index 1772c9ce3938..90a24eda70d4 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
@@ -375,7 +375,7 @@  void mlx5e_reporter_tx_err_cqe(struct mlx5e_txqsq *sq)
 	err_ctx.ctx = sq;
 	err_ctx.recover = mlx5e_tx_reporter_err_cqe_recover;
 	err_ctx.dump = mlx5e_tx_reporter_dump_sq;
-	sprintf(err_str, "ERR CQE on SQ: 0x%x", sq->sqn);
+	snprintf(err_str, sizeof(err_str), "ERR CQE on SQ: 0x%x", sq->sqn);
 
 	mlx5e_health_report(priv, priv->tx_reporter, err_str, &err_ctx);
 }
@@ -389,10 +389,10 @@  int mlx5e_reporter_tx_timeout(struct mlx5e_txqsq *sq)
 	err_ctx.ctx = sq;
 	err_ctx.recover = mlx5e_tx_reporter_timeout_recover;
 	err_ctx.dump = mlx5e_tx_reporter_dump_sq;
-	sprintf(err_str,
-		"TX timeout on queue: %d, SQ: 0x%x, CQ: 0x%x, SQ Cons: 0x%x SQ Prod: 0x%x, usecs since last trans: %u\n",
-		sq->channel->ix, sq->sqn, sq->cq.mcq.cqn, sq->cc, sq->pc,
-		jiffies_to_usecs(jiffies - sq->txq->trans_start));
+	snprintf(err_str, sizeof(err_str),
+		 "TX timeout on queue: %d, SQ: 0x%x, CQ: 0x%x, SQ Cons: 0x%x SQ Prod: 0x%x, usecs since last trans: %u\n",
+		 sq->channel->ix, sq->sqn, sq->cq.mcq.cqn, sq->cc, sq->pc,
+		 jiffies_to_usecs(jiffies - sq->txq->trans_start));
 
 	return mlx5e_health_report(priv, priv->tx_reporter, err_str, &err_ctx);
 }