Re: [PATCH 1/5] i2c: smbus: add unlocked __i2c_smbus_xfer variant

[Peter Rosin]

On June 26, 2018 3:46:07 PM GMT+02:00, Wolfram Sang <wsa@xxxxxxxxxxxxx> 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