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