On Fri, 2024-10-25 at 13:40 -0700, Oliver Upton wrote: > > No. You're leaving the work for the reader to: > > (1) Figure out what you're talking about > (2) Go dig up an "earlier version" of the spec > (3) Realise that it means certain hypervisors only take 0x0 as an > argument No. There's no need to dig up an 'earlier version' of the spec. The current F.b release says, "if the value of this parameter is 0x0, the implementation defaults to selecting HIBERNATE_OFF". That's what makes it an acceptable alternative. > If you speak *directly* about the problem you're trying to address then > reviewers are less likely to get hung up on what you're trying to do. The "problem" this comment addresses is a reader who looks at this code and wonders why it uses zero instead of HIBERNATE_OFF. Firstly, that reader needs to spot the text, in the *current* version of the spec as cited above, which makes it clear that it's a perfectly acceptable alternative. Secondly, that reader needs to know why we chose zero over HIBERNATE_OFF, which is also explained fairly succinctly: because it's compatible with hypervisors implementing an earlier version of the spec. > I'm genuinely at a loss for why you would want to present this as an > "acceptable alterantive" rather than a functional requirement for this > driver to run on your hypervisor. Because "my" hypervisor is a live product which actually gets security fixes and updates. If it wasn't for the silly season of "thou shalt not ship *anything* except security fixes and stuff that's going to be announced at a big conference at the end of the month", the change to make it accept the new value of HIBERNATE_OFF would already have been deployed. And *even* with the silly season delaying non-critical hypervisor changes, your suggested wording will *still* probably not be true by the time the comment is included in an actual release of the Linux kernel. But honestly, it isn't a hill I'm prepared to die on. If you insist on changing the comment to your 'There are hypervisors in the wile that violate the spec...' version, by all means go ahead and do so. I'll follow up with a patch to correct it in a few weeks when it becomes obsolete.
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
