[RFC,v2,0/7] i2c: core: introduce atomic transfers
mbox series

Message ID 20190302134735.4393-1-wsa+renesas@sang-engineering.com
Headers show
Series
  • i2c: core: introduce atomic transfers
Related show

Message

Wolfram Sang March 2, 2019, 1:47 p.m. UTC
So, finally, here is the second RFC for supporting I2C transfers in atomic
contexts (i.e. very late). This will need some text because I tried some things
on the way but had to discard them. However, I think it is important to have
that documented.

One thing I really wanted to have is a kind of whitelist for devices which are
allowed to use atomic transfers. So we could identify the "unauthorized" ones
as buggy. To be useful, this should not add new API calls for transfers,
otherwise things would have become way more complicated for I2C users like
regmap. So, I tried e.g. to flag clients and provide that information
throughout the i2c tree (think muxes here). In the end, I concluded that this
is not an I2C specific problem, so it can't have an I2C specifc solution.
Imagine a GPIO which is needed to reboot (drivers/power/reset/gpio-restart.c).
This is the device which needs to whitelisted but the driver doesn't even know
if the GPIO is behind I2C or not. So, if we want this, it should probably be
handled on 'struct device' level. Including all the hierarchy. Postponed.

So, this RFC v2 is much more similar to v1 than I expected. Main changes:

* cleaned up 'struct i2c_adapter' a bit before adding the new stuff
* added an atomic callback for SMBus, too. Only build-tested so far. But spent
  a few braincells of getting the SMBus logic readable because we could have
  an I2C fallback just for the atomic case
* add a WARN for atomic transfers with no atomic transfer handler
* added support for the i2c-demuxer, so I could test the series. Support
  for I2C muxes is missing because of the locking issue (see later) which
  may mean a redesign anyhow
* imported the omap support into this series to have another user. I didn't
  pick up the patch for imx from Stefan because it is bigger and probably
  needs seperate review first
* I converted the tegra-bpmp driver which already had handling for the atomic
  case*. I did not convert the pxa driver which has a polling-only mode, too.
  This also seems like a bigger task and its current behaviour shouldn't be
  affected by this series. *only build tested
* added a HACK to allow the i2c-gpio driver atomic transfers. This will only
  work if accessing the GPIO can be done in atomic contexts, too, so this is
  for testing only

For the regular cases this series works well on my Renesas Lager board*
which needs an I2C access to the PMIC to reboot the board. *if I use the
i2c-gpio driver, the i2c-sh_mobile is not converted yet.

However, during the last review, Russell King brought up an interesting corner
case. What if we want to reboot because of a panic and the bus is not in a
consistent state? To create this situation, I recently created the 'inject-panic'
fault injector [1] which is merged into i2c/for-next meanwhile.

With this fault injector and 'reboot after panic' settings, I can create
the problem Russell described: a) the bus is in an inconsistent state because
the driver was interrupted (SCL/SDA both low) and b) the lock for this driver
is taken, so trylock fails.

I think b) is an interesting question: shall we give atomic transfers priority
and ignore the lock? Do we need a seperate one then (SMP is turned off already,
or?)? If so, that would probably mean way more complicated mux-locking code
(Peter?)? And what if some mux in the path needs interrupts? And how academic
is all that? Because someone putting the reboot functionality behind muxed I2C
is kind of asking for problems :)

That being said: this is an issue I think it is worth tackling. However, this
issue is not introduced by this series. It is already there. It might just
become more visible.

Sidenote: I think problem a) is easier once we solved b). E.g. if we decide on
a higher priority, we can postulate that IP cores should be reset first and
bus recovery can also be applied. This will not help all cases but IMO is all
we can do.

Another topic where I'd like input from other people is the use of 'in_atomic'
in this series. It was already there, so I kept it to avoid regressions. I am
aware that 'in_atomic' should not be used in drivers. So, if someone has
expertise to say if it can be removed or replaced with something else, I am
all ears.

A branch (based on i2c/for-next) can be found here:
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/atomic_xfer

Sorry, no TLDR; text here - I think this topic deserves a few words ;)

