Message-ID: <rkyncpnstlsg4lt7hl47dly2ps7hbbj344wernpkekyruyo3yh@kpys6k5rmhbp>
Date: Wed, 12 Nov 2025 13:55:52 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Saeed Mahameed <saeed@...nel.org>, 
	Daniel Zahka <daniel.zahka@...il.com>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	Jonathan Corbet <corbet@....net>, Srujana Challa <schalla@...vell.com>, 
	Bharat Bhushan <bbhushan2@...vell.com>, Herbert Xu <herbert@...dor.apana.org.au>, 
	Brett Creeley <brett.creeley@....com>, Andrew Lunn <andrew+netdev@...n.ch>, 
	Michael Chan <michael.chan@...adcom.com>, Pavan Chebbi <pavan.chebbi@...adcom.com>, 
	Tony Nguyen <anthony.l.nguyen@...el.com>, Przemek Kitszel <przemyslaw.kitszel@...el.com>, 
	Sunil Goutham <sgoutham@...vell.com>, Linu Cherian <lcherian@...vell.com>, 
	Geetha sowjanya <gakula@...vell.com>, Jerin Jacob <jerinj@...vell.com>, 
	hariprasad <hkelam@...vell.com>, Subbaraya Sundeep <sbhatta@...vell.com>, 
	Tariq Toukan <tariqt@...dia.com>, Saeed Mahameed <saeedm@...dia.com>, 
	Leon Romanovsky <leon@...nel.org>, Mark Bloch <mbloch@...dia.com>, Ido Schimmel <idosch@...dia.com>, 
	Petr Machata <petrm@...dia.com>, Manish Chopra <manishc@...vell.com>, 
	Maxime Coquelin <mcoquelin.stm32@...il.com>, Alexandre Torgue <alexandre.torgue@...s.st.com>, 
	Siddharth Vadapalli <s-vadapalli@...com>, Roger Quadros <rogerq@...nel.org>, 
	Loic Poulain <loic.poulain@....qualcomm.com>, Sergey Ryazanov <ryazanov.s.a@...il.com>, 
	Johannes Berg <johannes@...solutions.net>, Vladimir Oltean <olteanv@...il.com>, 
	Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>, Aleksandr Loktionov <aleksandr.loktionov@...el.com>, 
	Dave Ertman <david.m.ertman@...el.com>, Vlad Dumitrescu <vdumitrescu@...dia.com>, 
	"Russell King (Oracle)" <rmk+kernel@...linux.org.uk>, Alexander Sverdlin <alexander.sverdlin@...il.com>, 
	Lorenzo Bianconi <lorenzo@...nel.org>, netdev@...r.kernel.org, linux-doc@...r.kernel.org, 
	intel-wired-lan@...ts.osuosl.org, linux-rdma@...r.kernel.org
Subject: Re: [PATCH net-next v3 2/2] net/mlx5: implement swp_l4_csum_mode via
 devlink params

Tue, Nov 11, 2025 at 04:48:57PM +0100, kuba@...nel.org wrote:
>[stripping some of the bouncy CCs.]
>
>On Tue, 11 Nov 2025 15:39:03 +0100 Jiri Pirko wrote:
>> Tue, Nov 11, 2025 at 04:26:34AM +0100, saeed@...nel.org wrote:
>> >On 10 Nov 15:46, Jakub Kicinski wrote:  
>> >> On Sun, 9 Nov 2025 11:46:37 +0100 Jiri Pirko wrote:  
>> >> > >So, I checked a couple of flows internally, and it seems this allows
>> >> > >some flexibility in the FW to decide later on which mode to pick,
>> >> > >based on other parameters, which practically means
>> >> > >"user has no preference on this param". Driver can only find out
>> >> > >after boot, when it reads the runtime capabilities, but still
>> >> > >this is a bug, by the time the driver reads this (in devlink), the
>> >> > >default value should've already been determined by FW, so FW must
>> >> > >return the actual runtime value. Which can only be one of the following  
>> >> > 
>> >> > I don't think it is correct to expose the "default" as a value.
>> >> > 
>> >> > On read, user should see the configured value, either "full_csum" or
>> >> > "l4_only". Reporting "default" to the user does not make any sense.
>> >> > On write, user should pass either "full_csum" or "l4_only". Why we would
>> >> > ever want to pass "default"?  
>> >> 
>> >> FWIW I agree that this feels a bit odd. Should the default be a flag
>> >> attr? On get flag being present means the value is the FW default (no
>> >> override present). On set passing the flag means user wants to reset
>> >> to FW default (remove override)?
>
>Y'all did not respond to this part, should we assume that what 
>I described is clear and makes sense? I think we should make that
>part of the series, unlike the pending indication.

