Re: [Devel] [PATCH 1/2] acpi: Avoid soft lockup complaints during boot
by Rafael J. Wysocki
On Fri, Feb 23, 2018 at 5:18 PM, Bart Van Assche <Bart.VanAssche(a)wdc.com> wrote:
> On Fri, 2018-02-23 at 11:05 +0100, Rafael J. Wysocki wrote:
>> On Fri, Feb 23, 2018 at 1:22 AM, Bart Van Assche <bart.vanassche(a)wdc.com> wrote:
>> > Avoid that the following is reported during boot with kmemleak
>> > enabled:
>>
>> OK, so why is this fix the right one in your view?
>
> No Linux kernel code should keep running for multiple seconds without giving
> the scheduler a chance to schedule another thread. Hence the insertion of the
> cond_resched() call.
I see.
> It is very well possible that the fact that this code keeps the CPU for
> itself so long is a bug by itself. However, I'm not familiar enough with the
> ACPI code to figure out whether any alternative solutions exist that would be
> better than the patch I posted.
OK, we'll look at the code and see how it can be improved upstream.
Thanks a lot for letting us know about this issue!
4 years, 5 months
Re: [Devel] [PATCH] ACPICA: Silent warnings about empty body in if/else statement
by Rafael J. Wysocki
On Tue, Feb 27, 2018 at 1:37 PM, Jean Delvare <jdelvare(a)suse.de> wrote:
> On Tue, 27 Feb 2018 11:14:07 +0100, Rafael J. Wysocki wrote:
>> On Tue, Feb 27, 2018 at 10:23 AM, Jean Delvare <jdelvare(a)suse.de> wrote:
>> > OK, thanks for the information. I'll update the i2c-scmi driver to
>> > no longer use ACPI_DEBUG_PRINT. There are a few other drivers using it as
>> > well though (xo15-ebook and panasonic-laptop, as well as
>> > xen-acpi-memhotplug - not sure if you consider that one as more
>> > legitimate.)
>>
>> None of them is valid IMO. ACPI_DEBUG_PRINT() really should only be
>> used in the ACPICA code.
>
> What about ACPI_ERROR()? The i2c-scmi driver uses these as well. Should
> I convert them to dev_err()?
I think so.
That or acpi_handle_error().
4 years, 5 months
Re: [Devel] [PATCH] ACPICA: Silent warnings about empty body in if/else statement
by Rafael J. Wysocki
On Tue, Feb 27, 2018 at 10:23 AM, Jean Delvare <jdelvare(a)suse.de> wrote:
> Hi Rafael,
Hi,
> On Mon, 26 Feb 2018 23:11:37 +0100, Rafael J. Wysocki wrote:
>> On Mon, Feb 26, 2018 at 11:34 AM, Jean Delvare <jdelvare(a)suse.de> wrote:
>> > When ACPI debugging is disabled, I see warnings like this one:
>> >
>> > drivers/i2c/busses/i2c-scmi.c: In function "acpi_smbus_cmi_add_cap":
>> > drivers/i2c/busses/i2c-scmi.c:328:39: warning: suggest braces around empty body in an "else" statement [-Wempty-body]
>> > drivers/i2c/busses/i2c-scmi.c:338:12: warning: suggest braces around empty body in an "else" statement [-Wempty-body]
>> >
>> > It is caused by ACPI_DEBUG_PRINT (or other similar macros) resolving
>> > to nothing. Make them resolve to the classic "do {} while (0)"
>> > construct instead if the compiler likes that, or just {} if not, to
>> > silent all such warnings.
>>
>> So first of all, acpi_smbus_cmi_add_cap() shouldn't really use
>> ACPI_DEBUG_PRINT() and similar. They belong to ACPICA and their use
>> should be limited to it.
>
> OK, thanks for the information. I'll update the i2c-scmi driver to
> no longer use ACPI_DEBUG_PRINT. There are a few other drivers using it as
> well though (xo15-ebook and panasonic-laptop, as well as
> xen-acpi-memhotplug - not sure if you consider that one as more
> legitimate.)
None of them is valid IMO. ACPI_DEBUG_PRINT() really should only be
used in the ACPICA code.
And we have acpi_handle_debug() even for everybody else.
>> I know that they are used in the other parts of the ACPI subsystem,
>> but they really should be replaced with the kernel's proper debug
>> statements in there.
>
> Ideally all these macros shouldn't be exposed to drivers which are not
> supposed to use them. Could they be moved to a header file internal to
> at least acpi?
We get that code from the upstream and I don't want to differ from it
in arbitrary ways. I'm not sure about the possible consequences of
that change in the upstream code ATM, though.
4 years, 5 months
Re: [Devel] [PATCH] ACPICA: Silent warnings about empty body in if/else statement
by Rafael J. Wysocki
On Mon, Feb 26, 2018 at 11:34 AM, Jean Delvare <jdelvare(a)suse.de> wrote:
> When ACPI debugging is disabled, I see warnings like this one:
>
> drivers/i2c/busses/i2c-scmi.c: In function "acpi_smbus_cmi_add_cap":
> drivers/i2c/busses/i2c-scmi.c:328:39: warning: suggest braces around empty body in an "else" statement [-Wempty-body]
> drivers/i2c/busses/i2c-scmi.c:338:12: warning: suggest braces around empty body in an "else" statement [-Wempty-body]
>
> It is caused by ACPI_DEBUG_PRINT (or other similar macros) resolving
> to nothing. Make them resolve to the classic "do {} while (0)"
> construct instead if the compiler likes that, or just {} if not, to
> silent all such warnings.
So first of all, acpi_smbus_cmi_add_cap() shouldn't really use
ACPI_DEBUG_PRINT() and similar. They belong to ACPICA and their use
should be limited to it.
I know that they are used in the other parts of the ACPI subsystem,
but they really should be replaced with the kernel's proper debug
statements in there.
Thanks,
Rafael
4 years, 5 months
Re: [Devel] [PATCH v13 3/4] thinkpad_acpi: Add support for battery thresholds
by Andy Shevchenko
On Sat, Feb 10, 2018 at 10:48 AM, Ognjen Galić <smclt30p(a)gmail.com> wrote:
> Do you guys want me to send in another revision of the patch with some
> documentation on the sysfs API?
I noticed I didn't apply it because of some pending changes discussed,
perhaps this one above.
So, definitely please send a new version which addresses comments.
--
With Best Regards,
Andy Shevchenko
4 years, 5 months
Re: [Devel] [PATCH v13 3/4] thinkpad_acpi: Add support for battery thresholds
by Rafael J. Wysocki
On Sat, Feb 24, 2018 at 11:31 AM, Ognjen Galić <smclt30p(a)gmail.com> wrote:
> On Sat, Feb 24, 2018 at 10:07:20AM +0100, Rafael J. Wysocki wrote:
>> On Fri, Feb 23, 2018 at 6:21 PM, Andy Shevchenko
>> <andy.shevchenko(a)gmail.com> wrote:
>> > On Sat, Feb 10, 2018 at 10:48 AM, Ognjen Galić <smclt30p(a)gmail.com> wrote:
>> >
>> >> Do you guys want me to send in another revision of the patch with some
>> >> documentation on the sysfs API?
>> >
>> > I noticed I didn't apply it because of some pending changes discussed,
>> > perhaps this one above.
>> >
>> > So, definitely please send a new version which addresses comments.
>>
>> No, it actually has been applied already.
>
> So it has been queued up for 4.17?
Yes, it has.
> If it has, that is awesome! Thanks!
No biggie.
That said, I would like you to send a follow-up patch moving the
definitions of the charge_start_threshold and charge_stop_threshold
attributes to the power supply core as requested by Sebastian and
documenting them.
Of course, it still needs to be possible to add/remove them on demand
after registering the power supply to cover the use case at hand
AFAICS.
Thanks,
Rafael
4 years, 5 months
Re: [Devel] [PATCH] Fix null deref in acpi_ns_check_package_list
by Moore, Robert
> -----Original Message-----
> From: C0deAi [mailto:techsupport@mycode.ai]
> Sent: Thursday, February 22, 2018 9:14 AM
> To: Moore, Robert <robert.moore(a)intel.com>; Schmauss, Erik
> <erik.schmauss(a)intel.com>; Wysocki, Rafael J
> <rafael.j.wysocki(a)intel.com>; lenb(a)kernel.org; linux-
> acpi(a)vger.kernel.org; devel(a)acpica.org
> Subject: [PATCH] Fix null deref in acpi_ns_check_package_list
>
> Hi my name is Benjamin Bales.
>
> I am the founder and creator of CodeAI,
> the first non-human contributor to your software project. CodeAI finds
> and fixes security defects for you. It fixed 327. It wants to merge a
> fix for a null dereference. To view all 327 fixed issues from the run
> claim your free open source account at mycode.ai and the Dockerfile used
> to build and run your project in CodeAI, here-
> https://drive.google.com/drive/folders/1KB9WQQyWQgYccmiSjy2E1JWJ4vWuoLYd
> .
> It is always free for open source projects.
>
> If you have any questions about these results or have general inquiries
> about CodeAI, please send an email to techsupport(a)mycode.ai
>
> Signed-off-by: Benjamin Bales <techsupport(a)mycode.ai>
> ---
> drivers/acpi/acpica/nsprepkg.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/acpi/acpica/nsprepkg.c
> b/drivers/acpi/acpica/nsprepkg.c index 7805d5c..d1bfd86 100644
> --- a/drivers/acpi/acpica/nsprepkg.c
> +++ b/drivers/acpi/acpica/nsprepkg.c
> @@ -437,6 +437,8 @@ acpi_ns_check_package_list(struct acpi_evaluate_info
> *info,
> */
> for (i = 0; i < count; i++) {
> sub_package = *elements;
> + if (sub_package == NULL)
> + break;
> sub_elements = sub_package->package.elements;
> info->parent_package = sub_package;
>
The story here is that the SubPackage pointer is *guaranteed* to be valid at this point in the code. It has already be validated and all NULL elements have been removed.
/*
* Validate each subpackage in the parent Package
*
* NOTE: assumes list of subpackages contains no NULL elements.
* Any NULL elements should have been removed by earlier call
* to AcpiNsRemoveNullElements.
*/
for (i = 0; i < Count; i++)
{
SubPackage = *Elements;
SubElements = SubPackage->Package.Elements;
Info->ParentPackage = SubPackage;
> --
> 2.7.4
4 years, 5 months
Re: [Devel] [PATCH 2/2] acpi: Avoid that acpi_ns_evaluate() triggers a soft lockup complaint
by Rafael J. Wysocki
On Fri, Feb 23, 2018 at 1:22 AM, Bart Van Assche <bart.vanassche(a)wdc.com> wrote:
> This patch avoids that the following is reported during boot with
> kmemleak and other kernel debugging options enabled:
Again, it would be sort of nice to explain why you want to fix the
issue this particular way.
We actually can't add cond_resched() to these code paths directly,
because the code in question comes from an external project, so we
need to understand the issue and see if it can be fixed differently.
> watchdog: BUG: soft lockup - CPU#0 stuck for 22s! [swapper:1]
> CPU: 0 PID: 1 Comm: swapper Not tainted 4.15.0-rc7-00645-g248c211 #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
> RIP: 0010:_raw_spin_unlock_irqrestore+0x43/0x51
> Call Trace:
> free_debug_processing+0x273/0x35a
> __slab_free+0x9d/0x613
> qlist_free_all+0x68/0x89
> quarantine_reduce+0x17c/0x1ad
> kasan_kmalloc+0x23/0x8e
> kmem_cache_alloc+0x143/0x1b4
> acpi_ut_create_generic_state+0x54/0x8c
> acpi_ut_create_update_state+0xe/0x9c
> acpi_ut_create_update_state_and_push+0x13/0x31
> acpi_ut_update_object_reference+0x243/0x40e
> acpi_ds_clear_operands+0x8c/0xc4
> acpi_ds_exec_end_op+0x5d3/0xbef
> acpi_ps_parse_loop+0x107f/0x1108
> acpi_ps_parse_aml+0x1e0/0x6a6
> acpi_ps_execute_method+0x48d/0x4ea
> acpi_ns_evaluate+0x665/0x857
> acpi_ut_evaluate_object+0xdc/0x32a
> acpi_rs_get_prt_method_data+0x64/0xa5
> acpi_get_irq_routing_table+0x72/0x94
> acpi_pci_irq_find_prt_entry+0x155/0x7f4
> acpi_pci_irq_lookup+0x6e/0x46c
> acpi_pci_irq_enable+0x161/0x461
> do_pci_enable_device+0x85/0x143
> pci_enable_device_flags+0x206/0x23c
> virtio_pci_probe+0x17a/0x232
> pci_device_probe+0x1a8/0x292
> driver_probe_device+0x30c/0x644
> __driver_attach+0xfa/0x139
> bus_for_each_dev+0xf8/0x12e
> bus_add_driver+0x29b/0x44e
> driver_register+0x252/0x2c6
> do_one_initcall+0xfc/0x212
> kernel_init_freeable+0x25a/0x2ea
> kernel_init+0x7/0xef
> ret_from_fork+0x24/0x30
>
> Reported-by: Fenguang Wu <fengguang.wu(a)intel.com>
> Signed-off-by: Bart Van Assche <bart.vanassche(a)wdc.com>
> Cc: Fenguang Wu <fengguang.wu(a)intel.com>
> ---
> drivers/acpi/acpica/psparse.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/acpi/acpica/psparse.c b/drivers/acpi/acpica/psparse.c
> index 2474ff961294..482d7d5ac021 100644
> --- a/drivers/acpi/acpica/psparse.c
> +++ b/drivers/acpi/acpica/psparse.c
> @@ -694,6 +694,8 @@ acpi_status acpi_ps_parse_aml(struct acpi_walk_state *walk_state)
> }
>
> acpi_ds_delete_walk_state(previous_walk_state);
> +
> + cond_resched();
> }
>
> /* Normal exit */
> --
> 2.16.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo(a)vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
4 years, 5 months
Re: [Devel] [PATCH 1/2] acpi: Avoid soft lockup complaints during boot
by Rafael J. Wysocki
On Fri, Feb 23, 2018 at 1:22 AM, Bart Van Assche <bart.vanassche(a)wdc.com> wrote:
> Avoid that the following is reported during boot with kmemleak
> enabled:
OK, so why is this fix the right one in your view?
> NMI watchdog: BUG: soft lockup - CPU#0 stuck for 21s! [swapper/0:1]
> Modules linked in:
> irq event stamp: 18944138
> hardirqs last enabled at (18944137): [<ffffffff810ac8ac>] __raw_spin_lock_init+0x1c/0x50
> hardirqs last disabled at (18944138): [<ffffffff815b2fe4>] apic_timer_interrupt+0x84/0x90
> softirqs last enabled at (18940166): [<ffffffff8105e59b>] __do_softirq+0x1cb/0x230
> softirqs last disabled at (18940161): [<ffffffff8105e788>] irq_exit+0xa8/0xb0
> RIP: print_context_stack+0x7b/0xc0
> Call Trace:
> [<ffffffff8101ccb6>] dump_trace+0x116/0x300
> [<ffffffff81028726>] save_stack_trace+0x26/0x50
> [<ffffffff811939f7>] create_object+0x137/0x2d0
> [<ffffffff815a7509>] kmemleak_alloc+0x49/0xa0
> [<ffffffff811789c5>] kmem_cache_alloc+0xd5/0x190
> [<ffffffff8137d204>] acpi_ut_allocate_object_desc_dbg+0x57/0x101
> [<ffffffff8137d399>] acpi_ut_create_internal_object_dbg+0x4e/0x10f
> [<ffffffff8134f2df>] acpi_ds_build_internal_object+0x201/0x2b3
> [<ffffffff8134f4fa>] acpi_ds_build_internal_package_obj+0x169/0x281
> [<ffffffff81350043>] acpi_ds_eval_data_object_operands+0x13d/0x1db
> [<ffffffff81351877>] acpi_ds_exec_end_op+0x41f/0x6be
> [<ffffffff81371876>] acpi_ps_parse_loop+0x7e8/0x884
> [<ffffffff81372ade>] acpi_ps_parse_aml+0x1ab/0x48e
> [<ffffffff8134b762>] acpi_ds_execute_arguments+0x183/0x1ca
> [<ffffffff8134bbf6>] acpi_ds_get_package_arguments+0xf3/0x11c
> [<ffffffff8136b142>] acpi_ns_init_one_object+0xb0/0xfc
> [<ffffffff8136f140>] acpi_ns_walk_namespace+0x120/0x271
> [<ffffffff8136fa20>] acpi_walk_namespace+0xf4/0x13f
> [<ffffffff8136b4fa>] acpi_ns_initialize_objects+0x103/0x1f0
> [<ffffffff81af548a>] acpi_load_tables+0xa5/0xf4
> [<ffffffff81af3ec0>] acpi_init+0x7e/0x29c
> [<ffffffff81000418>] do_one_initcall+0x38/0x150
> [<ffffffff81ac7076>] kernel_init_freeable+0x1b1/0x239
> [<ffffffff815a4c39>] kernel_init+0x9/0x100
> [<ffffffff815b25ef>] ret_from_fork+0x1f/0x40
>
> Signed-off-by: Bart Van Assche <bart.vanassche(a)sandisk.com>
> ---
> drivers/acpi/acpica/nswalk.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/acpi/acpica/nswalk.c b/drivers/acpi/acpica/nswalk.c
> index dd7ae1bc8af8..358969f5afe1 100644
> --- a/drivers/acpi/acpica/nswalk.c
> +++ b/drivers/acpi/acpica/nswalk.c
> @@ -350,6 +350,7 @@ acpi_ns_walk_namespace(acpi_object_type type,
>
> node_previously_visited = TRUE;
> }
> + cond_resched();
> }
>
> /* Complete walk, not terminated by user function */
> --
> 2.16.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo(a)vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
4 years, 5 months
Re: [Devel] [PATCH v9 3/7] acpi: apei: Add SEI notification type support for ARMv8
by gengdongjiu
[...]
>
> > Yes, I know you are dong that. Your serial's patch will consider all above
> things, right?
>
> Assuming I got it right, yes. It currently makes the race Xie XiuQi spotted worse,
> which I want to fix too. (details on the cover letter)
Ok.
>
>
> > If your patch can be consider that, this patch can based on your patchset.
> thanks.
>
> I'd like to pick these patches onto the end of that series, but first I want to
> know what NOTIFY_SEI means for any OS. The ACPI spec doesn't say, and
> because its asynchronous, route-able and mask-able, there are many more
> corners than NOTFIY_SEA.
>
> This thing is a notification using an emulated SError exception. (emulated
> because physical-SError must be routed to EL3 for firmware-first, and
> virtual-SError belongs to EL2).
>
> Does your firmware emulate SError exactly as the TakeException() pseudo code
> in the Arm-Arm?
Yes, it is.
> Is the emulated SError routed following the routing rules for HCR_EL2.{AMO,
> TGE}?
Yes, it is.
> What does your firmware do when it wants to emulate SError but its masked?
> (e.g.1: The physical-SError interrupted EL2 and the SPSR shows EL2 had
> PSTATE.A set.
> e.g.2: The physical-SError interrupted EL2 but HCR_EL2 indicates the
> emulated SError should go to EL1. This effectively masks SError.)
Currently we does not consider much about the mask status(SPSR).
I remember that you ever suggested firmware should reboot if the mask status is set(SPSR), right?
I ever suggest our firmware team to evaluate that, but there is no response.
I CC "liu jun" <liujun88(a)hisilicon.com> who is our UEFI firmware Architect, if you have firmware requirements, you can
raise again.
>
> Answers to these let us determine whether a bug is in the firmware or the
> kernel. If firmware is expecting the OS to do something special, I'd like to know
> about it from the beginning!
I know your meaning, thanks for raising it again.
>
>
> >>> Expose API ghes_notify_sei() to external users. External modules can
> >>> call this exposed API to parse APEI table and handle the SEI
> >>> notification.
> >>
> >> external modules? You mean called by the arch code when it gets this
> NOTIFY_SEI?
>
> > yes, called by kernel ARCH code, such as below, I remember I have discussed
> with you.
>
> Sure. The phrase 'external modules' usually means the '.ko' files that live in
> /lib/modules, nothing outside the kernel tree should be doing this stuff.
I will rename 'external modules' to other name. Thanks.
>
>
> Thanks,
>
> James
4 years, 6 months