Looking forward to comments, thanks!

   Wolfram


[1] http://patchwork.ozlabs.org/patch/1044789/


Tero Kristo (1):
  i2c: busses: omap: Add the master_xfer_irqless hook

Wolfram Sang (6):
  i2c: apply coding style for struct i2c_adapter
  i2c: core: use I2C locking behaviour also for SMBUS
  i2c: core: introduce callbacks for atomic transfers
  i2c: demux: WIP: handle the new atomic callbacks
  i2c: tegra-bpmp: convert to use new atomic callbacks
  i2c: algo: bit: HACK! add atomic callback

 drivers/i2c/algos/i2c-algo-bit.c      |  5 ++-
 drivers/i2c/busses/i2c-omap.c         | 79 +++++++++++++++++++++++++++++------
 drivers/i2c/busses/i2c-tegra-bpmp.c   | 27 +++++++++---
 drivers/i2c/i2c-core-base.c           | 17 ++++----
 drivers/i2c/i2c-core-smbus.c          | 25 ++++++++---
 drivers/i2c/i2c-core.h                | 15 +++++++
 drivers/i2c/muxes/i2c-demux-pinctrl.c |  3 ++
 include/linux/i2c.h                   | 38 +++++++++++------
 8 files changed, 162 insertions(+), 47 deletions(-)

Comments

Andy Shevchenko March 2, 2019, 4:22 p.m. UTC | #1
On Sat, Mar 2, 2019 at 3:49 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> So, finally, here is the second RFC for supporting I2C transfers in atomic
> contexts (i.e. very late). This will need some text because I tried some things
> on the way but had to discard them. However, I think it is important to have
> that documented.

> Sorry, no TLDR; text here - I think this topic deserves a few words ;)

Thank you for this work! It was indeed interesting reading.
And since your series is targetting some exiting use cases, I would
drop as well academic variants of brain-damaged hw design, I think it
worth to go.

WRT patches,

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

for patches 1-2
and FWIW for patches 3-4.

For the individual drivers I can't say much, code looks good, but I
dunno if it's correct or not.

