Skip to content

Commit cfc69c2

Browse files
chaithcoWolfram Sang
authored andcommitted
i2c: i801: Revert "i2c: i801: replace acpi_lock with I2C bus lock"
This reverts commit f707d6b. Under rare circumstances, multiple udev threads can collect i801 device info on boot and walk i801_acpi_io_handler somewhat concurrently. The first will note the area is reserved by acpi to prevent further touches. This ultimately causes the area to be deregistered. The second will enter i801_acpi_io_handler after the area is unregistered but before a check can be made that the area is unregistered. i2c_lock_bus relies on the now unregistered area containing lock_ops to lock the bus. The end result is a kernel panic on boot with the following backtrace; [ 14.971872] ioatdma 0000:09:00.2: enabling device (0100 -> 0102) [ 14.971873] BUG: kernel NULL pointer dereference, address: 0000000000000000 [ 14.971880] #PF: supervisor read access in kernel mode [ 14.971884] #PF: error_code(0x0000) - not-present page [ 14.971887] PGD 0 P4D 0 [ 14.971894] Oops: 0000 [#1] PREEMPT SMP PTI [ 14.971900] CPU: 5 PID: 956 Comm: systemd-udevd Not tainted 5.14.0-611.5.1.el9_7.x86_64 #1 [ 14.971905] Hardware name: XXXXXXXXXXXXXXXXXXXXXXX BIOS 1.20.10.SV91 01/30/2023 [ 14.971908] RIP: 0010:i801_acpi_io_handler+0x2d/0xb0 [i2c_i801] [ 14.971929] Code: 00 00 49 8b 40 20 41 57 41 56 4d 8b b8 30 04 00 00 49 89 ce 41 55 41 89 d5 41 54 49 89 f4 be 02 00 00 00 55 4c 89 c5 53 89 fb <48> 8b 00 4c 89 c7 e8 18 61 54 e9 80 bd 80 04 00 00 00 75 09 4c 3b [ 14.971933] RSP: 0018:ffffbaa841483838 EFLAGS: 00010282 [ 14.971938] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff9685e01ba568 [ 14.971941] RDX: 0000000000000008 RSI: 0000000000000002 RDI: 0000000000000000 [ 14.971944] RBP: ffff9685ca22f028 R08: ffff9685ca22f028 R09: ffff9685ca22f028 [ 14.971948] R10: 000000000000000b R11: 0000000000000580 R12: 0000000000000580 [ 14.971951] R13: 0000000000000008 R14: ffff9685e01ba568 R15: ffff9685c222f000 [ 14.971954] FS: 00007f8287c0ab40(0000) GS:ffff96a47f940000(0000) knlGS:0000000000000000 [ 14.971959] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 14.971963] CR2: 0000000000000000 CR3: 0000000168090001 CR4: 00000000003706f0 [ 14.971966] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 14.971968] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 14.971972] Call Trace: [ 14.971977] <TASK> [ 14.971981] ? show_trace_log_lvl+0x1c4/0x2df [ 14.971994] ? show_trace_log_lvl+0x1c4/0x2df [ 14.972003] ? acpi_ev_address_space_dispatch+0x16e/0x3c0 [ 14.972014] ? __die_body.cold+0x8/0xd [ 14.972021] ? page_fault_oops+0x132/0x170 [ 14.972028] ? exc_page_fault+0x61/0x150 [ 14.972036] ? asm_exc_page_fault+0x22/0x30 [ 14.972045] ? i801_acpi_io_handler+0x2d/0xb0 [i2c_i801] [ 14.972061] acpi_ev_address_space_dispatch+0x16e/0x3c0 [ 14.972069] ? __pfx_i801_acpi_io_handler+0x10/0x10 [i2c_i801] [ 14.972085] acpi_ex_access_region+0x5b/0xd0 [ 14.972093] acpi_ex_field_datum_io+0x73/0x2e0 [ 14.972100] acpi_ex_read_data_from_field+0x8e/0x230 [ 14.972106] acpi_ex_resolve_node_to_value+0x23d/0x310 [ 14.972114] acpi_ds_evaluate_name_path+0xad/0x110 [ 14.972121] acpi_ds_exec_end_op+0x321/0x510 [ 14.972127] acpi_ps_parse_loop+0xf7/0x680 [ 14.972136] acpi_ps_parse_aml+0x17a/0x3d0 [ 14.972143] acpi_ps_execute_method+0x137/0x270 [ 14.972150] acpi_ns_evaluate+0x1f4/0x2e0 [ 14.972158] acpi_evaluate_object+0x134/0x2f0 [ 14.972164] acpi_evaluate_integer+0x50/0xe0 [ 14.972173] ? vsnprintf+0x24b/0x570 [ 14.972181] acpi_ac_get_state.part.0+0x23/0x70 [ 14.972189] get_ac_property+0x4e/0x60 [ 14.972195] power_supply_show_property+0x90/0x1f0 [ 14.972205] add_prop_uevent+0x29/0x90 [ 14.972213] power_supply_uevent+0x109/0x1d0 [ 14.972222] dev_uevent+0x10e/0x2f0 [ 14.972228] uevent_show+0x8e/0x100 [ 14.972236] dev_attr_show+0x19/0x40 [ 14.972246] sysfs_kf_seq_show+0x9b/0x100 [ 14.972253] seq_read_iter+0x120/0x4b0 [ 14.972262] ? selinux_file_permission+0x106/0x150 [ 14.972273] vfs_read+0x24f/0x3a0 [ 14.972284] ksys_read+0x5f/0xe0 [ 14.972291] do_syscall_64+0x5f/0xe0 ... The kernel panic is mitigated by setting limiting the count of udev children to 1. Revert to using the acpi_lock to continue protecting marking the area as owned by firmware without relying on a lock in a potentially unmapped region of memory. Fixes: f707d6b ("i2c: i801: replace acpi_lock with I2C bus lock") Signed-off-by: Charles Haithcock <chaithco@redhat.com> [wsa: added Fixes-tag and updated comment stating the importance of the lock] Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
1 parent 11439c4 commit cfc69c2

