linux-i2c.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: Wolfram Sang <wsa@the-dreams.de>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Hans de Goede <hdegoede@redhat.com>,
	Kieran Bingham <kieran.bingham@ideasonboard.com>,
	Maximilian Luz <luzmaximilian@gmail.com>,
	stable@vger.kernel.org, linux-i2c@vger.kernel.org,
	linux-acpi@vger.kernel.org
Subject: [PATCH 5.8+ regression fix 0/1] i2c: core: Restore acpi_walk_dep_device_list() getting called after registering the ACPI i2c devs
Date: Wed, 14 Oct 2020 16:41:57 +0200	[thread overview]
Message-ID: <20201014144158.18036-1-hdegoede@redhat.com> (raw)

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


             reply	other threads:[~2020-10-14 14:42 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-14 14:41 Hans de Goede [this message]
2020-10-14 14:41 ` [PATCH 5.8+ regression fix] i2c: core: Restore acpi_walk_dep_device_list() getting called after registering the ACPI i2c devs 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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201014144158.18036-1-hdegoede@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=kieran.bingham@ideasonboard.com \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-i2c@vger.kernel.org \
    --cc=luzmaximilian@gmail.com \
    --cc=mika.westerberg@linux.intel.com \
    --cc=stable@vger.kernel.org \
    --cc=wsa@the-dreams.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).