Hi Wolfram,
On Thu, Jun 18, 2020 at 6:39 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> 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>
> > --- 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
--
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