>
> Looking forward to comments, thanks!
>
>    Wolfram
>
>
> [1] http://patchwork.ozlabs.org/patch/1044789/
>
>
> Tero Kristo (1):
>   i2c: busses: omap: Add the master_xfer_irqless hook
>
> Wolfram Sang (6):
>   i2c: apply coding style for struct i2c_adapter
>   i2c: core: use I2C locking behaviour also for SMBUS
>   i2c: core: introduce callbacks for atomic transfers
>   i2c: demux: WIP: handle the new atomic callbacks
>   i2c: tegra-bpmp: convert to use new atomic callbacks
>   i2c: algo: bit: HACK! add atomic callback
>
>  drivers/i2c/algos/i2c-algo-bit.c      |  5 ++-
>  drivers/i2c/busses/i2c-omap.c         | 79 +++++++++++++++++++++++++++++------
>  drivers/i2c/busses/i2c-tegra-bpmp.c   | 27 +++++++++---
>  drivers/i2c/i2c-core-base.c           | 17 ++++----
>  drivers/i2c/i2c-core-smbus.c          | 25 ++++++++---
>  drivers/i2c/i2c-core.h                | 15 +++++++
>  drivers/i2c/muxes/i2c-demux-pinctrl.c |  3 ++
>  include/linux/i2c.h                   | 38 +++++++++++------
>  8 files changed, 162 insertions(+), 47 deletions(-)
>
> --
> 2.11.0
>
Peter Rosin March 4, 2019, 6:11 p.m. UTC | #2
On 2019-03-02 14:47, Wolfram Sang wrote:
> So, finally, here is the second RFC for supporting I2C transfers in atomic
> contexts (i.e. very late). This will need some text because I tried some things
> on the way but had to discard them. However, I think it is important to have
> that documented.
> 
> One thing I really wanted to have is a kind of whitelist for devices which are
> allowed to use atomic transfers. So we could identify the "unauthorized" ones
> as buggy. To be useful, this should not add new API calls for transfers,
> otherwise things would have become way more complicated for I2C users like
> regmap. So, I tried e.g. to flag clients and provide that information
> throughout the i2c tree (think muxes here). In the end, I concluded that this
> is not an I2C specific problem, so it can't have an I2C specifc solution.
> Imagine a GPIO which is needed to reboot (drivers/power/reset/gpio-restart.c).
> This is the device which needs to whitelisted but the driver doesn't even know
> if the GPIO is behind I2C or not. So, if we want this, it should probably be
> handled on 'struct device' level. Including all the hierarchy. Postponed.
> 
> So, this RFC v2 is much more similar to v1 than I expected. Main changes:
> 
> * cleaned up 'struct i2c_adapter' a bit before adding the new stuff
> * added an atomic callback for SMBus, too. Only build-tested so far. But spent
>   a few braincells of getting the SMBus logic readable because we could have
>   an I2C fallback just for the atomic case
> * add a WARN for atomic transfers with no atomic transfer handler
> * added support for the i2c-demuxer, so I could test the series. Support
>   for I2C muxes is missing because of the locking issue (see later) which
>   may mean a redesign anyhow
> * imported the omap support into this series to have another user. I didn't
>   pick up the patch for imx from Stefan because it is bigger and probably
>   needs seperate review first
> * I converted the tegra-bpmp driver which already had handling for the atomic
>   case*. I did not convert the pxa driver which has a polling-only mode, too.
>   This also seems like a bigger task and its current behaviour shouldn't be
>   affected by this series. *only build tested
> * added a HACK to allow the i2c-gpio driver atomic transfers. This will only
>   work if accessing the GPIO can be done in atomic contexts, too, so this is
>   for testing only
> 
> For the regular cases this series works well on my Renesas Lager board*
> which needs an I2C access to the PMIC to reboot the board. *if I use the
> i2c-gpio driver, the i2c-sh_mobile is not converted yet.
> 
> However, during the last review, Russell King brought up an interesting corner
> case. What if we want to reboot because of a panic and the bus is not in a
> consistent state? To create this situation, I recently created the 'inject-panic'
> fault injector [1] which is merged into i2c/for-next meanwhile.
> 
> With this fault injector and 'reboot after panic' settings, I can create
> the problem Russell described: a) the bus is in an inconsistent state because
> the driver was interrupted (SCL/SDA both low) and b) the lock for this driver
> is taken, so trylock fails.
> 
> I think b) is an interesting question: shall we give atomic transfers priority
> and ignore the lock? Do we need a seperate one then (SMP is turned off already,
> or?)? If so, that would probably mean way more complicated mux-locking code
> (Peter?)? And what if some mux in the path needs interrupts? And how academic
> is all that? Because someone putting the reboot functionality behind muxed I2C
> is kind of asking for problems :)
> 
> That being said: this is an issue I think it is worth tackling. However, this
> issue is not introduced by this series. It is already there. It might just
> become more visible.

The way I read this series, you are not giving atomic transfers priority. The
only thing that happens is that if an xfer happens in atomic/irq context,
trylock is used instead of an ordinary (unconditional) lock (this is just
like it is already). If a mux is sitting in between the client device and
the root adapter, the trylock operation will percolate to the root. Sure,
there are more trylock ops that may fail and abort the xfer, but if
everything is uncontended, then things should proceed in orderly fashion.
Also, sure, the mux may need additional resources that are no longer
available if the machine is half way down (or worse). But I don't see any
fundamental *locking* issue with muxes that is different from the case
without a mux.

That said, if you then want to introduce xfers that want to circumvent the
locking, then parent-locked muxes are easier since the actual muxing operation
is performed as an unlocked xfer (if one is needed) while the client device
has grabbed the adapter lock "from the outside". Sure, there is a list of
locks going up through the adapter tree to handle, but that can probably be
handled in one place. I.e. the locking must have been avoided prior to the
actual muxing operation, but the code to do so can be in one place. The
mux-locked case is where the trouble is, since the muxing operation is done
as a normal xfer and needs to be classified as a special xfer that just like
the original client xfer also needs to break through any existing locks in
the adapter tree. And those muxing xfers might come from anywhere, e.g.

	- IO-expander controlling a gpio/pinctrl mux
	- dedicated I2C mux (e.g. the LTC4306)
	- regmap device
	- etc, who knows what muxing options will evolve?

