linux-kernel - Re: [PATCH 0/2] i2c-designware: Add support for AMD PSP semaphore

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: <CAOtMz3MDntus2OzRS_QDfnFpzrVjj4swQjTkgkFYMGuwgbZcWA@mail.gmail.com>
Date:   Thu, 20 Jan 2022 13:29:46 +0100
From:   Jan Dąbroś <jsd@...ihalf.com>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-i2c <linux-i2c@...r.kernel.org>,
        Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Wolfram Sang <wsa@...nel.org>,
        Raul E Rangel <rrangel@...omium.org>,
        Marcin Wojtas <mw@...ihalf.com>,
        Grzegorz Jaszczyk <jaz@...ihalf.com>, upstream@...ihalf.com
Subject: Re: [PATCH 0/2] i2c-designware: Add support for AMD PSP semaphore

Hi Hans,


czw., 20 sty 2022 o 12:15 Hans de Goede <hdegoede@...hat.com> napisał(a):
>
> Hi Jan,
>
> On 1/20/22 01:16, Jan Dabros wrote:
> > This patchset comprises support for new i2c-designware controller setup on some
> > AMD Cezanne SoCs, where x86 is sharing i2c bus with PSP. PSP uses the same
> > controller and acts as an i2c arbitrator there (x86 is leasing bus from it).
> >
> > First commit aims to improve generic i2c-designware code by adding extra locking
> > on probe() and disable() paths. I would like to ask someone with access to
> > boards which use Intel BayTrail(CONFIG_I2C_DESIGNWARE_BAYTRAIL) to verify
> > behavior of my changes on such setup.
> >
> > Second commit adds support for new PSP semaphore arbitration mechanism.
> > Implementation is similar to the one from i2c-designware-baytrail.c however
> > there are two main differences:
> > 1) Add new ACPI ID in order to protect against silent binding of the old driver
> > to the setup with PSP semaphore. Extra flag ARBITRATION_SEMAPHORE added to this
> > new _HID allows to recognize setup with PSP.
> > 2) Beside acquire_lock() and release_lock() methods we are also applying quirks
> > to the lock_bus() and unlock_bus() global adapter methods. With this in place
> > all i2c clients drivers may lock i2c bus for a desired number of i2c
> > transactions (e.g. write-wait-read) without being aware of that such bus is
> > shared with another entity.
> >
> > This patchset is a follow-up to the RFC sent earlier on LKML [1], with review
> > comments applied.
> >
> > Looking forward to some feedback.
> >
> > [1] https://lkml.org/lkml/2021/12/22/219
>
>
> Thank you for your patch series.
>
> As you may have seen I've done a lot of work on the Bay Trail semaphore
> thing. I also own several Bay Trail and Cherry Trail based devices which
> use this setup.
>
> I'll add your patches to my personal WIP tree which I regularly run
> on these devices and I'll report back if I notice any issues.

Thanks in advance, this will be really helpful! I don't have Bay
Trail/Cherry Trail, so I've only tested that build of Bay Trail
semaphore isn't broken.

I would like to point to new locks in i2c_dw_disable() method as
something to be the most fragile and error-prone, will be great if you
can verify this thoroughly. This function is invoked on both
dw_i2c_driver.remove() and dw_i2c_plat_suspend() paths. Considering
that Bay Trail semaphore means that i2c bus is shared with PMIC, I'm
not sure whether all corner cases are secured especially on platform
suspend.

>
> One remark, I notice that there are no AMD people in the Cc, it
> would be good if you can find someone from AMD to look at this,
> also see my remarks to the 2nd patch in my reply to that patch.

This was partially discussed with AMD folks and you are right that I
should include someone from AMD to take a look at this. Thanks for all
your comments!

> Regards,
>
> Hans
>
>
>
>
> >
> > Jan Dabros (2):
> >   i2c: designware: Add missing locks
> >   i2c: designware: Add AMD PSP I2C bus support
> >
> >  MAINTAINERS                                  |   1 +
> >  drivers/acpi/acpi_apd.c                      |   1 +
> >  drivers/i2c/busses/Kconfig                   |  10 +
> >  drivers/i2c/busses/Makefile                  |   1 +
> >  drivers/i2c/busses/i2c-designware-amdpsp.c   | 357 +++++++++++++++++++
> >  drivers/i2c/busses/i2c-designware-baytrail.c |  10 +-
> >  drivers/i2c/busses/i2c-designware-common.c   |  12 +
> >  drivers/i2c/busses/i2c-designware-core.h     |  18 +-
> >  drivers/i2c/busses/i2c-designware-master.c   |   6 +
> >  drivers/i2c/busses/i2c-designware-platdrv.c  |  61 ++++
> >  10 files changed, 469 insertions(+), 8 deletions(-)
> >  create mode 100644 drivers/i2c/busses/i2c-designware-amdpsp.c
> >
>