netdev - Re: [PATCH] net: davinci_emac: Fix interrupt pacing disable

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <717f9769-aac5-d005-e15a-a6a2ff61bb69@ti.com>
Date:   Mon, 1 Nov 2021 13:54:25 +0200
From:   Grygorii Strashko <grygorii.strashko@...com>
To:     Maxim Kiselev <bigunclemax@...il.com>
CC:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Yang Yingliang <yangyingliang@...wei.com>,
        Colin Ian King <colin.king@...onical.com>,
        Yufeng Mo <moyufeng@...wei.com>,
        Michael Walle <michael@...le.cc>, Sriram <srk@...com>,
        <linux-omap@...r.kernel.org>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: davinci_emac: Fix interrupt pacing disable



On 01/11/2021 11:21, Maxim Kiselev wrote:
> This patch allows to use 0 for `coal->rx_coalesce_usecs` param to
> disable rx irq coalescing.
> 
> Previously we could enable rx irq coalescing via ethtool
> (For ex: `ethtool -C eth0 rx-usecs 2000`) but we couldn't disable
> it because this part rejects 0 value:
> 
>         if (!coal->rx_coalesce_usecs)
>                 return -EINVAL;
> 
> Fixes: 84da2658a619 ("TI DaVinci EMAC : Implement interrupt pacing
> functionality.")
> 
> Signed-off-by: Maxim Kiselev <bigunclemax@...il.com>
> ---
>   drivers/net/ethernet/ti/davinci_emac.c | 77 ++++++++++++++------------
>   1 file changed, 41 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> index e8291d8488391..a3a02c4e5eb68 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -417,46 +417,47 @@ static int emac_set_coalesce(struct net_device *ndev,
>   			     struct netlink_ext_ack *extack)
>   {
>   	struct emac_priv *priv = netdev_priv(ndev);
> -	u32 int_ctrl, num_interrupts = 0;
> +	u32 int_ctrl = 0, num_interrupts = 0;
>   	u32 prescale = 0, addnl_dvdr = 1, coal_intvl = 0;
>   
> -	if (!coal->rx_coalesce_usecs)
> -		return -EINVAL;
> -
>   	coal_intvl = coal->rx_coalesce_usecs;

Wouldn't be more simple if you just handle !coal->rx_coalesce_usecs here and exit?
it seems you can just write 0 t0 INTCTRL.

>   
>   	switch (priv->version) {
>   	case EMAC_VERSION_2:
> -		int_ctrl =  emac_ctrl_read(EMAC_DM646X_CMINTCTRL);
> -		prescale = priv->bus_freq_mhz * 4;
> -
> -		if (coal_intvl < EMAC_DM646X_CMINTMIN_INTVL)
> -			coal_intvl = EMAC_DM646X_CMINTMIN_INTVL;
> -
> -		if (coal_intvl > EMAC_DM646X_CMINTMAX_INTVL) {
> -			/*
> -			 * Interrupt pacer works with 4us Pulse, we can
> -			 * throttle further by dilating the 4us pulse.
> -			 */
> -			addnl_dvdr = EMAC_DM646X_INTPRESCALE_MASK / prescale;
> -
> -			if (addnl_dvdr > 1) {
> -				prescale *= addnl_dvdr;
> -				if (coal_intvl > (EMAC_DM646X_CMINTMAX_INTVL
> -							* addnl_dvdr))
> -					coal_intvl = (EMAC_DM646X_CMINTMAX_INTVL
> -							* addnl_dvdr);
> -			} else {
> -				addnl_dvdr = 1;
> -				coal_intvl = EMAC_DM646X_CMINTMAX_INTVL;
> +		if (coal->rx_coalesce_usecs) {
> +			int_ctrl =  emac_ctrl_read(EMAC_DM646X_CMINTCTRL);
> +			prescale = priv->bus_freq_mhz * 4;
> +
> +			if (coal_intvl < EMAC_DM646X_CMINTMIN_INTVL)
> +				coal_intvl = EMAC_DM646X_CMINTMIN_INTVL;
> +
> +			if (coal_intvl > EMAC_DM646X_CMINTMAX_INTVL) {
> +				/*
> +				 * Interrupt pacer works with 4us Pulse, we can
> +				 * throttle further by dilating the 4us pulse.
> +				 */
> +				addnl_dvdr =
> +					EMAC_DM646X_INTPRESCALE_MASK / prescale;
> +
> +				if (addnl_dvdr > 1) {
> +					prescale *= addnl_dvdr;
> +					if (coal_intvl > (EMAC_DM646X_CMINTMAX_INTVL
> +								* addnl_dvdr))
> +						coal_intvl = (EMAC_DM646X_CMINTMAX_INTVL
> +								* addnl_dvdr);
> +				} else {
> +					addnl_dvdr = 1;
> +					coal_intvl = EMAC_DM646X_CMINTMAX_INTVL;
> +				}
>   			}
> -		}
>   
> -		num_interrupts = (1000 * addnl_dvdr) / coal_intvl;
> +			num_interrupts = (1000 * addnl_dvdr) / coal_intvl;
> +
> +			int_ctrl |= EMAC_DM646X_INTPACEEN;
> +			int_ctrl &= (~EMAC_DM646X_INTPRESCALE_MASK);
> +			int_ctrl |= (prescale & EMAC_DM646X_INTPRESCALE_MASK);
> +		}
>   
> -		int_ctrl |= EMAC_DM646X_INTPACEEN;
> -		int_ctrl &= (~EMAC_DM646X_INTPRESCALE_MASK);
> -		int_ctrl |= (prescale & EMAC_DM646X_INTPRESCALE_MASK);
>   		emac_ctrl_write(EMAC_DM646X_CMINTCTRL, int_ctrl);
>   
>   		emac_ctrl_write(EMAC_DM646X_CMRXINTMAX, num_interrupts);
> @@ -466,17 +467,21 @@ static int emac_set_coalesce(struct net_device *ndev,
>   	default:
>   		int_ctrl = emac_ctrl_read(EMAC_CTRL_EWINTTCNT);
>   		int_ctrl &= (~EMAC_DM644X_EWINTCNT_MASK);
> -		prescale = coal_intvl * priv->bus_freq_mhz;
> -		if (prescale > EMAC_DM644X_EWINTCNT_MASK) {
> -			prescale = EMAC_DM644X_EWINTCNT_MASK;
> -			coal_intvl = prescale / priv->bus_freq_mhz;
> +
> +		if (coal->rx_coalesce_usecs) {
> +			prescale = coal_intvl * priv->bus_freq_mhz;
> +			if (prescale > EMAC_DM644X_EWINTCNT_MASK) {
> +				prescale = EMAC_DM644X_EWINTCNT_MASK;
> +				coal_intvl = prescale / priv->bus_freq_mhz;
> +			}
>   		}
> +
>   		emac_ctrl_write(EMAC_CTRL_EWINTTCNT, (int_ctrl | prescale));
>   
>   		break;
>   	}
>   
> -	printk(KERN_INFO"Set coalesce to %d usecs.\n", coal_intvl);
> +	netdev_info(ndev, "Set coalesce to %d usecs.\n", coal_intvl);
>   	priv->coal_intvl = coal_intvl;
>   
>   	return 0;
> 

-- 
Best regards,
grygorii