So, any scheme that require a white-list will work poorly for mux-locked
muxes, unless you can add some new grip/pinctrl/regmap flags to
gpios/pins/registers so that the particular accesses can be white-listed.
Adding those flags seem rather invasive?

But of course, you need to actually do something about the added FIXME in
the demux-pinctrl driver... BTW, that driver should forward ->smbus_xfer
just like it does for ->master_xfer, no?

Cheers,
Peter

> Sidenote: I think problem a) is easier once we solved b). E.g. if we decide on
> a higher priority, we can postulate that IP cores should be reset first and
> bus recovery can also be applied. This will not help all cases but IMO is all
> we can do.
> 
> Another topic where I'd like input from other people is the use of 'in_atomic'
> in this series. It was already there, so I kept it to avoid regressions. I am
> aware that 'in_atomic' should not be used in drivers. So, if someone has
> expertise to say if it can be removed or replaced with something else, I am
> all ears.
> 
> A branch (based on i2c/for-next) can be found here:
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/atomic_xfer
> 
> Sorry, no TLDR; text here - I think this topic deserves a few words ;)
> 
> Looking forward to comments, thanks!
> 
>    Wolfram
> 
> 
> [1] http://patchwork.ozlabs.org/patch/1044789/
> 
> 
> Tero Kristo (1):
>   i2c: busses: omap: Add the master_xfer_irqless hook
> 
> Wolfram Sang (6):
>   i2c: apply coding style for struct i2c_adapter
>   i2c: core: use I2C locking behaviour also for SMBUS
>   i2c: core: introduce callbacks for atomic transfers
>   i2c: demux: WIP: handle the new atomic callbacks
>   i2c: tegra-bpmp: convert to use new atomic callbacks
>   i2c: algo: bit: HACK! add atomic callback
> 
>  drivers/i2c/algos/i2c-algo-bit.c      |  5 ++-
>  drivers/i2c/busses/i2c-omap.c         | 79 +++++++++++++++++++++++++++++------
>  drivers/i2c/busses/i2c-tegra-bpmp.c   | 27 +++++++++---
>  drivers/i2c/i2c-core-base.c           | 17 ++++----
>  drivers/i2c/i2c-core-smbus.c          | 25 ++++++++---
>  drivers/i2c/i2c-core.h                | 15 +++++++
>  drivers/i2c/muxes/i2c-demux-pinctrl.c |  3 ++
>  include/linux/i2c.h                   | 38 +++++++++++------
>  8 files changed, 162 insertions(+), 47 deletions(-)
>
Wolfram Sang March 4, 2019, 10:48 p.m. UTC | #3
Hi Peda,

> The way I read this series, you are not giving atomic transfers priority. The

You are reading correctly. I could have made more clear that the issue
pointed out by Russell is not handled by this series but discussion
about it is welcome / needed to decide if we can take this series as is
or if we need to redesign it. But here we are anyhow :)

> only thing that happens is that if an xfer happens in atomic/irq context,
> trylock is used instead of an ordinary (unconditional) lock (this is just
> like it is already). If a mux is sitting in between the client device and
> the root adapter, the trylock operation will percolate to the root. Sure,
> there are more trylock ops that may fail and abort the xfer, but if
> everything is uncontended, then things should proceed in orderly fashion.
> Also, sure, the mux may need additional resources that are no longer
> available if the machine is half way down (or worse). But I don't see any
> fundamental *locking* issue with muxes that is different from the case
> without a mux.

Good, that was my conclusion as well. The series, as is, doesn't change
the locking behaviour, so that will work exactly as before. Or, it will
not work in the case described by Russell. Like before.

> That said, if you then want to introduce xfers that want to circumvent the
> locking, then parent-locked muxes are easier since the actual muxing operation
> is performed as an unlocked xfer (if one is needed) while the client device
> has grabbed the adapter lock "from the outside". Sure, there is a list of
> locks going up through the adapter tree to handle, but that can probably be
> handled in one place. I.e. the locking must have been avoided prior to the
> actual muxing operation, but the code to do so can be in one place. The