1 file changed

Lines changed: 10 additions & 4 deletions

File tree

drivers/i2c/busses/i2c-i801.c

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -310,9 +310,10 @@ struct i801_priv {
310310

311311
/*
312312
* If set to true the host controller registers are reserved for
313-
* ACPI AML use.
313+
* ACPI AML use. Needs extra protection by acpi_lock.
314314
*/
315315
bool acpi_reserved;
316+
struct mutex acpi_lock;
316317
};
317318

318319
#define FEATURE_SMBUS_PEC BIT(0)
@@ -894,8 +895,11 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
894895
int hwpec, ret;
895896
struct i801_priv *priv = i2c_get_adapdata(adap);
896897

897-
if (priv->acpi_reserved)
898+
mutex_lock(&priv->acpi_lock);
899+
if (priv->acpi_reserved) {
900+
mutex_unlock(&priv->acpi_lock);
898901
return -EBUSY;
902+
}
899903

900904
pm_runtime_get_sync(&priv->pci_dev->dev);
901905

@@ -935,6 +939,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
935939
iowrite8(SMBHSTSTS_INUSE_STS | STATUS_FLAGS, SMBHSTSTS(priv));
936940

937941
pm_runtime_put_autosuspend(&priv->pci_dev->dev);
942+
mutex_unlock(&priv->acpi_lock);
938943
return ret;
939944
}
940945

@@ -1465,7 +1470,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
14651470
* further access from the driver itself. This device is now owned
14661471
* by the system firmware.
14671472
*/
1468-
i2c_lock_bus(&priv->adapter, I2C_LOCK_SEGMENT);
1473+
mutex_lock(&priv->acpi_lock);
14691474

14701475
if (!priv->acpi_reserved && i801_acpi_is_smbus_ioport(priv, address)) {
14711476
priv->acpi_reserved = true;
@@ -1485,7 +1490,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits,
14851490
else
14861491
status = acpi_os_write_port(address, (u32)*value, bits);
14871492

1488-
i2c_unlock_bus(&priv->adapter, I2C_LOCK_SEGMENT);
1493+
mutex_unlock(&priv->acpi_lock);
14891494

14901495
return status;
14911496
}
@@ -1545,6 +1550,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
15451550
priv->adapter.dev.parent = &dev->dev;
15461551
acpi_use_parent_companion(&priv->adapter.dev);
15471552
priv->adapter.retries = 3;
1553+
mutex_init(&priv->acpi_lock);
15481554

15491555
priv->pci_dev = dev;
15501556
priv->features = id->driver_data;

0 commit comments

Comments
 (0)