Linux-i2c Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 5.8+ regression fix 0/1] i2c: core: Restore acpi_walk_dep_device_list() getting called after registering the ACPI i2c devs
@ 2020-10-14 14:41 Hans de Goede
  2020-10-14 14:41 ` [PATCH 5.8+ regression fix] " Hans de Goede
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Hans de Goede @ 2020-10-14 14:41 UTC (permalink / raw)
  To: Wolfram Sang, Mika Westerberg, Greg Kroah-Hartman
  Cc: Hans de Goede, Kieran Bingham, Maximilian Luz, stable, linux-i2c,
	linux-acpi

Hi All,

I am afraid that commit 21653a4181ff ("i2c: core: Call
i2c_acpi_install_space_handler() before i2c_acpi_register_devices()")
which is in 5.9 and was also added to 5.8.13 (and possible other
stable series releases) causes a regression on some devices including
on the Microsoft Surface Go 2 (and possibly also the Go 1) where the
system no longer boots.

Before the troublesome commit the ACPI related i2c core code looked
like this:

        /* create pre-declared device nodes */
        of_i2c_register_devices(adap);
        i2c_acpi_register_devices(adap);
        i2c_acpi_install_space_handler(adap);

	if (...

The trouble some commit changed this to:

        /* create pre-declared device nodes */
        of_i2c_register_devices(adap);
        i2c_acpi_install_space_handler(adap);
        i2c_acpi_register_devices(adap);

	if (...

The goal here was to only move the acpi_install_address_space_handler()
which is wrapped by i2c_acpi_install_space_handler() which should be
pretty safe. But as Maximilian Luz' sharp eyes pointed out while
debugging the regression, i2c_acpi_install_space_handler() does more,
it also contains a call to acpi_walk_dep_device_list() at the end.
Which I missed and which is causing the trouble we are seeing now.

The original code flow looked like this:

1.  i2c_acpi_register_devices()
1.1  Create ACPI declared i2c_clients for adap
2.  i2c_acpi_install_space_handler(adap);
2.1  acpi_install_address_space_handler()
2.2  acpi_walk_dep_device_list()

And after the troublesome commit it now looks like this:

1.  i2c_acpi_install_space_handler(adap);
1.1  acpi_install_address_space_handler()
1.2  acpi_walk_dep_device_list()
2.  i2c_acpi_register_devices()
2.1  Create ACPI declared i2c_clients for adap

Notice that the acpi_walk_dep_device_list() now happens before
the i2c_clients below this adapter are created.

The patch in this 1 patch series fixes this by moving the
acpi_walk_dep_device_list() call to the end of
i2c_acpi_register_devices(), partly restoring the original
order, so that we now get:

1.  i2c_acpi_install_space_handler(adap);
1.1  acpi_install_address_space_handler()
2.  i2c_acpi_register_devices()
2.1  Create ACPI declared i2c_clients for adap
2.2  acpi_walk_dep_device_list()

So we end up calling acpi_walk_dep_device_list() last
again, as before.

Sorry for missing this the first time around.

Wolfram, can we get this queued up to go to Linus
soonish ?

Greg, in essence this is a partial revert of the trouble
some commit. With the 2 combined acpi_walk_dep_device_list()
is called last again, while still doing the
acpi_install_address_space_handler() call earlier.

Since this is a somewhat nasty regression and since the poor
timing wrt the merge-window, I was hoping that you would be
willing to make an exception and pick up this fix before
it hits Linus' tree?

The alternative would be to revert this now and then pick
up both again later, when the fix has landed in Linus'
tree. The problem with that is that reverting the original
troublesome commit will regress all these bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=1842039
https://bugzilla.redhat.com/show_bug.cgi?id=1871793
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1861610

Regards,

Hans


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 5.8+ regression fix] i2c: core: Restore acpi_walk_dep_device_list() getting called after registering the ACPI i2c devs
  2020-10-14 14:41 [PATCH 5.8+ regression fix 0/1] i2c: core: Restore acpi_walk_dep_device_list() getting called after registering the ACPI i2c devs Hans de Goede
@ 2020-10-14 14:41 ` Hans de Goede
  2020-10-25 12:21   ` Wolfram Sang
  2020-10-14 15:13 ` [PATCH 5.8+ regression fix 0/1] " Greg Kroah-Hartman
  2020-10-15 15:17 ` Sasha Levin
  2 siblings, 1 reply; 5+ messages in thread
From: Hans de Goede @ 2020-10-14 14:41 UTC (permalink / raw)
  To: Wolfram Sang, Mika Westerberg, Greg Kroah-Hartman
  Cc: Hans de Goede, Kieran Bingham, Maximilian Luz, stable, linux-i2c,
	linux-acpi

Commit 21653a4181ff ("i2c: core: Call i2c_acpi_install_space_handler()
before i2c_acpi_register_devices()")'s intention was to only move the
acpi_install_address_space_handler() call to the point before where
the ACPI declared i2c-children of the adapter where instantiated by
i2c_acpi_register_devices().

But i2c_acpi_install_space_handler() had a call to
acpi_walk_dep_device_list() hidden (that is I missed it) at the end
of it, so as an unwanted side-effect now acpi_walk_dep_device_list()
was also being called before i2c_acpi_register_devices().

Move the acpi_walk_dep_device_list() call to the end of
i2c_acpi_register_devices(), so that it is once again called *after*
the i2c_client-s hanging of the adapter have been created.

This fixes the Microsoft Surface Go 2 hanging at boot.

Fixes: 21653a4181ff ("i2c: core: Call i2c_acpi_install_space_handler() before i2c_acpi_register_devices()")
Suggested-by: Maximilian Luz <luzmaximilian@gmail.com>
Reported-and-tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/i2c-core-acpi.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index e627d7b2790f..37c510d9347a 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -264,6 +264,7 @@ static acpi_status i2c_acpi_add_device(acpi_handle handle, u32 level,
 void i2c_acpi_register_devices(struct i2c_adapter *adap)
 {
 	acpi_status status;
+	acpi_handle handle;
 
 	if (!has_acpi_companion(&adap->dev))
 		return;
@@ -274,6 +275,15 @@ void i2c_acpi_register_devices(struct i2c_adapter *adap)
 				     adap, NULL);
 	if (ACPI_FAILURE(status))
 		dev_warn(&adap->dev, "failed to enumerate I2C slaves\n");
+
+	if (!adap->dev.parent)
+		return;
+
+	handle = ACPI_HANDLE(adap->dev.parent);
+	if (!handle)
+		return;
+
+	acpi_walk_dep_device_list(handle);
 }
 
 static const struct acpi_device_id i2c_acpi_force_400khz_device_ids[] = {
@@ -719,7 +729,6 @@ int i2c_acpi_install_space_handler(struct i2c_adapter *adapter)
 		return -ENOMEM;
 	}
 
-	acpi_walk_dep_device_list(handle);
 	return 0;
 }
 
-- 
2.28.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 5.8+ regression fix 0/1] i2c: core: Restore acpi_walk_dep_device_list() getting called after registering the ACPI i2c devs
  2020-10-14 14:41 [PATCH 5.8+ regression fix 0/1] i2c: core: Restore acpi_walk_dep_device_list() getting called after registering the ACPI i2c devs Hans de Goede
  2020-10-14 14:41 ` [PATCH 5.8+ regression fix] " Hans de Goede
