Message-ID: <CAOtMz3P+sMBd2yttU_KPbRO0fa-WLewt+5iMMGwR_6Wp0heVtA@mail.gmail.com>
Date:   Fri, 28 Jan 2022 15:47:30 +0100
From:   Jan Dąbroś <jsd@...ihalf.com>
To:     "Limonciello, Mario" <Mario.Limonciello@....com>
Cc:     "Lendacky, Thomas" <Thomas.Lendacky@....com>,
        Hans de Goede <hdegoede@...hat.com>,
        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" <upstream@...ihalf.com>,
        "Deucher, Alexander" <Alexander.Deucher@....com>,
        "Easow, Nimesh" <Nimesh.Easow@....com>
Subject: Re: [PATCH 2/2] i2c: designware: Add AMD PSP I2C bus support

śr., 26 sty 2022 o 20:36 Limonciello, Mario
<Mario.Limonciello@....com> napisał(a):
>
> [Public]
>
> > >
> > > >>>>
> > > >>>> Through here seems to all be generic code for accessing
> > > >>>> the AMD PSP. To me this seems like something which belongs
> > > >>>> in a separate AMD-PSP-mbox driver/lib, which can then be
> > > >>>> shared between other kernel drivers which may also want
> > > >>>> to access PSP.
> > > >>>
> > > >>> I see your point clearly and actually it is not an accident that I've
> > > >>> put all PSP-mailbox methods in one "block". They are logically
> > > >>> different than the rest of i2c-adapter specific methods.
> > > >>>
> > > >>> That being said, above PSP mailbox was created by AMD solely for the
> > > >>> purpose of i2c_arbitration. It has its own set of commands and
> > > >>> specific format of the command-response buffer. Thus it is not and it
> > > >>> won't be generic in the future. There are already upstreamed drivers
> > > >>> from AMD (under drivers/crypto/ccp/) which made use of PSP, however
> > > >>> their channel of communication looks completely different than the
> > > >>> very simple i2c_arbitration model implemented above.
> > > >>>
> > > >
> > > > Can you please double confirm no other users for this mailbox and it's for
> > > > you only?  And if so is it only specific to this platform that no other users?
> > > > At least on some UEFI AMD platforms that mailbox is defined for
> > > > communication with something else.  We might need some way to attest
> > > > from the firmware that it's safe to use.
> > > >
> > > > The only mailbox that is defined for OS use across different silicon AFAIK is
> > > > the GPU driver mailbox.  It may be safer to use that, but I'm not sure if
> > > > GPU driver has come up early enough for your use.
> > >
> > > The CCP/PSP driver will load as a PCIe device driver on this platform and
> > > will ioremap the MMIO space. Today, that driver doesn't reference those
> > > mailbox registers, and I don't know that there will be a need in the
> > > future. But if there is a need in the future, you could end up with a
> > > conflict between these two drivers.
> >
> > Right, so I have confirmed this with AMD PSP firmware developers, that
> > this particular x86-PSP mailbox is created and will be reserved
> > _solely_ for the purpose of i2c arbitration (and thus this driver).
> > There is no intend to use it elsewhere or share with another users.
>
> I've learned never to say never.  People move on to different roles and history
> gets lost.  As it's exclusive to this use case of I2C arbitration it will probably still
> be useful to leave in the comments nearby for our future selves what model/family
> SOC this was introduced in case the number of mailboxes are extinguished some
> day and this ends up being needed for a secondary purpose for some future SOC.

OK. I put a comment in v2. Please take a look.

