linux-kernel - RE: [PATCH net-next 1/1] net: stmmac: add per-queue TX & RX coalesce ethtool support

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] [day] [month] [year] [list]

Message-ID: <DM6PR11MB2780933A34D80D27920DB977CA6B9@DM6PR11MB2780.namprd11.prod.outlook.com>
Date:   Tue, 16 Mar 2021 04:44:17 +0000
From:   "Ong, Boon Leong" <boon.leong.ong@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Jose Abreu <joabreu@...opsys.com>,
        "David S . Miller" <davem@...emloft.net>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-stm32@...md-mailman.stormreply.com" 
        <linux-stm32@...md-mailman.stormreply.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH net-next 1/1] net: stmmac: add per-queue TX & RX coalesce
 ethtool support

>> +	if (queue < tx_cnt) {
>> +		ec->tx_coalesce_usecs = priv->tx_coal_timer[queue];
>> +		ec->tx_max_coalesced_frames = priv->tx_coal_frames[queue];
>> +	} else {
>> +		ec->tx_coalesce_usecs = -1;
>> +		ec->tx_max_coalesced_frames = -1;
>> +	}
>> +
>> +	if (priv->use_riwt && queue < rx_cnt) {
>> +		ec->rx_max_coalesced_frames = priv->rx_coal_frames[queue];
>> +		ec->rx_coalesce_usecs = stmmac_riwt2usec(priv-
>>rx_riwt[queue],
>> +							 priv);
>> +	} else {
>> +		ec->rx_max_coalesced_frames = -1;
>> +		ec->rx_coalesce_usecs = -1;
>
>Why the use of negative values? why not leave them as 0?
The initial logic was to return negative value to unsupported TXQ & RXQ
since they are invalid. No preference here. So, we can leave it as all zeros.

>
>>  	}
>>
>>  	return 0;
>>  }
>>
>> -static int stmmac_set_coalesce(struct net_device *dev,
>> +static int stmmac_get_coalesce(struct net_device *dev,
>>  			       struct ethtool_coalesce *ec)
>> +{
>> +	return __stmmac_get_coalesce(dev, ec, -1);
>> +}
>> +
>> +static int stmmac_get_per_queue_coalesce(struct net_device *dev, u32
>queue,
>> +					 struct ethtool_coalesce *ec)
>> +{
>> +	return __stmmac_get_coalesce(dev, ec, queue);
>> +}
>> +
>> +static int __stmmac_set_coalesce(struct net_device *dev,
>> +				 struct ethtool_coalesce *ec,
>> +				 int queue)
>>  {
>>  	struct stmmac_priv *priv = netdev_priv(dev);
>> -	u32 rx_cnt = priv->plat->rx_queues_to_use;
>> +	bool all_queues = false;
>>  	unsigned int rx_riwt;
>> +	u32 max_cnt;
>> +	u32 rx_cnt;
>> +	u32 tx_cnt;
>> +
>> +	rx_cnt = priv->plat->rx_queues_to_use;
>> +	tx_cnt = priv->plat->tx_queues_to_use;
>> +	max_cnt = max(rx_cnt, tx_cnt);
>> +
>> +	if (queue < 0)
>> +		all_queues = true;
>> +	else if (queue >= max_cnt)
>> +		return -EINVAL;
>> +
>> +	/* Check not supported parameters  */
>> +	if (ec->rx_coalesce_usecs_irq ||
>> +	    ec->rx_max_coalesced_frames_irq || ec->tx_coalesce_usecs_irq ||
>> +	    ec->use_adaptive_rx_coalesce || ec->use_adaptive_tx_coalesce ||
>> +	    ec->pkt_rate_low || ec->rx_coalesce_usecs_low ||
>> +	    ec->rx_max_coalesced_frames_low || ec->tx_coalesce_usecs_high ||
>> +	    ec->tx_max_coalesced_frames_low || ec->pkt_rate_high ||
>> +	    ec->tx_coalesce_usecs_low || ec->rx_coalesce_usecs_high ||
>> +	    ec->rx_max_coalesced_frames_high ||
>> +	    ec->tx_max_coalesced_frames_irq ||
>> +	    ec->stats_block_coalesce_usecs ||
>> +	    ec->tx_max_coalesced_frames_high || ec->rate_sample_interval)
>> +		return -EOPNOTSUPP;
>
>This shouldn't be needed now that supporter types are expressed in
>dev->ethtool_ops->supported_coalesce_params, no?
Wil fix this. Thanks for pointing out.