| Message ID | 20180620085157.30121-2-peda@axentia.se |
|---|---|
| State | Accepted |
| Headers | show |
| Series | i2c: smbus: add unlocked __i2c_smbus_xfer variant | expand |
> This is not perfectly equivalent, since i2c_smbus_xfer was callable from > atomic/irq context if you happened to end up emulating SMBus with an I2C > transfer, and that is no longer the case with this patch. It is unknown > (to me) if anything depends on that quirk, but it seems fragile enough to > simply break those cases and require them to call i2c_transfer directly > instead. Couldn't we just add the same trylock-code path here as well? I always wondered why I2C and SMBus were not in sync when it came to that. Yet, I didn't want to change the code for no reason, but it seems we now have one? Rest of the series looks good to me, very nice diffstat!
On June 26, 2018 4:37:58 AM GMT+02:00, Wolfram Sang <wsa@the-dreams.de> wrote: > >> This is not perfectly equivalent, since i2c_smbus_xfer was callable >from >> atomic/irq context if you happened to end up emulating SMBus with an >I2C >> transfer, and that is no longer the case with this patch. It is >unknown >> (to me) if anything depends on that quirk, but it seems fragile >enough to >> simply break those cases and require them to call i2c_transfer >directly >> instead. > >Couldn't we just add the same trylock-code path here as well? I always >wondered why I2C and SMBus were not in sync when it came to that. Yet, >I >didn't want to change the code for no reason, but it seems we now have >one? > >Rest of the series looks good to me, very nice diffstat! I don't think it's that easy as I just thought about another problem with lifting the locking from the emulation function. It calls kzalloc(..., GFP_KERNEL), at least in some cases, and that's not a very good idea from atomic/irq context... Cheers, Peter
> I don't think it's that easy as I just thought about another problem > with lifting the locking from the emulation function. It calls > kzalloc(..., GFP_KERNEL), at least in some cases, and that's not a > very good idea from atomic/irq context... Right. However, we could simply bail out early when we are in atomic context. Simply no DMA then...
On June 26, 2018 3:46:07 PM GMT+02:00, Wolfram Sang <wsa@the-dreams.de> wrote: >> I don't think it's that easy as I just thought about another problem >> with lifting the locking from the emulation function. It calls >> kzalloc(..., GFP_KERNEL), at least in some cases, and that's not a >> very good idea from atomic/irq context... > >Right. However, we could simply bail out early when we are in atomic >context. Simply no DMA then... Yeah, we could bail out early, and perhaps we should. But we risk regressions, see below... And that should probably be fixed before we add the unlocked __i2c_smbus_xfer function. Because, thinking more about it, the problem with those allocs are not related to the locking details; adding another trylock to the mix just makes it so much more obvious. I mean, first we would specifically handle atomic/irq context with a trylock "documenting" that atomic/irq users are welcome to at least try xfers, and then we blattantly break the rulez with a GFP_KERNEL alloc... Also, the fact that the buffer is DMA-friendly does not mean that DMA is actually going to be used, so the patch that introduced those allocs might have regressed for this case: - SMBus user from atomic/irq context - SMBus emulated - emulation triggering a GFP_KERNEL alloc - the existing trylock in i2c_transfer succeeding - driver not ending up doing DMA Bailing out would break these users, always. Currently, I assume they are only broken when the alloc happens to need to do more than is allowed from the given context. Something which might or might not be common? But as usual, I might be missing something... Cheers, Peter
> Because, thinking more about it, the problem with those allocs are not > related to the locking details; adding another trylock to the mix just > makes it so much more obvious. I mean, first we would specifically > handle atomic/irq context with a trylock "documenting" that atomic/irq > users are welcome to at least try xfers, and then we blattantly break > the rulez with a GFP_KERNEL alloc... Yes, thinking more about it, I came to the conclusion that we should not add trylock to SMBus and keep the requirement to allow sleeping. True, SMBus is not consistent with I2C then, but actually, I'd prefer the consistency the other way around: I wish we had a clear statement that i2c_transfer may sleep. And have a dedicated irqless, non-sleeping callback for handling the atomic case instead. I really don't like the commit which introduced the trylock into i2c_transfer[1]. Its commit message even says: "It is the reponsability of the caller to ensure that the underlying i2c bus driver will not sleep either." Which seems broken to me because I can't see how the caller should do that? And most bus drivers will sleep. But that commit is upstream for 10 years now, so there are probably users. Which also are very hard to spot, I am afraid. I wouldn't see a way to convert them off the top of my head. [1] cea443a81c9c ("i2c: Support i2c_transfer in atomic contexts") > Currently, I assume they are only broken when the alloc happens to > need to do more than is allowed from the given context. Something > which might or might not be common? The only regression now would be using smbus_emulated from atomic context. Which is a bug on the caller side because it cannot know if smbus_emulated will be used or not. For the non-emulated case, it must not be atomic anyhow. So, unless I overlooked something, if we decide to not add trylock to smbus_xfer, we are all fine? And I think we should really keep this clean rule of smbus functions being non-atomic. D'accord?
On Wed, Jun 27, 2018 at 09:08:18AM +0200, Wolfram Sang wrote: > > > Because, thinking more about it, the problem with those allocs are not > > related to the locking details; adding another trylock to the mix just > > makes it so much more obvious. I mean, first we would specifically > > handle atomic/irq context with a trylock "documenting" that atomic/irq > > users are welcome to at least try xfers, and then we blattantly break > > the rulez with a GFP_KERNEL alloc... > > Yes, thinking more about it, I came to the conclusion that we should not > add trylock to SMBus and keep the requirement to allow sleeping. > > True, SMBus is not consistent with I2C then, but actually, I'd prefer > the consistency the other way around: I wish we had a clear statement > that i2c_transfer may sleep. And have a dedicated irqless, non-sleeping > callback for handling the atomic case instead. > > I really don't like the commit which introduced the trylock > into i2c_transfer[1]. Its commit message even says: "It is the > reponsability of the caller to ensure that the underlying i2c bus driver > will not sleep either." Which seems broken to me because I can't see how > the caller should do that? And most bus drivers will sleep. But that > commit is upstream for 10 years now, so there are probably users. Which > also are very hard to spot, I am afraid. I wouldn't see a way to convert > them off the top of my head. > > [1] cea443a81c9c ("i2c: Support i2c_transfer in atomic contexts") > > > Currently, I assume they are only broken when the alloc happens to > > need to do more than is allowed from the given context. Something > > which might or might not be common? > > The only regression now would be using smbus_emulated from atomic > context. Which is a bug on the caller side because it cannot know if > smbus_emulated will be used or not. For the non-emulated case, it must > not be atomic anyhow. > > So, unless I overlooked something, if we decide to not add trylock to > smbus_xfer, we are all fine? > > And I think we should really keep this clean rule of smbus functions > being non-atomic. > > D'accord? So, if no other arguments drop in, I'll apply this series as is next week.
On July 1, 2018 2:13:20 PM GMT+02:00, Wolfram Sang <wsa@the-dreams.de> wrote: >On Wed, Jun 27, 2018 at 09:08:18AM +0200, Wolfram Sang wrote: >> >> > Because, thinking more about it, the problem with those allocs are >not >> > related to the locking details; adding another trylock to the mix >just >> > makes it so much more obvious. I mean, first we would specifically >> > handle atomic/irq context with a trylock "documenting" that >atomic/irq >> > users are welcome to at least try xfers, and then we blattantly >break >> > the rulez with a GFP_KERNEL alloc... >> >> Yes, thinking more about it, I came to the conclusion that we should >not >> add trylock to SMBus and keep the requirement to allow sleeping. >> >> True, SMBus is not consistent with I2C then, but actually, I'd prefer >> the consistency the other way around: I wish we had a clear statement >> that i2c_transfer may sleep. And have a dedicated irqless, >non-sleeping >> callback for handling the atomic case instead. >> >> I really don't like the commit which introduced the trylock >> into i2c_transfer[1]. Its commit message even says: "It is the >> reponsability of the caller to ensure that the underlying i2c bus >driver >> will not sleep either." Which seems broken to me because I can't see >how >> the caller should do that? And most bus drivers will sleep. But that >> commit is upstream for 10 years now, so there are probably users. >Which >> also are very hard to spot, I am afraid. I wouldn't see a way to >convert >> them off the top of my head. >> >> [1] cea443a81c9c ("i2c: Support i2c_transfer in atomic contexts") >> >> > Currently, I assume they are only broken when the alloc happens to >> > need to do more than is allowed from the given context. Something >> > which might or might not be common? >> >> The only regression now would be using smbus_emulated from atomic >> context. Which is a bug on the caller side because it cannot know if >> smbus_emulated will be used or not. For the non-emulated case, it >must >> not be atomic anyhow. >> >> So, unless I overlooked something, if we decide to not add trylock to >> smbus_xfer, we are all fine? >> >> And I think we should really keep this clean rule of smbus functions >> being non-atomic. >> >> D'accord? > >So, if no other arguments drop in, I'll apply this series as is next >week. Right, I had the below response sitting in my drafts folder. I thought I had sent it, but apparently I didn't... Well, IF there are SMBus users that in fact do rely on the emulation allowing calls from atomic/irq context then it will be a regression even if those users are "buggy". But if someone complains, I think the correct response is to open-code the trylock dance and call the new unlocked __i2c_smbus_xfer at the affected location. So, I think we have a contingency plan. Other than that, we are in violent agreement, and I think you also agree with the above. I guess that also means the series is fine as-is (modulo the fixes recently made in the tail of the first hunk of patch 1/5 that causes a trivial but annoying conflict when applying it, i.e. below the i2c_transfer -> __i2c_transfer update in the emulation function). As far as I'm concerned, you can take this whole series directly even if most patches are i2c-mux patches. I don't have anything in my tree yet so I'll simply base any other stuff on this once I can fetch it from you... Ok? Cheers, Peter
diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c index f3f683041e7f..e89956ec5e27 100644 --- a/drivers/i2c/i2c-core-smbus.c +++ b/drivers/i2c/i2c-core-smbus.c @@ -463,7 +463,7 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr, msg[num-1].len++; } - status = i2c_transfer(adapter, msg, num); + status = __i2c_transfer(adapter, msg, num); if (status < 0) return status; if (status != num) @@ -520,9 +520,24 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr, * This executes an SMBus protocol operation, and returns a negative * errno code else zero on success. */ -s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags, - char read_write, u8 command, int protocol, - union i2c_smbus_data *data) +s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, + unsigned short flags, char read_write, + u8 command, int protocol, union i2c_smbus_data *data) +{ + s32 res; + + i2c_lock_bus(adapter, I2C_LOCK_SEGMENT); + res = __i2c_smbus_xfer(adapter, addr, flags, read_write, + command, protocol, data); + i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT); + + return res; +} +EXPORT_SYMBOL(i2c_smbus_xfer); + +s32 __i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, + unsigned short flags, char read_write, + u8 command, int protocol, union i2c_smbus_data *data) { unsigned long orig_jiffies; int try; @@ -539,8 +554,6 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags, flags &= I2C_M_TEN | I2C_CLIENT_PEC | I2C_CLIENT_SCCB; if (adapter->algo->smbus_xfer) { - i2c_lock_bus(adapter, I2C_LOCK_SEGMENT); - /* Retry automatically on arbitration loss */ orig_jiffies = jiffies; for (res = 0, try = 0; try <= adapter->retries; try++) { @@ -553,7 +566,6 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags, orig_jiffies + adapter->timeout)) break; } - i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT); if (res != -EOPNOTSUPP || !adapter->algo->master_xfer) goto trace; @@ -575,7 +587,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags, return res; } -EXPORT_SYMBOL(i2c_smbus_xfer); +EXPORT_SYMBOL(__i2c_smbus_xfer); /** * i2c_smbus_read_i2c_block_data_or_emulated - read block or emulate diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 254cd34eeae2..465afb092fa7 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -140,9 +140,14 @@ extern int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, 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. */ -extern s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, - unsigned short flags, char read_write, u8 command, - int size, union i2c_smbus_data *data); +s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, + unsigned short flags, char read_write, u8 command, + int protocol, union i2c_smbus_data *data); + +/* Unlocked flavor */ +s32 __i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, + unsigned short flags, char read_write, u8 command, + int protocol, union i2c_smbus_data *data); /* Now follow the 'nice' access routines. These also document the calling conventions of i2c_smbus_xfer. */
Removes all locking from i2c_smbus_xfer and renames it to __i2c_smbus_xfer, then adds a new i2c_smbus_xfer function that simply grabs the lock while calling the unlocked variant. This is not perfectly equivalent, since i2c_smbus_xfer was callable from atomic/irq context if you happened to end up emulating SMBus with an I2C transfer, and that is no longer the case with this patch. It is unknown (to me) if anything depends on that quirk, but it seems fragile enough to simply break those cases and require them to call i2c_transfer directly instead. While at it, for consistency rename the 2nd to last argument (size) of the i2c_smbus_xfer declaration to protocol and remove the surplus extern marker. Signed-off-by: Peter Rosin <peda@axentia.se> --- drivers/i2c/i2c-core-smbus.c | 28 ++++++++++++++++++++-------- include/linux/i2c.h | 11 ++++++++--- 2 files changed, 28 insertions(+), 11 deletions(-)