Date:   Mon, 13 Feb 2023 19:07:21 +0100
From:   Andrew Lunn <andrew@lunn.ch>
To:     Alexander Lobakin <alexandr.lobakin@intel.com>
Cc:     wei.fang@nxp.com, shenwei.wang@nxp.com, xiaoning.wang@nxp.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, simon.horman@corigine.com,
        netdev@vger.kernel.org, linux-imx@nxp.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH V2 net-next] net: fec: add CBS offload support
Message-ID: <Y+p8WZCPKhp4/RIH@lunn.ch>
References: <20230213092912.2314029-1-wei.fang@nxp.com>
 <ed27795a-f81f-a913-8275-b6f516b4f384@intel.com>
 <Y+pjl3vzi7TQcLKm@lunn.ch>
 <8b25bd1f-4265-33ea-bdb9-bc700eff0b0e@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8b25bd1f-4265-33ea-bdb9-bc700eff0b0e@intel.com>
Precedence: bulk

On Mon, Feb 13, 2023 at 06:44:05PM +0100, Alexander Lobakin wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Mon, 13 Feb 2023 17:21:43 +0100
> 
> >>> +	if (!speed) {
> >>> +		netdev_err(ndev, "Link speed is 0!\n");
> >>
> >> ??? Is this possible? If so, why is it checked only here and why can it
> >> be possible?
> > 
> > The obvious way this happens is that there is no link partner, so
> > auto-neg has not completed yet. The link speed is unknown.
> 
> Sure, but why treat it an error path then?

You need to treat is somehow. I would actually disagree with
netdev_err(), netdev_dbg() seems more appropriate. But if you don't
know the link speed, you cannot program the scheduler.

This also comes back to my question about what should happen with a TC
configuration which works fine for 1000BaseT, but will not work for
10BaseT. Should the driver accept it only if the current link speed is
sufficient? Should it always accept it, and not program it into the
hardware if the current link speed does not support it?

Since we are talking about hardware acceleration here, what does the
pure software version do? Ideally we want the accelerated version to
do the same as the software version.

Wei, please disable all clever stuff in the hardware, setup a pure
software qdisc and a 10BaseT link. Oversubscribe the link and see what
happens. Does other traffic get starved?

	Andrew