Tue, Nov 11, 2025 at 04:48:57PM +0100, [email protected] 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, [email protected] 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.
