net/ncsi/internal.h | 21 ++++++++++----------- net/ncsi/ncsi-pkt.h | 23 +++++++++++------------ net/ncsi/ncsi-rsp.c | 21 ++++++++++----------- 3 files changed, 31 insertions(+), 34 deletions(-)
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
Signed-off-by: Hari Kalavakunta <kalavakunta.hari.prasad@gmail.com>
---
net/ncsi/internal.h | 21 ++++++++++-----------
net/ncsi/ncsi-pkt.h | 23 +++++++++++------------
net/ncsi/ncsi-rsp.c | 21 ++++++++++-----------
3 files changed, 31 insertions(+), 34 deletions(-)
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..8668888c5a2f 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_cpu(rsp->cnt);
+ ncs->hnc_rx_bytes = be64_to_cpu(rsp->rx_bytes);
+ ncs->hnc_tx_bytes = be64_to_cpu(rsp->tx_bytes);
+ ncs->hnc_rx_uc_pkts = be64_to_cpu(rsp->rx_uc_pkts);
+ ncs->hnc_rx_mc_pkts = be64_to_cpu(rsp->rx_mc_pkts);
+ ncs->hnc_rx_bc_pkts = be64_to_cpu(rsp->rx_bc_pkts);
+ ncs->hnc_tx_uc_pkts = be64_to_cpu(rsp->tx_uc_pkts);
+ ncs->hnc_tx_mc_pkts = be64_to_cpu(rsp->tx_mc_pkts);
+ ncs->hnc_tx_bc_pkts = be64_to_cpu(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_cpu(rsp->rx_valid_bytes);
ncs->hnc_rx_runt_pkts = ntohl(rsp->rx_runt_pkts);
ncs->hnc_rx_jabber_pkts = ntohl(rsp->rx_jabber_pkts);
--
2.47.1Hello, On Wed, Apr 09, 2025 at 06:23:08PM -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 > > Signed-off-by: Hari Kalavakunta <kalavakunta.hari.prasad@gmail.com> This version looks correct to me (other than the changelog being before ---) so Reviewed-by: Paul Fertser <fercerpav@gmail.com>
Hello Hari,
Thank you for the patch, it looks really clean. However I have one
more question now.
On Wed, Apr 09, 2025 at 06:23:08PM -0700, kalavakunta.hari.prasad@gmail.com wrote:
> @@ -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);
This made me check the Specification and indeed somehow it happened
that they have forgotten to ensure natural alignment for 64-bit fields
(at least they cared enough to do it for 32-bit values). [0] is the
relevant read.
> + ncs->hnc_cnt = be64_to_cpu(rsp->cnt);
This means that while it works fine on common BMCs now (since they run
in 32-bit mode) the access will be trappped as unaligned on 64-bit
Arms which one day will be common (Aspeed AST2700, Nuvoton NPCM8XX).
So I guess you should be doing `be64_to_cpup(&rsp->cnt)` there.
[0] https://www.catb.org/esr/structure-packing/
On Thu, 10 Apr 2025 13:50:58 +0300
Paul Fertser <fercerpav@gmail.com> wrote:
> Hello Hari,
>
> Thank you for the patch, it looks really clean. However I have one
> more question now.
>
> On Wed, Apr 09, 2025 at 06:23:08PM -0700, kalavakunta.hari.prasad@gmail.com wrote:
> > @@ -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);
>
> This made me check the Specification and indeed somehow it happened
> that they have forgotten to ensure natural alignment for 64-bit fields
> (at least they cared enough to do it for 32-bit values). [0] is the
> relevant read.
>
> > + ncs->hnc_cnt = be64_to_cpu(rsp->cnt);
Doesn't look related to the structure above.
>
> This means that while it works fine on common BMCs now (since they run
> in 32-bit mode) the access will be trappped as unaligned on 64-bit
> Arms which one day will be common (Aspeed AST2700, Nuvoton NPCM8XX).
>
> So I guess you should be doing `be64_to_cpup(&rsp->cnt)` there.
That is is the one that fails - the compiler is likely to warn about
taking the address of a member of a packed structure.
If the compiler knows the value might be misaligned (eg if the structure
is __packed) then it will do multiple reads and shifts.
IIRC it is enough to mark the member rx_valid_bytes __packed.
That removes the padding before it and the compiler will then assume
it is 4-byte aligned.
David
>
> [0] https://www.catb.org/esr/structure-packing/
>
Hello David,
Many thanks for chiming in and correcting my mistake. Hari, sorry for
misleading you on the last iteration.
On Sat, Apr 12, 2025 at 10:23:04AM +0100, David Laight wrote:
> On Thu, 10 Apr 2025 13:50:58 +0300
> Paul Fertser <fercerpav@gmail.com> wrote:
>
> > Hello Hari,
> >
> > Thank you for the patch, it looks really clean. However I have one
> > more question now.
> >
> > On Wed, Apr 09, 2025 at 06:23:08PM -0700, kalavakunta.hari.prasad@gmail.com wrote:
> > > @@ -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);
> >
> > This made me check the Specification and indeed somehow it happened
> > that they have forgotten to ensure natural alignment for 64-bit fields
> > (at least they cared enough to do it for 32-bit values). [0] is the
> > relevant read.
> >
> > > + ncs->hnc_cnt = be64_to_cpu(rsp->cnt);
>
> Doesn't look related to the structure above.
This is related to the earlier change of the same struct, I just
omitted that hunk while quoting.
> > This means that while it works fine on common BMCs now (since they run
> > in 32-bit mode) the access will be trappped as unaligned on 64-bit
> > Arms which one day will be common (Aspeed AST2700, Nuvoton NPCM8XX).
> >
> > So I guess you should be doing `be64_to_cpup(&rsp->cnt)` there.
>
> That is is the one that fails - the compiler is likely to warn about
> taking the address of a member of a packed structure.
Indeed I was confused. I was reading just
include/linux/byteorder/generic.h and expecting some kind of tricky
behaviour from the compiler (for the sake of optimisation as usual)
and wasn't aware of [0]. Taking address of such a field would indeed
produce a warning with -Waddress-of-packed-member.
> IIRC it is enough to mark the member rx_valid_bytes __packed.
> That removes the padding before it and the compiler will then assume
> it is 4-byte aligned.
The problem is that this struct is full of members that happen to be
unaligned (as the NC-SI standard specified that), it's used to unpack
a response packet returned by networking interface, so ratehr than
having every __be64 value there __packed it's cleaner to have the
attribute specified for the whole struct I guess.
Thanks again!
[0] https://www.kernel.org/doc/html/latest/core-api/unaligned-memory-access.html#code-that-does-not-cause-unaligned-access
On 4/13/2025 3:52 PM, Paul Fertser wrote: > > Many thanks for chiming in and correcting my mistake. Hari, sorry for > misleading you on the last iteration. > Good morning, Paul. I've completed three versions so far, and the test results for v3 are clean. However, I'd like to confirm the next steps to address the incorrect GCPS structure. I believed v3 would be the definitive fix. Could you please provide guidance if that's not the case?
© 2016 - 2025 Red Hat, Inc.