| Message ID | 20200618150532.2923-1-uli+renesas@fpond.eu (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [v2] i2c: sh_mobile: implement atomic transfers | expand |
Hi Uli, On Thu, Jun 18, 2020 at 5:05 PM Ulrich Hecht <uli+renesas@fpond.eu> wrote: > Implements atomic transfers to fix reboot/shutdown on r8a7790 Lager and > similar boards. > > Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu> > Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> Thanks for your patch! > --- a/drivers/i2c/busses/i2c-sh_mobile.c > +++ b/drivers/i2c/busses/i2c-sh_mobile.c > @@ -581,12 +585,14 @@ static void start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg, > pd->pos = -1; > pd->sr = 0; > if (pd->atomic_xfer) return; and be done with it? > - pd->dma_buf = i2c_get_dma_safe_msg_buf(pd->msg, 8); > - if (pd->dma_buf) > - sh_mobile_i2c_xfer_dma(pd); > - > - /* Enable all interrupts to begin with */ > - iic_wr(pd, ICIC, ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE); > + if (!pd->atomic_xfer) { > + pd->dma_buf = i2c_get_dma_safe_msg_buf(pd->msg, 8); > + if (pd->dma_buf) > + sh_mobile_i2c_xfer_dma(pd); > + /* Enable all interrupts to begin with */ > + iic_wr(pd, ICIC, > + ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE); > + } > } > > static int poll_dte(struct sh_mobile_i2c_data *pd) > @@ -637,15 +643,13 @@ static int poll_busy(struct sh_mobile_i2c_data *pd) > return i ? 0 : -ETIMEDOUT; > } > > -static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter, > - struct i2c_msg *msgs, > - int num) > +static int sh_mobile_xfer(struct sh_mobile_i2c_data *pd, > + struct i2c_msg *msgs, int num) > { > - struct sh_mobile_i2c_data *pd = i2c_get_adapdata(adapter); > struct i2c_msg *msg; > int err = 0; > int i; > - long timeout; > + long time_left; > > /* Wake up device and enable clock */ > pm_runtime_get_sync(pd->dev); pm_runtime_get_sync() is a wrapper around __pm_runtime_resume(), which does: might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe && dev->power.runtime_status != RPM_ACTIVE); So if the device is not active (it is not), the might_sleep() is triggered, and I expect a BUG splat. However, with CONFIG_DEBUG_ATOMIC_SLEEP disabled (I disabled it on koelsch, as it increases kernel size beyond the bootloader limit), might_sleep() is a no-op, so nothing happens. After enabling it (and disabling drm and media), still nothing... It turns out ___might_sleep() does: if ((preempt_count_equals(preempt_offset) && !irqs_disabled() && !is_idle_task(current) && !current->non_block_count) || system_state == SYSTEM_BOOTING || system_state > SYSTEM_RUNNING || oops_in_progress) return; and as per: static inline bool i2c_in_atomic_xfer_mode(void) { return system_state > SYSTEM_RUNNING && irqs_disabled(); } system_state > SYSTEM_RUNNING, and ___might_sleep() just ignores any issues. Oops... After removing that check, it starts complaining: BUG: sleeping function called from invalid context at kernel/locking/mutex.c:281 in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 1, name: systemd-shutdow In general, pm_runtime_get_sync() is not safe to call from atomic context. For Renesas SoCs, I think both the power and clock domains are safe, as the respective drivers don't sleep. The PM core might, though. > @@ -662,15 +666,35 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter, > if (do_start) > i2c_op(pd, OP_START); > > - /* The interrupt handler takes care of the rest... */ > - timeout = wait_event_timeout(pd->wait, > - pd->sr & (ICSR_TACK | SW_DONE), > - adapter->timeout); > + if (pd->atomic_xfer) { > + unsigned long j = jiffies + pd->adap.timeout; > + > + time_left = time_before_eq(jiffies, j); > + while (time_left && Who's updating time_left? > + !(pd->sr & (ICSR_TACK | SW_DONE))) { > + unsigned char sr = iic_rd(pd, ICSR); > + > + if (sr & (ICSR_AL | ICSR_TACK | > + ICSR_WAIT | ICSR_DTE)) { > + sh_mobile_i2c_isr(0, pd); > + udelay(150); > + } else { > + cpu_relax(); > + } > + } > + } else { > + /* The interrupt handler takes care of the rest... */ > + time_left = wait_event_timeout(pd->wait, > + pd->sr & (ICSR_TACK | SW_DONE), > + pd->adap.timeout); > + > + /* 'stop_after_dma' tells if DMA xfer was complete */ > + i2c_put_dma_safe_msg_buf(pd->dma_buf, pd->msg, > + pd->stop_after_dma); > > - /* 'stop_after_dma' tells if DMA transfer was complete */ > - i2c_put_dma_safe_msg_buf(pd->dma_buf, pd->msg, pd->stop_after_dma); > + } > > - if (!timeout) { > + if (!time_left) { > dev_err(pd->dev, "Transfer request timed out\n"); > if (pd->dma_direction != DMA_NONE) > sh_mobile_i2c_cleanup_dma(pd); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Wolfram, On Thu, Jun 18, 2020 at 6:39 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, Jun 18, 2020 at 5:05 PM Ulrich Hecht <uli+renesas@fpond.eu> wrote: > > Implements atomic transfers to fix reboot/shutdown on r8a7790 Lager and > > similar boards. > > > > Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu> > > Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- a/drivers/i2c/busses/i2c-sh_mobile.c > > +++ b/drivers/i2c/busses/i2c-sh_mobile.c > > @@ -637,15 +643,13 @@ static int poll_busy(struct sh_mobile_i2c_data *pd) > > return i ? 0 : -ETIMEDOUT; > > } > > > > -static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter, > > - struct i2c_msg *msgs, > > - int num) > > +static int sh_mobile_xfer(struct sh_mobile_i2c_data *pd, > > + struct i2c_msg *msgs, int num) > > { > > - struct sh_mobile_i2c_data *pd = i2c_get_adapdata(adapter); > > struct i2c_msg *msg; > > int err = 0; > > int i; > > - long timeout; > > + long time_left; > > > > /* Wake up device and enable clock */ > > pm_runtime_get_sync(pd->dev); > > pm_runtime_get_sync() is a wrapper around __pm_runtime_resume(), which > does: > > might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe && > dev->power.runtime_status != RPM_ACTIVE); > > So if the device is not active (it is not), the might_sleep() is > triggered, and I expect a BUG splat. > However, with CONFIG_DEBUG_ATOMIC_SLEEP disabled (I disabled it on > koelsch, as it increases kernel size beyond the bootloader limit), > might_sleep() is a no-op, so nothing happens. > After enabling it (and disabling drm and media), still nothing... > > It turns out ___might_sleep() does: > > if ((preempt_count_equals(preempt_offset) && !irqs_disabled() && > !is_idle_task(current) && !current->non_block_count) || > system_state == SYSTEM_BOOTING || system_state > SYSTEM_RUNNING || > oops_in_progress) > return; > > and as per: > > static inline bool i2c_in_atomic_xfer_mode(void) > { > return system_state > SYSTEM_RUNNING && irqs_disabled(); So i2c atomic transfers are really meant to be used during late system suspend only, and not in atomic context before, when irqs_disabled() is true? > } > > system_state > SYSTEM_RUNNING, and ___might_sleep() just ignores any > issues. Oops... > After removing that check, it starts complaining: > > BUG: sleeping function called from invalid context at > kernel/locking/mutex.c:281 > in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 1, name: > systemd-shutdow Perhaps we need a checker config option, to make sure the atomic transfer operation is exercised at least once during boot? I guess scanning the i2c bus is an unsafe operation, but there may be something we can do in a safe way, without side effects (e.g. redoing the first i2c read message using atomic transfers)? Cfr. CONFIG_ARM_PSCI_CHECKER, which cycles through hotplug and suspend operations during boot. > In general, pm_runtime_get_sync() is not safe to call from atomic > context. > For Renesas SoCs, I think both the power and clock domains are safe, as > the respective drivers don't sleep. The PM core might, though. Do we need a way to let i2c slaves indicate they plan to use atomic transfers later, so the i2c core can keep the i2c controller resumed? Gr{oetje,eeting}s, Geert
Hi Geert, > > static inline bool i2c_in_atomic_xfer_mode(void) > > { > > return system_state > SYSTEM_RUNNING && irqs_disabled(); > > So i2c atomic transfers are really meant to be used during late system suspend > only, and not in atomic context before, when irqs_disabled() is true? Yes. It is all some time ago, I recall we agreed that there shouldn't be any other I2C communication at irqs_disabled() stage. > Perhaps we need a checker config option, to make sure the atomic transfer > operation is exercised at least once during boot? Testing I2C controllers (in various ways) is a well-desired feature :/ > Do we need a way to let i2c slaves indicate they plan to use atomic > transfers later, so the i2c core can keep the i2c controller resumed? I wanted to have this originally, but in the end I gave up on it. IIRC, you don't want to whitelist a client in general, but only the very late messages it sends. However, if a message needs to be flagged may be board specific. It all looked messy and hard to configure, so we ended up with what we have now. Take all this with a grain of salt, it's been a while since the discussions happened. All the best, Wolfram
Hi Geert, thanks for the review! > > @@ -581,12 +585,14 @@ static void start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg, > > pd->pos = -1; > > pd->sr = 0; > > > > if (pd->atomic_xfer) > return; > > and be done with it? I like Uli's version a tad better in case we ever add something for both cases after the following if-block. But I don't care much, we could change it later. > > - pd->dma_buf = i2c_get_dma_safe_msg_buf(pd->msg, 8); > > - if (pd->dma_buf) > > - sh_mobile_i2c_xfer_dma(pd); > > - > > - /* Enable all interrupts to begin with */ > > - iic_wr(pd, ICIC, ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE); > > + if (!pd->atomic_xfer) { > > + pd->dma_buf = i2c_get_dma_safe_msg_buf(pd->msg, 8); > > + if (pd->dma_buf) > > + sh_mobile_i2c_xfer_dma(pd); > > + /* Enable all interrupts to begin with */ > > + iic_wr(pd, ICIC, > > + ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE); > > + } ... > After removing that check, it starts complaining: > > BUG: sleeping function called from invalid context at > kernel/locking/mutex.c:281 > in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 1, name: > systemd-shutdow > > In general, pm_runtime_get_sync() is not safe to call from atomic > context. > For Renesas SoCs, I think both the power and clock domains are safe, as > the respective drivers don't sleep. The PM core might, though. Still, that sounds to me like we should protect these calls as in V1? > > + time_left = time_before_eq(jiffies, j); > > + while (time_left && > > Who's updating time_left? Good question :) Kind regards, Wolfram
Hi Wolfram, On Thu, Jun 25, 2020 at 9:06 AM Wolfram Sang <wsa@the-dreams.de> wrote: > ... > > > After removing that check, it starts complaining: > > > > BUG: sleeping function called from invalid context at > > kernel/locking/mutex.c:281 > > in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 1, name: > > systemd-shutdow > > > > In general, pm_runtime_get_sync() is not safe to call from atomic > > context. > > For Renesas SoCs, I think both the power and clock domains are safe, as > > the respective drivers don't sleep. The PM core might, though. > > Still, that sounds to me like we should protect these calls as in V1? And talk to the i2c controller while it is disabled? That does seem to work on R-Car Gen2 (similar to SMP bringup accessing registers of a disabled WDT?), though. Needs testing on R-Mobile A1.... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
> And talk to the i2c controller while it is disabled?
Hah, stupid me! Coming back from another topic, I overlooked this. You
are right, of course. Hmmm....
Hi Geert, I spend some more thoughts on this. > > > In general, pm_runtime_get_sync() is not safe to call from atomic > > > context. > > > For Renesas SoCs, I think both the power and clock domains are safe, as > > > the respective drivers don't sleep. The PM core might, though. > > > > Still, that sounds to me like we should protect these calls as in V1? I still think we should guard these calls just because it is not safe to call them from atomic contexts. > And talk to the i2c controller while it is disabled? Is there maybe some "always-on" property which we could add to the respective IIC clock? > That does seem to work on R-Car Gen2 (similar to SMP bringup accessing > registers of a disabled WDT?), though. Yes. Uli's patch will not cause a regression because we are already calling i2c_transfer very late. And we do call the runtime_pm functions currently. So, it will improve the situation there. > Needs testing on R-Mobile A1.... That's armadillo, right? I don't have that, sadly. Thanks, Wolfram
On Thu, Jun 25, 2020 at 05:16:58PM +0200, Wolfram Sang wrote: > Hi Geert, > > I spend some more thoughts on this. > > > > > In general, pm_runtime_get_sync() is not safe to call from atomic > > > > context. > > > > For Renesas SoCs, I think both the power and clock domains are safe, as > > > > the respective drivers don't sleep. The PM core might, though. > > > > > > Still, that sounds to me like we should protect these calls as in V1? > > I still think we should guard these calls just because it is not safe to > call them from atomic contexts. > > > And talk to the i2c controller while it is disabled? > > Is there maybe some "always-on" property which we could add to the > respective IIC clock? Ping to this question... > > > That does seem to work on R-Car Gen2 (similar to SMP bringup accessing > > registers of a disabled WDT?), though. > > Yes. Uli's patch will not cause a regression because we are already > calling i2c_transfer very late. And we do call the runtime_pm functions > currently. So, it will improve the situation there. > > > Needs testing on R-Mobile A1.... > > That's armadillo, right? I don't have that, sadly. > > Thanks, > > Wolfram >
Hi Wolfram, On Tue, Jun 30, 2020 at 9:45 PM Wolfram Sang <wsa@the-dreams.de> wrote: > On Thu, Jun 25, 2020 at 05:16:58PM +0200, Wolfram Sang wrote: > > I spend some more thoughts on this. > > > > > > > In general, pm_runtime_get_sync() is not safe to call from atomic > > > > > context. > > > > > For Renesas SoCs, I think both the power and clock domains are safe, as > > > > > the respective drivers don't sleep. The PM core might, though. > > > > > > > > Still, that sounds to me like we should protect these calls as in V1? > > > > I still think we should guard these calls just because it is not safe to > > call them from atomic contexts. > > > > > And talk to the i2c controller while it is disabled? > > > > Is there maybe some "always-on" property which we could add to the > > respective IIC clock? > > Ping to this question... You mean in DT? DT describes hardware, not software policy. Anyway, won't help on R-Mobile A1, as there's a real power domain. > > > That does seem to work on R-Car Gen2 (similar to SMP bringup accessing > > > registers of a disabled WDT?), though. > > > > Yes. Uli's patch will not cause a regression because we are already > > calling i2c_transfer very late. And we do call the runtime_pm functions > > currently. So, it will improve the situation there. > > > > > Needs testing on R-Mobile A1.... > > > > That's armadillo, right? I don't have that, sadly. Gr{oetje,eeting}s, Geert
diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c index 2cca1b21e26e..c863f9640fcc 100644 --- a/drivers/i2c/busses/i2c-sh_mobile.c +++ b/drivers/i2c/busses/i2c-sh_mobile.c @@ -129,6 +129,7 @@ struct sh_mobile_i2c_data { int sr; bool send_stop; bool stop_after_dma; + bool atomic_xfer; struct resource *res; struct dma_chan *dma_tx; @@ -330,13 +331,15 @@ static unsigned char i2c_op(struct sh_mobile_i2c_data *pd, enum sh_mobile_i2c_op ret = iic_rd(pd, ICDR); break; case OP_RX_STOP: /* enable DTE interrupt, issue stop */ - iic_wr(pd, ICIC, - ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE); + if (!pd->atomic_xfer) + iic_wr(pd, ICIC, + ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE); iic_wr(pd, ICCR, ICCR_ICE | ICCR_RACK); break; case OP_RX_STOP_DATA: /* enable DTE interrupt, read data, issue stop */ - iic_wr(pd, ICIC, - ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE); + if (!pd->atomic_xfer) + iic_wr(pd, ICIC, + ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE); ret = iic_rd(pd, ICDR); iic_wr(pd, ICCR, ICCR_ICE | ICCR_RACK); break; @@ -429,7 +432,8 @@ static irqreturn_t sh_mobile_i2c_isr(int irq, void *dev_id) if (wakeup) { pd->sr |= SW_DONE; - wake_up(&pd->wait); + if (!pd->atomic_xfer) + wake_up(&pd->wait); } /* defeat write posting to avoid spurious WAIT interrupts */ @@ -581,12 +585,14 @@ static void start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg, pd->pos = -1; pd->sr = 0; - pd->dma_buf = i2c_get_dma_safe_msg_buf(pd->msg, 8); - if (pd->dma_buf) - sh_mobile_i2c_xfer_dma(pd); - - /* Enable all interrupts to begin with */ - iic_wr(pd, ICIC, ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE); + if (!pd->atomic_xfer) { + pd->dma_buf = i2c_get_dma_safe_msg_buf(pd->msg, 8); + if (pd->dma_buf) + sh_mobile_i2c_xfer_dma(pd); + /* Enable all interrupts to begin with */ + iic_wr(pd, ICIC, + ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE); + } } static int poll_dte(struct sh_mobile_i2c_data *pd) @@ -637,15 +643,13 @@ static int poll_busy(struct sh_mobile_i2c_data *pd) return i ? 0 : -ETIMEDOUT; } -static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter, - struct i2c_msg *msgs, - int num) +static int sh_mobile_xfer(struct sh_mobile_i2c_data *pd, + struct i2c_msg *msgs, int num) { - struct sh_mobile_i2c_data *pd = i2c_get_adapdata(adapter); struct i2c_msg *msg; int err = 0; int i; - long timeout; + long time_left; /* Wake up device and enable clock */ pm_runtime_get_sync(pd->dev); @@ -662,15 +666,35 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter, if (do_start) i2c_op(pd, OP_START); - /* The interrupt handler takes care of the rest... */ - timeout = wait_event_timeout(pd->wait, - pd->sr & (ICSR_TACK | SW_DONE), - adapter->timeout); + if (pd->atomic_xfer) { + unsigned long j = jiffies + pd->adap.timeout; + + time_left = time_before_eq(jiffies, j); + while (time_left && + !(pd->sr & (ICSR_TACK | SW_DONE))) { + unsigned char sr = iic_rd(pd, ICSR); + + if (sr & (ICSR_AL | ICSR_TACK | + ICSR_WAIT | ICSR_DTE)) { + sh_mobile_i2c_isr(0, pd); + udelay(150); + } else { + cpu_relax(); + } + } + } else { + /* The interrupt handler takes care of the rest... */ + time_left = wait_event_timeout(pd->wait, + pd->sr & (ICSR_TACK | SW_DONE), + pd->adap.timeout); + + /* 'stop_after_dma' tells if DMA xfer was complete */ + i2c_put_dma_safe_msg_buf(pd->dma_buf, pd->msg, + pd->stop_after_dma); - /* 'stop_after_dma' tells if DMA transfer was complete */ - i2c_put_dma_safe_msg_buf(pd->dma_buf, pd->msg, pd->stop_after_dma); + } - if (!timeout) { + if (!time_left) { dev_err(pd->dev, "Transfer request timed out\n"); if (pd->dma_direction != DMA_NONE) sh_mobile_i2c_cleanup_dma(pd); @@ -696,14 +720,35 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter, return err ?: num; } +static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter, + struct i2c_msg *msgs, + int num) +{ + struct sh_mobile_i2c_data *pd = i2c_get_adapdata(adapter); + + pd->atomic_xfer = false; + return sh_mobile_xfer(pd, msgs, num); +} + +static int sh_mobile_i2c_xfer_atomic(struct i2c_adapter *adapter, + struct i2c_msg *msgs, + int num) +{ + struct sh_mobile_i2c_data *pd = i2c_get_adapdata(adapter); + + pd->atomic_xfer = true; + return sh_mobile_xfer(pd, msgs, num); +} + static u32 sh_mobile_i2c_func(struct i2c_adapter *adapter) { return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_PROTOCOL_MANGLING; } static const struct i2c_algorithm sh_mobile_i2c_algorithm = { - .functionality = sh_mobile_i2c_func, - .master_xfer = sh_mobile_i2c_xfer, + .functionality = sh_mobile_i2c_func, + .master_xfer = sh_mobile_i2c_xfer, + .master_xfer_atomic = sh_mobile_i2c_xfer_atomic, }; static const struct i2c_adapter_quirks sh_mobile_i2c_quirks = {