>
> >
> > > Thanks,
> > > Tom
> > >
> > > >
> > > >>> Because of this I'm treating this as an i2c_semaphore-related code and
> > > >>> putting this in this module. In my opinion moving this into some
> > > >>> separate driver (which will be actually used only here) makes code
> > > >>> less clear. But let's also hear some voice from AMD.
> > > >>
> > > >> Since as you say this mailbox is special and only for i2c-arbitration,
> > > >> keeping it inside this patch / .c file is fine.
> > > >>
> > > >>>
> > > >>>>
> > > >>>> Sorta like the generic iosf_mbi_read() and
> > > >>>> iosf_mbi_write() functions from:
> > > >>>>
> > > >>>> arch/x86/platform/intel/iosf_mbi.c
> > > >>>>
> > > >>>> used on the Intel chips, which are also used outside of
> > > >>>> the I2C bus-locking code.
> > > >>>>
> > > >>>> This is also one of the reasons why I think it would be
> > > >>>> good to get some AMD folks involved in this, since they
> > > >>>> may be aware of other drivers which also need to access
> > > >>>> the PSP mbox.
> > > >>>>
> > > >>>
> > > >>> Right, I'm adding mario.limonciello@....com to the CC, so that he can
> > > >> comment.
> > > >>>
> > > >>> (...)
> > > >>>
> > > >>>>> +/*
> > > >>>>> + * Locking methods are based on the default implementation from
> > > >>>>> + * drivers/i2c/i2c-core.base.c, but with psp acquire and release
> > operations
> > > >>>>> + * added. With this in place we can ensure that i2c clients on the bus
> > shared
> > > >>>>> + * with psp are able to lock HW access to the bus for arbitrary number
> > of
> > > >>>>> + * operations - that is e.g. write-wait-read.
> > > >>>>> + */
> > > >>>>> +static void i2c_adapter_dw_psp_lock_bus(struct i2c_adapter *adapter,
> > > >>>>> +                                     unsigned int flags)
> > > >>>>> +{
> > > >>>>> +     psp_acquire_i2c_bus();
> > > >>>>> +     rt_mutex_lock_nested(&adapter->bus_lock,
> > > >> i2c_adapter_depth(adapter));
> > > >>>>
> > > >>>> This does not do what you think it does and you will still deadlock
> > > >>>> when things nest because of someone taking the bus-lock and then
> > > >>>> the main i2c-designware transfer function calling the acquire_lock
> > > >>>> callback.
> > > >>>
> > > >>> I haven't used rt_mutex_lock_nested() with the intent to prevent me
> > > >>> from deadlock when i2c-designware calls acquire_lock with bus-lock
> > > >>> already taken. This is a method copied from
> > > >>> drivers/i2c/i2c-core-base.c (BTW, I have a typo in above comment).
> > > >>> This is the default implementation applied by i2c-core when particular
> > > >>> adapter doesn't register its own locking callbacks - thus it is called
> > > >>> for i2c-designware for all platforms.
> > > >>>
> > > >>> In case of this driver internal i2c-designware acquire_lock() is equal
> > > >>> to psp_acquire_i2c_bus(). In other words, bus-level lock
> > > >>> i2c_adapter_dw_psp_lock_bus() is a superset of internal adapter's
> > > >>> acquire_lock().
> > > >>
> > > >> Ah I missed that this is just mimicking the core functions +
> > > >> an extra call to psp_acquire_i2c_bus().
> > > >>
> > > >> I assumed that the dwc->acquire callback path was also taking
> > > >> the mutex and I thought you had fallen for the _nested meaning
> > > >> something different then it does, my bad.
> > > >>
> > > >>> In order to prevent deadlock which you are talking about, I'm using
> > > >>> reference lock counter inside psp_acquire_i2c_bus() thus it is safe to
> > > >>> invoke acquire_lock() when bus-lock is already taken.
> > > >>
> > > >> Ah good, that is pretty much is the same as what the Bay Trail code
> > > >> is doing.
> > > >>
> > > >>>
> > > >>>>
> > > >>>> The _nested postfix is only for the lockdep lock-debugger, this
> > > >>>> actually turns into a regular mutex_lock when lockdep is not enabled:
> > > >>>>
> > > >>>> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> > > >>>> extern void rt_mutex_lock_nested(struct rt_mutex *lock, unsigned int
> > > >> subclass);
> > > >>>> #define rt_mutex_lock(lock) rt_mutex_lock_nested(lock, 0)
> > > >>>> #else
> > > >>>> extern void rt_mutex_lock(struct rt_mutex *lock);
> > > >>>> #define rt_mutex_lock_nested(lock, subclass) rt_mutex_lock(lock)
> > > >>>> #endif
> > > >>>>
> > > >>>> The _nested postfix as such is only to tell the lockdep code that
> > > >>>> even though it seems we are trying to take the same mutex twice
> > > >>>> since in both cases it is of i2c_adapter.rt_mutex "lock class"
> > > >>>> that we are sure it is never the same i2c_adapter (but rather
> > > >>>> one which always gets called in a nested fashion from another
> > > >>>> i2c_adapter).
> > > >>>>
> > > >>>> IOW this only disables a false-positive lockdep warning, it does
> > > >>>> not allow taking the same mutex twice, you will still hang on
> > > >>>> the second mutex_lock call on the same lock.
> > > >>>
> > > >>> Thanks for the technical background about rt_mutex_lock_nested. I
> > > >>> think we should keep using it as is, since as I wrote above I don't
> > > >>> have any reasoning to modify it here.
> > > >>
> > > >> Ack, now that my misreading of the code has been cleared up
> > > >> I agree.
> > > >>
> > > >>>> Also I don't think you are allowed to use the bus_locking code
> > > >>>> like this. The i2c bus-locking code is intended to deal with
> > > >>>> busses which have muxes in them, where the mux must be set
> > > >>>> to the right branch of the bus to reach the client and then
> > > >>>> not be changed during the transfer to that client.
> > > >>>>
> > > >>>> So i2c-client drivers are never supposed to directly call
> > > >>>> the bus-locking functions.
> > > >>>
> > > >>> I think you are not correct here. There are examples of i2c-clients
> > > >>> which are using i2c bus_locking for the purpose of locking adapter for
> > > >>> the bunch of i2c transactions.
> > > >>>
> > > >>> As an example let's take drivers/char/tpm/tpm_tis_i2c_cr50.c. It
> > > >>> operates in write-wait-read model and there is i2c_lock_bus() call
> > > >>> used to ensure that bus won't be released -
> > > >>>
> > > >>
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.c
> > %2F&amp;data=04%7C01%7CMario.Limonciello%40amd.com%7Cf8cd037a72b
> > 749fb003408d9df3f5ced%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0
> > %7C637786285501955886%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> > MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata
> > =s4N9vTWbuRUww0qvsRz16fV%2Fnj41SxyLOB%2BjGFjyOaA%3D&amp;reserved
> > =0
> > > >>
> > om%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fchar%2Ftpm%2Ftpm_
> > > >>
> > tis_i2c_cr50.c%23L202&amp;data=04%7C01%7Cmario.limonciello%40amd.com
> > > >>
> > %7C1bdc742bc2a24f59b7d908d9dcc95438%7C3dd8961fe4884e608e11a82d994
> > > >>
> > e183d%7C0%7C0%7C637783579554955840%7CUnknown%7CTWFpbGZsb3d8ey
> > > >>
> > JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> > > >>
> > 3000&amp;sdata=fr0UEka%2BxYyPxqUG6oOZo%2Bc6pWgto2mD7hWA20YulVQ
> > > >> %3D&amp;reserved=0.
> > > >>>
> > > >>> Similar model is followed in drivers/char/tpm/tpm_i2c_infineon.c and
> > > >>> couple of other i2c-client drivers.
> > > >>
> > > >> Ah I see, interesting (live and learn).
> > > >>
> > > >> But this is then combined with using the special __i2c_transfer()
> > > >> function for the actual i2c reads/writes, since using the regular
> > > >> i2c_transfer() function after already taking the lock would deadlock.
> > > >>
> > > >> There is a similar unlocked raw __i2c_smbus_xfer(), but as the
> > > >> comment in include/linux/i2c.h above the locked i2c_smbus_xfer() says:
> > > >>
> > > >> /* This is the very generalized SMBus access routine. You probably do not
> > > >>     want to use this, though; one of the functions below may be much easier,
> > > >>     and probably just as fast.
> > > >>     Note that we use i2c_adapter here, because you do not need a specific
> > > >>     smbus adapter to call this function. */
> > > >> s32 i2c_smbus_xfer(...);
> > > >>
> > > >> So in this case a driver cannot use the usual
> > > >> i2c_smbus_read_byte/word/byte_data/word_data() helpers and
> > > >> the same for writes. Also using an i2c_regmap (which is used
> > > >> in a ton of places like PMIC drivers) will not work this way.
> > > >>
> > > >> So yes you can use i2c_bus_lock() for this; but only if all the
> > > >> drivers where you want to do that limit themselves to
> > > >> __i2c_transfer() and __i2c_smbus_xfer() calls and/or are
> > > >> rewritten to only use those.
> > > >>>> This is why in the Bay Trail case we have i2c-drivers
> > > >>>> directly calling iosf_mbi_block_punit_i2c_access() and
> > > >>>> iosf_mbi_unblock_punit_i2c_access() to lock the bus
> > > >>>> for multiple i2c-transfers. We can get away with this there
> > > >>>> because the bus in question is only used to access the
> > > >>>> PMIC and that PMIC is only used on Bay Trail (and CHT)
> > > >>>> boards, so the PMIC drivers can just hard-code these
> > > >>>> calls.
> > > >>>>
> > > >>>> If you need to take the PSP I2C semaphore for multiple
> > > >>>> transfers in some generic drivers, then I guess that the
> > > >>>> i2c-subsys will need to get some new i2c_adapter callbacks
> > > >>>> to acquire / release the bus for i2c-controllers where
> > > >>>> the bus/controller is shared with some co-processor like
> > > >>>> in the PSP case.
> > > >>>
> > > >>> This is exactly my intention to support generic i2c-clients drivers
> > > >>> without them being aware that i2c-adapter above is using some
> > > >>> semaphore/arbitration. Hopefully you can agree with me that currently
> > > >>> available bus_locking can be used and is enough for this purpose.
> > > >>
> > > >> It can be used, but with limitations, see above.
> > > >>
> > > >>>
> > > >>>> Also note that iosf_mbi_block_punit_i2c_access() and
> > > >>>> iosf_mbi_unblock_punit_i2c_access() do their own
> > > >>>> ref/lock-counting to allow calling them multiple times and
> > > >>>> the first block call takes the bus and the last unblock
> > > >>>> call releases it.
> > > >>>
> > > >>> This is exactly what I was talking about above and also implemented
> > > >>> within psp_acquire_i2c_bus() and psp_release_i2c_bus().
> > > >>
> > > >> Right, I was to quick in skimming over your code when
> > > >> I wrote down my concerns about there being a deadlock
> > > >> there, sorry.
> > > >>
> > > >> Regards,
> > > >>
> > > >> Hans

Powered by blists - more mailing lists