That was my gut feeling, too...

> mux-locked case is where the trouble is, since the muxing operation is done
> as a normal xfer and needs to be classified as a special xfer that just like
> the original client xfer also needs to break through any existing locks in
> the adapter tree. And those muxing xfers might come from anywhere, e.g.
> 
> 	- IO-expander controlling a gpio/pinctrl mux
> 	- dedicated I2C mux (e.g. the LTC4306)
> 	- regmap device
> 	- etc, who knows what muxing options will evolve?
> 
> So, any scheme that require a white-list will work poorly for mux-locked
> muxes, unless you can add some new grip/pinctrl/regmap flags to
> gpios/pins/registers so that the particular accesses can be white-listed.
> Adding those flags seem rather invasive?

... and sadly, this too. We would need the same kind of flag which I
described in my first paragraph of the original posting where I wanted
the flag to detect "unauthorized" uses of late I2C transfers. And this
is gonna be invasive. And I am not sure it is worth the effort.

I wonder what a reasonable effort is? Simply ignore the lock from the
"current" adapter and hope for the best that there is no mux or at
least no mux which needs interrupts / a lock attached to it?

> But of course, you need to actually do something about the added FIXME in
> the demux-pinctrl driver... BTW, that driver should forward ->smbus_xfer
> just like it does for ->master_xfer, no?

Yes. The idea of having two seperate SMBus controllers in one SoC which
would need demuxing is amusing, but still, it exists for I2C and you are
right.

Thanks,

   Wolfram
Peter Rosin March 7, 2019, 12:02 a.m. UTC | #4
On 2019-03-04 23:48, Wolfram Sang wrote:
> Hi Peda,
> 
>> The way I read this series, you are not giving atomic transfers priority. The
> 
> You are reading correctly. I could have made more clear that the issue
> pointed out by Russell is not handled by this series but discussion
> about it is welcome / needed to decide if we can take this series as is
> or if we need to redesign it. But here we are anyhow :)
> 
>> only thing that happens is that if an xfer happens in atomic/irq context,
>> trylock is used instead of an ordinary (unconditional) lock (this is just
>> like it is already). If a mux is sitting in between the client device and
>> the root adapter, the trylock operation will percolate to the root. Sure,
>> there are more trylock ops that may fail and abort the xfer, but if
>> everything is uncontended, then things should proceed in orderly fashion.
>> Also, sure, the mux may need additional resources that are no longer
>> available if the machine is half way down (or worse). But I don't see any
>> fundamental *locking* issue with muxes that is different from the case
>> without a mux.
> 
> Good, that was my conclusion as well. The series, as is, doesn't change
> the locking behaviour, so that will work exactly as before. Or, it will
> not work in the case described by Russell. Like before.
> 
>> That said, if you then want to introduce xfers that want to circumvent the
>> locking, then parent-locked muxes are easier since the actual muxing operation
>> is performed as an unlocked xfer (if one is needed) while the client device
>> has grabbed the adapter lock "from the outside". Sure, there is a list of
>> locks going up through the adapter tree to handle, but that can probably be
>> handled in one place. I.e. the locking must have been avoided prior to the
>> actual muxing operation, but the code to do so can be in one place. The
> 
> That was my gut feeling, too...
> 
>> mux-locked case is where the trouble is, since the muxing operation is done
>> as a normal xfer and needs to be classified as a special xfer that just like
>> the original client xfer also needs to break through any existing locks in
>> the adapter tree. And those muxing xfers might come from anywhere, e.g.
>>
>> 	- IO-expander controlling a gpio/pinctrl mux
>> 	- dedicated I2C mux (e.g. the LTC4306)
>> 	- regmap device
>> 	- etc, who knows what muxing options will evolve?
>>
>> So, any scheme that require a white-list will work poorly for mux-locked
>> muxes, unless you can add some new grip/pinctrl/regmap flags to

s/grip/gpio/ of course

