* Re: [REGRESSION? v4.8] i2c-core: acpi_i2c_get_info() touches non-existent devices
[not found] <87a8f4kfhe.fsf@gmail.com>
@ 2016-09-19 8:48 ` Mika Westerberg
2016-09-19 13:03 ` Mika Westerberg
0 siblings, 1 reply; 16+ messages in thread
From: Mika Westerberg @ 2016-09-19 8:48 UTC (permalink / raw)
To: Nicolai Stange
Cc: Wolfram Sang, Octavian Purdila, Rafael J. Wysocki, linux-i2c,
linux-kernel
On Mon, Sep 19, 2016 at 12:30:53AM +0200, Nicolai Stange wrote:
> Hi,
>
> I'm encountering the following:
>
> [ 10.409490] ERROR: Unable to locate IOAPIC for GSI 37
>
> Note that the system works fine, so it's a "cosmetic" regression, I think.
>
>
> I added a dump_stack() right below the printk() in question and it reads
> as
>
> [ 10.410290] CPU: 6 PID: 710 Comm: systemd-udevd Not tainted 4.7.0-rc4+ #348
> [ 10.410962] Hardware name: Dell Inc. Latitude E6540/0725FP, BIOS A10 06/26/2014
> [ 10.411772] 0000000000000286 00000000b9050627 ffff8800c2e5f590 ffffffffa54161e7
> [ 10.412569] 0000000000000025 0000000000000001 ffff8800c2e5f5a0 ffffffffa50465df
> [ 10.413292] ffff8800c2e5f5d0 ffffffffa5046ffd 0000000000000000 0000000000000025
> [ 10.414016] Call Trace:
> [ 10.414713] [<ffffffffa54161e7>] dump_stack+0x68/0xa1
> [ 10.415406] [<ffffffffa50465df>] mp_find_ioapic+0x4f/0x60
> [ 10.416131] [<ffffffffa5046ffd>] mp_map_gsi_to_irq+0x1d/0xc0
> [ 10.416806] [<ffffffffa503dbbb>] acpi_register_gsi_ioapic+0x7b/0x170
> [ 10.417494] [<ffffffffa503da6f>] acpi_register_gsi+0xf/0x20
> [ 10.418217] [<ffffffffa54a14d5>] acpi_dev_get_irqresource.part.3+0xd7/0x11d
> [ 10.418871] [<ffffffffa54a139a>] ? acpi_dev_resource_address_space+0x31/0x67
> [ 10.419655] [<ffffffffa54a168d>] acpi_dev_resource_interrupt+0x9b/0xab
> [ 10.420408] [<ffffffffa54a1848>] acpi_dev_process_resource+0xbc/0xf7
> [ 10.421070] [<ffffffffa54a178c>] ? acpi_dev_resource_memory+0x7c/0x7c
> [ 10.421732] [<ffffffffa54c3ba2>] acpi_walk_resource_buffer+0x4d/0x85
> [ 10.422399] [<ffffffffa54a178c>] ? acpi_dev_resource_memory+0x7c/0x7c
> [ 10.423158] [<ffffffffa54c3e89>] acpi_walk_resources+0x83/0xb6
> [ 10.423831] [<ffffffffa54a15b1>] acpi_dev_get_resources+0x96/0xd7
> [ 10.424505] [<ffffffffa563f7c4>] acpi_i2c_get_info+0xe4/0x1a0
> [ 10.425181] [<ffffffffa5642c06>] acpi_i2c_add_device+0x56/0xa0
> [ 10.425856] [<ffffffffa54bf2ff>] acpi_ns_walk_namespace+0xe8/0x19d
> [ 10.426564] [<ffffffffa5642bb0>] ? acpi_i2c_register_device+0x70/0x70
> [ 10.427418] [<ffffffffa5642bb0>] ? acpi_i2c_register_device+0x70/0x70
> [ 10.428179] [<ffffffffa54bf83d>] acpi_walk_namespace+0xa0/0xd5
> [ 10.428858] [<ffffffffa56437a9>] i2c_register_adapter+0x369/0x500
> [ 10.429499] [<ffffffffa564399c>] i2c_add_adapter+0x5c/0x70
> [ 10.430125] [<ffffffffc07df7dd>] i801_probe+0x2bd/0x6a0 [i2c_i801]
> [ 10.431159] [<ffffffffa50eb8dd>] ? trace_hardirqs_on+0xd/0x10
> [ 10.432196] [<ffffffffa54682f2>] local_pci_probe+0x42/0xa0
> [ 10.432826] [<ffffffffa5468e2a>] ? pci_match_device+0xca/0x110
> [ 10.433460] [<ffffffffa5469243>] pci_device_probe+0x103/0x150
> [ 10.434083] [<ffffffffa5545acc>] driver_probe_device+0x22c/0x440
> [ 10.434712] [<ffffffffa5545db5>] __driver_attach+0xd5/0x100
> [ 10.435341] [<ffffffffa5545ce0>] ? driver_probe_device+0x440/0x440
> [ 10.435963] [<ffffffffa5543313>] bus_for_each_dev+0x73/0xc0
> [ 10.436676] [<ffffffffa554519e>] driver_attach+0x1e/0x20
> [ 10.437356] [<ffffffffa5544bc6>] bus_add_driver+0x1c6/0x290
> [ 10.437978] [<ffffffffc07e5000>] ? 0xffffffffc07e5000
> [ 10.438699] [<ffffffffa5546a50>] driver_register+0x60/0xe0
> [ 10.439502] [<ffffffffc07e5000>] ? 0xffffffffc07e5000
> [ 10.440310] [<ffffffffa5467e4d>] __pci_register_driver+0x5d/0x60
> [ 10.440313] [<ffffffffc07e50af>] i2c_i801_init+0xaf/0x1000 [i2c_i801]
> [ 10.440314] [<ffffffffc07e5000>] ? 0xffffffffc07e5000
> [ 10.440316] [<ffffffffa5000450>] do_one_initcall+0x50/0x180
> [ 10.440319] [<ffffffffa5101cc5>] ? rcu_read_lock_sched_held+0x45/0x80
> [ 10.440322] [<ffffffffa5234e55>] ? kmem_cache_alloc_trace+0x2d5/0x340
> [ 10.440325] [<ffffffffa51c1252>] do_init_module+0x5f/0x1da
> [ 10.440329] [<ffffffffa512d615>] load_module+0x2195/0x2950
> [ 10.440331] [<ffffffffa512a0e0>] ? __symbol_put+0x70/0x70
> [ 10.440334] [<ffffffffa525dd3b>] ? vfs_read+0x11b/0x130
> [ 10.440337] [<ffffffffa512e066>] SYSC_finit_module+0xe6/0x120
> [ 10.440339] [<ffffffffa512e0be>] SyS_finit_module+0xe/0x10
> [ 10.440340] [<ffffffffa5002fb1>] do_syscall_64+0x61/0x170
> [ 10.440343] [<ffffffffa581e1da>] entry_SYSCALL64_slow_path+0x25/0x25
>
>
> I bisected this to commit 525e6fabeae2 ("i2c / ACPI: add support for
> ACPI reconfigure notifications").
>
> The reason for the above message seems to be that acpi_i2c_get_info()
> configures the IRQs for any ACPI devices that have got some
> I2cSerialBus() resource, regardless of the actual adapter those are
> attached to. This behaviour is different from before that commit.
>
> My ACPI DSDT has got a PCI I2C adapter that isn't physically present, it
> seems. No clue why.
>
> That non-existent PCI I2C adapter is in turn I2cSerialBus()-referenced
> by some ACPI device that has got exactly this interrupt 37 assigned.
>
> So it looks like an attempt is made to configure this non-existent,
> ACPI-listed I2C slave's IRQs when an actually existing I2C adapter (i801
> SMBus) gets probed.
>
>
> Let me know if I can provide you with any additional information,
Can you send me acpidump from that that machine?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [REGRESSION? v4.8] i2c-core: acpi_i2c_get_info() touches non-existent devices
2016-09-19 8:48 ` [REGRESSION? v4.8] i2c-core: acpi_i2c_get_info() touches non-existent devices Mika Westerberg
@ 2016-09-19 13:03 ` Mika Westerberg
2016-09-19 13:58 ` Nicolai Stange
0 siblings, 1 reply; 16+ messages in thread
From: Mika Westerberg @ 2016-09-19 13:03 UTC (permalink / raw)
To: Nicolai Stange
Cc: Wolfram Sang, Octavian Purdila, Rafael J. Wysocki, linux-i2c,
linux-kernel
On Mon, Sep 19, 2016 at 11:48:07AM +0300, Mika Westerberg wrote:
> On Mon, Sep 19, 2016 at 12:30:53AM +0200, Nicolai Stange wrote:
> > Hi,
> >
> > I'm encountering the following:
> >
> > [ 10.409490] ERROR: Unable to locate IOAPIC for GSI 37
> >
> > Note that the system works fine, so it's a "cosmetic" regression, I think.
> >
> >
> > I added a dump_stack() right below the printk() in question and it reads
> > as
> >
> > [ 10.410290] CPU: 6 PID: 710 Comm: systemd-udevd Not tainted 4.7.0-rc4+ #348
> > [ 10.410962] Hardware name: Dell Inc. Latitude E6540/0725FP, BIOS A10 06/26/2014
> > [ 10.411772] 0000000000000286 00000000b9050627 ffff8800c2e5f590 ffffffffa54161e7
> > [ 10.412569] 0000000000000025 0000000000000001 ffff8800c2e5f5a0 ffffffffa50465df
> > [ 10.413292] ffff8800c2e5f5d0 ffffffffa5046ffd 0000000000000000 0000000000000025
> > [ 10.414016] Call Trace:
> > [ 10.414713] [<ffffffffa54161e7>] dump_stack+0x68/0xa1
> > [ 10.415406] [<ffffffffa50465df>] mp_find_ioapic+0x4f/0x60
> > [ 10.416131] [<ffffffffa5046ffd>] mp_map_gsi_to_irq+0x1d/0xc0
> > [ 10.416806] [<ffffffffa503dbbb>] acpi_register_gsi_ioapic+0x7b/0x170
> > [ 10.417494] [<ffffffffa503da6f>] acpi_register_gsi+0xf/0x20
> > [ 10.418217] [<ffffffffa54a14d5>] acpi_dev_get_irqresource.part.3+0xd7/0x11d
> > [ 10.418871] [<ffffffffa54a139a>] ? acpi_dev_resource_address_space+0x31/0x67
> > [ 10.419655] [<ffffffffa54a168d>] acpi_dev_resource_interrupt+0x9b/0xab
> > [ 10.420408] [<ffffffffa54a1848>] acpi_dev_process_resource+0xbc/0xf7
> > [ 10.421070] [<ffffffffa54a178c>] ? acpi_dev_resource_memory+0x7c/0x7c
> > [ 10.421732] [<ffffffffa54c3ba2>] acpi_walk_resource_buffer+0x4d/0x85
> > [ 10.422399] [<ffffffffa54a178c>] ? acpi_dev_resource_memory+0x7c/0x7c
> > [ 10.423158] [<ffffffffa54c3e89>] acpi_walk_resources+0x83/0xb6
> > [ 10.423831] [<ffffffffa54a15b1>] acpi_dev_get_resources+0x96/0xd7
> > [ 10.424505] [<ffffffffa563f7c4>] acpi_i2c_get_info+0xe4/0x1a0
> > [ 10.425181] [<ffffffffa5642c06>] acpi_i2c_add_device+0x56/0xa0
> > [ 10.425856] [<ffffffffa54bf2ff>] acpi_ns_walk_namespace+0xe8/0x19d
> > [ 10.426564] [<ffffffffa5642bb0>] ? acpi_i2c_register_device+0x70/0x70
> > [ 10.427418] [<ffffffffa5642bb0>] ? acpi_i2c_register_device+0x70/0x70
> > [ 10.428179] [<ffffffffa54bf83d>] acpi_walk_namespace+0xa0/0xd5
> > [ 10.428858] [<ffffffffa56437a9>] i2c_register_adapter+0x369/0x500
> > [ 10.429499] [<ffffffffa564399c>] i2c_add_adapter+0x5c/0x70
> > [ 10.430125] [<ffffffffc07df7dd>] i801_probe+0x2bd/0x6a0 [i2c_i801]
> > [ 10.431159] [<ffffffffa50eb8dd>] ? trace_hardirqs_on+0xd/0x10
> > [ 10.432196] [<ffffffffa54682f2>] local_pci_probe+0x42/0xa0
> > [ 10.432826] [<ffffffffa5468e2a>] ? pci_match_device+0xca/0x110
> > [ 10.433460] [<ffffffffa5469243>] pci_device_probe+0x103/0x150
> > [ 10.434083] [<ffffffffa5545acc>] driver_probe_device+0x22c/0x440
> > [ 10.434712] [<ffffffffa5545db5>] __driver_attach+0xd5/0x100
> > [ 10.435341] [<ffffffffa5545ce0>] ? driver_probe_device+0x440/0x440
> > [ 10.435963] [<ffffffffa5543313>] bus_for_each_dev+0x73/0xc0
> > [ 10.436676] [<ffffffffa554519e>] driver_attach+0x1e/0x20
> > [ 10.437356] [<ffffffffa5544bc6>] bus_add_driver+0x1c6/0x290
> > [ 10.437978] [<ffffffffc07e5000>] ? 0xffffffffc07e5000
> > [ 10.438699] [<ffffffffa5546a50>] driver_register+0x60/0xe0
> > [ 10.439502] [<ffffffffc07e5000>] ? 0xffffffffc07e5000
> > [ 10.440310] [<ffffffffa5467e4d>] __pci_register_driver+0x5d/0x60
> > [ 10.440313] [<ffffffffc07e50af>] i2c_i801_init+0xaf/0x1000 [i2c_i801]
> > [ 10.440314] [<ffffffffc07e5000>] ? 0xffffffffc07e5000
> > [ 10.440316] [<ffffffffa5000450>] do_one_initcall+0x50/0x180
> > [ 10.440319] [<ffffffffa5101cc5>] ? rcu_read_lock_sched_held+0x45/0x80
> > [ 10.440322] [<ffffffffa5234e55>] ? kmem_cache_alloc_trace+0x2d5/0x340
> > [ 10.440325] [<ffffffffa51c1252>] do_init_module+0x5f/0x1da
> > [ 10.440329] [<ffffffffa512d615>] load_module+0x2195/0x2950
> > [ 10.440331] [<ffffffffa512a0e0>] ? __symbol_put+0x70/0x70
> > [ 10.440334] [<ffffffffa525dd3b>] ? vfs_read+0x11b/0x130
> > [ 10.440337] [<ffffffffa512e066>] SYSC_finit_module+0xe6/0x120
> > [ 10.440339] [<ffffffffa512e0be>] SyS_finit_module+0xe/0x10
> > [ 10.440340] [<ffffffffa5002fb1>] do_syscall_64+0x61/0x170
> > [ 10.440343] [<ffffffffa581e1da>] entry_SYSCALL64_slow_path+0x25/0x25
> >
> >
> > I bisected this to commit 525e6fabeae2 ("i2c / ACPI: add support for
> > ACPI reconfigure notifications").
> >
> > The reason for the above message seems to be that acpi_i2c_get_info()
> > configures the IRQs for any ACPI devices that have got some
> > I2cSerialBus() resource, regardless of the actual adapter those are
> > attached to. This behaviour is different from before that commit.
> >
> > My ACPI DSDT has got a PCI I2C adapter that isn't physically present, it
> > seems. No clue why.
> >
> > That non-existent PCI I2C adapter is in turn I2cSerialBus()-referenced
> > by some ACPI device that has got exactly this interrupt 37 assigned.
> >
> > So it looks like an attempt is made to configure this non-existent,
> > ACPI-listed I2C slave's IRQs when an actually existing I2C adapter (i801
> > SMBus) gets probed.
> >
> >
> > Let me know if I can provide you with any additional information,
>
> Can you send me acpidump from that that machine?
Can you try if the following patch cures the problem?
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index da3a02ef4a31..4e6c6fde9bdb 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -205,7 +205,7 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
void *data, void **return_value)
{
struct i2c_adapter *adapter = data;
- struct acpi_device *adev;
+ struct acpi_device *adev, *adapter_adev;
acpi_handle adapter_handle;
struct i2c_board_info info;
@@ -218,6 +218,12 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
if (adapter_handle != ACPI_HANDLE(&adapter->dev))
return AE_OK;
+ /* The adapter must be present */
+ if (acpi_bus_get_device(adapter_handle, &adapter_adev))
+ return AE_OK;
+ if (acpi_bus_get_status(adapter_adev) || !adapter_adev->status.present)
+ return AE_OK;
+
acpi_i2c_register_device(adapter, adev, &info);
return AE_OK;
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [REGRESSION? v4.8] i2c-core: acpi_i2c_get_info() touches non-existent devices
2016-09-19 13:03 ` Mika Westerberg
@ 2016-09-19 13:58 ` Nicolai Stange
2016-09-20 7:55 ` Mika Westerberg
2016-09-20 8:00 ` [PATCH] i2c / ACPI: Do not touch an I2C device if it belongs to another adapter Mika Westerberg
0 siblings, 2 replies; 16+ messages in thread
From: Nicolai Stange @ 2016-09-19 13:58 UTC (permalink / raw)
To: Mika Westerberg
Cc: Nicolai Stange, Wolfram Sang, Octavian Purdila,
Rafael J. Wysocki, linux-i2c, linux-kernel
Mika Westerberg <mika.westerberg@linux.intel.com> writes:
> On Mon, Sep 19, 2016 at 11:48:07AM +0300, Mika Westerberg wrote:
>> On Mon, Sep 19, 2016 at 12:30:53AM +0200, Nicolai Stange wrote:
>> > I'm encountering the following:
>> >
>> > [ 10.409490] ERROR: Unable to locate IOAPIC for GSI 37
>> >
>> > Note that the system works fine, so it's a "cosmetic" regression, I think.
>> >
>> >
>> > I added a dump_stack() right below the printk() in question and it reads
>> > as
>> >
>> > [ 10.410290] CPU: 6 PID: 710 Comm: systemd-udevd Not tainted 4.7.0-rc4+ #348
>> > [ 10.410962] Hardware name: Dell Inc. Latitude E6540/0725FP, BIOS A10 06/26/2014
>> > [ 10.411772] 0000000000000286 00000000b9050627 ffff8800c2e5f590 ffffffffa54161e7
>> > [ 10.412569] 0000000000000025 0000000000000001 ffff8800c2e5f5a0 ffffffffa50465df
>> > [ 10.413292] ffff8800c2e5f5d0 ffffffffa5046ffd 0000000000000000 0000000000000025
>> > [ 10.414016] Call Trace:
>> > [ 10.414713] [<ffffffffa54161e7>] dump_stack+0x68/0xa1
>> > [ 10.415406] [<ffffffffa50465df>] mp_find_ioapic+0x4f/0x60
>> > [ 10.416131] [<ffffffffa5046ffd>] mp_map_gsi_to_irq+0x1d/0xc0
>> > [ 10.416806] [<ffffffffa503dbbb>] acpi_register_gsi_ioapic+0x7b/0x170
>> > [ 10.417494] [<ffffffffa503da6f>] acpi_register_gsi+0xf/0x20
>> > [ 10.418217] [<ffffffffa54a14d5>] acpi_dev_get_irqresource.part.3+0xd7/0x11d
>> > [ 10.418871] [<ffffffffa54a139a>] ? acpi_dev_resource_address_space+0x31/0x67
>> > [ 10.419655] [<ffffffffa54a168d>] acpi_dev_resource_interrupt+0x9b/0xab
>> > [ 10.420408] [<ffffffffa54a1848>] acpi_dev_process_resource+0xbc/0xf7
>> > [ 10.421070] [<ffffffffa54a178c>] ? acpi_dev_resource_memory+0x7c/0x7c
>> > [ 10.421732] [<ffffffffa54c3ba2>] acpi_walk_resource_buffer+0x4d/0x85
>> > [ 10.422399] [<ffffffffa54a178c>] ? acpi_dev_resource_memory+0x7c/0x7c
>> > [ 10.423158] [<ffffffffa54c3e89>] acpi_walk_resources+0x83/0xb6
>> > [ 10.423831] [<ffffffffa54a15b1>] acpi_dev_get_resources+0x96/0xd7
>> > [ 10.424505] [<ffffffffa563f7c4>] acpi_i2c_get_info+0xe4/0x1a0
>> > [ 10.425181] [<ffffffffa5642c06>] acpi_i2c_add_device+0x56/0xa0
>> > [ 10.425856] [<ffffffffa54bf2ff>] acpi_ns_walk_namespace+0xe8/0x19d
>> > [ 10.426564] [<ffffffffa5642bb0>] ? acpi_i2c_register_device+0x70/0x70
>> > [ 10.427418] [<ffffffffa5642bb0>] ? acpi_i2c_register_device+0x70/0x70
>> > [ 10.428179] [<ffffffffa54bf83d>] acpi_walk_namespace+0xa0/0xd5
>> > [ 10.428858] [<ffffffffa56437a9>] i2c_register_adapter+0x369/0x500
>> > [ 10.429499] [<ffffffffa564399c>] i2c_add_adapter+0x5c/0x70
>> > [ 10.430125] [<ffffffffc07df7dd>] i801_probe+0x2bd/0x6a0 [i2c_i801]
>> > I bisected this to commit 525e6fabeae2 ("i2c / ACPI: add support for
>> > ACPI reconfigure notifications").
>> >
>> > The reason for the above message seems to be that acpi_i2c_get_info()
>> > configures the IRQs for any ACPI devices that have got some
>> > I2cSerialBus() resource, regardless of the actual adapter those are
>> > attached to. This behaviour is different from before that commit.
>> >
>> > My ACPI DSDT has got a PCI I2C adapter that isn't physically present, it
>> > seems. No clue why.
>> >
>> > That non-existent PCI I2C adapter is in turn I2cSerialBus()-referenced
>> > by some ACPI device that has got exactly this interrupt 37 assigned.
>> >
>> > So it looks like an attempt is made to configure this non-existent,
>> > ACPI-listed I2C slave's IRQs when an actually existing I2C adapter (i801
>> > SMBus) gets probed.
> Can you try if the following patch cures the problem?
Unfortunately not. That patch installs the check after the
acpi_i2c_get_info() invocation which is part of the backtrace above.
I moved your check into i2c_get_info(), right in front of the IRQ
handling and this works.
So,
Tested-by: Nicolai Stange <nicstange@gmail.com>
for this:
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 74e5aea..3f2b3cf 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -141,6 +141,7 @@ static int acpi_i2c_get_info(struct acpi_device *adev,
struct list_head resource_list;
struct resource_entry *entry;
struct acpi_i2c_lookup lookup;
+ struct acpi_device *adapter_adev;
int ret;
if (acpi_bus_get_status(adev) || !adev->status.present ||
@@ -163,6 +164,12 @@ static int acpi_i2c_get_info(struct acpi_device *adev,
if (ret < 0 || !info->addr)
return -EINVAL;
+ /* The adapter must be present */
+ if (acpi_bus_get_device(lookup.adapter_handle, &adapter_adev))
+ return -EINVAL;
+ if (acpi_bus_get_status(adapter_adev) || !adapter_adev->status.present)
+ return -EINVAL;;
+
*adapter_handle = lookup.adapter_handle;
/* Then fill IRQ number if any */
But it is still true that acpi_i2c_register_devices() configures the
interrupts for all ACPI I2C slaves attached to an available adapter,
independent of whether their adapter is the one given as an argument or
not. I can't tell whether this is desired, just a note...
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [REGRESSION? v4.8] i2c-core: acpi_i2c_get_info() touches non-existent devices
2016-09-19 13:58 ` Nicolai Stange
@ 2016-09-20 7:55 ` Mika Westerberg
2016-09-20 8:00 ` [PATCH] i2c / ACPI: Do not touch an I2C device if it belongs to another adapter Mika Westerberg
1 sibling, 0 replies; 16+ messages in thread
From: Mika Westerberg @ 2016-09-20 7:55 UTC (permalink / raw)
To: Nicolai Stange
Cc: Wolfram Sang, Octavian Purdila, Rafael J. Wysocki, linux-i2c,
linux-kernel
On Mon, Sep 19, 2016 at 03:58:52PM +0200, Nicolai Stange wrote:
> > Can you try if the following patch cures the problem?
>
> Unfortunately not. That patch installs the check after the
> acpi_i2c_get_info() invocation which is part of the backtrace above.
Ah, indeed it needs to happen before we parse resources.
> I moved your check into i2c_get_info(), right in front of the IRQ
> handling and this works.
>
> So,
>
> Tested-by: Nicolai Stange <nicstange@gmail.com>
Thanks.
> for this:
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 74e5aea..3f2b3cf 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -141,6 +141,7 @@ static int acpi_i2c_get_info(struct acpi_device *adev,
> struct list_head resource_list;
> struct resource_entry *entry;
> struct acpi_i2c_lookup lookup;
> + struct acpi_device *adapter_adev;
> int ret;
>
> if (acpi_bus_get_status(adev) || !adev->status.present ||
> @@ -163,6 +164,12 @@ static int acpi_i2c_get_info(struct acpi_device *adev,
> if (ret < 0 || !info->addr)
> return -EINVAL;
>
> + /* The adapter must be present */
> + if (acpi_bus_get_device(lookup.adapter_handle, &adapter_adev))
> + return -EINVAL;
> + if (acpi_bus_get_status(adapter_adev) || !adapter_adev->status.present)
> + return -EINVAL;;
> +
> *adapter_handle = lookup.adapter_handle;
>
> /* Then fill IRQ number if any */
>
>
>
> But it is still true that acpi_i2c_register_devices() configures the
> interrupts for all ACPI I2C slaves attached to an available adapter,
> independent of whether their adapter is the one given as an argument or
> not. I can't tell whether this is desired, just a note...
You are right it should not do that. I'll send you an updated patch
shortly, can you try if it works?
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] i2c / ACPI: Do not touch an I2C device if it belongs to another adapter
2016-09-19 13:58 ` Nicolai Stange
2016-09-20 7:55 ` Mika Westerberg
@ 2016-09-20 8:00 ` Mika Westerberg
2016-09-20 10:32 ` Nicolai Stange
1 sibling, 1 reply; 16+ messages in thread
From: Mika Westerberg @ 2016-09-20 8:00 UTC (permalink / raw)
To: Wolfram Sang
Cc: Nicolai Stange, Octavian Purdila, Rafael J . Wysocki,
Jarkko Nikula, Mika Westerberg, linux-i2c, linux-kernel
When enumerating I2C devices connected to an I2C adapter we scan the whole
namespace (as it is possible to have devices anywhere in that namespace,
not just below the I2C adapter device) and add each found device to the I2C
bus in question.
Now after commit 525e6fabeae2 ("i2c / ACPI: add support for ACPI
reconfigure notifications") checking of the adapter handle to the one found
in the I2cSerialBus() resource was moved to happen after resources of the
I2C device has been parsed. This means that if the I2cSerialBus() resource
points to an adapter that does not exists in the system we still parse
those resources. This is problematic in particular because
acpi_dev_resource_interrupt() tries to configure GSI if the device also has
an Interrupt() resource. Failing to do that results errrors like this to be
printed on the console:
[ 10.409490] ERROR: Unable to locate IOAPIC for GSI 37
To fix this we pass the I2C adapter to i2c_acpi_get_info() and make sure
the handle matches the one in the I2cSerialBus() resource before doing
anything else to the device.
Reported-by: Nicolai Stange <nicstange@gmail.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
drivers/i2c/i2c-core.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index c61c961cf8f9..eb32cb783fc8 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -168,6 +168,7 @@ static int i2c_acpi_do_lookup(struct acpi_device *adev,
static int i2c_acpi_get_info(struct acpi_device *adev,
struct i2c_board_info *info,
+ struct i2c_adapter *adapter,
acpi_handle *adapter_handle)
{
struct list_head resource_list;
@@ -182,6 +183,10 @@ static int i2c_acpi_get_info(struct acpi_device *adev,
if (ret)
return ret;
+ /* The adapter must match the one in I2cSerialBus() connector */
+ if (adapter && ACPI_HANDLE(&adapter->dev) != lookup.adapter_handle)
+ return -ENODEV;
+
info->fwnode = acpi_fwnode_handle(adev);
*adapter_handle = lookup.adapter_handle;
@@ -231,10 +236,7 @@ static acpi_status i2c_acpi_add_device(acpi_handle handle, u32 level,
if (acpi_bus_get_device(handle, &adev))
return AE_OK;
- if (i2c_acpi_get_info(adev, &info, &adapter_handle))
- return AE_OK;
-
- if (adapter_handle != ACPI_HANDLE(&adapter->dev))
+ if (i2c_acpi_get_info(adev, &info, adapter, &adapter_handle))
return AE_OK;
i2c_acpi_register_device(adapter, adev, &info);
@@ -368,7 +370,7 @@ static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
switch (value) {
case ACPI_RECONFIG_DEVICE_ADD:
- if (i2c_acpi_get_info(adev, &info, &adapter_handle))
+ if (i2c_acpi_get_info(adev, &info, NULL, &adapter_handle))
break;
adapter = i2c_acpi_find_adapter_by_handle(adapter_handle);
--
2.9.3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] i2c / ACPI: Do not touch an I2C device if it belongs to another adapter
2016-09-20 8:00 ` [PATCH] i2c / ACPI: Do not touch an I2C device if it belongs to another adapter Mika Westerberg
@ 2016-09-20 10:32 ` Nicolai Stange
2016-09-20 10:45 ` Mika Westerberg
2016-09-20 13:59 ` [PATCH v2] " Mika Westerberg
0 siblings, 2 replies; 16+ messages in thread
From: Nicolai Stange @ 2016-09-20 10:32 UTC (permalink / raw)
To: Mika Westerberg
Cc: Wolfram Sang, Nicolai Stange, Octavian Purdila,
Rafael J . Wysocki, Jarkko Nikula, linux-i2c, linux-kernel
Your patch fixes my issue, so feel free to add a
Tested-by: Nicolai Stange <nicstange@gmail.com>
for this either.
But please see my remark below.
Mika Westerberg <mika.westerberg@linux.intel.com> writes:
> When enumerating I2C devices connected to an I2C adapter we scan the whole
> namespace (as it is possible to have devices anywhere in that namespace,
> not just below the I2C adapter device) and add each found device to the I2C
> bus in question.
>
> Now after commit 525e6fabeae2 ("i2c / ACPI: add support for ACPI
> reconfigure notifications") checking of the adapter handle to the one found
> in the I2cSerialBus() resource was moved to happen after resources of the
> I2C device has been parsed. This means that if the I2cSerialBus() resource
> points to an adapter that does not exists in the system we still parse
> those resources. This is problematic in particular because
> acpi_dev_resource_interrupt() tries to configure GSI if the device also has
> an Interrupt() resource. Failing to do that results errrors like this to be
> printed on the console:
>
> [ 10.409490] ERROR: Unable to locate IOAPIC for GSI 37
>
> To fix this we pass the I2C adapter to i2c_acpi_get_info() and make sure
> the handle matches the one in the I2cSerialBus() resource before doing
> anything else to the device.
>
> Reported-by: Nicolai Stange <nicstange@gmail.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> drivers/i2c/i2c-core.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index c61c961cf8f9..eb32cb783fc8 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -168,6 +168,7 @@ static int i2c_acpi_do_lookup(struct acpi_device *adev,
>
> static int i2c_acpi_get_info(struct acpi_device *adev,
> struct i2c_board_info *info,
> + struct i2c_adapter *adapter,
> acpi_handle *adapter_handle)
> {
> struct list_head resource_list;
> @@ -182,6 +183,10 @@ static int i2c_acpi_get_info(struct acpi_device *adev,
> if (ret)
> return ret;
>
> + /* The adapter must match the one in I2cSerialBus() connector */
> + if (adapter && ACPI_HANDLE(&adapter->dev) != lookup.adapter_handle)
> + return -ENODEV;
> +
Would it be sensible to add the adapter presence check you provided
earlier, i.e.
+ else if (!adapter) {
+ /* The adapter must be present */
+ if (acpi_bus_get_device(lookup.adapter_handle, &adapter_adev))
+ return -ENODEV;
+ if (acpi_bus_get_status(adapter_adev) || !adapter_adev->status.present)
+ return -ENODEV;
+
+ }
here, because we can't know if ...
> info->fwnode = acpi_fwnode_handle(adev);
> *adapter_handle = lookup.adapter_handle;
>
> @@ -231,10 +236,7 @@ static acpi_status i2c_acpi_add_device(acpi_handle handle, u32 level,
> if (acpi_bus_get_device(handle, &adev))
> return AE_OK;
>
> - if (i2c_acpi_get_info(adev, &info, &adapter_handle))
> - return AE_OK;
> -
> - if (adapter_handle != ACPI_HANDLE(&adapter->dev))
> + if (i2c_acpi_get_info(adev, &info, adapter, &adapter_handle))
> return AE_OK;
>
> i2c_acpi_register_device(adapter, adev, &info);
> @@ -368,7 +370,7 @@ static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
>
> switch (value) {
> case ACPI_RECONFIG_DEVICE_ADD:
> - if (i2c_acpi_get_info(adev, &info, &adapter_handle))
> + if (i2c_acpi_get_info(adev, &info, NULL, &adapter_handle))
> break;
... the ACPI device added here is physically existent?
>
> adapter = i2c_acpi_find_adapter_by_handle(adapter_handle);
I suppose that it is always true that adev has been LoadTable()'d from
some SSDT? Can't this SSDT be just as broken as my DSDT is? Not that
I've seen such a case in the real world, I'm just asking.
Thanks,
Nicolai
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] i2c / ACPI: Do not touch an I2C device if it belongs to another adapter
2016-09-20 10:32 ` Nicolai Stange
@ 2016-09-20 10:45 ` Mika Westerberg
2016-09-20 13:59 ` [PATCH v2] " Mika Westerberg
1 sibling, 0 replies; 16+ messages in thread
From: Mika Westerberg @ 2016-09-20 10:45 UTC (permalink / raw)
To: Nicolai Stange
Cc: Wolfram Sang, Octavian Purdila, Rafael J . Wysocki,
Jarkko Nikula, linux-i2c, linux-kernel
On Tue, Sep 20, 2016 at 12:32:11PM +0200, Nicolai Stange wrote:
> Your patch fixes my issue, so feel free to add a
>
> Tested-by: Nicolai Stange <nicstange@gmail.com>
Thanks!
> for this either.
>
> But please see my remark below.
>
>
> Mika Westerberg <mika.westerberg@linux.intel.com> writes:
>
> > When enumerating I2C devices connected to an I2C adapter we scan the whole
> > namespace (as it is possible to have devices anywhere in that namespace,
> > not just below the I2C adapter device) and add each found device to the I2C
> > bus in question.
> >
> > Now after commit 525e6fabeae2 ("i2c / ACPI: add support for ACPI
> > reconfigure notifications") checking of the adapter handle to the one found
> > in the I2cSerialBus() resource was moved to happen after resources of the
> > I2C device has been parsed. This means that if the I2cSerialBus() resource
> > points to an adapter that does not exists in the system we still parse
> > those resources. This is problematic in particular because
> > acpi_dev_resource_interrupt() tries to configure GSI if the device also has
> > an Interrupt() resource. Failing to do that results errrors like this to be
> > printed on the console:
> >
> > [ 10.409490] ERROR: Unable to locate IOAPIC for GSI 37
> >
> > To fix this we pass the I2C adapter to i2c_acpi_get_info() and make sure
> > the handle matches the one in the I2cSerialBus() resource before doing
> > anything else to the device.
> >
> > Reported-by: Nicolai Stange <nicstange@gmail.com>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> > drivers/i2c/i2c-core.c | 12 +++++++-----
> > 1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index c61c961cf8f9..eb32cb783fc8 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -168,6 +168,7 @@ static int i2c_acpi_do_lookup(struct acpi_device *adev,
> >
> > static int i2c_acpi_get_info(struct acpi_device *adev,
> > struct i2c_board_info *info,
> > + struct i2c_adapter *adapter,
> > acpi_handle *adapter_handle)
> > {
> > struct list_head resource_list;
> > @@ -182,6 +183,10 @@ static int i2c_acpi_get_info(struct acpi_device *adev,
> > if (ret)
> > return ret;
> >
> > + /* The adapter must match the one in I2cSerialBus() connector */
> > + if (adapter && ACPI_HANDLE(&adapter->dev) != lookup.adapter_handle)
> > + return -ENODEV;
> > +
>
> Would it be sensible to add the adapter presence check you provided
> earlier, i.e.
>
> + else if (!adapter) {
> + /* The adapter must be present */
> + if (acpi_bus_get_device(lookup.adapter_handle, &adapter_adev))
> + return -ENODEV;
> + if (acpi_bus_get_status(adapter_adev) || !adapter_adev->status.present)
> + return -ENODEV;
> +
> + }
>
>
> here, because we can't know if ...
>
>
> > info->fwnode = acpi_fwnode_handle(adev);
> > *adapter_handle = lookup.adapter_handle;
> >
> > @@ -231,10 +236,7 @@ static acpi_status i2c_acpi_add_device(acpi_handle handle, u32 level,
> > if (acpi_bus_get_device(handle, &adev))
> > return AE_OK;
> >
> > - if (i2c_acpi_get_info(adev, &info, &adapter_handle))
> > - return AE_OK;
> > -
> > - if (adapter_handle != ACPI_HANDLE(&adapter->dev))
> > + if (i2c_acpi_get_info(adev, &info, adapter, &adapter_handle))
> > return AE_OK;
> >
> > i2c_acpi_register_device(adapter, adev, &info);
> > @@ -368,7 +370,7 @@ static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
> >
> > switch (value) {
> > case ACPI_RECONFIG_DEVICE_ADD:
> > - if (i2c_acpi_get_info(adev, &info, &adapter_handle))
> > + if (i2c_acpi_get_info(adev, &info, NULL, &adapter_handle))
> > break;
>
> ... the ACPI device added here is physically existent?
Good point.
>
> >
> > adapter = i2c_acpi_find_adapter_by_handle(adapter_handle);
>
> I suppose that it is always true that adev has been LoadTable()'d from
> some SSDT? Can't this SSDT be just as broken as my DSDT is? Not that
> I've seen such a case in the real world, I'm just asking.
Yes it can be broken and since the adapter reference is just a string in
I2cSerialBus() resource we definitely need to check that it actually
exists.
I'll submit v2 soon.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] i2c / ACPI: Do not touch an I2C device if it belongs to another adapter
2016-09-20 10:32 ` Nicolai Stange
2016-09-20 10:45 ` Mika Westerberg
@ 2016-09-20 13:59 ` Mika Westerberg
2016-09-20 18:49 ` Nicolai Stange
` (2 more replies)
1 sibling, 3 replies; 16+ messages in thread
From: Mika Westerberg @ 2016-09-20 13:59 UTC (permalink / raw)
To: Wolfram Sang
Cc: Nicolai Stange, Octavian Purdila, Rafael J . Wysocki,
Jarkko Nikula, Mika Westerberg, linux-i2c, linux-kernel
When enumerating I2C devices connected to an I2C adapter we scan the whole
namespace (as it is possible to have devices anywhere in that namespace,
not just below the I2C adapter device) and add each found device to the I2C
bus in question.
Now after commit 525e6fabeae2 ("i2c / ACPI: add support for ACPI
reconfigure notifications") checking of the adapter handle to the one found
in the I2cSerialBus() resource was moved to happen after resources of the
I2C device has been parsed. This means that if the I2cSerialBus() resource
points to an adapter that does not exists in the system we still parse
those resources. This is problematic in particular because
acpi_dev_resource_interrupt() tries to configure GSI if the device also has
an Interrupt() resource. Failing to do that results errrors like this to be
printed on the console:
[ 10.409490] ERROR: Unable to locate IOAPIC for GSI 37
To fix this we pass the I2C adapter to i2c_acpi_get_info() and make sure
the handle matches the one in the I2cSerialBus() resource before doing
anything else to the device.
Reported-and-tested-by: Nicolai Stange <nicstange@gmail.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
Changes from v1:
* Require that ACPI adapter device is present if !adapter
* Fill in adapter_handle only if not NULL
* Remove passing of adapter_handle in i2c_acpi_add_device()
drivers/i2c/i2c-core.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index c61c961cf8f9..5476f906b9e0 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -168,6 +168,7 @@ static int i2c_acpi_do_lookup(struct acpi_device *adev,
static int i2c_acpi_get_info(struct acpi_device *adev,
struct i2c_board_info *info,
+ struct i2c_adapter *adapter,
acpi_handle *adapter_handle)
{
struct list_head resource_list;
@@ -182,8 +183,24 @@ static int i2c_acpi_get_info(struct acpi_device *adev,
if (ret)
return ret;
+ if (adapter) {
+ /* The adapter must match the one in I2cSerialBus() connector */
+ if (ACPI_HANDLE(&adapter->dev) != lookup.adapter_handle)
+ return -ENODEV;
+ } else {
+ struct acpi_device *adapter_adev;
+
+ /* The adapter must be present */
+ if (acpi_bus_get_device(lookup.adapter_handle, &adapter_adev))
+ return -ENODEV;
+ if (acpi_bus_get_status(adapter_adev) ||
+ !adapter_adev->status.present)
+ return -ENODEV;
+ }
+
info->fwnode = acpi_fwnode_handle(adev);
- *adapter_handle = lookup.adapter_handle;
+ if (adapter_handle)
+ *adapter_handle = lookup.adapter_handle;
/* Then fill IRQ number if any */
INIT_LIST_HEAD(&resource_list);
@@ -225,16 +242,12 @@ static acpi_status i2c_acpi_add_device(acpi_handle handle, u32 level,
{
struct i2c_adapter *adapter = data;
struct acpi_device *adev;
- acpi_handle adapter_handle;
struct i2c_board_info info;
if (acpi_bus_get_device(handle, &adev))
return AE_OK;
- if (i2c_acpi_get_info(adev, &info, &adapter_handle))
- return AE_OK;
-
- if (adapter_handle != ACPI_HANDLE(&adapter->dev))
+ if (i2c_acpi_get_info(adev, &info, adapter, NULL))
return AE_OK;
i2c_acpi_register_device(adapter, adev, &info);
@@ -368,7 +381,7 @@ static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
switch (value) {
case ACPI_RECONFIG_DEVICE_ADD:
- if (i2c_acpi_get_info(adev, &info, &adapter_handle))
+ if (i2c_acpi_get_info(adev, &info, NULL, &adapter_handle))
break;
adapter = i2c_acpi_find_adapter_by_handle(adapter_handle);
--
2.9.3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2] i2c / ACPI: Do not touch an I2C device if it belongs to another adapter
2016-09-20 13:59 ` [PATCH v2] " Mika Westerberg
@ 2016-09-20 18:49 ` Nicolai Stange
2016-09-21 5:48 ` Wolfram Sang
2016-09-22 17:46 ` Wolfram Sang
2 siblings, 0 replies; 16+ messages in thread
From: Nicolai Stange @ 2016-09-20 18:49 UTC (permalink / raw)
To: Mika Westerberg
Cc: Wolfram Sang, Nicolai Stange, Octavian Purdila,
Rafael J . Wysocki, Jarkko Nikula, linux-i2c, linux-kernel
Mika Westerberg <mika.westerberg@linux.intel.com> writes:
> When enumerating I2C devices connected to an I2C adapter we scan the whole
> namespace (as it is possible to have devices anywhere in that namespace,
> not just below the I2C adapter device) and add each found device to the I2C
> bus in question.
>
> Now after commit 525e6fabeae2 ("i2c / ACPI: add support for ACPI
> reconfigure notifications") checking of the adapter handle to the one found
> in the I2cSerialBus() resource was moved to happen after resources of the
> I2C device has been parsed. This means that if the I2cSerialBus() resource
> points to an adapter that does not exists in the system we still parse
> those resources. This is problematic in particular because
> acpi_dev_resource_interrupt() tries to configure GSI if the device also has
> an Interrupt() resource. Failing to do that results errrors like this to be
> printed on the console:
>
> [ 10.409490] ERROR: Unable to locate IOAPIC for GSI 37
>
> To fix this we pass the I2C adapter to i2c_acpi_get_info() and make sure
> the handle matches the one in the I2cSerialBus() resource before doing
> anything else to the device.
>
> Reported-and-tested-by: Nicolai Stange <nicstange@gmail.com>
I've just retested this v2 explicitly and it works like a charm.
Also,
Reviewed-by: Nicolai Stange <nicstange@gmail.com>
if you want.
Thanks,
Nicolai
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] i2c / ACPI: Do not touch an I2C device if it belongs to another adapter
2016-09-20 13:59 ` [PATCH v2] " Mika Westerberg
2016-09-20 18:49 ` Nicolai Stange
@ 2016-09-21 5:48 ` Wolfram Sang
2016-09-21 8:45 ` Mika Westerberg
2016-09-22 17:46 ` Wolfram Sang
2 siblings, 1 reply; 16+ messages in thread
From: Wolfram Sang @ 2016-09-21 5:48 UTC (permalink / raw)
To: Mika Westerberg
Cc: Nicolai Stange, Octavian Purdila, Rafael J . Wysocki,
Jarkko Nikula, linux-i2c, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1481 bytes --]
On Tue, Sep 20, 2016 at 04:59:25PM +0300, Mika Westerberg wrote:
> When enumerating I2C devices connected to an I2C adapter we scan the whole
> namespace (as it is possible to have devices anywhere in that namespace,
> not just below the I2C adapter device) and add each found device to the I2C
> bus in question.
>
> Now after commit 525e6fabeae2 ("i2c / ACPI: add support for ACPI
> reconfigure notifications") checking of the adapter handle to the one found
> in the I2cSerialBus() resource was moved to happen after resources of the
> I2C device has been parsed. This means that if the I2cSerialBus() resource
> points to an adapter that does not exists in the system we still parse
> those resources. This is problematic in particular because
> acpi_dev_resource_interrupt() tries to configure GSI if the device also has
> an Interrupt() resource. Failing to do that results errrors like this to be
> printed on the console:
>
> [ 10.409490] ERROR: Unable to locate IOAPIC for GSI 37
>
> To fix this we pass the I2C adapter to i2c_acpi_get_info() and make sure
> the handle matches the one in the I2cSerialBus() resource before doing
> anything else to the device.
>
> Reported-and-tested-by: Nicolai Stange <nicstange@gmail.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Considering this for for-current. So shall we add:
Fixes: 525e6fabeae2 ("i2c / ACPI: add support for ACPI reconfigure notifications")
?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] i2c / ACPI: Do not touch an I2C device if it belongs to another adapter
2016-09-21 5:48 ` Wolfram Sang
@ 2016-09-21 8:45 ` Mika Westerberg
2016-09-21 16:14 ` Wolfram Sang
0 siblings, 1 reply; 16+ messages in thread
From: Mika Westerberg @ 2016-09-21 8:45 UTC (permalink / raw)
To: Wolfram Sang
Cc: Nicolai Stange, Octavian Purdila, Rafael J . Wysocki,
Jarkko Nikula, linux-i2c, linux-kernel
On Wed, Sep 21, 2016 at 07:48:35AM +0200, Wolfram Sang wrote:
> On Tue, Sep 20, 2016 at 04:59:25PM +0300, Mika Westerberg wrote:
> > When enumerating I2C devices connected to an I2C adapter we scan the whole
> > namespace (as it is possible to have devices anywhere in that namespace,
> > not just below the I2C adapter device) and add each found device to the I2C
> > bus in question.
> >
> > Now after commit 525e6fabeae2 ("i2c / ACPI: add support for ACPI
> > reconfigure notifications") checking of the adapter handle to the one found
> > in the I2cSerialBus() resource was moved to happen after resources of the
> > I2C device has been parsed. This means that if the I2cSerialBus() resource
> > points to an adapter that does not exists in the system we still parse
> > those resources. This is problematic in particular because
> > acpi_dev_resource_interrupt() tries to configure GSI if the device also has
> > an Interrupt() resource. Failing to do that results errrors like this to be
> > printed on the console:
> >
> > [ 10.409490] ERROR: Unable to locate IOAPIC for GSI 37
> >
> > To fix this we pass the I2C adapter to i2c_acpi_get_info() and make sure
> > the handle matches the one in the I2cSerialBus() resource before doing
> > anything else to the device.
> >
> > Reported-and-tested-by: Nicolai Stange <nicstange@gmail.com>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
> Considering this for for-current. So shall we add:
>
> Fixes: 525e6fabeae2 ("i2c / ACPI: add support for ACPI reconfigure notifications")
>
> ?
Yes please :)
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] i2c / ACPI: Do not touch an I2C device if it belongs to another adapter
2016-09-21 8:45 ` Mika Westerberg
@ 2016-09-21 16:14 ` Wolfram Sang
2016-09-22 8:49 ` Mika Westerberg
0 siblings, 1 reply; 16+ messages in thread
From: Wolfram Sang @ 2016-09-21 16:14 UTC (permalink / raw)
To: Mika Westerberg
Cc: Nicolai Stange, Octavian Purdila, Rafael J . Wysocki,
Jarkko Nikula, linux-i2c, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1823 bytes --]
On Wed, Sep 21, 2016 at 11:45:02AM +0300, Mika Westerberg wrote:
> On Wed, Sep 21, 2016 at 07:48:35AM +0200, Wolfram Sang wrote:
> > On Tue, Sep 20, 2016 at 04:59:25PM +0300, Mika Westerberg wrote:
> > > When enumerating I2C devices connected to an I2C adapter we scan the whole
> > > namespace (as it is possible to have devices anywhere in that namespace,
> > > not just below the I2C adapter device) and add each found device to the I2C
> > > bus in question.
> > >
> > > Now after commit 525e6fabeae2 ("i2c / ACPI: add support for ACPI
> > > reconfigure notifications") checking of the adapter handle to the one found
> > > in the I2cSerialBus() resource was moved to happen after resources of the
> > > I2C device has been parsed. This means that if the I2cSerialBus() resource
> > > points to an adapter that does not exists in the system we still parse
> > > those resources. This is problematic in particular because
> > > acpi_dev_resource_interrupt() tries to configure GSI if the device also has
> > > an Interrupt() resource. Failing to do that results errrors like this to be
> > > printed on the console:
> > >
> > > [ 10.409490] ERROR: Unable to locate IOAPIC for GSI 37
> > >
> > > To fix this we pass the I2C adapter to i2c_acpi_get_info() and make sure
> > > the handle matches the one in the I2cSerialBus() resource before doing
> > > anything else to the device.
> > >
> > > Reported-and-tested-by: Nicolai Stange <nicstange@gmail.com>
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> >
> > Considering this for for-current. So shall we add:
> >
> > Fixes: 525e6fabeae2 ("i2c / ACPI: add support for ACPI reconfigure notifications")
> >
> > ?
>
> Yes please :)
Huh? It doesn't apply on top of rc7 here? What did you base it on?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] i2c / ACPI: Do not touch an I2C device if it belongs to another adapter
2016-09-21 16:14 ` Wolfram Sang
@ 2016-09-22 8:49 ` Mika Westerberg
2016-09-22 8:59 ` Wolfram Sang
0 siblings, 1 reply; 16+ messages in thread
From: Mika Westerberg @ 2016-09-22 8:49 UTC (permalink / raw)
To: Wolfram Sang
Cc: Nicolai Stange, Octavian Purdila, Rafael J . Wysocki,
Jarkko Nikula, linux-i2c, linux-kernel
On Wed, Sep 21, 2016 at 06:14:38PM +0200, Wolfram Sang wrote:
> On Wed, Sep 21, 2016 at 11:45:02AM +0300, Mika Westerberg wrote:
> > On Wed, Sep 21, 2016 at 07:48:35AM +0200, Wolfram Sang wrote:
> > > On Tue, Sep 20, 2016 at 04:59:25PM +0300, Mika Westerberg wrote:
> > > > When enumerating I2C devices connected to an I2C adapter we scan the whole
> > > > namespace (as it is possible to have devices anywhere in that namespace,
> > > > not just below the I2C adapter device) and add each found device to the I2C
> > > > bus in question.
> > > >
> > > > Now after commit 525e6fabeae2 ("i2c / ACPI: add support for ACPI
> > > > reconfigure notifications") checking of the adapter handle to the one found
> > > > in the I2cSerialBus() resource was moved to happen after resources of the
> > > > I2C device has been parsed. This means that if the I2cSerialBus() resource
> > > > points to an adapter that does not exists in the system we still parse
> > > > those resources. This is problematic in particular because
> > > > acpi_dev_resource_interrupt() tries to configure GSI if the device also has
> > > > an Interrupt() resource. Failing to do that results errrors like this to be
> > > > printed on the console:
> > > >
> > > > [ 10.409490] ERROR: Unable to locate IOAPIC for GSI 37
> > > >
> > > > To fix this we pass the I2C adapter to i2c_acpi_get_info() and make sure
> > > > the handle matches the one in the I2cSerialBus() resource before doing
> > > > anything else to the device.
> > > >
> > > > Reported-and-tested-by: Nicolai Stange <nicstange@gmail.com>
> > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > >
> > > Considering this for for-current. So shall we add:
> > >
> > > Fixes: 525e6fabeae2 ("i2c / ACPI: add support for ACPI reconfigure notifications")
> > >
> > > ?
> >
> > Yes please :)
>
> Huh? It doesn't apply on top of rc7 here? What did you base it on?
It is based on linux-next as it is on top of Jarkko's I2C ACPI namespace
cleanup patches. I'm wondering if I make an updated patch on top of
v4.8-rc7 does it conflict with the I2C stuff in linux-next? What's your
preference?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] i2c / ACPI: Do not touch an I2C device if it belongs to another adapter
2016-09-22 8:49 ` Mika Westerberg
@ 2016-09-22 8:59 ` Wolfram Sang
2016-09-22 9:25 ` Mika Westerberg
0 siblings, 1 reply; 16+ messages in thread
From: Wolfram Sang @ 2016-09-22 8:59 UTC (permalink / raw)
To: Mika Westerberg
Cc: Nicolai Stange, Octavian Purdila, Rafael J . Wysocki,
Jarkko Nikula, linux-i2c, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 431 bytes --]
> > Huh? It doesn't apply on top of rc7 here? What did you base it on?
>
> It is based on linux-next as it is on top of Jarkko's I2C ACPI namespace
> cleanup patches. I'm wondering if I make an updated patch on top of
> v4.8-rc7 does it conflict with the I2C stuff in linux-next? What's your
> preference?
I see. I'll add it to for-next, then. If someone wants it backported, it
needs to be rewritten and re-tested.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] i2c / ACPI: Do not touch an I2C device if it belongs to another adapter
2016-09-22 8:59 ` Wolfram Sang
@ 2016-09-22 9:25 ` Mika Westerberg
0 siblings, 0 replies; 16+ messages in thread
From: Mika Westerberg @ 2016-09-22 9:25 UTC (permalink / raw)
To: Wolfram Sang
Cc: Nicolai Stange, Octavian Purdila, Rafael J . Wysocki,
Jarkko Nikula, linux-i2c, linux-kernel
On Thu, Sep 22, 2016 at 10:59:24AM +0200, Wolfram Sang wrote:
>
> > > Huh? It doesn't apply on top of rc7 here? What did you base it on?
> >
> > It is based on linux-next as it is on top of Jarkko's I2C ACPI namespace
> > cleanup patches. I'm wondering if I make an updated patch on top of
> > v4.8-rc7 does it conflict with the I2C stuff in linux-next? What's your
> > preference?
>
> I see. I'll add it to for-next, then. If someone wants it backported, it
> needs to be rewritten and re-tested.
OK, thanks.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] i2c / ACPI: Do not touch an I2C device if it belongs to another adapter
2016-09-20 13:59 ` [PATCH v2] " Mika Westerberg
2016-09-20 18:49 ` Nicolai Stange
2016-09-21 5:48 ` Wolfram Sang
@ 2016-09-22 17:46 ` Wolfram Sang
2 siblings, 0 replies; 16+ messages in thread
From: Wolfram Sang @ 2016-09-22 17:46 UTC (permalink / raw)
To: Mika Westerberg
Cc: Nicolai Stange, Octavian Purdila, Rafael J . Wysocki,
Jarkko Nikula, linux-i2c, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]
On Tue, Sep 20, 2016 at 04:59:25PM +0300, Mika Westerberg wrote:
> When enumerating I2C devices connected to an I2C adapter we scan the whole
> namespace (as it is possible to have devices anywhere in that namespace,
> not just below the I2C adapter device) and add each found device to the I2C
> bus in question.
>
> Now after commit 525e6fabeae2 ("i2c / ACPI: add support for ACPI
> reconfigure notifications") checking of the adapter handle to the one found
> in the I2cSerialBus() resource was moved to happen after resources of the
> I2C device has been parsed. This means that if the I2cSerialBus() resource
> points to an adapter that does not exists in the system we still parse
> those resources. This is problematic in particular because
> acpi_dev_resource_interrupt() tries to configure GSI if the device also has
> an Interrupt() resource. Failing to do that results errrors like this to be
> printed on the console:
>
> [ 10.409490] ERROR: Unable to locate IOAPIC for GSI 37
>
> To fix this we pass the I2C adapter to i2c_acpi_get_info() and make sure
> the handle matches the one in the I2cSerialBus() resource before doing
> anything else to the device.
>
> Reported-and-tested-by: Nicolai Stange <nicstange@gmail.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Applied to for-next, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-09-22 18:28 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <87a8f4kfhe.fsf@gmail.com>
2016-09-19 8:48 ` [REGRESSION? v4.8] i2c-core: acpi_i2c_get_info() touches non-existent devices Mika Westerberg
2016-09-19 13:03 ` Mika Westerberg
2016-09-19 13:58 ` Nicolai Stange
2016-09-20 7:55 ` Mika Westerberg
2016-09-20 8:00 ` [PATCH] i2c / ACPI: Do not touch an I2C device if it belongs to another adapter Mika Westerberg
2016-09-20 10:32 ` Nicolai Stange
2016-09-20 10:45 ` Mika Westerberg
2016-09-20 13:59 ` [PATCH v2] " Mika Westerberg
2016-09-20 18:49 ` Nicolai Stange
2016-09-21 5:48 ` Wolfram Sang
2016-09-21 8:45 ` Mika Westerberg
2016-09-21 16:14 ` Wolfram Sang
2016-09-22 8:49 ` Mika Westerberg
2016-09-22 8:59 ` Wolfram Sang
2016-09-22 9:25 ` Mika Westerberg
2016-09-22 17:46 ` Wolfram Sang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).