linux-kernel - Re: [PATCH] platform/chromeos: cros

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGXv+5Gnc6tCjO95VjBJ4F5p5bnS+M96at3ErpSX==3qPwNUsg@mail.gmail.com>
Date:   Sat, 7 Jan 2023 13:43:57 +0800
From:   Chen-Yu Tsai <wenst@...omium.org>
To:     Tzung-Bi Shih <tzungbi@...nel.org>
Cc:     Benson Leung <bleung@...omium.org>,
        Guenter Roeck <groeck@...omium.org>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        chrome-platform@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] platform/chromeos: cros_ec: Use per-device lockdep key

On Fri, Jan 6, 2023 at 5:08 PM Tzung-Bi Shih <tzungbi@...nel.org> wrote:
>
> On Fri, Jan 06, 2023 at 12:55:37PM +0800, Chen-Yu Tsai wrote:
> > Lockdep reports a bogus possible deadlock on MT8192 Chromebooks due to
> > the following lock sequences:
> >
> > 1. lock(i2c_register_adapter) [1]; lock(&ec_dev->lock)
> > 2. lock(&ec_dev->lock); lock(prepare_lock);
> >
> > The actual dependency chains are much longer. The shortened version
> > looks somewhat like:
> >
> > 1. cros-ec-rpmsg on mtk-scp
> >    ec_dev->lock -> prepare_lock
> > 2. In rt5682_i2c_probe() on native I2C bus:
> >    prepare_lock -> regmap->lock -> (possibly) i2c_adapter->bus_lock
> > 3. In rt5682_i2c_probe() on native I2C bus:
> >    regmap->lock -> i2c_adapter->bus_lock
> > 4. In sbs_probe() on cros-ec-i2c (passthrough) I2C bus on cros-ec
> >    i2c_adapter->bus_lock -> ec_dev->lock
> >
> > While lockdep is correct that the shared lockdep classes have a circular
> > dependency, it is bogus because
> >
> >   a) 2+3 happen on a native I2C bus
> >   b) 4 happens on the actual EC on ChromeOS devices
> >   c) 1 happens on the SCP coprocessor on MediaTek Chromebooks that just
> >      happen to expose a cros-ec interface, but do not have a passthrough
> >      I2C bus
> >
> > In short, the "dependencies" are actually on different devices.
>
> Path of 4 looks weird to me.
>
> Could you point out where sbs_probe() gets to acquire ec_dev->lock?

sbs_probe() calls sbs_get_battery_presence_and_health(), which

  -> does an I2C transfer. This SBS instance is connected on the I2C bus
     on the EC, so the I2C transfer

     -> acquires i2c_adapter->bus_lock, and

       -> calls ec_i2c_xfer(), which

          -> calls cros_ec_cmd_xfer_status(), which

             -> calls cros_ec_cmd_xfer(), which

               -> acquires ec_dev->lock


> I may misunderstand: I thought there is no such I2C bus for passthrough
> from kernel's point of view (as the bus and devices behind the EC).
> See also [2].

It is an I2C adapter on the EC, also known as i2c-cros-ec-tunnel.
Passthrough probably isn't the right word.

> [2]: https://elixir.bootlin.com/linux/v6.2-rc2/source/drivers/platform/chrome/cros_ec.c#L241
>
>
> On a related note, for the commit title: s/chromeos/chrome/ if it gets
> chance to have next version.

OK.


Thanks
ChenYu