>> gpios/pins/registers so that the particular accesses can be white-listed.
>> Adding those flags seem rather invasive?
> 
> ... and sadly, this too. We would need the same kind of flag which I
> described in my first paragraph of the original posting where I wanted
> the flag to detect "unauthorized" uses of late I2C transfers. And this
> is gonna be invasive. And I am not sure it is worth the effort.
> 
> I wonder what a reasonable effort is? Simply ignore the lock from the
> "current" adapter and hope for the best that there is no mux or at
> least no mux which needs interrupts / a lock attached to it?

Just wanted to add a note that the underlying problem is similar to why
I introduced the mux-locked concept. There is no simple way to identify
*exactly* which xfers that need to be unlocked. Going only by call site
is not enough, since the same call in different context may need to be
muxed (in my case) or irq-less (in this case). If someone comes up with
a solution for that, all muxes can be converted to the parent-locked
scheme and we can get rid of a bunch of complexity. I just don't see how
though, all ideas I have come up with I have immediately discarded as way
too invasive, ugly and/or error prone.

>> But of course, you need to actually do something about the added FIXME in
>> the demux-pinctrl driver... BTW, that driver should forward ->smbus_xfer
>> just like it does for ->master_xfer, no?
> 
> Yes. The idea of having two seperate SMBus controllers in one SoC which
> would need demuxing is amusing, but still, it exists for I2C and you are
> right.

Right, I didn't actually think all that far... :-)

Cheers,
Peter
Wolfram Sang March 12, 2019, 3:45 p.m. UTC | #5
> > So, finally, here is the second RFC for supporting I2C transfers in atomic
> > contexts (i.e. very late). This will need some text because I tried some things
> > on the way but had to discard them. However, I think it is important to have
> > that documented.
> 
> > Sorry, no TLDR; text here - I think this topic deserves a few words ;)
> 
> Thank you for this work! It was indeed interesting reading.

Thanks, glad to hear that :)

> And since your series is targetting some exiting use cases, I would
> drop as well academic variants of brain-damaged hw design, I think it
> worth to go.

Well, yes and no. I am with you that some complicated muxed setups could
be argued to be way over the top. However, with the panic
fault-injector, I can get my simple "PMIC directly (even exclusively)
attached to root adapter" setup to stall. By simply ignoring the lock,
such setups could work again. But this series does not implement this
because it would need a redesign, i.e. tie into i2c_transfer and not
later in __i2c_transfer.

Yet, before doing this, I was interested in discussion if ignoring the
lock is really desirable.

This series seems like a valid approach to me if we still want to
respect the locking.
Wolfram Sang March 25, 2019, 1:40 p.m. UTC | #6
> This series seems like a valid approach to me if we still want to
> respect the locking.

And I am leaning to that it is good enough. I think pragmatism is OK
here: The users who want this feature simply want to reboot their
system, mostly development systems. They can't reboot otherwise. Except
for the HW switch they are currently using anyhow.

The panic fault-injector can create an inconsistent situation on the I2C
bus when you want to reboot after an OOPS while a transfer was in
progress. However, if rebooting in such scenarios is critical for you,
you a) shouldn't reboot via I2C, and/or b) should have a watchdog in
place. We can't guarantee to always fix this sitution. At best, we could
just try to be better for some cases. However, this would mean having a
backdoor to skip the locking scheme which doesn't sound right. Maybe
just accepting the deadlock is not too bad because it will reliably
point out a design flaw of the system (hopefully during the development
stage)?

Final call for other thoughts/comments.

PS: I am still interested in the use of in_atomic() here. I wonder if it is
correct. If a change is needed, it should probably be a seperate series,
though.
Wolfram Sang March 27, 2019, 1:53 p.m. UTC | #7
> >> the demux-pinctrl driver... BTW, that driver should forward ->smbus_xfer
> >> just like it does for ->master_xfer, no?
> > 
> > Yes. The idea of having two seperate SMBus controllers in one SoC which
> > would need demuxing is amusing, but still, it exists for I2C and you are
> > right.
> 
> Right, I didn't actually think all that far... :-)

On second thought, we can add smbus support when we need it.