Date:   Mon, 20 Feb 2023 21:07:36 +0100 (CET)
From:   Richard Weinberger <richard@nod.at>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     netdev <netdev@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>, pabeni@redhat.com,
        kuba@kernel.org, edumazet@google.com, davem <davem@davemloft.net>,
        linux-imx <linux-imx@nxp.com>,
        xiaoning wang <xiaoning.wang@nxp.com>,
        shenwei wang <shenwei.wang@nxp.com>,
        wei fang <wei.fang@nxp.com>
Message-ID: <1575439606.155156.1676923656294.JavaMail.zimbra@nod.at>
In-Reply-To: <Y/LIS3xd1iZRyVGe@lunn.ch>
References: <20230218214037.16977-1-richard@nod.at> <Y/LIS3xd1iZRyVGe@lunn.ch>
Subject: Re: [PATCH] [RFC] net: fec: Allow turning off IRQ coalescing
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8BIT
Thread-Topic: Allow turning off IRQ coalescing
Thread-Index: 24/x27NspbuH3aH+Y++BvGo/nXKLNA==
Precedence: bulk

Andrew,

----- Ursprüngliche Mail -----
>> -	/* Must be greater than zero to avoid unpredictable behavior */
>> -	if (!fep->rx_time_itr || !fep->rx_pkts_itr ||
>> -	    !fep->tx_time_itr || !fep->tx_pkts_itr)
>> -		return;
>> +	if (!fep->rx_time_itr || !fep->rx_pkts_itr) {
>> +		if (fep->rx_time_itr || fep->rx_pkts_itr) {
>> +			dev_warn(dev, "Rx coalesced frames and usec have to be "
>> +				      "both positive or both zero to disable Rx "
>> +				      "coalescence completely\n");
>> +			return -EINVAL;
>> +		}
> 
> Hi Richard
> 
> Why do this validation here, and not in fec_enet_set_coalesce() where
> there are already checks? fec_enet_set_coalesce() also has extack, so
> you can return useful messages to user space, not just the kernel log.

Using extack is a good point, the driver does not use it at all so far.
So I'd do a second patch which cleans this up.

I did the check in fec_enet_itr_coal_set() because the check is used to
set both disable_rx_itr and disable_tx_itr.
Of course I can place the check into fec_enet_set_coalesce() and then
pass disable_rx_itr and disable_tx_itr to fec_enet_itr_coal_set().

Thanks,
//richard