Messages in this thread | | | Date | Wed, 6 Sep 2023 09:32:47 +0200 | From | Andi Shyti <> | Subject | Re: [PATCH v3] i2c: stm32f7: Add atomic_xfer method to driver |
| |
Hi Sean,
> >>>> @@ -357,6 +357,7 @@ struct stm32f7_i2c_dev { > >>>> u32 dnf_dt; > >>>> u32 dnf; > >>>> struct stm32f7_i2c_alert *alert; > >>>> + bool atomic; > >>> > >>> this smells a bit racy here, this works only if the xfer's are > >>> always sequential. > >>> > >>> What happens when we receive at the same time two xfer's, one > >>> atomic and one non atomic? > >> > >> From the include/i2c.h: > >> * @master_xfer_atomic: same as @master_xfer. Yet, only using atomic context > >> * so e.g. PMICs can be accessed very late before shutdown. Optional. > >> > >> So it’s only used very late in the shutdown. > >> > >> It’s implemented the same way as in: > >> drivers/i2c/busses/i2c-imx.c > >> drivers/i2c/busses/i2c-meson.c > >> drivers/i2c/busses/i2c-mv64xxx.c > >> drivers/i2c/busses/i2c-tegra.c > >> … etc… > >> > >> > >> In drivers/i2c/i2c-core.h it’s determined whether it’s atomic transfer or not: > >> > >> /* > >> * We only allow atomic transfers for very late communication, e.g. to access a > >> * PMIC when powering down. Atomic transfers are a corner case and not for > >> * generic use! > >> */ > >> static inline bool i2c_in_atomic_xfer_mode(void) > >> { > >> return system_state > SYSTEM_RUNNING && irqs_disabled(); > >> } > >> > >> So you would not have an atomic transfer and later an non atomic. > > > > What about the opposite? I.e. a non atomic and later an atomic, > > for very late tardive communications :) > > Sure it’s the opposite? Normal scenario is “non atomic” transfers going on and under shutdown it switches to “atomic”. > From i2c_in_atomic_xfer_mode() it can’t go from “atomic” -> “non atomic”.
well at some point we move from non atomic to atomic and we preempt whatever is non atomic in order to go atomic, including non atomic transfers.
A "global" variable thrown there without protection is a bit weak and we need to be sure to be covering all possible scenarios when this variable is used.
> extern enum system_states { > SYSTEM_BOOTING, > SYSTEM_SCHEDULING, > SYSTEM_FREEING_INITMEM, > SYSTEM_RUNNING, > SYSTEM_HALT, > SYSTEM_POWER_OFF, > SYSTEM_RESTART, > SYSTEM_SUSPEND, > } system_state; > > If you are asking what happens if a “non atomic” transfer is ongoing and irq’s is disabled, IDK. > > Let’s get Wolfram in the loop (Sorry I forgot to add you) :)
Nah, it's OK... I am thinking aloud here and trying to cover possible scenarios. I also think that setting up a spinlock might be too much paranoiac and not necessary.
I'm going to ack it... but I will keep a few thoughts on thinking what can happen wrong here.
Acked-by: Andi Shyti <andi.shyti@kernel.org>
Andi
| |