lkml.org 
[lkml]   [2020]   [Feb]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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@megous.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@megous.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@megous.com>
    > >
    > > Acked-by: Chen-Yu Tsai <wens@csie.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@googlegroups.com.
    > To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20200219121424.dfvrwfcavupmwbvw%40core.my.home.

    \
     
     \ /
      Last update: 2020-02-21 04:05    [W:8.075 / U:0.092 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site