linux-kernel - Re: [PATCH 2/2] i2c: designware: Add AMD PSP I2C bus support

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: <ca28b3eb-c06d-f8e3-c930-95da631e9a93@redhat.com>
Date:   Mon, 24 Jan 2022 15:27:45 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     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,
        mario.limonciello@....com
Subject: Re: [PATCH 2/2] i2c: designware: Add AMD PSP I2C bus support

Hi,

On 1/24/22 13:35, Jan Dąbroś wrote:
> pt., 21 sty 2022 o 11:32 Hans de Goede <hdegoede@...hat.com> napisał(a):
>>
>> Hi Jan,
>>
>> On 1/21/22 10:59, Jan Dąbroś wrote:

<snip>

>>>> 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://github.com/torvalds/linux/blob/master/drivers/char/tpm/tpm_tis_i2c_cr50.c#L202.
>>>
>>> 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.
> 
> Correct. In other words, if i2c-client wants to block the bus/adapter
> for more than one transaction it must use some special methods. This
> isn't changed with my patchset. If one is using "normal"
> i2c_transfer(), we should be on the safe side, nothing will change
> from the i2c-client point of view. The same if one is using
> __i2c_transfer().
> 
>>
>> 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.
> 
> Right, however this behavior is not altered by my patch. I just wanted
> to ensure that drivers which are already using i2c bus_locking will
> still work as expected.

Ah I see, I thought that maybe you wanted to add extra i2c bus-locking
calls to some drivers which don't have them yet to ensure that the
AMD PSP semaphore would be hold over multiple transfers in
drivers which don't do this yet.

>> 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.
> 
> My goal is to not modify current behavior, that is - we don't need to
> modify clients' drivers and extra quirks applied by amdpsp semaphore
> will be "transparent" for them. IOW, switch from generic
> i2c-designware to i2c-designware-amdpsp should be invisible from the
> i2c bus perspective for i2c-clients.

Ok, I believe everything is fine as is then. My worries where about
extending the i2c bus-locking to more drivers, but if there are no
plans for that then everything is good.

Regards,

Hans