Date:   Thu, 9 Feb 2023 21:38:25 +0100
From:   Andrew Lunn <andrew@lunn.ch>
To:     wei.fang@nxp.com
Cc:     shenwei.wang@nxp.com, xiaoning.wang@nxp.com, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        netdev@vger.kernel.org, linux-imx@nxp.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next] net: fec: add CBS offload support
Message-ID: <Y+VZwaFbLBAj2Ng0@lunn.ch>
References: <20230209092422.1655062-1-wei.fang@nxp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230209092422.1655062-1-wei.fang@nxp.com>
Precedence: bulk

> +	/* cbs->idleslope is in kilobits per second. speed is the port rate
> +	 * in megabits per second. So bandwidth ratio bw = (idleslope /
> +	 * (speed * 1000)) * 100, the unit is percentage.
> +	 */
> +	bw = cbs->idleslope / (speed * 10UL);

This appears to be a / 0 when the link is not up yet?  Also, if the
link goes does, fep->speed keeps the old value, so if it comes up
again at a different speed, your calculations are all wrong. So i
think you need fec_enet_adjust_link() involved in this.


> +	/* bw% can not >= 100% */
> +	if (bw >= 100)
> +		return -EINVAL;

Well > 100% could happen when the link goes from 1G to 10Half, or even
100Full. You should probably document the policy of what you do
then. Do you dedicate all the available bandwidth to the high priority
queue, or do you go back to best effort? Is it possible to indicate in
the tc show command that the configuration is no longer possible?

Presumably other drivers have already addressed all these issues, so
you just need to find out what they do.

    Andrew