From: Jesper Dangaard Brouer <jbrouer@redhat.com> To: Song Yoong Siang <yoong.siang.song@intel.com>, Jesse Brandeburg <jesse.brandeburg@intel.com>, Tony Nguyen <anthony.l.nguyen@intel.com>, "David S . Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, Jesper Dangaard Brouer <hawk@kernel.org>, John Fastabend <john.fastabend@gmail.com>, Maciej Fijalkowski <maciej.fijalkowski@intel.com>, Vedang Patel <vedang.patel@intel.com>, Jithu Joseph <jithu.joseph@intel.com>, Andre Guedes <andre.guedes@intel.com>, Stanislav Fomichev <sdf@google.com> Cc: xdp-hints@xdp-project.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org, intel-wired-lan@lists.osuosl.org, brouer@redhat.com, bpf@vger.kernel.org Subject: Re: [Intel-wired-lan] [PATCH net 1/1] igc: read before write to SRRCTL register Date: Thu, 13 Apr 2023 20:42:16 +0200 [thread overview] Message-ID: <07a88087-bee8-e549-c069-63d52268aef1@redhat.com> (raw) In-Reply-To: <20230413151222.1864307-1-yoong.siang.song@intel.com> On 13/04/2023 17.12, Song Yoong Siang wrote: > igc_configure_rx_ring() function will be called as part of XDP program > setup. If Rx hardware timestamp is enabled prio to XDP program setup, > this timestamp enablement will be overwritten when buffer size is > written into SRRCTL register. > Ah, I believe I have hit this bug with my igc patches. Thanks for fixing. > Thus, this commit read the register value before write to SRRCTL > register. This commit is tested by using xdp_hw_metadata bpf selftest > tool. The tool enables Rx hardware timestamp and then attach XDP program > to igc driver. It will display hardware timestamp of UDP packet with > port number 9092. Below are detail of test steps and results. > > Command on DUT: > sudo ./xdp_hw_metadata <interface name> > Why port 9092 ? The ./xdp_hw_metadata prog will redirect port 9091 > Command on Link Partner: > echo -n skb | nc -u -q1 <destination IPv4 addr> 9092 > Again port 9092 ? > Result before this patch: > skb hwtstamp is not found! > > Result after this patch: > found skb hwtstamp = 1677762212.590696226 I usually use this cmd to see if number is sane: $ date -d @1677762212 2023-03-02T14:03:32 CET > > Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy") > Cc: <stable@vger.kernel.org> # 5.14+ > Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com> > --- > drivers/net/ethernet/intel/igc/igc_base.h | 7 +++++-- > drivers/net/ethernet/intel/igc/igc_main.c | 5 ++++- > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc_base.h b/drivers/net/ethernet/intel/igc/igc_base.h > index 7a992befca24..b95007d51d13 100644 > --- a/drivers/net/ethernet/intel/igc/igc_base.h > +++ b/drivers/net/ethernet/intel/igc/igc_base.h > @@ -87,8 +87,11 @@ union igc_adv_rx_desc { > #define IGC_RXDCTL_SWFLUSH 0x04000000 /* Receive Software Flush */ > > /* SRRCTL bit definitions */ > -#define IGC_SRRCTL_BSIZEPKT_SHIFT 10 /* Shift _right_ */ > -#define IGC_SRRCTL_BSIZEHDRSIZE_SHIFT 2 /* Shift _left_ */ > +#define IGC_SRRCTL_BSIZEPKT_MASK GENMASK(6, 0) > +#define IGC_SRRCTL_BSIZEPKT_SHIFT 10 /* Shift _right_ */ > +#define IGC_SRRCTL_BSIZEHDRSIZE_MASK GENMASK(13, 8) > +#define IGC_SRRCTL_BSIZEHDRSIZE_SHIFT 2 /* Shift _left_ */ > +#define IGC_SRRCTL_DESCTYPE_MASK GENMASK(27, 25) > #define IGC_SRRCTL_DESCTYPE_ADV_ONEBUF 0x02000000 > > #endif /* _IGC_BASE_H */ > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index 25fc6c65209b..de7b21c2ccd6 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -641,7 +641,10 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter, > else > buf_size = IGC_RXBUFFER_2048; > > - srrctl = IGC_RX_HDR_LEN << IGC_SRRCTL_BSIZEHDRSIZE_SHIFT; > + srrctl = rd32(IGC_SRRCTL(reg_idx)); > + srrctl &= ~(IGC_SRRCTL_BSIZEPKT_MASK | IGC_SRRCTL_BSIZEHDRSIZE_MASK | > + IGC_SRRCTL_DESCTYPE_MASK); > + srrctl |= IGC_RX_HDR_LEN << IGC_SRRCTL_BSIZEHDRSIZE_SHIFT; > srrctl |= buf_size >> IGC_SRRCTL_BSIZEPKT_SHIFT; > srrctl |= IGC_SRRCTL_DESCTYPE_ADV_ONEBUF; > _______________________________________________ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
WARNING: multiple messages have this Message-ID (diff)
From: Jesper Dangaard Brouer <jbrouer@redhat.com> To: Song Yoong Siang <yoong.siang.song@intel.com>, Jesse Brandeburg <jesse.brandeburg@intel.com>, Tony Nguyen <anthony.l.nguyen@intel.com>, "David S . Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, Jesper Dangaard Brouer <hawk@kernel.org>, John Fastabend <john.fastabend@gmail.com>, Maciej Fijalkowski <maciej.fijalkowski@intel.com>, Vedang Patel <vedang.patel@intel.com>, Jithu Joseph <jithu.joseph@intel.com>, Andre Guedes <andre.guedes@intel.com>, Stanislav Fomichev <sdf@google.com> Cc: brouer@redhat.com, intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, bpf@vger.kernel.org, xdp-hints@xdp-project.net, stable@vger.kernel.org Subject: Re: [PATCH net 1/1] igc: read before write to SRRCTL register Date: Thu, 13 Apr 2023 20:42:16 +0200 [thread overview] Message-ID: <07a88087-bee8-e549-c069-63d52268aef1@redhat.com> (raw) In-Reply-To: <20230413151222.1864307-1-yoong.siang.song@intel.com> On 13/04/2023 17.12, Song Yoong Siang wrote: > igc_configure_rx_ring() function will be called as part of XDP program > setup. If Rx hardware timestamp is enabled prio to XDP program setup, > this timestamp enablement will be overwritten when buffer size is > written into SRRCTL register. > Ah, I believe I have hit this bug with my igc patches. Thanks for fixing. > Thus, this commit read the register value before write to SRRCTL > register. This commit is tested by using xdp_hw_metadata bpf selftest > tool. The tool enables Rx hardware timestamp and then attach XDP program > to igc driver. It will display hardware timestamp of UDP packet with > port number 9092. Below are detail of test steps and results. > > Command on DUT: > sudo ./xdp_hw_metadata <interface name> > Why port 9092 ? The ./xdp_hw_metadata prog will redirect port 9091 > Command on Link Partner: > echo -n skb | nc -u -q1 <destination IPv4 addr> 9092 > Again port 9092 ? > Result before this patch: > skb hwtstamp is not found! > > Result after this patch: > found skb hwtstamp = 1677762212.590696226 I usually use this cmd to see if number is sane: $ date -d @1677762212 2023-03-02T14:03:32 CET > > Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy") > Cc: <stable@vger.kernel.org> # 5.14+ > Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com> > --- > drivers/net/ethernet/intel/igc/igc_base.h | 7 +++++-- > drivers/net/ethernet/intel/igc/igc_main.c | 5 ++++- > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc_base.h b/drivers/net/ethernet/intel/igc/igc_base.h > index 7a992befca24..b95007d51d13 100644 > --- a/drivers/net/ethernet/intel/igc/igc_base.h > +++ b/drivers/net/ethernet/intel/igc/igc_base.h > @@ -87,8 +87,11 @@ union igc_adv_rx_desc { > #define IGC_RXDCTL_SWFLUSH 0x04000000 /* Receive Software Flush */ > > /* SRRCTL bit definitions */ > -#define IGC_SRRCTL_BSIZEPKT_SHIFT 10 /* Shift _right_ */ > -#define IGC_SRRCTL_BSIZEHDRSIZE_SHIFT 2 /* Shift _left_ */ > +#define IGC_SRRCTL_BSIZEPKT_MASK GENMASK(6, 0) > +#define IGC_SRRCTL_BSIZEPKT_SHIFT 10 /* Shift _right_ */ > +#define IGC_SRRCTL_BSIZEHDRSIZE_MASK GENMASK(13, 8) > +#define IGC_SRRCTL_BSIZEHDRSIZE_SHIFT 2 /* Shift _left_ */ > +#define IGC_SRRCTL_DESCTYPE_MASK GENMASK(27, 25) > #define IGC_SRRCTL_DESCTYPE_ADV_ONEBUF 0x02000000 > > #endif /* _IGC_BASE_H */ > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index 25fc6c65209b..de7b21c2ccd6 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -641,7 +641,10 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter, > else > buf_size = IGC_RXBUFFER_2048; > > - srrctl = IGC_RX_HDR_LEN << IGC_SRRCTL_BSIZEHDRSIZE_SHIFT; > + srrctl = rd32(IGC_SRRCTL(reg_idx)); > + srrctl &= ~(IGC_SRRCTL_BSIZEPKT_MASK | IGC_SRRCTL_BSIZEHDRSIZE_MASK | > + IGC_SRRCTL_DESCTYPE_MASK); > + srrctl |= IGC_RX_HDR_LEN << IGC_SRRCTL_BSIZEHDRSIZE_SHIFT; > srrctl |= buf_size >> IGC_SRRCTL_BSIZEPKT_SHIFT; > srrctl |= IGC_SRRCTL_DESCTYPE_ADV_ONEBUF; >
next prev parent reply other threads:[~2023-04-13 18:42 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-04-13 15:12 [PATCH net 1/1] igc: read before write to SRRCTL register Song Yoong Siang 2023-04-13 15:12 ` [Intel-wired-lan] " Song Yoong Siang 2023-04-13 16:54 ` Jacob Keller 2023-04-13 16:54 ` [Intel-wired-lan] " Jacob Keller 2023-04-13 18:42 ` Jesper Dangaard Brouer [this message] 2023-04-13 18:42 ` Jesper Dangaard Brouer 2023-04-14 1:33 ` Song, Yoong Siang 2023-04-14 1:33 ` [Intel-wired-lan] " Song, Yoong Siang 2023-04-13 19:05 ` Jesper Dangaard Brouer 2023-04-13 19:05 ` [Intel-wired-lan] " Jesper Dangaard Brouer 2023-04-14 1:36 ` Song, Yoong Siang 2023-04-14 1:36 ` [Intel-wired-lan] " Song, Yoong Siang 2023-04-30 5:03 ` naamax.meir 2023-04-30 5:03 ` naamax.meir
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=07a88087-bee8-e549-c069-63d52268aef1@redhat.com \ --to=jbrouer@redhat.com \ --cc=andre.guedes@intel.com \ --cc=anthony.l.nguyen@intel.com \ --cc=ast@kernel.org \ --cc=bpf@vger.kernel.org \ --cc=brouer@redhat.com \ --cc=daniel@iogearbox.net \ --cc=davem@davemloft.net \ --cc=edumazet@google.com \ --cc=hawk@kernel.org \ --cc=intel-wired-lan@lists.osuosl.org \ --cc=jesse.brandeburg@intel.com \ --cc=jithu.joseph@intel.com \ --cc=john.fastabend@gmail.com \ --cc=kuba@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=maciej.fijalkowski@intel.com \ --cc=netdev@vger.kernel.org \ --cc=pabeni@redhat.com \ --cc=sdf@google.com \ --cc=stable@vger.kernel.org \ --cc=vedang.patel@intel.com \ --cc=xdp-hints@xdp-project.net \ --cc=yoong.siang.song@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.