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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 1/24/22 13:35, Jan Dąbroś wrote:
> pt., 21 sty 2022 o 11:32 Hans de Goede <hdegoede@redhat.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