On Tue, Apr 8, 2025 at 10:48 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > On Wed, Apr 02, 2025 at 09:36:01PM +0200, Rafael J. Wysocki wrote: > > On Wed, Apr 2, 2025 at 8:48 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > > > I don't *expect* rev 5 to be different. But as a user of it, why > > > would I change working, tested code that is not broken? > > > > > > The PCI DPC function 0xc is an example where rev 5 (per ECN) and rev 6 > > > (per r3.3) are not compatible. > > > > > > If the OS implemented rev 5, then learns via function 0 that function > > > 0xc is also supported at rev 6, and starts using the same OS code with > > > rev 6, the OS is broken. > > > > Yes, in this case the backward compatibility language in the _DSM > > definition is obviously not followed. > > Rev 5 in the ECN isn't compatible with rev 6 in the PCI FW r3.3 spec, > so it doesn't follow the ACPI compatibility requirement. And this is > documented in PCI FW, which says "Fn 0xC was added with rev 5 (see ECN > for rev 5 details); here is how rev 6 works." > > An OS implemented to the ECN doesn't know that rev 6 is different from > rev 5; it assumes they're the same because ACPI says we can assume > that, and PCI FW r3.3 even says the OS should use the same rev for all > functions. OK (and this is important because PCI FW r3.3 is the spec defining the interface) > If OS adds support for rev 6 of a some other function, it is supposed to use > rev 6 of Fn 0xC, which doesn't work as the OS expects. IMV with respect to _DSM, the spec that has defined the interface (PCI FW r3.3) takes precedence over the ACPI spec regardless of what the latter is saying. In this case ACPI provides a framework the interface can be based on, but the actual interface is not defined by it. Also, I think that the OS should use rev 6 if it is supported by the firmware (for all functions) and it should literally follow the definition of rev 6. If rev 6 is not supported (or some functions needed by the OS are not implemented by the firmware), it should fall back to rev 5 and in this case it should literally follow the definition of rev 5. And so on. > I guess one could argue that "OS didn't add rev 6 support for anything > until PCI FW r3.3 added a function at rev 6, r3.3 did mention the > difference between Fn 0xC rev 5 and 6, and OS should have looked at > all the already-implemented unrelated functions for possible changes > between rev 5 and rev 6." Yes, it should. > I think that causes unnecessary changes in unrelated code. The OS can > avoid the problem by *always* using Fn 0xC rev 5, regardless of what > rev it uses for other functions. What if the functions on the firmware side depend on each other interfally and the firmware gets confused if revisions are mixed up on the OS side? Since PCI FW r3.3 says that the OS should use the same rev for all functions, that's what it should do because the firmware may reasonably assume that it will. > Of course, the OS can include support for multiple revs, e.g., it > could add support for Fn 0xC rev 6 and use that when available. And > it could retain support for Fn 0xC rev 5 and use that if rev 6 isn't > available. That's what I mean basically. In practice, the OS needs to assume that the firmware may not implement a certain rev and so it needs to support other revisions anyway.
