On 03/28/2018 02:23 PM, Tal Gilboa wrote:
> On 3/27/2018 10:47 PM, Florian Fainelli wrote:
>> There were a number of issues with setting the RX coalescing parameters:
>>
>> - we would not be preserving values that would have been configured
>> across close/open calls, instead we would always reset to no timeout
>> and 1 interrupt per packet, this would also prevent DIM from
>> setting its
>> default usec/pkts values
>>
>> - when adaptive RX would be turned on, we woud not be fetching the
>> default parameters, we would stay with no timeout/1 packet per
>> interrupt until the estimator kicks in and changes that
>>
>> - finally disabling adaptive RX coalescing while providing parameters
>> would not be honored, and we would stay with whatever DIM had
>> previously determined instead of the user requested parameters
>>
>> Fixes: b6e0e875421e ("net: systemport: Implement adaptive interrupt
>> coalescing")
>> Signed-off-by: Florian Fainelli <f.faine...@gmail.com>
>> ---
>> drivers/net/ethernet/broadcom/bcmsysport.c | 57
>> ++++++++++++++++++++----------
>> drivers/net/ethernet/broadcom/bcmsysport.h | 4 +--
>> 2 files changed, 41 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c
>> b/drivers/net/ethernet/broadcom/bcmsysport.c
>> index 1e52bb7d822e..43ad6300c351 100644
>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
>> @@ -574,16 +574,16 @@ static int bcm_sysport_set_wol(struct net_device
>> *dev,
>> return 0;
>> }
>> -static void bcm_sysport_set_rx_coalesce(struct bcm_sysport_priv *priv)
>> +static void bcm_sysport_set_rx_coalesce(struct bcm_sysport_priv *priv,
>> + u32 usecs, u32 pkts)
>> {
>> u32 reg;
>> reg = rdma_readl(priv, RDMA_MBDONE_INTR);
>> reg &= ~(RDMA_INTR_THRESH_MASK |
>> RDMA_TIMEOUT_MASK << RDMA_TIMEOUT_SHIFT);
>> - reg |= priv->dim.coal_pkts;
>> - reg |= DIV_ROUND_UP(priv->dim.coal_usecs * 1000, 8192) <<
>> - RDMA_TIMEOUT_SHIFT;
>> + reg |= pkts;
>> + reg |= DIV_ROUND_UP(usecs * 1000, 8192) << RDMA_TIMEOUT_SHIFT;
>> rdma_writel(priv, reg, RDMA_MBDONE_INTR);
>> }
>> @@ -626,6 +626,8 @@ static int bcm_sysport_set_coalesce(struct
>> net_device *dev,
>> struct ethtool_coalesce *ec)
>> {
>> struct bcm_sysport_priv *priv = netdev_priv(dev);
>> + struct net_dim_cq_moder moder;
>> + u32 usecs, pkts;
>> unsigned int i;
>> /* Base system clock is 125Mhz, DMA timeout is this reference
>> clock
>> @@ -646,15 +648,22 @@ static int bcm_sysport_set_coalesce(struct
>> net_device *dev,
>> for (i = 0; i < dev->num_tx_queues; i++)
>> bcm_sysport_set_tx_coalesce(&priv->tx_rings[i], ec);
>> - priv->dim.coal_usecs = ec->rx_coalesce_usecs;
>> - priv->dim.coal_pkts = ec->rx_max_coalesced_frames;
>> + priv->rx_coalesce_usecs = ec->rx_coalesce_usecs;
>> + priv->rx_max_coalesced_frames = ec->rx_max_coalesced_frames;
>> + usecs = priv->rx_coalesce_usecs;
>> + pkts = priv->rx_max_coalesced_frames;
>> - if (!ec->use_adaptive_rx_coalesce && priv->dim.use_dim) {
>> - priv->dim.coal_pkts = 1;
>> - priv->dim.coal_usecs = 0;
>> + /* If DIM is enabled, immediately obtain default parameters */
>> + if (ec->use_adaptive_rx_coalesce) {
>> + moder = net_dim_get_def_profile(priv->dim.dim.mode);
>> + usecs = moder.usec;
>> + pkts = moder.pkts;
>> }
>
> What if DIM is already in use? We don't want to set default values if
> DIM is already working.
Indeed, good catch, thank you!
>
>> +
>> priv->dim.use_dim = ec->use_adaptive_rx_coalesce;
>> - bcm_sysport_set_rx_coalesce(priv);
>> +
>> + /* Apply desired coalescing parameters */
>> + bcm_sysport_set_rx_coalesce(priv, usecs, pkts);
>> return 0;
>> }
>> @@ -1058,10 +1067,7 @@ static void bcm_sysport_dim_work(struct
>> work_struct *work)
>> struct net_dim_cq_moder cur_profile =
>> net_dim_get_profile(dim->mode, dim->profile_ix);
>> - priv->dim.coal_usecs = cur_profile.usec;
>> - priv->dim.coal_pkts = cur_profile.pkts;
>> -
>> - bcm_sysport_set_rx_coalesce(priv);
>> + bcm_sysport_set_rx_coalesce(priv, cur_profile.usec,
>> cur_profile.pkts);
>> dim->state = NET_DIM_START_MEASURE;
>> }
>> @@ -1408,14 +1414,30 @@ static void bcm_sysport_adj_link(struct
>> net_device *dev)
>> phy_print_status(phydev);
>> }
>> -static void bcm_sysport_init_dim(struct bcm_sysport_net_dim *dim,
>> +static void bcm_sysport_init_dim(struct bcm_sysport_priv *priv,
>> void (*cb)(struct work_struct *work))
>> {
>> + struct bcm_sysport_net_dim *dim = &priv->dim;
>> + struct net_dim_cq_moder moder;
>> + u32 usecs, pkts;
>> +
>> INIT_WORK(&dim->dim.work, cb);
>> dim->dim.mode = NET_DIM_CQ_PERIOD_MODE_START_FROM_EQE;
>> dim->event_ctr = 0;
>> dim->packets = 0;
>> dim->bytes = 0;
>
> I would expect only dim related code in a function called init_dim(). I
> think the code below should be elsewhere. Or maybe change the function
> name to init_rx_coalesce().
Sure, I can definitively do split the initialization, that makes sense.
--
Florian
