lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <6e596bc99585020112f02fb9d46081e129ceda8e.camel@microchip.com> Date: Thu, 5 Oct 2023 10:58:37 +0000 From: <VishvambarPanth.S@...rochip.com> To: <jacob.e.keller@...el.com>, <linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org> CC: <pabeni@...hat.com>, <richardcochran@...il.com>, <edumazet@...gle.com>, <davem@...emloft.net>, <UNGLinuxDriver@...rochip.com>, <Bryan.Whitehead@...rochip.com>, <kuba@...nel.org> Subject: Re: [PATCH net-next] net: microchip: lan743x: improve throughput with rx timestamp config On Tue, 2023-09-26 at 16:48 -0700, Jacob Keller wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On 9/26/2023 8:56 AM, Vishvambar Panth S wrote: > > Currently all RX frames are timestamped which results in a > > performance > > penalty when timestamping is not needed. The default is now being > > changed to not timestamp any Rx frames (HWTSTAMP_FILTER_NONE), but > > support has been added to allow changing the desired RX > > timestamping > > mode (HWTSTAMP_FILTER_ALL - which was the previous setting, > > HWTSTAMP_FILTER_PTP_V2_EVENT, HWTSTAMP_FILTER_PTP_V2_SYNC and > > HWTSTAMP_FILTER_PTP_V2_DELAY_REQ are the supported options) using > > SIOCSHWTSTAMP. All settings were tested using the hwstamp_ctl > > application. > > It is also noted that ptp4l, when started, preconfigures the device > > to > > timestamp using HWTSTAMP_FILTER_PTP_V2_EVENT, so this driver > > continues > > to work properly "out of the box". > > > > Test setup: x64 PC with LAN7430 ---> x64 PC as partner > > > > I don't think I would bother to support > HWTSTAMP_FILTER_PTP_V2_DELAY_REQ > or HWTSTAMP_FILTER_PTP_V2_SYNC as these are pretty historic and only > useful for hardware which can't do HWTSTAMP_FILTER_PTP_V2_EVENT. > Hi Jacob, Thanks for the comments. Just added these filters, as the HW supports these individually. Since HW also supports HWTSTAMP_FILTER_PTP_V2_EVENT, will drop the support for individual HWTSTAMP_FILTER_PTP_V2_SYNC and HWTSTAMP_FILTER_PTP_V2_DELAY_REQ cases in v2 patch. > > iperf3 with - Timestamp all incoming packets: > > - - - - - - - - - - - - - - - - - - - - - - - - - > > [ ID] Interval Transfer Bitrate Retr > > [ 5] 0.00-5.05 sec 517 MBytes 859 Mbits/sec 0 sender > > [ 5] 0.00-5.00 sec 515 MBytes 864 Mbits/sec > > receiver > > > > iperf Done. > > > > iperf3 with - Timestamp only PTP packets: > > - - - - - - - - - - - - - - - - - - - - - - - - - > > [ ID] Interval Transfer Bitrate Retr > > [ 5] 0.00-5.04 sec 563 MBytes 937 Mbits/sec 0 sender > > [ 5] 0.00-5.00 sec 561 MBytes 941 Mbits/sec > > receiver > > > > > > Pretty significant cost here for the timestamping all frames. Makes > sense to leave the default to NONE unless requested. Thanks for the comment, will maintain as is. > > Please find the earlier conversation at the link below > > Link: > > https://lore.kernel.org/all/20230731125418.75140-1-vishvambarpanth.s@microchip.com/ > > > > Signed-off-by: Vishvambar Panth S <vishvambarpanth.s@...rochip.com> > > --- > > .../net/ethernet/microchip/lan743x_ethtool.c | 5 +- > > drivers/net/ethernet/microchip/lan743x_main.c | 58 > > ++++++++++++++++++- > > drivers/net/ethernet/microchip/lan743x_main.h | 8 +++ > > drivers/net/ethernet/microchip/lan743x_ptp.c | 9 +++ > > 4 files changed, 78 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c > > b/drivers/net/ethernet/microchip/lan743x_ethtool.c > > index 2db5949b4c7e..855844df5ea1 100644 > > --- a/drivers/net/ethernet/microchip/lan743x_ethtool.c > > +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c > > @@ -1047,7 +1047,10 @@ static int > > lan743x_ethtool_get_ts_info(struct net_device *netdev, > > BIT(HWTSTAMP_TX_ON) | > > BIT(HWTSTAMP_TX_ONESTEP_SYNC); > > ts_info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) | > > - BIT(HWTSTAMP_FILTER_ALL); > > + BIT(HWTSTAMP_FILTER_ALL) | > > + BIT(HWTSTAMP_FILTER_PTP_V2_EVENT) | > > + BIT(HWTSTAMP_FILTER_PTP_V2_SYNC) | > > + BIT(HWTSTAMP_FILTER_PTP_V2_DELAY_REQ); > > return 0; > > } > > > > diff --git a/drivers/net/ethernet/microchip/lan743x_main.c > > b/drivers/net/ethernet/microchip/lan743x_main.c > > index f940895b14e8..0389bc7cf603 100644 > > --- a/drivers/net/ethernet/microchip/lan743x_main.c > > +++ b/drivers/net/ethernet/microchip/lan743x_main.c > > @@ -1870,6 +1870,63 @@ static int lan743x_tx_get_avail_desc(struct > > lan743x_tx *tx) > > return last_head - last_tail - 1; > > } > > > > +int lan743x_rx_set_tstamp_mode(struct lan743x_adapter *adapter, > > + int rx_filter) > > +{ > > + int channel_number; > > + int index; > > + u32 data; > > + > > + switch (rx_filter) { > > + case HWTSTAMP_FILTER_PTP_V2_SYNC: > > + data = lan743x_csr_read(adapter, > > PTP_RX_TS_CFG); > > + data &= ~PTP_RX_TS_CFG_EVENT_MSGS_; > > + data |= PTP_RX_TS_CFG_SYNC_MSG_; > > + lan743x_csr_write(adapter, PTP_RX_TS_CFG, > > data); > > + break; > > + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ: > > + data = lan743x_csr_read(adapter, > > PTP_RX_TS_CFG); > > + data &= ~PTP_RX_TS_CFG_EVENT_MSGS_; > > + data |= PTP_RX_TS_CFG_DELAY_REQ_MSG_; > > + lan743x_csr_write(adapter, PTP_RX_TS_CFG, > > data); > > + break; > > + case HWTSTAMP_FILTER_PTP_V2_EVENT: > > + data = lan743x_csr_read(adapter, > > PTP_RX_TS_CFG); > > + data |= PTP_RX_TS_CFG_EVENT_MSGS_; > > + lan743x_csr_write(adapter, PTP_RX_TS_CFG, > > data); > > + break; > > + case HWTSTAMP_FILTER_NONE: > > + case HWTSTAMP_FILTER_ALL: > > + break; > > At first this break was a bit confusing to me, since nothing is set > here. The idea was that in the NONE and ALL case, there need not be any modification in this register. But will adapt to a more readable implementation in v2 patch. > > > + default: > > + netif_warn(adapter, drv, adapter->netdev, > > + "rx timestamp = %d is not > > supported\n", > > + rx_filter); > > + return -EINVAL; > > + } > > + > > + for (index = 0; index < LAN743X_USED_RX_CHANNELS; index++) { > > + channel_number = adapter->rx[index].channel_number; > > + data = lan743x_csr_read(adapter, > > RX_CFG_B(channel_number)); > > + if (rx_filter == HWTSTAMP_FILTER_NONE) { > > + data &= ~(RX_CFG_B_TS_ALL_RX_ | > > + RX_CFG_B_TS_DESCR_EN_); > > + } else if (rx_filter == HWTSTAMP_FILTER_ALL) { > > + data |= RX_CFG_B_TS_ALL_RX_; > > + } else { > > + /* enable storing timestamping in extension > > descriptor > > + * instead of timestamping all the packets > > + */ > > + data &= ~RX_CFG_B_TS_ALL_RX_; > > + data |= RX_CFG_B_TS_DESCR_EN_; > > + } > > I might have made the decision of what to program in the switch case > above and then done the write here with "data &= ~MASK; data |= > setting" > rather than having two separate decision points. Sure, will try and adapt to this. > > > + lan743x_csr_write(adapter, RX_CFG_B(channel_number), > > + data); > > + } > > + > > + return 0; > > +} > > + > > Rest of the code seems fine, and the implementation looks ok. > > I'd suggest dropping the not so useful timestamp filters for only > sync > or only delay request, keeping only V2_EVENT, but either way: Thanks, will drop the not so useful filters and submit a v2 patch with required changes. > > Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
Powered by blists - more mailing lists