[v2] i2c: sh_mobile: implement atomic transfers

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>
---

Hi!

This revision drops disabling runtime PM as it is unnecessary and implements
a couple of style and logic tweaks. Thanks to Geert and Wolfram for the review.
  
CU
Uli
   
   
Changes since v1:
- don't disable runtime PM operations for atomic transfers
- rename xfer() to sh_mobile_xfer()
- rename timeout to time_left in sh_mobile_xfer() and simplify logic
- minor style tweaks
- rebase
- add Tested-by's


 drivers/i2c/busses/i2c-sh_mobile.c | 95 ++++++++++++++++++++++--------
 1 file changed, 70 insertions(+), 25 deletions(-)

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