| Message ID | 20241216064720.931522-9-faizal.abdul.rahim@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | igc: Add support for Frame Preemption feature in IGC | expand |
On Mon, Dec 16, 2024 at 01:47:19AM -0500, Faizal Rahim wrote: > Implement "ethtool --show-mm" callback for IGC. > > Tested with command: > $ ethtool --show-mm enp1s0. > MAC Merge layer state for enp1s0: > pMAC enabled: on > TX enabled: on > TX active: on > TX minimum fragment size: 252 > RX minimum fragment size: 252 I'm going to ask "why so high?" and then I'm going to answer that I suspect this is a positive feedback loop created by openlldp, because of the driver incorrectly reporting: - 60 as 68, ..., 252 as 260, and openlldp always (correctly) rounding up these non-standard values to the closest upper multiple of an addFragSize, which is all that can be advertised over LLDP - on RX what was configured on TX (see below), which in turn makes the link partner again want to readjust (increase) its TX, to satisfy the new RX requirement But I'm open to hearing the correct answer, coming from you :) > Verify enabled: on > Verify time: 128 > Max verify time: 128 > Verification status: SUCCEEDED > > Verified that the fields value are retrieved correctly. > > Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com> > --- > diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c > index 7cde0e5a7320..16aa6e4e1727 100644 > --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c > +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c > @@ -1782,6 +1782,25 @@ static int igc_ethtool_set_eee(struct net_device *netdev, > return 0; > } > > +static int igc_ethtool_get_mm(struct net_device *netdev, > + struct ethtool_mm_state *cmd) > +{ > + struct igc_adapter *adapter = netdev_priv(netdev); > + struct fpe_t *fpe = &adapter->fpe; > + > + cmd->tx_min_frag_size = fpe->tx_min_frag_size; > + cmd->rx_min_frag_size = fpe->tx_min_frag_size; This is most likely a mistake. rx_min_frag_size means what is the smallest fragment size that the i225 can receive. Whereas tx_min_frag_size means what minimum fragment size it is configured to transmit (based, among others, on the link partner's minimum RX requirements). To say that the i225's minimum RX fragment size depends on how small it was configured to transmit seems wrong. I would expect a constant, or if this is correct, an explanation. TI treats rx_min_frag_size != ETH_ZLEN as errata. > + cmd->pmac_enabled = fpe->pmac_enabled; > + cmd->verify_enabled = fpe->verify_enabled; > + cmd->verify_time = fpe->verify_time; > + cmd->tx_active = igc_fpe_is_tx_preempt_allowed(&adapter->fpe); > + cmd->tx_enabled = fpe->tx_enabled; > + cmd->verify_status = igc_fpe_get_verify_status(&adapter->fpe); > + cmd->max_verify_time = MAX_VERIFY_TIME; > + > + return 0; > +}
On 17/12/2024 8:35 am, Vladimir Oltean wrote: > On Mon, Dec 16, 2024 at 01:47:19AM -0500, Faizal Rahim wrote: >> Implement "ethtool --show-mm" callback for IGC. >> >> Tested with command: >> $ ethtool --show-mm enp1s0. >> MAC Merge layer state for enp1s0: >> pMAC enabled: on >> TX enabled: on >> TX active: on >> TX minimum fragment size: 252 >> RX minimum fragment size: 252 > > I'm going to ask "why so high?" and then I'm going to answer that I > suspect this is a positive feedback loop created by openlldp, because of > the driver incorrectly reporting: > > - 60 as 68, ..., 252 as 260, and openlldp always (correctly) rounding up > these non-standard values to the closest upper multiple of an > addFragSize, which is all that can be advertised over LLDP > - on RX what was configured on TX (see below), which in turn makes the > link partner again want to readjust (increase) its TX, to satisfy the > new RX requirement > > But I'm open to hearing the correct answer, coming from you :) > Actually ... it was so high 252 ... because I mistakenly copied the result from my past openlldp test that did: sudo lldptool -T -i enp1s0 -V addEthCaps addFragSize=3 Which sets is to 252 ..sorry causing confusion Without OpenLLDP, with just ethtool and with default tx min frag size, it will look like: user@localhost:~$ sudo ethtool --show-mm enp1s0 MAC Merge layer state for enp1s0: pMAC enabled: off TX enabled: off TX active: off TX minimum fragment size: 68 RX minimum fragment size: 68 Verify enabled: off Verify time: 10 Max verify time: 128 Verification status: DISABLED When verify handshake is done with OpenLLDP, it will look like: user@localhost:~$ sudo lldptool -t -i enp1s0 -V addEthCaps Additional Ethernet Capabilities TLV Preemption capability supported Preemption capability enabled Preemption capability active Additional fragment size: 1 (124 octets) user@localhost:~$ sudo ethtool --show-mm enp1s0 MAC Merge layer state for enp1s0: pMAC enabled: on TX enabled: on TX active: on TX minimum fragment size: 124 RX minimum fragment size: 124 Verify enabled: on Verify time: 128 Max verify time: 128 Verification status: SUCCEEDED Which makes sense, due to the rounding up 68 to the closest upper multiple of addFragSize which is 124 octet in OpenLLDP, as what you mentioned. >> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c >> index 7cde0e5a7320..16aa6e4e1727 100644 >> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c >> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c >> @@ -1782,6 +1782,25 @@ static int igc_ethtool_set_eee(struct net_device *netdev, >> return 0; >> } >> >> +static int igc_ethtool_get_mm(struct net_device *netdev, >> + struct ethtool_mm_state *cmd) >> +{ >> + struct igc_adapter *adapter = netdev_priv(netdev); >> + struct fpe_t *fpe = &adapter->fpe; >> + >> + cmd->tx_min_frag_size = fpe->tx_min_frag_size; >> + cmd->rx_min_frag_size = fpe->tx_min_frag_size; > > This is most likely a mistake. rx_min_frag_size means what is the > smallest fragment size that the i225 can receive. Whereas tx_min_frag_size > means what minimum fragment size it is configured to transmit (based, > among others, on the link partner's minimum RX requirements). > To say that the i225's minimum RX fragment size depends on how small it > was configured to transmit seems wrong. I would expect a constant, or if > this is correct, an explanation. TI treats rx_min_frag_size != ETH_ZLEN > as errata. > My bad. I got your point, it's clearly explained, thanks :). Just got to know i226 is able to handle any frag size for RX. Since standard for min TX is 60, I'll use 60 then.
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h index fc1960925e28..3199da9b87ba 100644 --- a/drivers/net/ethernet/intel/igc/igc.h +++ b/drivers/net/ethernet/intel/igc/igc.h @@ -40,7 +40,7 @@ void igc_ethtool_set_ops(struct net_device *); #define IGC_MAX_TX_TSTAMP_REGS 4 -/* Verification state defined as per section 30.14.1.2 in 802.3br spec */ +/* Verify state defined as per section 99.4.8, Figure 99-8 in 802.3br spec */ enum verify_state { VERIFY_FAIL, INIT_VERIFICATION, diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c index 7cde0e5a7320..16aa6e4e1727 100644 --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c @@ -1782,6 +1782,25 @@ static int igc_ethtool_set_eee(struct net_device *netdev, return 0; } +static int igc_ethtool_get_mm(struct net_device *netdev, + struct ethtool_mm_state *cmd) +{ + struct igc_adapter *adapter = netdev_priv(netdev); + struct fpe_t *fpe = &adapter->fpe; + + cmd->tx_min_frag_size = fpe->tx_min_frag_size; + cmd->rx_min_frag_size = fpe->tx_min_frag_size; + cmd->pmac_enabled = fpe->pmac_enabled; + cmd->verify_enabled = fpe->verify_enabled; + cmd->verify_time = fpe->verify_time; + cmd->tx_active = igc_fpe_is_tx_preempt_allowed(&adapter->fpe); + cmd->tx_enabled = fpe->tx_enabled; + cmd->verify_status = igc_fpe_get_verify_status(&adapter->fpe); + cmd->max_verify_time = MAX_VERIFY_TIME; + + return 0; +} + static int igc_ethtool_set_mm(struct net_device *netdev, struct ethtool_mm_cfg *cmd, struct netlink_ext_ack *extack) @@ -2103,6 +2122,7 @@ static const struct ethtool_ops igc_ethtool_ops = { .set_rxfh = igc_ethtool_set_rxfh, .get_ts_info = igc_ethtool_get_ts_info, .get_channels = igc_ethtool_get_channels, + .get_mm = igc_ethtool_get_mm, .set_mm = igc_ethtool_set_mm, .set_channels = igc_ethtool_set_channels, .get_priv_flags = igc_ethtool_get_priv_flags, diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c index efd2a9f676d8..919a7f088a72 100644 --- a/drivers/net/ethernet/intel/igc/igc_tsn.c +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c @@ -258,6 +258,39 @@ void igc_fpe_preprocess_verify_response(struct fpe_t *fpe, int smd_type) schedule_delayed_work(&fpe->verification_work, 0); } +enum ethtool_mm_verify_status igc_fpe_get_verify_status(const struct fpe_t *fpe) +{ + enum ethtool_mm_verify_status verify_status; + + switch (fpe->verify_state) { + case VERIFY_FAIL: + verify_status = ETHTOOL_MM_VERIFY_STATUS_FAILED; + break; + + case INIT_VERIFICATION: + if (fpe->verify_enabled) + verify_status = ETHTOOL_MM_VERIFY_STATUS_INITIAL; + else + verify_status = ETHTOOL_MM_VERIFY_STATUS_DISABLED; + break; + + case VERIFIED: + verify_status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED; + break; + + case SEND_VERIFY: + case WAIT_FOR_RESPONSE: + verify_status = ETHTOOL_MM_VERIFY_STATUS_VERIFYING; + break; + + default: + verify_status = ETHTOOL_MM_VERIFY_STATUS_UNKNOWN; + break; + } + + return verify_status; +} + static bool is_any_launchtime(struct igc_adapter *adapter) { int i; diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.h b/drivers/net/ethernet/intel/igc/igc_tsn.h index 2b67ecae99c9..913f983652e4 100644 --- a/drivers/net/ethernet/intel/igc/igc_tsn.h +++ b/drivers/net/ethernet/intel/igc/igc_tsn.h @@ -13,6 +13,7 @@ #define MAX_VERIFY_TIME 128 int igc_fpe_get_smd_type(__le32 status_error); +enum ethtool_mm_verify_status igc_fpe_get_verify_status(const struct fpe_t *fpe); void igc_fpe_init(struct fpe_t *fpe); bool igc_fpe_is_tx_preempt_allowed(const struct fpe_t *fpe); bool igc_fpe_is_verify_or_response(int smd_type, unsigned int size);
Implement "ethtool --show-mm" callback for IGC. Tested with command: $ ethtool --show-mm enp1s0. MAC Merge layer state for enp1s0: pMAC enabled: on TX enabled: on TX active: on TX minimum fragment size: 252 RX minimum fragment size: 252 Verify enabled: on Verify time: 128 Max verify time: 128 Verification status: SUCCEEDED Verified that the fields value are retrieved correctly. Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com> --- drivers/net/ethernet/intel/igc/igc.h | 2 +- drivers/net/ethernet/intel/igc/igc_ethtool.c | 20 ++++++++++++ drivers/net/ethernet/intel/igc/igc_tsn.c | 33 ++++++++++++++++++++ drivers/net/ethernet/intel/igc/igc_tsn.h | 1 + 4 files changed, 55 insertions(+), 1 deletion(-)