Re: [PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant
On July 1, 2018 2:13:20 PM GMT+02:00, Wolfram Sang 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
Re: [PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant
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.
signature.asc
Description: PGP signature
Re: [PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant
> 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?
signature.asc
Description: PGP signature
Re: [PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant
On June 26, 2018 3:46:07 PM GMT+02:00, Wolfram Sang 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
Re: [PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant
> 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... signature.asc Description: PGP signature
Re: [PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant
On June 26, 2018 4:37:58 AM GMT+02:00, Wolfram Sang 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
Re: [PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer 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. 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!
[PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant
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
---
drivers/i2c/i2c-core-smbus.c | 28
include/linux/i2c.h | 11 ---
2 files changed, 28 insertions(+), 11 deletions(-)
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. */
--
2.11.0
