netdev - Re: [PATCH net-next v2 5/6] enetc: Add interrupt coalescing 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] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 18 Jul 2020 20:20:10 +0300
From:   Claudiu Manoil <claudiu.manoil@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        Claudiu Manoil <claudiu.manoil@....com>
Cc:     "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 5/6] enetc: Add interrupt coalescing support

On 17.07.2020 22:32, Jakub Kicinski wrote:
> On Fri, 17 Jul 2020 18:37:03 +0300 Claudiu Manoil wrote:
>> +	if (ic->rx_max_coalesced_frames != ENETC_RXIC_PKTTHR)
>> +		netif_warn(priv, hw, ndev, "rx-frames fixed to %d\n",
>> +			   ENETC_RXIC_PKTTHR);
>> +
>> +	if (ic->tx_max_coalesced_frames != ENETC_TXIC_PKTTHR)
>> +		netif_warn(priv, hw, ndev, "tx-frames fixed to %d\n",
>> +			   ENETC_TXIC_PKTTHR);
> 
> On second thought - why not return an error here? Since only one value
> is supported seems like the right way to communicate to the users that
> they can't change this.
> 

Do you mean to return -EOPNOTSUPP without any error message instead?
If so, I think it's less punishing not to return an error code and 
invalidate the rest of the ethtool -C parameters that might have been
correct if the user forgets that rx/tx-frames cannot be changed.
There's also this flag:
	.supported_coalesce_params = .. |
				     ETHTOOL_COALESCE_MAX_FRAMES |
				     ..,
needed for printing the preconfigured values for the rx/tx packet 
thresholds, and this flag basically says that the 'rx/tx-frames'
parameters are supported (just that they cannot be changed... :) ).
But I don't have a strong bias for this, if you prefer the return
-EOPNOTSUPP option I'll make this change, just let me know if I got
it right.

>> +	if (netif_running(ndev) && changed) {
>> +		/* reconfigure the operation mode of h/w interrupts,
>> +		 * traffic needs to be paused in the process
>> +		 */
>> +		enetc_stop(ndev);
>> +		enetc_start(ndev);
> 
> Is start going to print an error when it fails? Kinda scary if this
> could turn into a silent failure.
> 

enetc_start() returns void, just like enetc_stop().  If you look it up
it only sets some run-time configurable registers and calls some basic
run-time commands that should no fail like napi_enable(), enable_irq(), 
phy_start(), all void returning functions. This function doesn't 
allocate resources or anything of that sort, and should be kept that 
way. And indeed, it should not fail. But regarding error codes there's
nothing I can do for this function, as nothing inside it generates any 
error code.