Message-ID: <CAGb2v64+C1Q8au_cR0PjY=0GJx1c+kk7Tw0=12+fFEhBF_Q-Rg@mail.gmail.com>
Date:   Fri, 21 Feb 2020 11:04:14 +0800
From:   Chen-Yu Tsai <wens@...e.org>
To:     Ondřej Jirman <megous@...ous.com>,
        Chen-Yu Tsai <wens@...e.org>,
        linux-sunxi <linux-sunxi@...glegroups.com>,
        Maxime Ripard <mripard@...nel.org>,
        Samuel Holland <samuel@...lland.org>,
        Stephen Boyd <swboyd@...omium.org>,
        "moderated list:ARM/Allwinner sunXi SoC support" 
        <linux-arm-kernel@...ts.infradead.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [linux-sunxi] Re: [PATCH] bus: sunxi-rsb: Return correct data
 when mixing 16-bit and 8-bit reads

On Wed, Feb 19, 2020 at 8:14 PM Ondřej Jirman <megous@...ous.com> wrote:
>
> On Wed, Feb 19, 2020 at 10:49:18AM +0800, Chen-Yu Tsai wrote:
> > On Wed, Feb 19, 2020 at 9:10 AM Ondrej Jirman <megous@...ous.com> wrote:
> > >
> > > When doing a 16-bit read that returns data in the MSB byte, the
> > > RSB_DATA register will keep the MSB byte unchanged when doing
> > > the following 8-bit read. sunxi_rsb_read() will then return
> > > a result that contains high byte from 16-bit read mixed with
> > > the 8-bit result.
> > >
> > > The consequence is that after this happens the PMIC's regmap will
> > > look like this: (0x33 is the high byte from the 16-bit read)
> > >
> > > % cat /sys/kernel/debug/regmap/sunxi-rsb-3a3/registers
> > > 00: 33
> > > 01: 33
> > > 02: 33
> > > 03: 33
> > > 04: 33
> > > 05: 33
> > > 06: 33
> > > 07: 33
> > > 08: 33
> > > 09: 33
> > > 0a: 33
> > > 0b: 33
> > > 0c: 33
> > > 0d: 33
> > > 0e: 33
> > > [snip]
> > >
> > > Fix this by masking the result of the read with the correct mask
> > > based on the size of the read. There are no 16-bit users in the
> > > mainline kernel, so this doesn't need to get into the stable tree.
> > >
> > > Signed-off-by: Ondrej Jirman <megous@...ous.com>
> >
> > Acked-by: Chen-Yu Tsai <wens@...e.org>
> >
> > for the fix, however it's not entirely clear to me how the MSB 0x33
> > value got into the regmap. Looks like I expected the regmap core to
> > handle any overflows, or didn't expect the lingering MSB from 16-bit
> > reads, as I didn't have any 16-bit register devices back when I wrote
> > this.
>
> Now I feel like I masked some other bug. Though explanation may be quite simple.
>
> For example when AXP core does regmap_read on some values for showing charging
> current or battery voltage, because regmap_read works with unsigned int, it
> will simply get a number that's too big. And that was the major symptom
> I observed. I got readings from sysfs that my tablet is consuming 600A or 200A,
> etc. And this value was jumping around based on AC100 activity (as the MSB
> byte got changed).
>
> In other places where the driver does regmap_update_bits, I think nothing bad
> happened. The write after the read would simply discard the MSB byte.
>
> And for the debugfs/regmap/*/registers, those are printed such:
>
> https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap-debugfs.c#L256
>
>         snprintf(buf + buf_pos, count - buf_pos,
>                 "%.*x", map->debugfs_val_len, val);
>
> This doesn't truncate values, so the larger number gets printed to (for example):
>
>         33fe
>
> But regmap debugfs code truncates this by cutting off the formatted string to
> this length:
>
>   https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap-debugfs.c#L189
>
> So in the end, only:
>
>         00: 33
>
> remains, instead of the correct value of:
>
>         00: fe
>
> So in the case of debugfs this is just a cosmetic/formatting issue, that didn't
> affect anything else.
>
> I think regmap_bus->reg_read API simply expects the returned value to not exceed
> the sensible range.

Sounds good. Thanks for digging through this. My ack still stands.

ChenYu

> regards,
>         o.
>
>
> > ChenYu
> >
> >
> > > ---
> > >  drivers/bus/sunxi-rsb.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
> > > index b8043b58568ac..8ab6a3865f569 100644
> > > --- a/drivers/bus/sunxi-rsb.c
> > > +++ b/drivers/bus/sunxi-rsb.c
> > > @@ -316,6 +316,7 @@ static int sunxi_rsb_read(struct sunxi_rsb *rsb, u8 rtaddr, u8 addr,
> > >  {
> > >         u32 cmd;
> > >         int ret;
> > > +       u32 mask;
> > >
> > >         if (!buf)
> > >                 return -EINVAL;
> > > @@ -323,12 +324,15 @@ static int sunxi_rsb_read(struct sunxi_rsb *rsb, u8 rtaddr, u8 addr,
> > >         switch (len) {
> > >         case 1:
> > >                 cmd = RSB_CMD_RD8;
> > > +               mask = 0xffu;
> > >                 break;
> > >         case 2:
> > >                 cmd = RSB_CMD_RD16;
> > > +               mask = 0xffffu;
> > >                 break;
> > >         case 4:
> > >                 cmd = RSB_CMD_RD32;
> > > +               mask = 0xffffffffu;
> > >                 break;
> > >         default:
> > >                 dev_err(rsb->dev, "Invalid access width: %zd\n", len);
> > > @@ -345,7 +349,7 @@ static int sunxi_rsb_read(struct sunxi_rsb *rsb, u8 rtaddr, u8 addr,
> > >         if (ret)
> > >                 goto unlock;
> > >
> > > -       *buf = readl(rsb->regs + RSB_DATA);
> > > +       *buf = readl(rsb->regs + RSB_DATA) & mask;
> > >
> > >  unlock:
> > >         mutex_unlock(&rsb->lock);
> > > --
> > > 2.25.1
> > >
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@...glegroups.com.
> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20200219121424.dfvrwfcavupmwbvw%40core.my.home.

Powered by blists - more mailing lists