Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20220128174625.321750-1-daniel.thompson@linaro.org>
In-Reply-To: <20220128174625.321750-1-daniel.thompson@linaro.org>
From:   Doug Anderson <dianders@chromium.org>
Date:   Fri, 28 Jan 2022 14:48:34 -0800
Message-ID: <CAD=FV=VpQoYkTiKVmC7hMX=bWVYW0rkHWMKU7CqyW_2tanPEpQ@mail.gmail.com>
Subject: Re: [PATCH] HID: i2c-hid: goodix: Fix a lockdep splat
To:     Daniel Thompson <daniel.thompson@linaro.org>
Cc:     Jiri Kosina <jikos@kernel.org>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        "open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Fri, Jan 28, 2022 at 9:48 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> I'm was on the receiving end of a lockdep splat from this driver and after
> scratching my head I couldn't be entirely sure it was a false positive
> given we would also have to think about whether the regulator locking is
> safe (since the notifier is called whilst holding regulator locks which
> are also needed for regulator_is_enabled() ).
>
> Regardless of whether it is a real bug or not, the mutex isn't needed.
> We can use reference counting tricks instead to avoid races with the
> notifier calls.
>
> The observed splat follows:
>
> ------------------------------------------------------
> kworker/u16:3/127 is trying to acquire lock:
> ffff00008021fb20 (&ihid_goodix->regulator_mutex){+.+.}-{4:4}, at: ihid_goodix_vdd_notify+0x30/0x94
>
> but task is already holding lock:
> ffff0000835c60c0 (&(&rdev->notifier)->rwsem){++++}-{4:4}, at: blocking_notifier_call_chain+0x30/0x70
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (&(&rdev->notifier)->rwsem){++++}-{4:4}:
>        down_write+0x68/0x8c
>        blocking_notifier_chain_register+0x54/0x70
>        regulator_register_notifier+0x1c/0x24
>        devm_regulator_register_notifier+0x58/0x98
>        i2c_hid_of_goodix_probe+0xdc/0x158
>        i2c_device_probe+0x25d/0x270
>        really_probe+0x174/0x2cc
>        __driver_probe_device+0xc0/0xd8
>        driver_probe_device+0x50/0xe4
>        __device_attach_driver+0xa8/0xc0
>        bus_for_each_drv+0x9c/0xc0
>        __device_attach_async_helper+0x6c/0xbc
>        async_run_entry_fn+0x38/0x100
>        process_one_work+0x294/0x438
>        worker_thread+0x180/0x258
>        kthread+0x120/0x130
>        ret_from_fork+0x10/0x20
>
> -> #0 (&ihid_goodix->regulator_mutex){+.+.}-{4:4}:
>        __lock_acquire+0xd24/0xfe8
>        lock_acquire+0x288/0x2f4
>        __mutex_lock+0xa0/0x338
>        mutex_lock_nested+0x3c/0x5c
>        ihid_goodix_vdd_notify+0x30/0x94
>        notifier_call_chain+0x6c/0x8c
>        blocking_notifier_call_chain+0x48/0x70
>        _notifier_call_chain.isra.0+0x18/0x20
>        _regulator_enable+0xc0/0x178
>        regulator_enable+0x40/0x7c
>        goodix_i2c_hid_power_up+0x18/0x20
>        i2c_hid_core_power_up.isra.0+0x1c/0x2c
>        i2c_hid_core_probe+0xd8/0x3d4
>        i2c_hid_of_goodix_probe+0x14c/0x158
>        i2c_device_probe+0x25c/0x270
>        really_probe+0x174/0x2cc
>        __driver_probe_device+0xc0/0xd8
>        driver_probe_device+0x50/0xe4
>        __device_attach_driver+0xa8/0xc0
>        bus_for_each_drv+0x9c/0xc0
>        __device_attach_async_helper+0x6c/0xbc
>        async_run_entry_fn+0x38/0x100
>        process_one_work+0x294/0x438
>        worker_thread+0x180/0x258
>        kthread+0x120/0x130
>        ret_from_fork+0x10/0x20
>
> other info that might help us debug this:
>
>  Possible unsafe locking scenario:
>
>        CPU0                    CPU1
>        ----                    ----
>   lock(&(&rdev->notifier)->rwsem);
>                                lock(&ihid_goodix->regulator_mutex);
>                                lock(&(&rdev->notifier)->rwsem);
>   lock(&ihid_goodix->regulator_mutex);
>
>  *** DEADLOCK ***
>
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---
>  drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 28 +++++++++++--------------
>  1 file changed, 12 insertions(+), 16 deletions(-)

Yes, this seems like a reasonable solution, thanks! Probably want:

Fixes: 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to true
state of the regulator")

Reviewed-by: Douglas Anderson <dianders@chromium.org>