Hi,
Sorry for the delay.
On Thu, Nov 21, 2024 at 10:57 PM Miao Wang <shankerwangmiao@xxxxxxxxx> wrote:
>
> 2023年11月7日 00:09，Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> 写道：
> >
> > +static acpi_status acpi_bus_check_add(acpi_handle handle, bool first_pass,
> > struct acpi_device **adev_p)
> > {
> > struct acpi_device *device = acpi_fetch_acpi_dev(handle);
> > @@ -2054,9 +2059,25 @@ static acpi_status acpi_bus_check_add(ac
> > if (acpi_device_should_be_hidden(handle))
> > return AE_OK;
> >
> > - /* Bail out if there are dependencies. */
> > - if (acpi_scan_check_dep(handle, check_dep) > 0)
> > - return AE_CTRL_DEPTH;
> > + if (first_pass) {
> > + acpi_mipi_check_crs_csi2(handle);
> > +
> > + /* Bail out if there are dependencies. */
> > + if (acpi_scan_check_dep(handle) > 0) {
> > + /*
> > + * The entire CSI-2 connection graph needs to be
> > + * extracted before any drivers or scan handlers
> > + * are bound to struct device objects, so scan
> > + * _CRS CSI-2 resource descriptors for all
> > + * devices below the current handle.
> > + */
> > + acpi_walk_namespace(ACPI_TYPE_DEVICE, handle,
> > + ACPI_UINT32_MAX,
> > + acpi_scan_check_crs_csi2_cb,
> > + NULL, NULL, NULL);
> > + return AE_CTRL_DEPTH;
> > + }
> > + }
> >
> > fallthrough;
> > case ACPI_TYPE_ANY: /* for ACPI_ROOT_OBJECT */
> >
>
> Hi, I'd like to report some issues caused by this patch. Correct me if I'm wrong
> since I'm not expert on ACPI. Before this patch, properties of ACPI devices with
> _DEP relationship declared were evaluated after the initialization of the
> depending devices, i.e. the execution of handler->attach(). With this patch,
> _CRS of all the ACPI devices are evaluated to check if the device contains a
> CSI2 resource, regardless of the declaration of _DEP relationship.
Yes, but the _DEP information is not relevant for whether or not _CRS
may be evaluated at all.
> The evaluation of _CRS is even before _STA
Not really. _STA has already been evaluated by
acpi_ns_init_one_device() for all devices at this point.
> and other methods indicating the status of the device.
No, but it doesn't matter, at least by the spec. Had the device been
disabled, _CRS would have been expected to work anyway:
"If a device is disabled, then _CRS returns a valid resource template
for the device, but the actual resource assignments in the return byte
stream are ignored." (ACPI 6.5, Section 6.2.2. _CRS (Current Resource
Settings)).
That said, adding a device status check before the _CRS evaluation in
acpi_bus_check_add() is not inconceivable.
> This has caused some issues in certain scenarios, where the DSDT contains legacy
> devices, which requires reading IO ports to form the result of its _CRS. An
> example of such a legacy device is declared in the DSDT is as below:
>
> Device (LPT)
> {
> Name (_HID, EisaId ("PNP0400") /* Standard LPT Parallel Port */)
> Name (_UID, 0x02) // _UID: Unique ID
> Name (_DDN, "LPT1") // _DDN: DOS Device Name
> Name (_DEP, Package (0x01) // _DEP: Dependencies
> {
> \_SB.PCI0
> })
> OperationRegion (ITE1, SystemIO, 0x2E, 0x02)
> Field (ITE1, ByteAcc, NoLock, Preserve) {INDX, 8, DATA, 8}
> IndexField (INDX, DATA, ByteAcc, NoLock, Preserve)
> {
> // Omitting some declarations of fields
> IOAH, 8,
> IOAL, 8,
> }
> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
> {
> Name (BUF0, ResourceTemplate ()
> {
> IO (Decode16,
> 0x0378, // Range Minimum
> 0x0378, // Range Maximum
> 0x01, // Alignment
> 0x08, // Length
> _Y01)
> IRQ (Level, ActiveLow, Exclusive, _Y02)
> {7}
> })
> CreateByteField (BUF0, \LPT._CRS._Y01._MIN, IOLO)
> CreateByteField (BUF0, 0x03, IOHI)
> CreateByteField (BUF0, \LPT._CRS._Y01._MAX, IORL)
> CreateByteField (BUF0, 0x05, IORH)
> CreateByteField (BUF0, \LPT._CRS._Y01._LEN, LEN1)
> CreateByteField (BUF0, \LPT._CRS._Y02._INT, IRQL)
>
> IOLO = IOAL /* \LPT_.IOAL */
> IORL = IOAL /* \LPT_.IOAL */
> IOHI = IOAH /* \LPT_.IOAH */
> IORH = IOAH /* \LPT_.IOAH */
> // Omitting some assignments and calculations
>
> Return (BUF0)
> }
> }
>
> On non-x86 platforms, IO ports are implemented using MMIO. The memory is
> remapped in the initialization of the PCI root, using its QWordIO resource
> declared in its _CRS, in acpi_pci_root_remap_iospace(). Before the
> initialization of the PCI root, reads and writes of the IO ports will result in
> accessing memory region which is not mapped.
That's not really straightforward.
> Before this patch, adding a _DEP of Package(){PCI0} to these legacy devices will
> postpone the initialization of these devices after the PCI root, solving the
> above problem. After this patch, however, the evaluation of _CRS regardless of
> _DEP declarations causes accessing IO ports before they are ready.
Well, before this patch, Linux simply did not have to evaluate _CRS
during device discovery.
Nowhere in the spec it is stated that _CRS cannot be evaluated for a
given device before enumerating all devices listed by its _DEP object.
As it is currently defined, _DEP is only about operation region
dependencies, not even about general device enumeration ordering (even
though it is used for enforcing specific enumeration ordering in the
field because OSes tend to enumerate devices in the order following
from _DEP as a matter of fact).
> I've checked the ACPI spec, and it states that ``OSPM must guarantee that the
> following operation regions are always accessible: SystemIO operation regions"
Which to me is clearly at odds with the SystemIO implementation
description above.
> and ``Since the region types above are permanently available, no _REG methods
> are required, nor will OSPM evaluate any _REG methods that appear in the same
> scope as the operation region declaration(s) of these types". It seems that
> from the view of the AML codes in the DSDT, it can never know when the IO ports
> are ready, and it is impossible for Linux on non-x86 platforms to ensure IO
> ports are always accessible.
They aren't always accessible anyhow. They become accessible after
enumerating the PCI host bridge and they aren't accessible earlier.
> I wonder if there would be a better solution to this problem.
Well, it is a bit unfortunate that it took 6 kernel release cycles to
realize that there was a problem. Had this been reported earlier,
there would have been more options on the table.
At this point, the functionality related to CSI-2 connection graphs
simply needs to be there, the only option for avoiding _CRS evaluation
in acpi_bus_check_add() would be to somehow defer the enumeration of
all devices using CSI-2 connections until the host bridge is
enumerated. Alternatively, the enumeration of the PCI host bridge
might be pushed forward, but that would require addressing at least a
few enumeration ordering issues. None of these would be a small
change and backporting any of that would have been a considerable
effort.
One of the things worth considering is whether or not CSI-2 is
relevant for the architectures that rely on the old behavior related
to _DEP. If not at all, a Kconfig option could be added to disable
CSI-2 graphs enumeration on those architectures and that would make
the problem go away.
Also I'm wondering if the firmware could be updated to survive a mere
evaluation of _CRS if the hardware is not ready.
Thanks!
