| Message ID | 20241021022901.318647-1-rosenp@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: bnxt: use ethtool string helpers | expand |
On Sun, Oct 20, 2024 at 07:29:01PM -0700, Rosen Penev wrote: > Avoids having to use manual pointer manipulation. > > Signed-off-by: Rosen Penev <rosenp@gmail.com> Reviewed-by: Simon Horman <horms@kernel.org>
On Sun, Oct 20, 2024 at 7:29 PM Rosen Penev <rosenp@gmail.com> wrote: > > Avoids having to use manual pointer manipulation. > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > --- > .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 110 ++++++++---------- > 1 file changed, 50 insertions(+), 60 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > index f71cc8188b4e..84d468ad3c8e 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > @@ -713,18 +713,17 @@ static void bnxt_get_strings(struct net_device *dev, u32 stringset, u8 *buf) > for (i = 0; i < bp->cp_nr_rings; i++) { > if (is_rx_ring(bp, i)) { > num_str = NUM_RING_RX_HW_STATS; > - for (j = 0; j < num_str; j++) { > - sprintf(buf, "[%d]: %s", i, > + for (j = 0; j < num_str; j++) > + ethtool_sprintf( > + &buf, "[%d]: %s", i, If you combine this line with the line above, I think it will look better: ethtool_sprintf(&buf, "[%d]: %s", i, checkpatch.pl may warn because the next line won't line up, but I think it will look better and it reduces the number of lines. The same comment applies to other similar spots in the patch. Thanks. > bnxt_ring_rx_stats_str[j]); > - buf += ETH_GSTRING_LEN; > - }
On Mon, Oct 21, 2024 at 1:26 PM Michael Chan <michael.chan@broadcom.com> wrote: > > On Sun, Oct 20, 2024 at 7:29 PM Rosen Penev <rosenp@gmail.com> wrote: > > > > Avoids having to use manual pointer manipulation. > > > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > > --- > > .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 110 ++++++++---------- > > 1 file changed, 50 insertions(+), 60 deletions(-) > > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > index f71cc8188b4e..84d468ad3c8e 100644 > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > @@ -713,18 +713,17 @@ static void bnxt_get_strings(struct net_device *dev, u32 stringset, u8 *buf) > > for (i = 0; i < bp->cp_nr_rings; i++) { > > if (is_rx_ring(bp, i)) { > > num_str = NUM_RING_RX_HW_STATS; > > - for (j = 0; j < num_str; j++) { > > - sprintf(buf, "[%d]: %s", i, > > + for (j = 0; j < num_str; j++) > > + ethtool_sprintf( > > + &buf, "[%d]: %s", i, > > If you combine this line with the line above, I think it will look better: > > ethtool_sprintf(&buf, "[%d]: %s", i, > > checkpatch.pl may warn because the next line won't line up, but I > think it will look better and it reduces the number of lines. The > same comment applies to other similar spots in the patch. Thanks. I use git-clang-format for this stuff. The only real way to satisfy everything is to use a temporary variable like for (i = 0; i < BNXT_NUM_PORT_STATS; i++) { const char *str = bnxt_port_stats_arr[i].string; ethtool_puts(&buf, str); } to make everything small. > > > bnxt_ring_rx_stats_str[j]); > > - buf += ETH_GSTRING_LEN; > > - }
On Mon, Oct 21, 2024 at 2:05 PM Rosen Penev <rosenp@gmail.com> wrote: > The only real way to satisfy everything is to use a temporary variable like > > for (i = 0; i < BNXT_NUM_PORT_STATS; i++) { > const char *str = bnxt_port_stats_arr[i].string; > > ethtool_puts(&buf, str); > } > > to make everything small. This looks fine too. The actual function call looks clean on one line.
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c index f71cc8188b4e..84d468ad3c8e 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c @@ -713,18 +713,17 @@ static void bnxt_get_strings(struct net_device *dev, u32 stringset, u8 *buf) for (i = 0; i < bp->cp_nr_rings; i++) { if (is_rx_ring(bp, i)) { num_str = NUM_RING_RX_HW_STATS; - for (j = 0; j < num_str; j++) { - sprintf(buf, "[%d]: %s", i, + for (j = 0; j < num_str; j++) + ethtool_sprintf( + &buf, "[%d]: %s", i, bnxt_ring_rx_stats_str[j]); - buf += ETH_GSTRING_LEN; - } } if (is_tx_ring(bp, i)) { num_str = NUM_RING_TX_HW_STATS; for (j = 0; j < num_str; j++) { - sprintf(buf, "[%d]: %s", i, + ethtool_sprintf( + &buf, "[%d]: %s", i, bnxt_ring_tx_stats_str[j]); - buf += ETH_GSTRING_LEN; } } num_str = bnxt_get_num_tpa_ring_stats(bp); @@ -736,81 +735,72 @@ static void bnxt_get_strings(struct net_device *dev, u32 stringset, u8 *buf) else str = bnxt_ring_tpa_stats_str; - for (j = 0; j < num_str; j++) { - sprintf(buf, "[%d]: %s", i, str[j]); - buf += ETH_GSTRING_LEN; - } + for (j = 0; j < num_str; j++) + ethtool_sprintf(&buf, "[%d]: %s", i, str[j]); skip_tpa_stats: if (is_rx_ring(bp, i)) { num_str = NUM_RING_RX_SW_STATS; - for (j = 0; j < num_str; j++) { - sprintf(buf, "[%d]: %s", i, + for (j = 0; j < num_str; j++) + ethtool_sprintf( + &buf, "[%d]: %s", i, bnxt_rx_sw_stats_str[j]); - buf += ETH_GSTRING_LEN; - } } num_str = NUM_RING_CMN_SW_STATS; - for (j = 0; j < num_str; j++) { - sprintf(buf, "[%d]: %s", i, - bnxt_cmn_sw_stats_str[j]); - buf += ETH_GSTRING_LEN; - } - } - for (i = 0; i < BNXT_NUM_RING_ERR_STATS; i++) { - strscpy(buf, bnxt_ring_err_stats_arr[i], ETH_GSTRING_LEN); - buf += ETH_GSTRING_LEN; + for (j = 0; j < num_str; j++) + ethtool_sprintf(&buf, "[%d]: %s", i, + bnxt_cmn_sw_stats_str[j]); } + for (i = 0; i < BNXT_NUM_RING_ERR_STATS; i++) + ethtool_puts(&buf, bnxt_ring_err_stats_arr[i]); + + if (bp->flags & BNXT_FLAG_PORT_STATS) + for (i = 0; i < BNXT_NUM_PORT_STATS; i++) + ethtool_puts(&buf, + bnxt_port_stats_arr[i].string); - if (bp->flags & BNXT_FLAG_PORT_STATS) { - for (i = 0; i < BNXT_NUM_PORT_STATS; i++) { - strcpy(buf, bnxt_port_stats_arr[i].string); - buf += ETH_GSTRING_LEN; - } - } if (bp->flags & BNXT_FLAG_PORT_STATS_EXT) { u32 len; len = min_t(u32, bp->fw_rx_stats_ext_size, ARRAY_SIZE(bnxt_port_stats_ext_arr)); - for (i = 0; i < len; i++) { - strcpy(buf, bnxt_port_stats_ext_arr[i].string); - buf += ETH_GSTRING_LEN; - } + for (i = 0; i < len; i++) + ethtool_puts(&buf, + bnxt_port_stats_ext_arr[i].string); + len = min_t(u32, bp->fw_tx_stats_ext_size, ARRAY_SIZE(bnxt_tx_port_stats_ext_arr)); - for (i = 0; i < len; i++) { - strcpy(buf, - bnxt_tx_port_stats_ext_arr[i].string); - buf += ETH_GSTRING_LEN; - } + for (i = 0; i < len; i++) + ethtool_puts( + &buf, + bnxt_tx_port_stats_ext_arr[i].string); + if (bp->pri2cos_valid) { - for (i = 0; i < 8; i++) { - strcpy(buf, - bnxt_rx_bytes_pri_arr[i].string); - buf += ETH_GSTRING_LEN; - } - for (i = 0; i < 8; i++) { - strcpy(buf, - bnxt_rx_pkts_pri_arr[i].string); - buf += ETH_GSTRING_LEN; - } - for (i = 0; i < 8; i++) { - strcpy(buf, - bnxt_tx_bytes_pri_arr[i].string); - buf += ETH_GSTRING_LEN; - } - for (i = 0; i < 8; i++) { - strcpy(buf, - bnxt_tx_pkts_pri_arr[i].string); - buf += ETH_GSTRING_LEN; - } + for (i = 0; i < 8; i++) + ethtool_puts( + &buf, + bnxt_rx_bytes_pri_arr[i].string); + + for (i = 0; i < 8; i++) + ethtool_puts( + &buf, + bnxt_rx_pkts_pri_arr[i].string); + + for (i = 0; i < 8; i++) + ethtool_puts( + &buf, + bnxt_tx_bytes_pri_arr[i].string); + + for (i = 0; i < 8; i++) + ethtool_puts( + &buf, + bnxt_tx_pkts_pri_arr[i].string); } } break; case ETH_SS_TEST: if (bp->num_tests) - memcpy(buf, bp->test_info->string, - bp->num_tests * ETH_GSTRING_LEN); + for (i = 0; i < bp->num_tests; i++) + ethtool_puts(&buf, bp->test_info->string[i]); break; default: netdev_err(bp->dev, "bnxt_get_strings invalid request %x\n",
Avoids having to use manual pointer manipulation. Signed-off-by: Rosen Penev <rosenp@gmail.com> --- .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 110 ++++++++---------- 1 file changed, 50 insertions(+), 60 deletions(-)