I agree. The "default" flag sounds good to me.


>
>> >> > Regardless this patch, since this is param to be reflected on fw reboot
>> >> > (permanent cmode), I think it would be nice to expose indication if
>> >> > param value passed to user currently affects the fw, or if it is going
>> >> > to be applied after fw reboot. Perhaps a simple bool attr would do?  
>> >> 
>> >> IIUC we're basically talking about user having no information that
>> >> the update is pending? Could this be done by the core? Core can do
>> >> a ->get prior to calling ->set and if the ->set succeeds and
>> >> cmode != runtime record that the update is pending?
>> >>   
>> >
>> >Could work if on GET driver reads 'current' value from FW, then it should
>> >be simpler if GET != SET then 'pending', one problem though is if SET was
>> >done by external tool or value wasn't applied after reboot, then we loose
>> >that information, but do we care? I think we shouldn't.
>> >  
>> >> That feels very separate from the series tho, there are 3 permanent
>> >> params in mlx5, already. Is there something that makes this one special?  
>> 
>> Agreed. That is why I wrote "regardless this patch". But I think the
>> pending indication is definitelly nice to have.
>
>Yes, I've been wondering why it's missing since the day devlink params
>were added :)

Puzzles me. Nobody probably cared that much :/


>
>> >In mlx5 they all have the same behavior, devlink sets 'next' value, devlink
>> >reads 'next' value. The only special thing about the new param
>> >is that it has a 'device_default' value and when you read that from 'next' it
>> >will always show 'device_default' as the actual value is only
>> >known at run time ,e.g. 'next boot'.
>> >
>> >I think the only valid solution for permanent and drv_init params is to
>> >have 'next' and 'current' values reported by driver on read. Or maybe go just
>> >with  'set' != 'get' then 'pending' as discussed above ?  
>> 
>> Hmm, is it possible to rebind the driver without fw going through
>> next-boot phase? I'm wondering if it wouldn't be safer to have this
>> pending flag set to be responsibility of the driver...
>
>The downside is that drivers may either have bugs or not implement 
>the new feature. So when there's no indication of pending change
>the user will have no idea whether its because there's none or the
>driver simply does not report both. 
>
>My experience implementing the pending FW version in a couple of
>products is that it takes a lot of "discussions" to get FW people 
>to implement this sort of a thing right. mlx5 already has the right 
>FW APIs so we should allow their full use. But I don't think the 
>"what if user change the setting with fwctl", "what if user reloaded
>the driver" corner cases should stop us from trying to get the core
>to implement 99% of the cases right.
>
>FTR I'm not aware of any Meta-internal products having permanent knobs.
>I just don't think we can depend on the random people that submit
>drivers these days to get this right. And devlink users will assume
>that it's Linux that sucks if it doesn't work right, not vendor X.
>
>Long story short I think we can add the reporting of both values via GET
>but I'd definitely still like to see the core trying to do the tracking
>automatically.

I agree with tracking in core, allowing driver to opt-in with pending
flag value if it knows better overriding the core value.

Powered by blists - more mailing lists