* [PATCH net-next] net: broadcom: use ethtool string helpers
@ 2024-10-23 1:27 Rosen Penev
2024-10-24 11:47 ` Simon Horman
2024-10-29 23:03 ` Jakub Kicinski
0 siblings, 2 replies; 9+ messages in thread
From: Rosen Penev @ 2024-10-23 1:27 UTC (permalink / raw)
To: netdev
Cc: Justin Chen, Florian Fainelli, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Broadcom internal kernel review list, Rafał Miłecki,
Sudarsana Kalluru, Manish Chopra, Doug Berger, Rosen Penev,
Jacob Keller, Sabrina Dubroca, Uwe Kleine-König, open list
The latter is the preferred way to copy ethtool strings.
Avoids manually incrementing the pointer. Cleans up the code quite well.
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
.../ethernet/broadcom/asp2/bcmasp_ethtool.c | 6 +-
drivers/net/ethernet/broadcom/bcm63xx_enet.c | 12 ++--
drivers/net/ethernet/broadcom/bcmsysport.c | 20 ++----
drivers/net/ethernet/broadcom/bgmac.c | 3 +-
.../ethernet/broadcom/bnx2x/bnx2x_ethtool.c | 66 ++++++++-----------
.../net/ethernet/broadcom/genet/bcmgenet.c | 6 +-
6 files changed, 47 insertions(+), 66 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_ethtool.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_ethtool.c
index 67928b5d8a26..9da5ae29a105 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp_ethtool.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_ethtool.c
@@ -101,14 +101,14 @@ static int bcmasp_get_sset_count(struct net_device *dev, int string_set)
static void bcmasp_get_strings(struct net_device *dev, u32 stringset,
u8 *data)
{
+ const char *str;
unsigned int i;
switch (stringset) {
case ETH_SS_STATS:
for (i = 0; i < BCMASP_STATS_LEN; i++) {
- memcpy(data + i * ETH_GSTRING_LEN,
- bcmasp_gstrings_stats[i].stat_string,
- ETH_GSTRING_LEN);
+ str = bcmasp_gstrings_stats[i].stat_string;
+ ethtool_puts(&data, str);
}
break;
default:
diff --git a/drivers/net/ethernet/broadcom/bcm63xx_enet.c b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
index e5e03aaa49f9..65e3a0656a4c 100644
--- a/drivers/net/ethernet/broadcom/bcm63xx_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
@@ -1339,14 +1339,14 @@ static int bcm_enet_get_sset_count(struct net_device *netdev,
static void bcm_enet_get_strings(struct net_device *netdev,
u32 stringset, u8 *data)
{
+ const char *str;
int i;
switch (stringset) {
case ETH_SS_STATS:
for (i = 0; i < BCM_ENET_STATS_LEN; i++) {
- memcpy(data + i * ETH_GSTRING_LEN,
- bcm_enet_gstrings_stats[i].stat_string,
- ETH_GSTRING_LEN);
+ str = bcm_enet_gstrings_stats[i].stat_string;
+ ethtool_puts(&data, str);
}
break;
}
@@ -2503,14 +2503,14 @@ static const struct bcm_enet_stats bcm_enetsw_gstrings_stats[] = {
static void bcm_enetsw_get_strings(struct net_device *netdev,
u32 stringset, u8 *data)
{
+ const char *str;
int i;
switch (stringset) {
case ETH_SS_STATS:
for (i = 0; i < BCM_ENETSW_STATS_LEN; i++) {
- memcpy(data + i * ETH_GSTRING_LEN,
- bcm_enetsw_gstrings_stats[i].stat_string,
- ETH_GSTRING_LEN);
+ str = bcm_enetsw_gstrings_stats[i].stat_string;
+ ethtool_puts(&data, str);
}
break;
}
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 0b7088ca4822..78058aa4e008 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -370,32 +370,22 @@ static void bcm_sysport_get_strings(struct net_device *dev,
{
struct bcm_sysport_priv *priv = netdev_priv(dev);
const struct bcm_sysport_stats *s;
- char buf[128];
- int i, j;
+ int i;
switch (stringset) {
case ETH_SS_STATS:
- for (i = 0, j = 0; i < BCM_SYSPORT_STATS_LEN; i++) {
+ for (i = 0; i < BCM_SYSPORT_STATS_LEN; i++) {
s = &bcm_sysport_gstrings_stats[i];
if (priv->is_lite &&
!bcm_sysport_lite_stat_valid(s->type))
continue;
- memcpy(data + j * ETH_GSTRING_LEN, s->stat_string,
- ETH_GSTRING_LEN);
- j++;
+ ethtool_puts(&data, s->stat_string);
}
for (i = 0; i < dev->num_tx_queues; i++) {
- snprintf(buf, sizeof(buf), "txq%d_packets", i);
- memcpy(data + j * ETH_GSTRING_LEN, buf,
- ETH_GSTRING_LEN);
- j++;
-
- snprintf(buf, sizeof(buf), "txq%d_bytes", i);
- memcpy(data + j * ETH_GSTRING_LEN, buf,
- ETH_GSTRING_LEN);
- j++;
+ ethtool_sprintf(&data, "txq%d_packets", i);
+ ethtool_sprintf(&data, "txq%d_bytes", i);
}
break;
default:
diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 2599ffe46e27..18badb51a2f8 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1367,8 +1367,7 @@ static void bgmac_get_strings(struct net_device *dev, u32 stringset,
return;
for (i = 0; i < BGMAC_STATS_LEN; i++)
- strscpy(data + i * ETH_GSTRING_LEN,
- bgmac_get_strings_stats[i].name, ETH_GSTRING_LEN);
+ ethtool_puts(&data, bgmac_get_strings_stats[i].name);
}
static void bgmac_get_ethtool_stats(struct net_device *dev,
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
index adf7b6b94941..c875faffbf1c 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
@@ -39,34 +39,34 @@ static const struct {
int size;
char string[ETH_GSTRING_LEN];
} bnx2x_q_stats_arr[] = {
-/* 1 */ { Q_STATS_OFFSET32(total_bytes_received_hi), 8, "[%s]: rx_bytes" },
+/* 1 */ { Q_STATS_OFFSET32(total_bytes_received_hi), 8, "[%d]: rx_bytes" },
{ Q_STATS_OFFSET32(total_unicast_packets_received_hi),
- 8, "[%s]: rx_ucast_packets" },
+ 8, "[%d]: rx_ucast_packets" },
{ Q_STATS_OFFSET32(total_multicast_packets_received_hi),
- 8, "[%s]: rx_mcast_packets" },
+ 8, "[%d]: rx_mcast_packets" },
{ Q_STATS_OFFSET32(total_broadcast_packets_received_hi),
- 8, "[%s]: rx_bcast_packets" },
- { Q_STATS_OFFSET32(no_buff_discard_hi), 8, "[%s]: rx_discards" },
+ 8, "[%d]: rx_bcast_packets" },
+ { Q_STATS_OFFSET32(no_buff_discard_hi), 8, "[%d]: rx_discards" },
{ Q_STATS_OFFSET32(rx_err_discard_pkt),
- 4, "[%s]: rx_phy_ip_err_discards"},
+ 4, "[%d]: rx_phy_ip_err_discards"},
{ Q_STATS_OFFSET32(rx_skb_alloc_failed),
- 4, "[%s]: rx_skb_alloc_discard" },
- { Q_STATS_OFFSET32(hw_csum_err), 4, "[%s]: rx_csum_offload_errors" },
- { Q_STATS_OFFSET32(driver_xoff), 4, "[%s]: tx_exhaustion_events" },
- { Q_STATS_OFFSET32(total_bytes_transmitted_hi), 8, "[%s]: tx_bytes" },
+ 4, "[%d]: rx_skb_alloc_discard" },
+ { Q_STATS_OFFSET32(hw_csum_err), 4, "[%d]: rx_csum_offload_errors" },
+ { Q_STATS_OFFSET32(driver_xoff), 4, "[%d]: tx_exhaustion_events" },
+ { Q_STATS_OFFSET32(total_bytes_transmitted_hi), 8, "[%d]: tx_bytes" },
/* 10 */{ Q_STATS_OFFSET32(total_unicast_packets_transmitted_hi),
- 8, "[%s]: tx_ucast_packets" },
+ 8, "[%d]: tx_ucast_packets" },
{ Q_STATS_OFFSET32(total_multicast_packets_transmitted_hi),
- 8, "[%s]: tx_mcast_packets" },
+ 8, "[%d]: tx_mcast_packets" },
{ Q_STATS_OFFSET32(total_broadcast_packets_transmitted_hi),
- 8, "[%s]: tx_bcast_packets" },
+ 8, "[%d]: tx_bcast_packets" },
{ Q_STATS_OFFSET32(total_tpa_aggregations_hi),
- 8, "[%s]: tpa_aggregations" },
+ 8, "[%d]: tpa_aggregations" },
{ Q_STATS_OFFSET32(total_tpa_aggregated_frames_hi),
- 8, "[%s]: tpa_aggregated_frames"},
- { Q_STATS_OFFSET32(total_tpa_bytes_hi), 8, "[%s]: tpa_bytes"},
+ 8, "[%d]: tpa_aggregated_frames"},
+ { Q_STATS_OFFSET32(total_tpa_bytes_hi), 8, "[%d]: tpa_bytes"},
{ Q_STATS_OFFSET32(driver_filtered_tx_pkt),
- 4, "[%s]: driver_filtered_tx_pkt" }
+ 4, "[%d]: driver_filtered_tx_pkt" }
};
#define BNX2X_NUM_Q_STATS ARRAY_SIZE(bnx2x_q_stats_arr)
@@ -3184,32 +3184,24 @@ static u32 bnx2x_get_private_flags(struct net_device *dev)
static void bnx2x_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
{
struct bnx2x *bp = netdev_priv(dev);
- int i, j, k, start;
- char queue_name[MAX_QUEUE_NAME_LEN+1];
+ const char *str;
+ int i, j, start;
switch (stringset) {
case ETH_SS_STATS:
- k = 0;
if (is_multi(bp)) {
for_each_eth_queue(bp, i) {
- memset(queue_name, 0, sizeof(queue_name));
- snprintf(queue_name, sizeof(queue_name),
- "%d", i);
- for (j = 0; j < BNX2X_NUM_Q_STATS; j++)
- snprintf(buf + (k + j)*ETH_GSTRING_LEN,
- ETH_GSTRING_LEN,
- bnx2x_q_stats_arr[j].string,
- queue_name);
- k += BNX2X_NUM_Q_STATS;
+ for (j = 0; j < BNX2X_NUM_Q_STATS; j++) {
+ str = bnx2x_q_stats_arr[j].string;
+ ethtool_sprintf(&buf, str, i);
+ }
}
}
- for (i = 0, j = 0; i < BNX2X_NUM_STATS; i++) {
+ for (i = 0; i < BNX2X_NUM_STATS; i++) {
if (HIDE_PORT_STAT(bp) && IS_PORT_STAT(i))
continue;
- strcpy(buf + (k + j)*ETH_GSTRING_LEN,
- bnx2x_stats_arr[i].string);
- j++;
+ ethtool_puts(&buf, bnx2x_stats_arr[i].string);
}
break;
@@ -3220,13 +3212,13 @@ static void bnx2x_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
start = 0;
else
start = 4;
- memcpy(buf, bnx2x_tests_str_arr + start,
- ETH_GSTRING_LEN * BNX2X_NUM_TESTS(bp));
+ for (i = start; i < BNX2X_NUM_TESTS(bp); i++)
+ ethtool_puts(&buf, bnx2x_tests_str_arr[i]);
break;
case ETH_SS_PRIV_FLAGS:
- memcpy(buf, bnx2x_private_arr,
- ETH_GSTRING_LEN * BNX2X_PRI_FLAG_LEN);
+ for (i = 0; i < BNX2X_PRI_FLAG_LEN; i++)
+ ethtool_puts(&buf, bnx2x_private_arr[i]);
break;
}
}
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 10966ab15373..244c5b9523b4 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1144,14 +1144,14 @@ static int bcmgenet_get_sset_count(struct net_device *dev, int string_set)
static void bcmgenet_get_strings(struct net_device *dev, u32 stringset,
u8 *data)
{
+ const char *str;
int i;
switch (stringset) {
case ETH_SS_STATS:
for (i = 0; i < BCMGENET_STATS_LEN; i++) {
- memcpy(data + i * ETH_GSTRING_LEN,
- bcmgenet_gstrings_stats[i].stat_string,
- ETH_GSTRING_LEN);
+ str = bcmgenet_gstrings_stats[i].stat_string;
+ ethtool_puts(&data, str);
}
break;
}
--
2.47.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] net: broadcom: use ethtool string helpers
2024-10-23 1:27 [PATCH net-next] net: broadcom: use ethtool string helpers Rosen Penev
@ 2024-10-24 11:47 ` Simon Horman
2024-10-29 23:03 ` Jakub Kicinski
1 sibling, 0 replies; 9+ messages in thread
From: Simon Horman @ 2024-10-24 11:47 UTC (permalink / raw)
To: Rosen Penev
Cc: netdev, Justin Chen, Florian Fainelli, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Broadcom internal kernel review list, Rafał Miłecki,
Sudarsana Kalluru, Manish Chopra, Doug Berger, Jacob Keller,
Sabrina Dubroca, Uwe Kleine-König, open list
On Tue, Oct 22, 2024 at 06:27:34PM -0700, Rosen Penev wrote:
> The latter is the preferred way to copy ethtool strings.
>
> Avoids manually incrementing the pointer. Cleans up the code quite well.
>
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
Reviewed-by: Simon Horman <horms@kernel.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] net: broadcom: use ethtool string helpers
2024-10-23 1:27 [PATCH net-next] net: broadcom: use ethtool string helpers Rosen Penev
2024-10-24 11:47 ` Simon Horman
@ 2024-10-29 23:03 ` Jakub Kicinski
2024-10-29 23:43 ` Rosen Penev
1 sibling, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2024-10-29 23:03 UTC (permalink / raw)
To: Rosen Penev
Cc: netdev, Justin Chen, Florian Fainelli, Andrew Lunn,
David S. Miller, Eric Dumazet, Paolo Abeni,
Broadcom internal kernel review list, Rafał Miłecki,
Sudarsana Kalluru, Manish Chopra, Doug Berger, Jacob Keller,
Sabrina Dubroca, Uwe Kleine-König, open list
On Tue, 22 Oct 2024 18:27:34 -0700 Rosen Penev wrote:
> @@ -3220,13 +3212,13 @@ static void bnx2x_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
> start = 0;
> else
> start = 4;
> - memcpy(buf, bnx2x_tests_str_arr + start,
> - ETH_GSTRING_LEN * BNX2X_NUM_TESTS(bp));
> + for (i = start; i < BNX2X_NUM_TESTS(bp); i++)
> + ethtool_puts(&buf, bnx2x_tests_str_arr[i]);
I don't think this is equivalent.
Also, please split bnx2x to a separate patch, the other drivers in this
patch IIUC are small embedded ones, the bnx2x is an "enterprise
product".
--
pw-bot: cr
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] net: broadcom: use ethtool string helpers
2024-10-29 23:03 ` Jakub Kicinski
@ 2024-10-29 23:43 ` Rosen Penev
2024-10-29 23:54 ` Jacob Keller
2024-10-30 0:37 ` Jakub Kicinski
0 siblings, 2 replies; 9+ messages in thread
From: Rosen Penev @ 2024-10-29 23:43 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, Justin Chen, Florian Fainelli, Andrew Lunn,
David S. Miller, Eric Dumazet, Paolo Abeni,
Broadcom internal kernel review list, Rafał Miłecki,
Sudarsana Kalluru, Manish Chopra, Doug Berger, Jacob Keller,
Sabrina Dubroca, Uwe Kleine-König, open list
On Tue, Oct 29, 2024 at 4:03 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 22 Oct 2024 18:27:34 -0700 Rosen Penev wrote:
> > @@ -3220,13 +3212,13 @@ static void bnx2x_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
> > start = 0;
> > else
> > start = 4;
> > - memcpy(buf, bnx2x_tests_str_arr + start,
> > - ETH_GSTRING_LEN * BNX2X_NUM_TESTS(bp));
> > + for (i = start; i < BNX2X_NUM_TESTS(bp); i++)
> > + ethtool_puts(&buf, bnx2x_tests_str_arr[i]);
>
> I don't think this is equivalent.
What's wrong here?
>
> Also, please split bnx2x to a separate patch, the other drivers in this
> patch IIUC are small embedded ones, the bnx2x is an "enterprise
> product".
> --
> pw-bot: cr
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] net: broadcom: use ethtool string helpers
2024-10-29 23:43 ` Rosen Penev
@ 2024-10-29 23:54 ` Jacob Keller
2024-10-30 0:07 ` Rosen Penev
2024-10-30 0:37 ` Jakub Kicinski
1 sibling, 1 reply; 9+ messages in thread
From: Jacob Keller @ 2024-10-29 23:54 UTC (permalink / raw)
To: Rosen Penev, Jakub Kicinski
Cc: netdev, Justin Chen, Florian Fainelli, Andrew Lunn,
David S. Miller, Eric Dumazet, Paolo Abeni,
Broadcom internal kernel review list, Rafał Miłecki,
Sudarsana Kalluru, Manish Chopra, Doug Berger, Sabrina Dubroca,
Uwe Kleine-König, open list
On 10/29/2024 4:43 PM, Rosen Penev wrote:
> On Tue, Oct 29, 2024 at 4:03 PM Jakub Kicinski <kuba@kernel.org> wrote:
>>
>> On Tue, 22 Oct 2024 18:27:34 -0700 Rosen Penev wrote:
>>> @@ -3220,13 +3212,13 @@ static void bnx2x_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
>>> start = 0;
>>> else
>>> start = 4;
>>> - memcpy(buf, bnx2x_tests_str_arr + start,
>>> - ETH_GSTRING_LEN * BNX2X_NUM_TESTS(bp));
>>> + for (i = start; i < BNX2X_NUM_TESTS(bp); i++)
>>> + ethtool_puts(&buf, bnx2x_tests_str_arr[i]);
>>
>> I don't think this is equivalent.
> What's wrong here?
I was trying to figure that out too...
I guess the memcpy does everything all at once and this does it via
iteration...?
memcpy would actually result in copying the padding between strings in
the bnx2x_tests_str_arr, while the ethtool_puts turns into strscpy which
doesn't pad the tail of the buffer with zeros?
>>
>> Also, please split bnx2x to a separate patch, the other drivers in this
>> patch IIUC are small embedded ones, the bnx2x is an "enterprise
>> product".
>> --
>> pw-bot: cr
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] net: broadcom: use ethtool string helpers
2024-10-29 23:54 ` Jacob Keller
@ 2024-10-30 0:07 ` Rosen Penev
0 siblings, 0 replies; 9+ messages in thread
From: Rosen Penev @ 2024-10-30 0:07 UTC (permalink / raw)
To: Jacob Keller
Cc: Jakub Kicinski, netdev, Justin Chen, Florian Fainelli,
Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni,
Broadcom internal kernel review list, Rafał Miłecki,
Sudarsana Kalluru, Manish Chopra, Doug Berger, Sabrina Dubroca,
Uwe Kleine-König, open list
On Tue, Oct 29, 2024 at 4:55 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
>
>
>
> On 10/29/2024 4:43 PM, Rosen Penev wrote:
> > On Tue, Oct 29, 2024 at 4:03 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >>
> >> On Tue, 22 Oct 2024 18:27:34 -0700 Rosen Penev wrote:
> >>> @@ -3220,13 +3212,13 @@ static void bnx2x_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
> >>> start = 0;
> >>> else
> >>> start = 4;
> >>> - memcpy(buf, bnx2x_tests_str_arr + start,
> >>> - ETH_GSTRING_LEN * BNX2X_NUM_TESTS(bp));
> >>> + for (i = start; i < BNX2X_NUM_TESTS(bp); i++)
> >>> + ethtool_puts(&buf, bnx2x_tests_str_arr[i]);
> >>
> >> I don't think this is equivalent.
> > What's wrong here?
>
> I was trying to figure that out too...
>
> I guess the memcpy does everything all at once and this does it via
> iteration...?
>
> memcpy would actually result in copying the padding between strings in
> the bnx2x_tests_str_arr, while the ethtool_puts turns into strscpy which
> doesn't pad the tail of the buffer with zeros?
I'll remove the change in the next version.
Still doesn't make much sense.
>
> >>
> >> Also, please split bnx2x to a separate patch, the other drivers in this
> >> patch IIUC are small embedded ones, the bnx2x is an "enterprise
> >> product".
> >> --
> >> pw-bot: cr
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] net: broadcom: use ethtool string helpers
2024-10-29 23:43 ` Rosen Penev
2024-10-29 23:54 ` Jacob Keller
@ 2024-10-30 0:37 ` Jakub Kicinski
2024-10-30 20:31 ` Jacob Keller
1 sibling, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2024-10-30 0:37 UTC (permalink / raw)
To: Rosen Penev
Cc: netdev, Justin Chen, Florian Fainelli, Andrew Lunn,
David S. Miller, Eric Dumazet, Paolo Abeni,
Broadcom internal kernel review list, Rafał Miłecki,
Sudarsana Kalluru, Manish Chopra, Doug Berger, Jacob Keller,
Sabrina Dubroca, Uwe Kleine-König, open list
On Tue, 29 Oct 2024 16:43:15 -0700 Rosen Penev wrote:
> > > - memcpy(buf, bnx2x_tests_str_arr + start,
> > > - ETH_GSTRING_LEN * BNX2X_NUM_TESTS(bp));
> > > + for (i = start; i < BNX2X_NUM_TESTS(bp); i++)
> > > + ethtool_puts(&buf, bnx2x_tests_str_arr[i]);
> >
> > I don't think this is equivalent.
> What's wrong here?
We used to copy ETH_GSTRING_LEN * BNX2X_NUM_TESTS(bp)
but i will no only go from start to BNX2X_NUM_TESTS(bp)
IOW the copied length is ETH_GSTRING_LEN * (BNX2X_NUM_TESTS(bp) - start)
No?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] net: broadcom: use ethtool string helpers
2024-10-30 0:37 ` Jakub Kicinski
@ 2024-10-30 20:31 ` Jacob Keller
2024-10-30 20:50 ` Rosen Penev
0 siblings, 1 reply; 9+ messages in thread
From: Jacob Keller @ 2024-10-30 20:31 UTC (permalink / raw)
To: Jakub Kicinski, Rosen Penev
Cc: netdev, Justin Chen, Florian Fainelli, Andrew Lunn,
David S. Miller, Eric Dumazet, Paolo Abeni,
Broadcom internal kernel review list, Rafał Miłecki,
Sudarsana Kalluru, Manish Chopra, Doug Berger, Sabrina Dubroca,
Uwe Kleine-König, open list
On 10/29/2024 5:37 PM, Jakub Kicinski wrote:
> On Tue, 29 Oct 2024 16:43:15 -0700 Rosen Penev wrote:
>>>> - memcpy(buf, bnx2x_tests_str_arr + start,
>>>> - ETH_GSTRING_LEN * BNX2X_NUM_TESTS(bp));
>>>> + for (i = start; i < BNX2X_NUM_TESTS(bp); i++)
>>>> + ethtool_puts(&buf, bnx2x_tests_str_arr[i]);
>>>
>>> I don't think this is equivalent.
>> What's wrong here?
>
> We used to copy ETH_GSTRING_LEN * BNX2X_NUM_TESTS(bp)
> but i will no only go from start to BNX2X_NUM_TESTS(bp)
> IOW the copied length is ETH_GSTRING_LEN * (BNX2X_NUM_TESTS(bp) - start)
> No?
Hmm. Yea that's right. I'm guessing BNX2X_NUM_TESTS(bp) changes the
number of tests based on what start would be...
Probably we could use a static size of the string array instead of
BNX2X_NUM_TESTS(bp), and that would fix things, but this specific change
needs more careful review of the surrounding code.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next] net: broadcom: use ethtool string helpers
2024-10-30 20:31 ` Jacob Keller
@ 2024-10-30 20:50 ` Rosen Penev
0 siblings, 0 replies; 9+ messages in thread
From: Rosen Penev @ 2024-10-30 20:50 UTC (permalink / raw)
To: Jacob Keller
Cc: Jakub Kicinski, netdev, Justin Chen, Florian Fainelli,
Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni,
Broadcom internal kernel review list, Rafał Miłecki,
Sudarsana Kalluru, Manish Chopra, Doug Berger, Sabrina Dubroca,
Uwe Kleine-König, open list
On Wed, Oct 30, 2024 at 1:31 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
>
>
>
> On 10/29/2024 5:37 PM, Jakub Kicinski wrote:
> > On Tue, 29 Oct 2024 16:43:15 -0700 Rosen Penev wrote:
> >>>> - memcpy(buf, bnx2x_tests_str_arr + start,
> >>>> - ETH_GSTRING_LEN * BNX2X_NUM_TESTS(bp));
> >>>> + for (i = start; i < BNX2X_NUM_TESTS(bp); i++)
> >>>> + ethtool_puts(&buf, bnx2x_tests_str_arr[i]);
> >>>
> >>> I don't think this is equivalent.
> >> What's wrong here?
> >
> > We used to copy ETH_GSTRING_LEN * BNX2X_NUM_TESTS(bp)
> > but i will no only go from start to BNX2X_NUM_TESTS(bp)
> > IOW the copied length is ETH_GSTRING_LEN * (BNX2X_NUM_TESTS(bp) - start)
> > No?
>
> Hmm. Yea that's right. I'm guessing BNX2X_NUM_TESTS(bp) changes the
> number of tests based on what start would be...
makes sense now.
Loop iteration variable should be ARRAY_SIZE(bnx2x_tests_str_arr),
which is BNX2X_NUM_TESTS_SF.
>
> Probably we could use a static size of the string array instead of
> BNX2X_NUM_TESTS(bp), and that would fix things, but this specific change
> needs more careful review of the surrounding code.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-10-30 20:50 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-23 1:27 [PATCH net-next] net: broadcom: use ethtool string helpers Rosen Penev
2024-10-24 11:47 ` Simon Horman
2024-10-29 23:03 ` Jakub Kicinski
2024-10-29 23:43 ` Rosen Penev
2024-10-29 23:54 ` Jacob Keller
2024-10-30 0:07 ` Rosen Penev
2024-10-30 0:37 ` Jakub Kicinski
2024-10-30 20:31 ` Jacob Keller
2024-10-30 20:50 ` Rosen Penev
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).