Date:   Fri, 21 Jan 2022 13:18:35 -0600
From:   Tom Lendacky <thomas.lendacky@....com>
To:     "Limonciello, Mario" <Mario.Limonciello@....com>,
        Hans de Goede <hdegoede@...hat.com>,
        Jan Dąbroś <jsd@...ihalf.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" <upstream@...ihalf.com>,
        "Deucher, Alexander" <Alexander.Deucher@....com>
Subject: Re: [PATCH 2/2] i2c: designware: Add AMD PSP I2C bus support

On 1/21/22 11:38 AM, Limonciello, Mario wrote:
> [Public]
> 
> +Thomas (ccp driver)
> +Alex (amdgpu driver)
> 
>>
>> On 1/21/22 10:59, Jan Dąbroś wrote:

>>>>
>>>> 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.

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
>> 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