@ 2020-10-14 15:13 ` Greg Kroah-Hartman
  2020-10-15 15:17 ` Sasha Levin
  2 siblings, 0 replies; 5+ messages in thread
From: Greg Kroah-Hartman @ 2020-10-14 15:13 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Wolfram Sang, Mika Westerberg, Kieran Bingham, Maximilian Luz,
	stable, linux-i2c, linux-acpi

On Wed, Oct 14, 2020 at 04:41:57PM +0200, Hans de Goede wrote:
> Hi All,
> 
> I am afraid that commit 21653a4181ff ("i2c: core: Call
> i2c_acpi_install_space_handler() before i2c_acpi_register_devices()")
> which is in 5.9 and was also added to 5.8.13 (and possible other
> stable series releases) causes a regression on some devices including
> on the Microsoft Surface Go 2 (and possibly also the Go 1) where the
> system no longer boots.

That commit is also in the following stable releases:
	4.9.238 4.14.200 4.19.149 5.4.69 5.8.13

so it would need to fixed in all of those places :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 5.8+ regression fix 0/1] i2c: core: Restore acpi_walk_dep_device_list() getting called after registering the ACPI i2c devs
  2020-10-14 14:41 [PATCH 5.8+ regression fix 0/1] i2c: core: Restore acpi_walk_dep_device_list() getting called after registering the ACPI i2c devs Hans de Goede
  2020-10-14 14:41 ` [PATCH 5.8+ regression fix] " Hans de Goede
  2020-10-14 15:13 ` [PATCH 5.8+ regression fix 0/1] " Greg Kroah-Hartman
