linux-kernel - Re: [PATCH net-next v10] net: ti: icssg-prueth: add TAPRIO offload 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]

Message-ID: <20250612151043.6wfefe42pzeeazvg@skbuf>
Date: Thu, 12 Jun 2025 18:10:43 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: MD Danish Anwar <danishanwar@...com>
Cc: Meghana Malladi <m-malladi@...com>,
	Vignesh Raghavendra <vigneshr@...com>,
	Simon Horman <horms@...nel.org>,
	Guillaume La Roque <glaroque@...libre.com>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Roger Quadros <rogerq@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	"David S. Miller" <davem@...emloft.net>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	linux-arm-kernel@...ts.infradead.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, srk@...com,
	Roger Quadros <rogerq@...com>
Subject: Re: [PATCH net-next v10] net: ti: icssg-prueth: add TAPRIO offload
 support

On Wed, Jun 11, 2025 at 03:10:35PM +0530, MD Danish Anwar wrote:
> > I am not very positive that even if adding the extra restrictions
> > discovered here (cycle-time cannot be != IEP_DEFAULT_CYCLE_TIME_NS),
> > the implementation will work as expected. I am not sure that our image
> > of "as expected" is the same.
> > 
> > Given that these don't seem to be hardware limitations, but constraints
> > imposed by the ICSSG firmware, I guess my suggestion would be to start
> > with the selftest I mentioned earlier (which may need to be adapted),
> 
> Yes I am working on running the selftest on ICSSG driver however there
> are some setup issues that I am encountering. I will try to test this
> using the selftest.
> 
> > and use it to get a better picture of the gaps. Then make a plan to fix
> > them in the firmware, and see what it takes. If it isn't going to be
> > sufficient to fix the bugs unless major API changes are introduced, then
> > maybe it doesn't make sense for Linux to support taprio offload on the
> > buggy firmware versions.
> > 
> > Or maybe it does (with the appropriate restrictions), but it would still
> > inspire more trust to see that the developer at least got some version
> > of the firmware to pass a selftest, and has a valid reference to follow.
> 
> Sure. I think we can go back to v9 implementation (no extend feature)
> and add two additional restrictions in the driver.
> 
> 1. Cycle-time needs to be 1ms
> 2. Base-time needs to be Multiple of 1ms
> 
> With these two restrictions we can have the basic taprio support. Once
> the firmware is fixed and has support for both the above cases, I will
> modify the driver as needed.
> 
> I know firmware is very buggy as of now. But we can still start the
> driver integration and fix these bugs with time.
> 
> I will try to test the implementation with these two limitations using
> the selftest and share the logs if it's okay with you to go ahead with
> these limitations.
> 
> > Not going to lie, it doesn't look great that we discover during v10 that
> > taprio offload only works with a cycle time of 1 ms. The schedule is
> 
> I understand that. Even I got to know about this limitation after my
> last response to v10
> (https://lore.kernel.org/all/5e928ff0-e75b-4618-b84c-609138598801@ti.com/)
> 
> > network-dependent and user-customizable, and maybe the users didn't get
> > the memo that only 1 ms was tested :-/
> 
> Let me know if it'll be okay to go ahead with the two limitations
> mentioned above for now (with selftest done).
> 
> If it's okay, I will try to send v11 with testing with selftest done as
> well. Thanks for the continuous feedback.

I don't want to gate your upstreaming efforts, but a new version with
just these extra restrictions, and no concrete plan to lift them, will
be seen with scepticism from reviewers. You can alleviate some of that
by showing results from a selftest.

The existing selftest uses a 2 ms schedule and a 10 ms schedule. Neither
of those is supported by your current proposal. You can modify the
schedules to be compatible with your current firmware, and the selftest
may pass that way, but I will not be in favor of accepting that change
upstream, because the cycle time is something that needs to be highly
adaptive to the network requirements.

So to summarize, you can try to move forward with a restricted version
of this feature, but you will have to be very transparent as to what
works and what are the next steps, as well as give assurance that you
intend to keep supporting the current firmware and its API when an
improved firmware will become available that lifts these restrictions.