| Message ID | 20170608213106.11982-1-kilobyte@angband.pl (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Jun 8, 2017 at 11:31 PM, Adam Borowski <kilobyte@angband.pl> wrote: > This is a port of https://github.com/acpica/acpica/commit/eaa455ac by > Robert Moore to the kernel: > >> This call was simply wrong, and resulted in a -1 index into the operand >> stack. > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=120351 > Signed-off-by: Adam Borowski <kilobyte@angband.pl> > --- > You guys fixed this in your tool months ago, but it's still unfixed in the > kernel. Let's apply that patch, then. The coding styles differ so much the > patch is hardly recognizable, but it's a direct port. Lv, any comments? > drivers/acpi/acpica/dsutils.c | 9 --------- > 1 file changed, 9 deletions(-) > > diff --git a/drivers/acpi/acpica/dsutils.c b/drivers/acpi/acpica/dsutils.c > index 406edec20de7..0dabd9b95684 100644 > --- a/drivers/acpi/acpica/dsutils.c > +++ b/drivers/acpi/acpica/dsutils.c > @@ -633,15 +633,6 @@ acpi_ds_create_operand(struct acpi_walk_state *walk_state, > > if ((op_info->flags & AML_HAS_RETVAL) || > (arg->common.flags & ACPI_PARSEOP_IN_STACK)) { > - ACPI_DEBUG_PRINT((ACPI_DB_DISPATCH, > - "Argument previously created, already stacked\n")); > - > - acpi_db_display_argument_object(walk_state-> > - operands[walk_state-> > - num_operands - > - 1], > - walk_state); > - > /* > * Use value that was already previously returned > * by the evaluation of this argument > -- > 2.11.0 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, > From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of Rafael J. Wysocki > Subject: Re: [PATCH] ACPICA: Remove unnecessary call to debugger > > On Thu, Jun 8, 2017 at 11:31 PM, Adam Borowski <kilobyte@angband.pl> wrote: > > This is a port of https://github.com/acpica/acpica/commit/eaa455ac by > > Robert Moore to the kernel: > > > >> This call was simply wrong, and resulted in a -1 index into the operand > >> stack. > > > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=120351 > > Signed-off-by: Adam Borowski <kilobyte@angband.pl> > > --- > > You guys fixed this in your tool months ago, but it's still unfixed in the > > kernel. Let's apply that patch, then. The coding styles differ so much the > > patch is hardly recognizable, but it's a direct port. > > Lv, any comments? The commit was originally sent by me with this commit: https://github.com/zetalog/linux/commit/2cd640e5fd50 You can see full bug triage story here: https://bugzilla.kernel.org/show_bug.cgi?id=194845 And upstream ACPICA discussion here: https://github.com/acpica/acpica/pull/245 After the merge, bob helped to close another bug report: https://bugzilla.kernel.org/show_bug.cgi?id=120351 I already sent linuxized commit to you in the ACPICA release with restored patch description: https://patchwork.kernel.org/patch/9765837/ Such modification is what we need to do in release cycle according to the community work. You don't need to take this one. Thanks and best regards Lv > > drivers/acpi/acpica/dsutils.c | 9 --------- > > 1 file changed, 9 deletions(-) > > > > diff --git a/drivers/acpi/acpica/dsutils.c b/drivers/acpi/acpica/dsutils.c > > index 406edec20de7..0dabd9b95684 100644 > > --- a/drivers/acpi/acpica/dsutils.c > > +++ b/drivers/acpi/acpica/dsutils.c > > @@ -633,15 +633,6 @@ acpi_ds_create_operand(struct acpi_walk_state *walk_state, > > > > if ((op_info->flags & AML_HAS_RETVAL) || > > (arg->common.flags & ACPI_PARSEOP_IN_STACK)) { > > - ACPI_DEBUG_PRINT((ACPI_DB_DISPATCH, > > - "Argument previously created, already stacked\n")); > > - > > - acpi_db_display_argument_object(walk_state-> > > - operands[walk_state-> > > - num_operands - > > - 1], > > - walk_state); > > - > > /* > > * Use value that was already previously returned > > * by the evaluation of this argument > > -- > > 2.11.0
Hi, Adam > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Adam > Borowski > Sent: Friday, June 9, 2017 5:31 AM > To: Moore, Robert <robert.moore@intel.com>; Zheng, Lv <lv.zheng@intel.com>; Wysocki, Rafael J > <rafael.j.wysocki@intel.com>; Len Brown <lenb@kernel.org>; linux-acpi@vger.kernel.org; > devel@acpica.org > Cc: Adam Borowski <kilobyte@angband.pl> > Subject: [PATCH] ACPICA: Remove unnecessary call to debugger > > This is a port of https://github.com/acpica/acpica/commit/eaa455ac by > Robert Moore to the kernel: > > > This call was simply wrong, and resulted in a -1 index into the operand > > stack. > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=120351 > Signed-off-by: Adam Borowski <kilobyte@angband.pl> I think my first fix also worked and your test on bug 120351 was wrong. I just added comment a moment ago. However I just pushed the removal of the wrong debugging statement fix to the upstream. It's really redundant, you need to enable the log and check them. The patch has already been sent here: https://patchwork.kernel.org/patch/9765837/ So you don't have to send it again. Thanks Lv > You guys fixed this in your tool months ago, but it's still unfixed in the > kernel. Let's apply that patch, then. The coding styles differ so much the > patch is hardly recognizable, but it's a direct port. > > drivers/acpi/acpica/dsutils.c | 9 --------- > 1 file changed, 9 deletions(-) > > diff --git a/drivers/acpi/acpica/dsutils.c b/drivers/acpi/acpica/dsutils.c > index 406edec20de7..0dabd9b95684 100644 > --- a/drivers/acpi/acpica/dsutils.c > +++ b/drivers/acpi/acpica/dsutils.c > @@ -633,15 +633,6 @@ acpi_ds_create_operand(struct acpi_walk_state *walk_state, > > if ((op_info->flags & AML_HAS_RETVAL) || > (arg->common.flags & ACPI_PARSEOP_IN_STACK)) { > - ACPI_DEBUG_PRINT((ACPI_DB_DISPATCH, > - "Argument previously created, already stacked\n")); > - > - acpi_db_display_argument_object(walk_state-> > - operands[walk_state-> > - num_operands - > - 1], > - walk_state); > - > /* > * Use value that was already previously returned > * by the evaluation of this argument > -- > 2.11.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 09, 2017 at 01:18:25AM +0000, Zheng, Lv wrote: > > Subject: [PATCH] ACPICA: Remove unnecessary call to debugger > > > > This is a port of https://github.com/acpica/acpica/commit/eaa455ac by > > Robert Moore to the kernel: > > > > > This call was simply wrong, and resulted in a -1 index into the operand > > > stack. > > > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=120351 > > Signed-off-by: Adam Borowski <kilobyte@angband.pl> > > I think my first fix also worked and your test on bug 120351 was wrong. > I just added comment a moment ago. > > However I just pushed the removal of the wrong debugging statement fix to the upstream. > It's really redundant, you need to enable the log and check them. > > The patch has already been sent here: > https://patchwork.kernel.org/patch/9765837/ > So you don't have to send it again. Date: June 5, and it's not even in next-20170608 yet, so I obviously didn't see you applying the fix (it seemed stalled since Apr 4). If it's already on the way, that's good. However, it is not redundant: without this removal, I see the UBSAN error (at least in current mainline). Whether an alternate fix would be enough is another question. You guys know more about the issue, all I know is whether I see warnings spewed into my dmesg :) Meow!
Hi, > From: Adam Borowski [mailto:kilobyte@angband.pl] > Subject: Re: [PATCH] ACPICA: Remove unnecessary call to debugger > > On Fri, Jun 09, 2017 at 01:18:25AM +0000, Zheng, Lv wrote: > > > Subject: [PATCH] ACPICA: Remove unnecessary call to debugger > > > > > > This is a port of https://github.com/acpica/acpica/commit/eaa455ac by > > > Robert Moore to the kernel: > > > > > > > This call was simply wrong, and resulted in a -1 index into the operand > > > > stack. > > > > > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=120351 > > > Signed-off-by: Adam Borowski <kilobyte@angband.pl> > > > > I think my first fix also worked and your test on bug 120351 was wrong. > > I just added comment a moment ago. > > > > However I just pushed the removal of the wrong debugging statement fix to the upstream. > > It's really redundant, you need to enable the log and check them. > > > > The patch has already been sent here: > > https://patchwork.kernel.org/patch/9765837/ > > So you don't have to send it again. > > Date: June 5, and it's not even in next-20170608 yet, so I obviously didn't > see you applying the fix (it seemed stalled since Apr 4). If it's already > on the way, that's good. Yes, ACPICA release was frozen, waiting for spec 6.2 release. > > However, it is not redundant: without this removal, I see the UBSAN error > (at least in current mainline). We have different meaning of redundant. :) I mean: I managed to enable the debugging log, and it always dumps things in this way: ArgObj: 00ADA610 Integer 0000000000000000 ArgObj: 00ADA4F0 Integer 00000000FFFF00FF ArgObj: 00ADA4F0 Integer 00000000FFFF00FF <= redundant log generated by this line ArgObj: 00ADA580 Integer 0000000000000000 ResultObj: 00ADA710 Integer 0000000000000000 In theory, AcpiDbDisplayArgumentObject() should be invoked for each AcpiDsObjStackPush(). There are only 2 AcpiDsObjStackPush() invocations. Both of them have already been paired with AcpiDbDisplayArgumentObject() in AcpiDsCreateOperand(). This isolated AcpiDbDisplayArgumentObject() tries to find back missing stacked object debugging information. ACPI_DEBUG_PRINT((ACPI_DB_DISPATCH, "Argument previously created, already stacked\n")); However there won't be such a missing object as all operands are created via AcpiDsCreateOperand(). Finally this invocation only generates above redundant debugging logs. Cheers, Lv > > Whether an alternate fix would be enough is another question. You guys know > more about the issue, all I know is whether I see warnings spewed into my > dmesg :) > > > Meow! > -- > ⢀⣴⠾⠻⢶⣦⠀ A tit a day keeps the vet away. > ⣾⠁⢰⠒⠀⣿⡁ > ⢿⡄⠘⠷⠚⠋⠀ (Rejoice as my small-animal-murder-machine got unbroken after > ⠈⠳⣄⠀⠀⠀⠀ nearly two years of no catch!)
diff --git a/drivers/acpi/acpica/dsutils.c b/drivers/acpi/acpica/dsutils.c index 406edec20de7..0dabd9b95684 100644 --- a/drivers/acpi/acpica/dsutils.c +++ b/drivers/acpi/acpica/dsutils.c @@ -633,15 +633,6 @@ acpi_ds_create_operand(struct acpi_walk_state *walk_state, if ((op_info->flags & AML_HAS_RETVAL) || (arg->common.flags & ACPI_PARSEOP_IN_STACK)) { - ACPI_DEBUG_PRINT((ACPI_DB_DISPATCH, - "Argument previously created, already stacked\n")); - - acpi_db_display_argument_object(walk_state-> - operands[walk_state-> - num_operands - - 1], - walk_state); - /* * Use value that was already previously returned * by the evaluation of this argument