| Message ID | 20250410172247.1932-1-kalavakunta.hari.prasad@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v3] net: ncsi: Fix GCPS 64-bit member variables | expand |
Hello Hari, On Thu, Apr 10, 2025 at 10:22:47AM -0700, kalavakunta.hari.prasad@gmail.com wrote: > From: Hari Kalavakunta <kalavakunta.hari.prasad@gmail.com> > > Correct Get Controller Packet Statistics (GCPS) 64-bit wide member > variables, as per DSP0222 v1.0.0 and forward specs. The Driver currently > collects these stats, but they are yet to be exposed to the user. > Therefore, no user impact. > > Statistics fixes: > Total Bytes Received (byte range 28..35) > Total Bytes Transmitted (byte range 36..43) > Total Unicast Packets Received (byte range 44..51) > Total Multicast Packets Received (byte range 52..59) > Total Broadcast Packets Received (byte range 60..67) > Total Unicast Packets Transmitted (byte range 68..75) > Total Multicast Packets Transmitted (byte range 76..83) > Total Broadcast Packets Transmitted (byte range 84..91) > Valid Bytes Received (byte range 204..11) > > v2: > - __be64 for all 64 bit GCPS counters > > v3: > - be64_to_cpup() instead of be64_to_cpu() Usually the changelog should go after --- so it's not included in the final commit message when merged. I hope in this case the maintainers will take care of this manually so no need to resend unless they ask to. Other than that, Reviewed-by: Paul Fertser <fercerpav@gmail.com> Thank you for working on this. I'll be looking forward to your next patch where response validation is added. BTW, have you already discussed how exactly you plan to expose the statistics to the userspace, is that something that should end up visible via e.g. `ethtool -S eth0 --groups nc-si` ?
On 4/11/25 1:22 PM, Paul Fertser wrote: > On Thu, Apr 10, 2025 at 10:22:47AM -0700, kalavakunta.hari.prasad@gmail.com wrote: >> From: Hari Kalavakunta <kalavakunta.hari.prasad@gmail.com> >> >> Correct Get Controller Packet Statistics (GCPS) 64-bit wide member >> variables, as per DSP0222 v1.0.0 and forward specs. The Driver currently >> collects these stats, but they are yet to be exposed to the user. >> Therefore, no user impact. >> >> Statistics fixes: >> Total Bytes Received (byte range 28..35) >> Total Bytes Transmitted (byte range 36..43) >> Total Unicast Packets Received (byte range 44..51) >> Total Multicast Packets Received (byte range 52..59) >> Total Broadcast Packets Received (byte range 60..67) >> Total Unicast Packets Transmitted (byte range 68..75) >> Total Multicast Packets Transmitted (byte range 76..83) >> Total Broadcast Packets Transmitted (byte range 84..91) >> Valid Bytes Received (byte range 204..11) >> >> v2: >> - __be64 for all 64 bit GCPS counters >> >> v3: >> - be64_to_cpup() instead of be64_to_cpu() > > Usually the changelog should go after --- so it's not included in the > final commit message when merged. I hope in this case the maintainers > will take care of this manually so no need to resend unless they ask > to. > > Other than that, > > Reviewed-by: Paul Fertser <fercerpav@gmail.com> @Paul: it's not clear to me if as a consequence of the discussion running on v2 of this patch you prefer reverting back to be64_to_cpu(). The packet alignement should yield to the correct code in both cases. /P
Hello Paolo, On Tue, Apr 15, 2025 at 01:09:42PM +0200, Paolo Abeni wrote: > On 4/11/25 1:22 PM, Paul Fertser wrote: > > On Thu, Apr 10, 2025 at 10:22:47AM -0700, kalavakunta.hari.prasad@gmail.com wrote: > >> From: Hari Kalavakunta <kalavakunta.hari.prasad@gmail.com> > >> > >> Correct Get Controller Packet Statistics (GCPS) 64-bit wide member > >> variables, as per DSP0222 v1.0.0 and forward specs. The Driver currently > >> collects these stats, but they are yet to be exposed to the user. > >> Therefore, no user impact. > >> > >> Statistics fixes: > >> Total Bytes Received (byte range 28..35) > >> Total Bytes Transmitted (byte range 36..43) > >> Total Unicast Packets Received (byte range 44..51) > >> Total Multicast Packets Received (byte range 52..59) > >> Total Broadcast Packets Received (byte range 60..67) > >> Total Unicast Packets Transmitted (byte range 68..75) > >> Total Multicast Packets Transmitted (byte range 76..83) > >> Total Broadcast Packets Transmitted (byte range 84..91) > >> Valid Bytes Received (byte range 204..11) > >> > >> v2: > >> - __be64 for all 64 bit GCPS counters > >> > >> v3: > >> - be64_to_cpup() instead of be64_to_cpu() > > > > Usually the changelog should go after --- so it's not included in the > > final commit message when merged. I hope in this case the maintainers > > will take care of this manually so no need to resend unless they ask > > to. > > > > Other than that, > > > > Reviewed-by: Paul Fertser <fercerpav@gmail.com> > > @Paul: it's not clear to me if as a consequence of the discussion > running on v2 of this patch you prefer reverting back to be64_to_cpu(). > > The packet alignement should yield to the correct code in both cases. But it might produce warnings for the v3 variant so my understanding is that v2 is perfect and can be picked up (other than the changelog included in the commit message). Thank you!
On 4/11/2025 4:22 AM, Paul Fertser wrote: > Thank you for working on this. I'll be looking forward to your next > patch where response validation is added. > > BTW, have you already discussed how exactly you plan to expose the > statistics to the userspace, is that something that should end up > visible via e.g. `ethtool -S eth0 --groups nc-si` ? Hello Paul, Thank you for taking the time to review and provide feedback. I will thoroughly investigate the response validation gaps and address them as soon as possible. Regarding your suggestion, I agree that making statistics visible to userspace via ethtool is a good idea.
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h index 4e0842df5234..2c260f33b55c 100644 --- a/net/ncsi/internal.h +++ b/net/ncsi/internal.h @@ -143,16 +143,15 @@ struct ncsi_channel_vlan_filter { }; struct ncsi_channel_stats { - u32 hnc_cnt_hi; /* Counter cleared */ - u32 hnc_cnt_lo; /* Counter cleared */ - u32 hnc_rx_bytes; /* Rx bytes */ - u32 hnc_tx_bytes; /* Tx bytes */ - u32 hnc_rx_uc_pkts; /* Rx UC packets */ - u32 hnc_rx_mc_pkts; /* Rx MC packets */ - u32 hnc_rx_bc_pkts; /* Rx BC packets */ - u32 hnc_tx_uc_pkts; /* Tx UC packets */ - u32 hnc_tx_mc_pkts; /* Tx MC packets */ - u32 hnc_tx_bc_pkts; /* Tx BC packets */ + u64 hnc_cnt; /* Counter cleared */ + u64 hnc_rx_bytes; /* Rx bytes */ + u64 hnc_tx_bytes; /* Tx bytes */ + u64 hnc_rx_uc_pkts; /* Rx UC packets */ + u64 hnc_rx_mc_pkts; /* Rx MC packets */ + u64 hnc_rx_bc_pkts; /* Rx BC packets */ + u64 hnc_tx_uc_pkts; /* Tx UC packets */ + u64 hnc_tx_mc_pkts; /* Tx MC packets */ + u64 hnc_tx_bc_pkts; /* Tx BC packets */ u32 hnc_fcs_err; /* FCS errors */ u32 hnc_align_err; /* Alignment errors */ u32 hnc_false_carrier; /* False carrier detection */ @@ -181,7 +180,7 @@ struct ncsi_channel_stats { u32 hnc_tx_1023_frames; /* Tx 512-1023 bytes frames */ u32 hnc_tx_1522_frames; /* Tx 1024-1522 bytes frames */ u32 hnc_tx_9022_frames; /* Tx 1523-9022 bytes frames */ - u32 hnc_rx_valid_bytes; /* Rx valid bytes */ + u64 hnc_rx_valid_bytes; /* Rx valid bytes */ u32 hnc_rx_runt_pkts; /* Rx error runt packets */ u32 hnc_rx_jabber_pkts; /* Rx error jabber packets */ u32 ncsi_rx_cmds; /* Rx NCSI commands */ diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h index f2f3b5c1b941..24edb2737972 100644 --- a/net/ncsi/ncsi-pkt.h +++ b/net/ncsi/ncsi-pkt.h @@ -252,16 +252,15 @@ struct ncsi_rsp_gp_pkt { /* Get Controller Packet Statistics */ struct ncsi_rsp_gcps_pkt { struct ncsi_rsp_pkt_hdr rsp; /* Response header */ - __be32 cnt_hi; /* Counter cleared */ - __be32 cnt_lo; /* Counter cleared */ - __be32 rx_bytes; /* Rx bytes */ - __be32 tx_bytes; /* Tx bytes */ - __be32 rx_uc_pkts; /* Rx UC packets */ - __be32 rx_mc_pkts; /* Rx MC packets */ - __be32 rx_bc_pkts; /* Rx BC packets */ - __be32 tx_uc_pkts; /* Tx UC packets */ - __be32 tx_mc_pkts; /* Tx MC packets */ - __be32 tx_bc_pkts; /* Tx BC packets */ + __be64 cnt; /* Counter cleared */ + __be64 rx_bytes; /* Rx bytes */ + __be64 tx_bytes; /* Tx bytes */ + __be64 rx_uc_pkts; /* Rx UC packets */ + __be64 rx_mc_pkts; /* Rx MC packets */ + __be64 rx_bc_pkts; /* Rx BC packets */ + __be64 tx_uc_pkts; /* Tx UC packets */ + __be64 tx_mc_pkts; /* Tx MC packets */ + __be64 tx_bc_pkts; /* Tx BC packets */ __be32 fcs_err; /* FCS errors */ __be32 align_err; /* Alignment errors */ __be32 false_carrier; /* False carrier detection */ @@ -290,11 +289,11 @@ struct ncsi_rsp_gcps_pkt { __be32 tx_1023_frames; /* Tx 512-1023 bytes frames */ __be32 tx_1522_frames; /* Tx 1024-1522 bytes frames */ __be32 tx_9022_frames; /* Tx 1523-9022 bytes frames */ - __be32 rx_valid_bytes; /* Rx valid bytes */ + __be64 rx_valid_bytes; /* Rx valid bytes */ __be32 rx_runt_pkts; /* Rx error runt packets */ __be32 rx_jabber_pkts; /* Rx error jabber packets */ __be32 checksum; /* Checksum */ -}; +} __packed __aligned(4); /* Get NCSI Statistics */ struct ncsi_rsp_gns_pkt { diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c index 4a8ce2949fae..d3f902a1c891 100644 --- a/net/ncsi/ncsi-rsp.c +++ b/net/ncsi/ncsi-rsp.c @@ -926,16 +926,15 @@ static int ncsi_rsp_handler_gcps(struct ncsi_request *nr) /* Update HNC's statistics */ ncs = &nc->stats; - ncs->hnc_cnt_hi = ntohl(rsp->cnt_hi); - ncs->hnc_cnt_lo = ntohl(rsp->cnt_lo); - ncs->hnc_rx_bytes = ntohl(rsp->rx_bytes); - ncs->hnc_tx_bytes = ntohl(rsp->tx_bytes); - ncs->hnc_rx_uc_pkts = ntohl(rsp->rx_uc_pkts); - ncs->hnc_rx_mc_pkts = ntohl(rsp->rx_mc_pkts); - ncs->hnc_rx_bc_pkts = ntohl(rsp->rx_bc_pkts); - ncs->hnc_tx_uc_pkts = ntohl(rsp->tx_uc_pkts); - ncs->hnc_tx_mc_pkts = ntohl(rsp->tx_mc_pkts); - ncs->hnc_tx_bc_pkts = ntohl(rsp->tx_bc_pkts); + ncs->hnc_cnt = be64_to_cpup(&rsp->cnt); + ncs->hnc_rx_bytes = be64_to_cpup(&rsp->rx_bytes); + ncs->hnc_tx_bytes = be64_to_cpup(&rsp->tx_bytes); + ncs->hnc_rx_uc_pkts = be64_to_cpup(&rsp->rx_uc_pkts); + ncs->hnc_rx_mc_pkts = be64_to_cpup(&rsp->rx_mc_pkts); + ncs->hnc_rx_bc_pkts = be64_to_cpup(&rsp->rx_bc_pkts); + ncs->hnc_tx_uc_pkts = be64_to_cpup(&rsp->tx_uc_pkts); + ncs->hnc_tx_mc_pkts = be64_to_cpup(&rsp->tx_mc_pkts); + ncs->hnc_tx_bc_pkts = be64_to_cpup(&rsp->tx_bc_pkts); ncs->hnc_fcs_err = ntohl(rsp->fcs_err); ncs->hnc_align_err = ntohl(rsp->align_err); ncs->hnc_false_carrier = ntohl(rsp->false_carrier); @@ -964,7 +963,7 @@ static int ncsi_rsp_handler_gcps(struct ncsi_request *nr) ncs->hnc_tx_1023_frames = ntohl(rsp->tx_1023_frames); ncs->hnc_tx_1522_frames = ntohl(rsp->tx_1522_frames); ncs->hnc_tx_9022_frames = ntohl(rsp->tx_9022_frames); - ncs->hnc_rx_valid_bytes = ntohl(rsp->rx_valid_bytes); + ncs->hnc_rx_valid_bytes = be64_to_cpup(&rsp->rx_valid_bytes); ncs->hnc_rx_runt_pkts = ntohl(rsp->rx_runt_pkts); ncs->hnc_rx_jabber_pkts = ntohl(rsp->rx_jabber_pkts);