Date:   Mon, 13 Feb 2023 14:32:23 +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, simon.horman@corigine.com,
        linux-imx@nxp.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH V2 net-next] net: fec: add CBS offload support
Message-ID: <Y+o75wT84f6RTohf@lunn.ch>
References: <20230213092912.2314029-1-wei.fang@nxp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230213092912.2314029-1-wei.fang@nxp.com>
Precedence: bulk

> 3. According to Andrew's comments, the speed may be equal to 0 when the
>    link is not up, so added a check to see if speed is equal to 0. In
>    addtion, the change in link speed also need to be taken into account.
>    Considering that the change of link speed has invalidated the original
>    configuration, so we just fall back to the default setting.

I don't think that is what you have actually implemented. A link
status change causes a fec_restart. And in fac_restart, you now
reprogram the hardware. So if the link speed is sufficient to support
the request, the hardware should be setup to support it.

What are the real uses cases here? VoIP? Video streaming? So 128kbps,
2Mbps. Both of those are fine over a 10Half limk. So i think you
should try to configure the hardware whenever possible, after link
change or any other condition which causes a reset of the hardware.

What i have not seen addresses here is my comment/question about what
tc shows when it is not possible to perform the request after a link
change? Did you look at how other drivers handle this? Maybe you need
to ask Jamal?

> +static int fec_enet_setup_tc_cbs(struct net_device *ndev, void *type_data)
> +{
> +	struct fec_enet_private *fep = netdev_priv(ndev);
> +	struct tc_cbs_qopt_offload *cbs = type_data;
> +	int queue = cbs->queue;
> +	int speed = fep->speed;
> +	int queue2;
> +
> +	if (!(fep->quirks & FEC_QUIRK_HAS_AVB))
> +		return -EOPNOTSUPP;
> +
> +	/* Queue 1 for Class A, Queue 2 for Class B, so the ENET must
> +	 * have three queues.
> +	 */
> +	if (fep->num_tx_queues != FEC_ENET_MAX_TX_QS)
> +		return -EOPNOTSUPP;
> +
> +	if (!speed) {
> +		netdev_err(ndev, "Link speed is 0!\n");
> +		return -ECANCELED;

ECANCLED? First time i've seen that one used. I had to go look it up
to see what it means. It does not really give the user any idea why it
failed. How about -ENETDOWN?

	Andrew