@ 2020-10-15 15:17 ` Sasha Levin
  2 siblings, 0 replies; 5+ messages in thread
From: Sasha Levin @ 2020-10-15 15:17 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Wolfram Sang, Mika Westerberg, Greg Kroah-Hartman,
	Kieran Bingham, Maximilian Luz, stable, linux-i2c, linux-acpi

On Wed, Oct 14, 2020 at 04:41:57PM +0200, Hans de Goede wrote:
>Wolfram, can we get this queued up to go to Linus
>soonish ?
>
>Greg, in essence this is a partial revert of the trouble
>some commit. With the 2 combined acpi_walk_dep_device_list()
>is called last again, while still doing the
>acpi_install_address_space_handler() call earlier.
>
>Since this is a somewhat nasty regression and since the poor
>timing wrt the merge-window, I was hoping that you would be
>willing to make an exception and pick up this fix before
>it hits Linus' tree?

I don't think the merge window plays any role here: this fix can go into
Linus's tree immediately and once it's there we can pick it up for
-stable right away.

-- 
Thanks,
Sasha

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 5.8+ regression fix] i2c: core: Restore acpi_walk_dep_device_list() getting called after registering the ACPI i2c devs
  2020-10-14 14:41 ` [PATCH 5.8+ regression fix] " Hans de Goede
@ 2020-10-25 12:21   ` Wolfram Sang
  0 siblings, 0 replies; 5+ messages in thread
From: Wolfram Sang @ 2020-10-25 12:21 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Mika Westerberg, Greg Kroah-Hartman, Kieran Bingham,
	Maximilian Luz, stable, linux-i2c, linux-acpi


[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]

On Wed, Oct 14, 2020 at 04:41:58PM +0200, Hans de Goede wrote:
> Commit 21653a4181ff ("i2c: core: Call i2c_acpi_install_space_handler()
> before i2c_acpi_register_devices()")'s intention was to only move the
> acpi_install_address_space_handler() call to the point before where
> the ACPI declared i2c-children of the adapter where instantiated by
> i2c_acpi_register_devices().
> 
> But i2c_acpi_install_space_handler() had a call to
> acpi_walk_dep_device_list() hidden (that is I missed it) at the end
> of it, so as an unwanted side-effect now acpi_walk_dep_device_list()
> was also being called before i2c_acpi_register_devices().
> 
> Move the acpi_walk_dep_device_list() call to the end of
> i2c_acpi_register_devices(), so that it is once again called *after*
> the i2c_client-s hanging of the adapter have been created.
> 
> This fixes the Microsoft Surface Go 2 hanging at boot.
> 
> Fixes: 21653a4181ff ("i2c: core: Call i2c_acpi_install_space_handler() before i2c_acpi_register_devices()")
> Suggested-by: Maximilian Luz <luzmaximilian@gmail.com>
> Reported-and-tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Applied to for-current, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-14 14:41 [PATCH 5.8+ regression fix 0/1] i2c: core: Restore acpi_walk_dep_device_list() getting called after registering the ACPI i2c devs Hans de Goede
2020-10-14 14:41 ` [PATCH 5.8+ regression fix] " Hans de Goede
2020-10-25 12:21   ` Wolfram Sang
2020-10-14 15:13 ` [PATCH 5.8+ regression fix 0/1] " Greg Kroah-Hartman
2020-10-15 15:17 ` Sasha Levin

Linux-i2c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i2c/0 linux-i2c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i2c linux-i2c/ https://lore.kernel.org/linux-i2c \
		linux-i2c@vger.kernel.org
	public-inbox-index linux-i2c

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-i2c


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git