Bug 9487
|
Description
Tom Jaeger
2007-12-01 18:27:38 UTC
Created attachment 13819 [details]
dmesg output
Created attachment 13820 [details]
kernel config
Created attachment 13821 [details]
dmesg output (latest kernel from git)
Created attachment 13822 [details]
the DSDT
Not sure if this is helpful, if I uncomment line 109 res->irq_resource[i].flags |= irq_flags(triggering, polarity); from drivers/pnp/pnpacpi/rsparser.c in the latest kernel, the problem goes away. Another observation:
rsparser.c, pnpacpi_encode_irq, around line 765:
if (triggering == ACPI_EDGE_SENSITIVE)
resource->data.irq.sharable = ACPI_EXCLUSIVE;
else
resource->data.irq.sharable = ACPI_SHARED;
It appears that the IRQ is considered to be shared iff it is level-triggered. Is this a safe assumption? Because if I delete the above code and always set resource->data.irq.sharable to be ACPI_SHARED, the serial port wakes up properly. Thanks, Tom.
Just wanted to confirm this bug on a Toshiba M200 Tablet laptop with a serial-based Wacom tablet which doesn't work after suspend to RAM or suspend to disk. Tom Jaegers modifications work for me on fixing this. Many Ubuntu users have confirmed this issue (https://bugs.launchpad.net/ubuntu/+source/linux-source-2.6.22/+bug/152187). While Tom's fixes work for tablets on TOshiba laptops, its probably not an ultimately safe or desirable way to fix this issue. Anyone more familiar with the hardware or issue at hand willing to commment? Created attachment 14149 [details]
Buffer that is passed to acpi_set_current_resources
Created attachment 14150 [details] Buffer that was passed to acpi_set_current_resources before 2.6.21 The serial line used to wake up in older kernels, but that was pure coincidence since the relevant part of the buffer was computed from uninitialized data (see bug 6316, referenced above). The AML buffers are more interesting (cf. ACPI spec, p. 203): Here's what _CRS returns: 47 01 38 03 38 03 01 08 // IO 23 10 00 01 // IRQ: IRQ 4, 3 bytes, High true, edge sensitive, non-shareable 79 8c // END TAG And here's what the kernel passes to _SRS: 47 01 38 03 3f 03 00 08 // IO 22 10 00 // IRQ: IRQ 4, 2 bytes, hence it defaults to High true, edge sensitive, non-shareable 79 00 00 // END TAG Apparently the firmware has problems interpreting 2-byte IRQ descriptors. If I remove the code in drivers/acpi/resources/rsio.c that checks if the third byte is necessary, the following AML buffer is passed instead: 47 01 38 03 3f 03 00 08 23 10 00 01 79 00 and the device resumes properly. I'm not sure what to do now, since what the kernel does is correct the problem is buggy firmware. Just disabling the check could potentially break devices that are not standard-compliant in the opposite direction (I'm not sure how likely this is though). I think there is something to be said about being as faithful as possible to what _CRS returns, though. So maybe adding a field u8 use_3_byte_descriptor to acpi_resource_irq that keeps track of whether _CRS returned a 3-byte descriptor even though this wasn't necessary would be a better idea? Created attachment 14185 [details]
patch that fixes the issue on my machine
I took the second route, adding a field preserve_four_byte_descriptor to acpi_resource_irq and acpi_resource_extended_irq that keeps track of whether _CRS returned a four-byte descriptor, in which case such a descriptor is also passed to _SRS.
Sorry about talking about three-byte descriptors instead of four-byte ones in my earlier port, the first byte certainly does belong to the descriptor.
The patch also fixes an obvious bug in pnpacpi_encode_irq where IRQ resource information is not properly restored if the IRQ is shared. I didn't change the part where shareable is deduced from triggering which I'm pretty sure is also a bug.
TODO: Choose shorter variable names.
I wrote earlier that > Just disabling the check could potentially break devices that are not > standard-compliant in the opposite direction Make that "*will* break devices". The Fujitsu Lifebook C1020 from bug 6316 always expects three-byte descriptors if they are allowed. So it seems that the additional bookkeeping that I did in the patch cannot be avoided. Created attachment 14193 [details]
new patch
* Fixes a number of bugs in the first patch
Created attachment 14195 [details]
patch
cosmetic changes
Your patch looks reasonable. One comment: I suggest not adding IORESOURCE_IRQ_DESCRIPTOR_4_BYTES in generic header, as this is quite ACPI specific. You can set resource's descriptor_length in 'pnpacpi_type_resources'. as a patch is attached that is intended to address the problem, I'm setting this report to RESOLVED, so it will be noticed for review and testing. Also setting sub-category to ACPICA-Core since this proposes a change to the generic ACPICA resource code. Created attachment 14230 [details]
patch
* moved the definition of IORESOURCE_IRQ_DESCRIPTOR_4_BYTES to rsparser.c.
Thanks for your comments. The only place where IORESOURCE_IRQ_DESCRIPTOR_4_BYTES is used is in rsparser.c, so I moved its definition there. I don't understand your comment about pnpacpi_type_resources.
Created attachment 14257 [details]
a modified version
Tom, how about the slightly modified version?
Moving ORESOURCE_IRQ_DESCRIPTOR_4_BYTES doesn't work, because this bit might be used by generic code later. Actually we don't need the extra bit.
mark bug as "patch available" according to comment# 16. David, thanks, I see, that is much more elegant. Just tested it and it works perfectly. patch in comment #18 applied to acpi test I'd almost say to heck with trying to optimize a 3-byte IRQ descriptor to the 2-byte version and always output the 3-byte version. This would actually *remove* some ACPICA code, and I'll bet this is exactly what Windows does. I don't think that is what Windows does since bug 6316 was caused by a device that couldn't deal with an unoptimized IRQ descriptor. What we seem to be saying here is that there is some requirement that the _SRS buffer be the exact same size as the _CRS buffer, at least on some systems. The ACPI spec only says that the descriptors must be in the same order as listed in the _CRS buffer. I suppose that we might infer from this that the *same* descriptors must appear, therefore we cannot change IRQ to IRQ_NO_FLAGS. I'll ask Phoenix what they think about this. From Phoenix: "The original PNP spec stated that the exact format of _CRS must be matched with the subsequent _SRS" So, the patch above seems to be moving in the right direction. The ACPI spec could also use a few more words about this. I think that there may be at least one more descriptor like this (DMA?) The only other descriptor that has optional fields is the StartDependentFunctions descriptor. I have an issue with: if (data.irq.descriptor_length == ACPI_DESCRIPTOR_4_BYTES) If the field is named "length", I think that it should hold the actual length of the descriptor, not an encoded length. If the length is indeed a flag, the field should be: if (data.irq.flags & ACPI_DESCRIPTOR_4_BYTES) Otherwise, the name "length" is misleading and could lead to bugs later. The options for the StartDependentFunctions are length 0 and length 1, so it may be best to simply use the actual length, otherwise we end up with flags like this: ACPI_DESCRIPTOR_IRQ_3_BYTES ACPI_DESCRIPTOR_IRQ_4_BYTES ACPI_DESCRIPTOR_SDF_0_BYTES ACPI_DESCRIPTOR_SDF_1 BYTE Created attachment 14744 [details]
Alternate implementation of ACPICA-core changes
This patch provides a more complete fix to the problem.
1. Handles both IRQ and StartDependentFn descriptors
2. For each descriptor type, handles both types of cases: where the descriptor should not be optimized, and where the descriptor should not be "de-optimized".
3. The DescriptorLength field is an actual length, not a "flags" byte.
Patch does not include Linux changes required, and is not the Linuxized version of the ACPICA code -- therefore, it is for review only at this time.
Looks good. I've just got one question: If an invalid DescriptorLength is passed, AcpiRsSet* falls back on the old behavior of optimizing the descriptor if possible. Is there a reason for this? Because if we fully trust DescriptorLength this shouldn't be necessary and if we don't it's probably safer to only optimize if the other flags allow it. I was thinking that if a "valid" DescriptorLength is passed (2 or 3), then we use that value. Anything else means "don't care", let the code figure out what is best. This at least makes some attempt to keep the existing behavior without forcing existing resource code above acpica to change. The problem I see concerns StartDependentFn descriptors more than it does IRQ descriptors. If we take the linux kernel as an example, pnpacpi_build_resource_template in rsparser.c fills the resource template with zeros before populating it. So in its current form, it would pass a descriptor_length of zero to the ACPICA code, which would mean it could never produce a StartDependentFn resource of length one, not even if the value of CompatibilityPriority or PerformanceRobustness requires this, which is the current behavior. Created attachment 14848 [details]
separated parts of the earlier patch independent from discriptor length issues
The issues were:
* if the ACPI_SHARED flag is set, triggering and polarity are not properly set (pnp_acpi_encode_irq).
* sharable is deduced from triggering instead of using the appropriate IORESOURCE_IRQ_SHAREABLE flag
* in addition to that, pnpacpi_encode_ext_irq writes into resource->data.irq.sharable instead of resource->data.extended_irq.sharable.
These issues are independent from the current bug, as far as I can see.
Created attachment 15002 [details]
Linuxized version of the patch in #28
I've adapted the proposed ACPCIA changes to the current linux kernel. I don't know if this is the way it's supposed to be done or if there's a script for this sort of thing; anyway, it wasn't much work and it makes it possible to test the proposed changes. I haven't noticed any problems with the patch and I can confirm that it fixes the original issue.
There was a minor issue in the earlier patch: Somewhere around line 350 in rsio.c,
{ACPI_RSC_LENGTH, 0, 0, sizeof (AML_RESOURCE_IRQ)},
should be replaced by
{ACPI_RSC_LENGTH, 0, 0, sizeof (AML_RESOURCE_START_DEPENDENT)},
Also, TotalSize could probably decremented for short START_DPF descriptors as well.
We have a utility called acpisrc that converts the code to Linux format. We then lindent the code and create the patch(es). I agree with both changes above. Bob, Is the fix in the recent ACPICA release? Yes, it is. The change was released in ACPICA 20080213: Fixed a problem where resource descriptor size optimization could cause a problem when a _CRS resource template is passed to a _SRS method. The _SRS resource template must use the same descriptors (with the same size) as returned from _CRS. This change affects the following resource descriptors: IRQ / IRQNoFlags and StartDependendentFn / StartDependentFnNoPri. (BZ 9487) this is technically "RESOLVED" b/c we have a patch that works. However, we need to: 1. attach the actual pach that made it into ACPICA (linuxized) 2. get that patch into Linux (when this bug will be CLOSED) Created attachment 15439 [details]
linuxified acpica resource changes b/w 20080123 and 20080321
Created attachment 15440 [details]
required linux-side changes
The patch in comment #39 was created automatically using acpisrc and lindent. #40 contains the required declarations and code that transfers the information from _CRS to _SRS using the method suggested by Shaohua. Also, the patch in comment #32 contains a few bugfixes that are unrelated to the issue at hand. Created attachment 15978 [details]
rsparser.c changes
It seems that in order for the descriptor length information to be passed to _SRS and in order to avoid regressions related to start_dpf, the above patch is still needed.
Comment 42#: Please explain in detail why this patch is needed, thanks. Linux doesn't use acpi_resources throughout, but instead stores the information in generic resources (pnpacpi_parse_allocated_*), which are encoded to acpi_resources (pnpacpi_encode_*) right before acpi_set_current_resources is called in pnpacpi_set_resources. Since we want to avoid keeping track of the descriptor_length by adding a flag to the generic struct resource, Shaohua suggested in comment #15 that the device's resource be bypassed by saving the descriptor_length information directly to the resource template when it is created. This is what the patch in comment #42 does. Unfortunately, I don't currently have working hardware to test this on. Created attachment 15993 [details]
patch included in linux-2.6.25-rc16
The attached patch shipped in linux-2.6.25-rc16.
1d5b285da1893b90507b081664ac27f1a8a3dc5b
ACPICA: Fix for resource descriptor optimization issues for _CRS/_SRC
Are we still broken?
> Are we still broken?
Yes, I think so, but that is based on looking at the code, not on actual testing since my R25 won't let me boot anymore and is lying disassembled on the floor of my apartment. I might be able to get someone on the ubuntu bug tracker to test this if you feel that's necessary.
yes, it would be great if we could find out if 2.6.25 fixed this or if we still have this problem. Created attachment 16262 [details] PNPACPI: use _CRS IRQ descriptor length for _SRS This bug is not fixed in 2.6.25. I have tested 2.6.26-rc3 + this patch, and it fixes the problem on my Toshiba Portege 4000. This is an extension of Tom Jaeger's patch in comment #42, which was not quite sufficient. shipped in 2.6.26-rc6 -- closed. commit e9fe9e188118a0a34c6200d9b10ea6247f53592d Author: Bjorn Helgaas <bjorn.helgaas@hp.com> Date: Mon Jun 9 16:52:04 2008 -0700 pnpacpi: fix IRQ flag decoding commit a993273beae8022390e48fe9205480565ad470ab Author: Bjorn Helgaas <bjorn.helgaas@hp.com> Date: Mon Jun 9 16:52:05 2008 -0700 pnpacpi: fix shareable IRQ encode/decode commit 36d872a370d3d10e5a7faa9dcacce744260fb13b Author: Bjorn Helgaas <bjorn.helgaas@hp.com> Date: Mon Jun 9 16:52:06 2008 -0700 PNPACPI: use _CRS IRQ descriptor length for _SRS |