Hi Uli,
On Thu, Jun 18, 2020 at 5:05 PM Ulrich Hecht <uli+renesas@xxxxxxxx> wrote:
> Implements atomic transfers to fix reboot/shutdown on r8a7790 Lager and
> similar boards.
>
> Signed-off-by: Ulrich Hecht <uli+renesas@xxxxxxxx>
> Tested-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> Tested-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
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@xxxxxxxxxxxxxx
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
