| Message ID | 1610292623-15564-15-git-send-email-stefanc@marvell.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: mvpp2: Add TX Flow Control support | expand |
> > @@ -5373,6 +5402,30 @@ static int > mvpp2_ethtool_set_pause_param(struct net_device *dev, > > struct ethtool_pauseparam *pause) { > > struct mvpp2_port *port = netdev_priv(dev); > > + int i; > > + > > + if (pause->tx_pause && port->priv->global_tx_fc) { > > + port->tx_fc = true; > > + mvpp2_rxq_enable_fc(port); > > + if (port->priv->percpu_pools) { > > + for (i = 0; i < port->nrxqs; i++) > > + mvpp2_bm_pool_update_fc(port, &port- > >priv->bm_pools[i], true); > > + } else { > > + mvpp2_bm_pool_update_fc(port, port->pool_long, > true); > > + mvpp2_bm_pool_update_fc(port, port->pool_short, > true); > > + } > > + > > + } else if (port->priv->global_tx_fc) { > > + port->tx_fc = false; > > + mvpp2_rxq_disable_fc(port); > > + if (port->priv->percpu_pools) { > > + for (i = 0; i < port->nrxqs; i++) > > + mvpp2_bm_pool_update_fc(port, &port- > >priv->bm_pools[i], false); > > + } else { > > + mvpp2_bm_pool_update_fc(port, port->pool_long, > false); > > + mvpp2_bm_pool_update_fc(port, port->pool_short, > false); > > + } > > + } > > > This looks wrong. Flow control is normally the result of auto negotiation. Both > ends need to agree to it. Which is why > mvpp2_ethtool_set_pause_param() passes the users request onto phylink. > phylink will handle the autoneg and then ask the MAC to setup flow control > depending on the result in mvpp2_mac_link_up(). > > Andrew Ok, I would move it to mvpp2_mac_link_up. Stefan, Thanks.
On Sun, Jan 10, 2021 at 05:30:18PM +0200, stefanc@marvell.com wrote: > @@ -5373,6 +5402,30 @@ static int mvpp2_ethtool_set_pause_param(struct net_device *dev, > struct ethtool_pauseparam *pause) > { > struct mvpp2_port *port = netdev_priv(dev); > + int i; > + > + if (pause->tx_pause && port->priv->global_tx_fc) { > + port->tx_fc = true; > + mvpp2_rxq_enable_fc(port); > + if (port->priv->percpu_pools) { > + for (i = 0; i < port->nrxqs; i++) > + mvpp2_bm_pool_update_fc(port, &port->priv->bm_pools[i], true); > + } else { > + mvpp2_bm_pool_update_fc(port, port->pool_long, true); > + mvpp2_bm_pool_update_fc(port, port->pool_short, true); > + } > + > + } else if (port->priv->global_tx_fc) { > + port->tx_fc = false; > + mvpp2_rxq_disable_fc(port); > + if (port->priv->percpu_pools) { > + for (i = 0; i < port->nrxqs; i++) > + mvpp2_bm_pool_update_fc(port, &port->priv->bm_pools[i], false); > + } else { > + mvpp2_bm_pool_update_fc(port, port->pool_long, false); > + mvpp2_bm_pool_update_fc(port, port->pool_short, false); > + } > + } This doesn't look correct to me. This function is only called when ethtool -A is used to change the flow control settings. This is not the place to be configuring flow control, as flow control is negotiated with the link partner. The final resolved flow control settings are available in mvpp2_mac_link_up() via the tx_pause and rx_pause parameters. What also concerns me is whether flow control is supported in the existing driver at all, given this patch set. If it isn't supported without the firmware's help, then we should _not_ be negotiating flow control with the link partner unless we actually support it, so the Pause and Asym_Pause bits in mvpp2_phylink_validate() should be cleared.
> > @@ -5373,6 +5402,30 @@ static int > mvpp2_ethtool_set_pause_param(struct net_device *dev, > > struct ethtool_pauseparam *pause) { > > struct mvpp2_port *port = netdev_priv(dev); > > + int i; > > + > > + if (pause->tx_pause && port->priv->global_tx_fc) { > > + port->tx_fc = true; > > + mvpp2_rxq_enable_fc(port); > > + if (port->priv->percpu_pools) { > > + for (i = 0; i < port->nrxqs; i++) > > + mvpp2_bm_pool_update_fc(port, &port- > >priv->bm_pools[i], true); > > + } else { > > + mvpp2_bm_pool_update_fc(port, port->pool_long, > true); > > + mvpp2_bm_pool_update_fc(port, port->pool_short, > true); > > + } > > + > > + } else if (port->priv->global_tx_fc) { > > + port->tx_fc = false; > > + mvpp2_rxq_disable_fc(port); > > + if (port->priv->percpu_pools) { > > + for (i = 0; i < port->nrxqs; i++) > > + mvpp2_bm_pool_update_fc(port, &port- > >priv->bm_pools[i], false); > > + } else { > > + mvpp2_bm_pool_update_fc(port, port->pool_long, > false); > > + mvpp2_bm_pool_update_fc(port, port->pool_short, > false); > > + } > > + } > > This doesn't look correct to me. This function is only called when ethtool -A is > used to change the flow control settings. This is not the place to be > configuring flow control, as flow control is negotiated with the link partner. > > The final resolved flow control settings are available in > mvpp2_mac_link_up() via the tx_pause and rx_pause parameters. I would move this to mvpp2_mac_link_up. Thanks. > What also concerns me is whether flow control is supported in the existing > driver at all, given this patch set. If it isn't supported without the firmware's > help, then we should _not_ be negotiating flow control with the link partner > unless we actually support it, so the Pause and Asym_Pause bits in > mvpp2_phylink_validate() should be cleared. RX FC supported, issue only with TX FC. Stefan, Regards.
On Sun, Jan 10, 2021 at 06:27:57PM +0000, Stefan Chulski wrote: > > What also concerns me is whether flow control is supported in the existing > > driver at all, given this patch set. If it isn't supported without the firmware's > > help, then we should _not_ be negotiating flow control with the link partner > > unless we actually support it, so the Pause and Asym_Pause bits in > > mvpp2_phylink_validate() should be cleared. > > RX FC supported, issue only with TX FC. That doesn't seem relevant given table 28B in IEEE 802.3. There is no advertisement combination that allows one to advertise an ability to receive FC frames but not transmit FC frames.
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c index 3b85aec..4869b14 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c @@ -1243,6 +1243,16 @@ static int mvpp2_bm_update_mtu(struct net_device *dev, int mtu) new_long_pool = MVPP2_BM_LONG; if (new_long_pool != port->pool_long->id) { + if (port->tx_fc) { + if (pkt_size > MVPP2_BM_LONG_PKT_SIZE) + mvpp2_bm_pool_update_fc(port, + port->pool_short, + false); + else + mvpp2_bm_pool_update_fc(port, port->pool_long, + false); + } + /* Remove port from old short & long pool */ port->pool_long = mvpp2_bm_pool_use(port, port->pool_long->id, port->pool_long->pkt_size); @@ -1260,6 +1270,25 @@ static int mvpp2_bm_update_mtu(struct net_device *dev, int mtu) mvpp2_swf_bm_pool_init(port); mvpp2_set_hw_csum(port, new_long_pool); + + if (port->tx_fc) { + if (pkt_size > MVPP2_BM_LONG_PKT_SIZE) + mvpp2_bm_pool_update_fc(port, port->pool_long, + true); + else + mvpp2_bm_pool_update_fc(port, port->pool_short, + true); + } + + /* Update L4 checksum when jumbo enable/disable on port */ + if (new_long_pool == MVPP2_BM_JUMBO && port->id != 0) { + dev->features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM); + dev->hw_features &= ~(NETIF_F_IP_CSUM | + NETIF_F_IPV6_CSUM); + } else { + dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM; + dev->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM; + } } out_set: @@ -5373,6 +5402,30 @@ static int mvpp2_ethtool_set_pause_param(struct net_device *dev, struct ethtool_pauseparam *pause) { struct mvpp2_port *port = netdev_priv(dev); + int i; + + if (pause->tx_pause && port->priv->global_tx_fc) { + port->tx_fc = true; + mvpp2_rxq_enable_fc(port); + if (port->priv->percpu_pools) { + for (i = 0; i < port->nrxqs; i++) + mvpp2_bm_pool_update_fc(port, &port->priv->bm_pools[i], true); + } else { + mvpp2_bm_pool_update_fc(port, port->pool_long, true); + mvpp2_bm_pool_update_fc(port, port->pool_short, true); + } + + } else if (port->priv->global_tx_fc) { + port->tx_fc = false; + mvpp2_rxq_disable_fc(port); + if (port->priv->percpu_pools) { + for (i = 0; i < port->nrxqs; i++) + mvpp2_bm_pool_update_fc(port, &port->priv->bm_pools[i], false); + } else { + mvpp2_bm_pool_update_fc(port, port->pool_long, false); + mvpp2_bm_pool_update_fc(port, port->pool_short, false); + } + } if (!port->phylink) return -ENOTSUPP;