Damien Le Moal reports a lockdep splat with the acpi_power_meter,
observed with Linux v5.5 and later.
======================================================
WARNING: possible circular locking dependency detected
5.6.0-rc2+ #629 Not tainted
------------------------------------------------------
python/1397 is trying to acquire lock:
ffff888619080070 (&resource->lock){+.+.}, at: show_power+0x3c/0xa0 [acpi_power_meter]
but task is already holding lock:
ffff88881643f188 (kn->count#119){++++}, at: kernfs_seq_start+0x6a/0x160
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (kn->count#119){++++}:
__kernfs_remove+0x626/0x7e0
kernfs_remove_by_name_ns+0x41/0x80
remove_attrs+0xcb/0x3c0 [acpi_power_meter]
acpi_power_meter_notify+0x1f7/0x310 [acpi_power_meter]
acpi_ev_notify_dispatch+0x198/0x1f3
acpi_os_execute_deferred+0x4d/0x70
process_one_work+0x7c8/0x1340
worker_thread+0x94/0xc70
kthread+0x2ed/0x3f0
ret_from_fork+0x24/0x30
-> #0 (&resource->lock){+.+.}:
__lock_acquire+0x20be/0x49b0
lock_acquire+0x127/0x340
__mutex_lock+0x15b/0x1350
show_power+0x3c/0xa0 [acpi_power_meter]
dev_attr_show+0x3f/0x80
sysfs_kf_seq_show+0x216/0x410
seq_read+0x407/0xf90
vfs_read+0x152/0x2c0
ksys_read+0xf3/0x1d0
do_syscall_64+0x95/0x1010
entry_SYSCALL_64_after_hwframe+0x49/0xbe
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(kn->count#119);
lock(&resource->lock);
lock(kn->count#119);
lock(&resource->lock);
*** DEADLOCK ***
4 locks held by python/1397:
#0: ffff8890242d64e0 (&f->f_pos_lock){+.+.}, at: __fdget_pos+0x9b/0xb0
#1: ffff889040be74e0 (&p->lock){+.+.}, at: seq_read+0x6b/0xf90
#2: ffff8890448eb880 (&of->mutex){+.+.}, at: kernfs_seq_start+0x47/0x160
#3: ffff88881643f188 (kn->count#119){++++}, at: kernfs_seq_start+0x6a/0x160
stack backtrace:
CPU: 10 PID: 1397 Comm: python Not tainted 5.6.0-rc2+ #629
Hardware name: Supermicro Super Server/X11DPL-i, BIOS 3.1 05/21/2019
Call Trace:
dump_stack+0x97/0xe0
check_noncircular+0x32e/0x3e0
? print_circular_bug.isra.0+0x1e0/0x1e0
? unwind_next_frame+0xb9a/0x1890
? entry_SYSCALL_64_after_hwframe+0x49/0xbe
? graph_lock+0x79/0x170
? __lockdep_reset_lock+0x3c0/0x3c0
? mark_lock+0xbc/0x1150
__lock_acquire+0x20be/0x49b0
? mark_held_locks+0xe0/0xe0
? stack_trace_save+0x91/0xc0
lock_acquire+0x127/0x340
? show_power+0x3c/0xa0 [acpi_power_meter]
? device_remove_bin_file+0x10/0x10
? device_remove_bin_file+0x10/0x10
__mutex_lock+0x15b/0x1350
? show_power+0x3c/0xa0 [acpi_power_meter]
? show_power+0x3c/0xa0 [acpi_power_meter]
? mutex_lock_io_nested+0x11f0/0x11f0
? lock_downgrade+0x6a0/0x6a0
? kernfs_seq_start+0x47/0x160
? lock_acquire+0x127/0x340
? kernfs_seq_start+0x6a/0x160
? device_remove_bin_file+0x10/0x10
? show_power+0x3c/0xa0 [acpi_power_meter]
show_power+0x3c/0xa0 [acpi_power_meter]
dev_attr_show+0x3f/0x80
? memset+0x20/0x40
sysfs_kf_seq_show+0x216/0x410
seq_read+0x407/0xf90
? security_file_permission+0x16f/0x2c0
vfs_read+0x152/0x2c0
Problem is that reading an attribute takes the kernfs lock in the kernfs
code, then resource->lock in the driver. During an ACPI notification, the
opposite happens: The resource lock is taken first, followed by the kernfs
lock when sysfs attributes are removed and re-created. Presumably this is
now seen due to some locking related changes in kernfs after v5.4, but it
was likely always a problem.
Fix the problem by not blindly acquiring the lock in the notification
function. It is only needed to protect the various update functions.
However, those update functions are called anyway when sysfs attributes
are read. This means that we can just stop calling those functions from
the notifier, and the resource lock in the notifier function is no longer
needed.
That leaves two situations:
First, METER_NOTIFY_CONFIG removes and re-allocates capability strings.
While it did so under the resource lock, _displaying_ those strings was not
protected, creating a race condition. To solve this problem, selectively
protect both removal/creation and reporting of capability attributes with
the resource lock.
Second, removing and re-creating the attribute files is no longer protected
by the resource lock. That doesn't matter since access to each individual
attribute is protected by the kernfs lock. Userspace may get messed up if
attributes disappear and reappear under its nose, but that is not different
than today, and there is nothing we can do about it without major driver
restructuring.
Last but not least, when removing the driver, remove attribute functions
first, then release capability strings. This avoids yet another race
condition.
Reported-by: Damien Le Moal <Damien.LeMoal@wdc.com>
Cc: Damien Le Moal <Damien.LeMoal@wdc.com>
Cc: stable@vger.kernel.org # v5.5+
Tested-by: Damien Le Moal <damien.lemoal@wdc.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
At boot time, the acpi_power_meter driver logs the following error level
message: "Ignoring unsafe software power cap". Having read about it from
a few sources, it seems that the error message can be quite misleading.
While the message can imply that Linux is ignoring the fact that the
system is operating in potentially dangerous conditions, the truth is
the driver found an ACPI_PMC object that supports software power
capping. The driver simply decides not to use it, perhaps because it
doesn't support the object.
The best solution is probably changing the log level from error to warning.
All sources I have found, regarding the error, have downplayed its
significance. There is not much of a reason for it to be on error level,
while causing potential confusions or misinterpretations.
Signed-off-by: Wang Shenran <shenran268@gmail.com>
Link: https://lore.kernel.org/r/20190724080110.6952-1-shenran268@gmail.com
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Based on 1 normalized pattern(s):
this program is free software you can redistribute it and or modify
it under the terms of the gnu general public license as published by
the free software foundation either version 2 of the license or at
your option any later version this program is distributed in the
hope that it will be useful but without any warranty without even
the implied warranty of merchantability or fitness for a particular
purpose see the gnu general public license for more details you
should have received a copy of the gnu general public license along
with this program if not write to the free software foundation inc
59 temple place suite 330 boston ma 02111 1307 usa
extracted by the scancode license scanner the SPDX license identifier
GPL-2.0-or-later
has been chosen to replace the boilerplate/reference in 1334 file(s).
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Allison Randal <allison@lohutok.net>
Reviewed-by: Richard Fontana <rfontana@redhat.com>
Cc: linux-spdx@vger.kernel.org
Link: https://lkml.kernel.org/r/20190527070033.113240726@linutronix.de
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Replace S_<PERMS> with octal values.
The conversion was done automatically with coccinelle. The semantic patches
and the scripts used to generate this commit log are available at
https://github.com/groeck/coccinelle-patches/hwmon/.
This patch does not introduce functional changes. It was verified by
compiling the old and new files and comparing text and data sizes.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
... and __initconst if applicable.
Based on similar work for an older kernel in the Grsecurity patch.
[JD: fix toshiba-wmi build]
[JD: add htcpen]
[JD: move __initconst where checkscript wants it]
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jean Delvare <jdelvare@suse.de>
Replace direct inclusions of <acpi/acpi.h>, <acpi/acpi_bus.h> and
<acpi/acpi_drivers.h>, which are incorrect, with <linux/acpi.h>
inclusions and remove some inclusions of those files that aren't
necessary.
First of all, <acpi/acpi.h>, <acpi/acpi_bus.h> and <acpi/acpi_drivers.h>
should not be included directly from any files that are built for
CONFIG_ACPI unset, because that generally leads to build warnings about
undefined symbols in !CONFIG_ACPI builds. For CONFIG_ACPI set,
<linux/acpi.h> includes those files and for CONFIG_ACPI unset it
provides stub ACPI symbols to be used in that case.
Second, there are ordering dependencies between those files that always
have to be met. Namely, it is required that <acpi/acpi_bus.h> be included
prior to <acpi/acpi_drivers.h> so that the acpi_pci_root declarations the
latter depends on are always there. And <acpi/acpi.h> which provides
basic ACPICA type declarations should always be included prior to any other
ACPI headers in CONFIG_ACPI builds. That also is taken care of including
<linux/acpi.h> as appropriate.
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com> (drivers/pci stuff)
Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> (Xen stuff)
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Since acpi_bus_get_device() returns plain int and not acpi_status,
ACPI_FAILURE() should not be used for checking its return value. Fix
that.
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
acpi_power_meter crashes the kernel if it detects an unexpected event
or an internal implementation error. While the detected conditions
suggest that there is a bug in the code, the condition is not fatal.
Replace BUG() with WARN().
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
I've changed employers, so change the email addresses to match.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
The second argument of ACPI driver .remove() operation is only used
by the ACPI processor driver and the value passed to that driver
through it is always available from the given struct acpi_device
object's removal_type field. For this reason, the second ACPI driver
.remove() argument is in fact useless, so drop it.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Jiang Liu <jiang.liu@huawei.com>
Acked-by: Toshi Kani <toshi.kani@hp.com>
Acked-by: Yinghai Lu <yinghai@kernel.org>
These drivers use IS_ERR so they should include <linux/err.h>.
Signed-off-by: Jean Delvare <khali@linux-fr.org>
Acked-by: Guenter Roeck <linux@roeck-us.net>
Acked-by: Luca Tettamanti <kronos.it@gmail.com>
Cc: Henrik Rydberg <rydberg@euromail.se>
Commit c5dec01822 (acpi_power_meter: Use struct
dev_pm_ops for power management) introduced the following build warning. It is
seen if CONFIG_PM_SLEEP is not defined.
acpi_power_meter.c:930:12: warning: acpi_power_meter_resume defined but not used
Fix it.
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
temperature sensors.
Convert drivers to use devm_ functions and to use dev_pm_ops.
Address a couple of Coverity errors/warnings as well as compile warnings.
Some functional improvements in applesmc driver.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
iQIcBAABAgAGBQJQDMjFAAoJEMsfJm/On5mBQy0QAIOMh+xxhfpIYmOz6e7lXyGK
IvsYvJ16jvMzkIHxIsCJ8lYo0tQJTVFZAoDE0KnXMt/vpDGuooa/L2Rq0NQwAadF
nb+XijtV042FMScLiGCbJQRyPmVWEpQ1sHvf6Ob+xUido4xTL/J0ObMv1P76Ro5v
OlCWqt5bly9JDDzwTdvmkmVQ7TSLV27IbEy1XFeQ3B9AquIzCsHjLXmyzTGtWuEa
e73pR62m+nEHlIwEyCnH9QBc5fhUyrOm+mzmGWaEZX+PUt4OC2VeBCOB4tyYA1f+
+PV9wcCsFyre4Xdi2mtfyMnqSs6qio07diJB33oAmApKs6dbsfZvwM1gSIVTp8W/
RooxePKN2fFgjnLSbAQZWaC3AMTcrsMlUMVxqZ75BPmQvrm1bxJh/OcWpo5lZM8s
yAmdx587eHABMqvYgs4QVIf3QqmPXi194Qzq4loLHteGDohK8Z6u6t7famx1sRLt
ApqfmcxPAjvpXfQB11VboYTLiFtXJK8T/VwOOoB9L24/dxoymSUa2UQuoeSBdESM
dTTKJQebUcJs6J0OWzGRjKwS/jH8wcGJCzMlcHW9dpxMdh98R4gHHi/UTkfgJi/n
j50/oxpJCyHqGthvKyA77oztWyyfuAdJbG2eKtRNVPriJMP3uuAbbBA9Cl9ytXyh
SUChQ49urUSABI9up1iR
=TcQ+
-----END PGP SIGNATURE-----
Merge tag 'hwmon-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging
Pull hwmon updates from Guenter Roeck:
"New drivers for DA9052/53 PMIC as well as HIH-6130/HIH-6131 humidity
and temperature sensors.
Convert drivers to use devm_ functions and to use dev_pm_ops. Address
a couple of Coverity errors/warnings as well as compile warnings.
Some functional improvements in applesmc driver."
* tag 'hwmon-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging: (72 commits)
hwmon: (applesmc) Ignore some temperature registers
hwmon: (applesmc) Allow negative temperature values
hwmon: (s3c-hwmon) Use devm_kzalloc instead of kzalloc
hwmon: (w83781d) Fix compile warning
hwmon: (applesmc) Shorten minimum wait time
hwmon: (exynos4_tmu) Use struct dev_pm_ops for power management
hwmon: (gpio-fan) Use struct dev_pm_ops for power management
hwmon: (abituguru3) Use struct dev_pm_ops for power management
hwmon: (abituguru) Use struct dev_pm_ops for power management
hwmon: (acpi_power_meter) Fix unintentional integer overflow
hwmon: (acpi_power_meter) Cleanup and optimizations
hwmon: Honeywell Humidicon HIH-6130/HIH-6131 humidity and temperature sensor driver
hwmon: (applesmc) Skip sensor mapping
hwmon: (ntc_thermistor) Ensure that data->name string is terminated
hwmon: (w83l785ts) Convert to use devm_ functions
hwmon: (w83l785ts) Simplify code and improve readability
hwmon: (smsc47m192) Convert to use devm_ functions
hwmon: (smsc47m1) Convert to use devm_ functions
hwmon: (smsc47b397) Convert to use devm_ functions
hwmon: (k8temp) Convert to use devm_ functions
...
Expression with two integer variables is calculated as integer before it is
converted to u64. This may result in an integer overflow. Fix by declaring
trip point variables as s64 instead of int.
This patch addresses Coverity #200596: Unintentional integer overflow.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Acked-by: Jean Delvare <khali@linux-fr.org>
An unsigned value can not be smaller than 0. Remove the check for it.
Use DIV_ROUND_CLOSEST for divide operations converting milli-degrees C into
degrees C. Limit maximum accepted trip point temperature to INT_MAX.
This patch fixes Coverity #115214: Unsigned compared against 0
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Acked-by: Jean Delvare <khali@linux-fr.org>
Make the ACPI power meter driver define its PM callbacks through
a struct dev_pm_ops object rather than by using legacy PM hooks
in struct acpi_device_ops.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
We don't need to duplicate if (res) checks if we're always running
one or the other.
Signed-off-by: Kyle McMartin <kyle@redhat.com>
Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
We always register these two together, so move meter_rw_attrs into
meter_ro_attrs and use the same for both since we no longer have two
register_attr paths.
Signed-off-by: Kyle McMartin <kyle@redhat.com>
Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
Key off the attr->set method being present to set the sysfs attribute
as writable.
Signed-off-by: Kyle McMartin <kyle@redhat.com>
Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
Similar to how we do PCI/USB device id structs.
Signed-off-by: Kyle McMartin <kyle@redhat.com>
Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
We don't need both, when we can just key the read/write off of the
presence of the .set member.
Signed-off-by: Kyle McMartin <kyle@redhat.com>
Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
In some configurations, BUG() does not result in an endless loop but returns
to the caller. This results in the following compiler warning:
drivers/hwmon/acpi_power_meter.c: In function 'show_str':
drivers/hwmon/acpi_power_meter.c:380: warning: 'val' may be used uninitialized in this function
Fix the warning by setting val to an empty string after BUG().
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Robert Coulson <robert.coulson@ericsson.com>
Similar to a30dcb4f which fixed asus_atk0110.ko, I recently received a
bug report from someone hitting the same issue in acpi_power_meter.
[ 13.963168] power_meter ACPI000D:00: Found ACPI power meter.
[ 13.963900] BUG: key ffff8802161f3920 not in .data!
[ 13.963904] ------------[ cut here ]------------
[ 13.963915] WARNING: at kernel/lockdep.c:2986
lockdep_init_map+0x52f/0x560()
So let's fix that up for them by statically declaring the
lockdep_class_key.
Signed-off-by: Kyle McMartin <kyle@redhat.com>
Cc: stable@vger.kernel.org # 3.0+
Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
module_param(bool) used to counter-intuitively take an int. In
fddd5201 (mid-2009) we allowed bool or int/unsigned int using a messy
trick.
It's time to remove the int/unsigned int option. For this version
it'll simply give a warning, but it'll break next kernel version.
Acked-by: Mauro Carvalho Chehab <mchehab@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
replaced strict_strtol with kstrtol and
replaced strict_strtuol with kstrtuol
This satisfies checkpatch -f
Compile tested only: no warnings or errors given
Signed-off-by: Frans Meulenbroeks <fransmeulenbroeks@gmail.com>
Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
As discussed earlier, the ACPI power meter driver would better live
in drivers/hwmon, as its only purpose is to create hwmon-style
interfaces for ACPI 4.0 power meter devices. Users are more likely to
look for it there, and less likely to accidentally hide it by
unselecting its dependencies.
Signed-off-by: Jean Delvare <khali@linux-fr.org>
Acked-by: "Darrick J. Wong" <djwong@us.ibm.com>
Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>
Cc: Len Brown <lenb@kernel.org>
