regmap: Add function check before called format_val
diff mbox

Message ID 1437396110-5192-1-git-send-email-henryc.chen@mediatek.com
State New
Headers show

Commit Message

Henry Chen July 20, 2015, 12:41 p.m. UTC
The regmap_format will not be initialize since regmap_bus is not assgined 
on regmap_init(). It should has a function check before using 
format_val() to avoid null function called on regmap_bulk_read().

Signed-off-by: Henry Chen <henryc.chen@mediatek.com>
---
Based on v4.2rc1
---
 drivers/base/regmap/regmap.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Mark Brown July 20, 2015, 3:02 p.m. UTC | #1
On Mon, Jul 20, 2015 at 08:41:50PM +0800, Henry Chen wrote:
> The regmap_format will not be initialize since regmap_bus is not assgined 
> on regmap_init(). It should has a function check before using 
> format_val() to avoid null function called on regmap_bulk_read().

> -			map->format.format_val(val + (i * val_bytes), ival, 0);
> +			if (map->format.format_val)
> +				map->format.format_val(val + (i * val_bytes), ival, 0);
> +			else
> +				memcpy(val + (i * val_bytes), &ival, val_bytes);

Your changelog doesn't explan why we are in this code path in the first
place without a format_val() and why a memcpy() is an appropriate
replacement.  It should, it's not clear to me that this is a good fix
but I don't feel I fully understand the problem.
Henry Chen July 21, 2015, 6:07 a.m. UTC | #2
On Mon, 2015-07-20 at 16:02 +0100, Mark Brown wrote:
> On Mon, Jul 20, 2015 at 08:41:50PM +0800, Henry Chen wrote:
> > The regmap_format will not be initialize since regmap_bus is not assgined 
> > on regmap_init(). It should has a function check before using 
> > format_val() to avoid null function called on regmap_bulk_read().
> 
> > -			map->format.format_val(val + (i * val_bytes), ival, 0);
> > +			if (map->format.format_val)
> > +				map->format.format_val(val + (i * val_bytes), ival, 0);
> > +			else
> > +				memcpy(val + (i * val_bytes), &ival, val_bytes);
> 
> Your changelog doesn't explan why we are in this code path in the first
> place without a format_val() and why a memcpy() is an appropriate
> replacement.  It should, it's not clear to me that this is a good fix
> but I don't feel I fully understand the problem.

Sorry for being unclear for issue, the call flow as following,

First, in drivers/mfd/mtk_pmic_wrap.c which registered regmap without
rebmap_bus.
devm_regmap_init(wrp->dev, NULL, wrp, &pwrap_regmap_config);

It call to regmap_init() and go to "skip_format_initialization" because
regmap_bus didn't assign by driver.

if (!bus) {
	map->reg_read  = config->reg_read;
	map->reg_write = config->reg_write;

	map->defer_caching = false;
	goto skip_format_initialization;" 

Then in driver rtc-mt6397.c, it used regmap_bulk_read() to get the time
of PMIC, and hit the null function of format_val(), because the
regmap_bus was null.

It skipped the initialization of format_val() because bus == null, but
called the format_val() at regmap_bulk_read() if bus == null.

Maybe it was not the good fix for this, but should be a problem need to
be reported, or should I need to give the regmap_bus on mtk_pmic_wrap.c?

I tested this on mediatek mt8173 evb platform.
Please see the error below, thanks.

Bad mode in Synchronous Abort handler detected, code 0x86000005 -- IABT
(current EL)
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.2.0-rc1+ #25
Hardware name: MediaTek MT8173 evaluation board (DT)
task: ffffffc077090000 ti: ffffffc07705c000 task.ti: ffffffc07705c000
PC is at 0x0
LR is at regmap_bulk_read+0x104/0x1c4
pc : [<0000000000000000>] lr : [<ffffffc00040efec>] pstate: 20000045
sp : ffffffc07705fa00
x29: ffffffc07705fa00 x28: ffffffc0008ac970
x27: ffffffc0009530f8 x26: 0000000000000001
x25: 0000000000000001 x24: 000000000000e00a
x23: ffffffc07705faa0 x22: 0000000000000002
x21: 0000000000000007 x20: ffffffc075ca8800
x19: 0000000000000001 x18: 0000000000000000
x17: 0000000000000001 x16: 0000000000000016
x15: 0000000000000cb0 x14: 0ffffffffffffffe
x13: 0000000000000010 x12: 0000000000000001
x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
x9 : 6a6473606863646c x8 : 00000000ffffffd0
x7 : 0000000000000000 x6 : 0000000000000000
x5 : 00000000ffffffff x4 : 0000000000000000
x3 : 0000000000000000 x2 : 0000000000000000
x1 : 000000000000001c x0 : ffffffc07705faa0

Internal error: Oops - bad mode: 0 [#1] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.2.0-rc1+ #25
Hardware name: MediaTek MT8173 evaluation board (DT)
task: ffffffc077090000 ti: ffffffc07705c000 task.ti: ffffffc07705c000
PC is at 0x0
LR is at regmap_bulk_read+0x104/0x1c4
pc : [<0000000000000000>] lr : [<ffffffc00040efec>] pstate: 20000045
sp : ffffffc07705fa00
x29: ffffffc07705fa00 x28: ffffffc0008ac970
x27: ffffffc0009530f8 x26: 0000000000000001
x25: 0000000000000001 x24: 000000000000e00a
x23: ffffffc07705faa0 x22: 0000000000000002
x21: 0000000000000007 x20: ffffffc075ca8800
x19: 0000000000000001 x18: 0000000000000000
x17: 0000000000000001 x16: 0000000000000016
x15: 0000000000000cb0 x14: 0ffffffffffffffe
x13: 0000000000000010 x12: 0000000000000001
x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
x9 : 6a6473606863646c x8 : 00000000ffffffd0
x7 : 0000000000000000 x6 : 0000000000000000
x5 : 00000000ffffffff x4 : 0000000000000000
x3 : 0000000000000000 x2 : 0000000000000000
x1 : 000000000000001c x0 : ffffffc07705faa0

Process swapper/0 (pid: 1, stack limit = 0xffffffc07705c020)
Stack: (0xffffffc07705fa00 to 0xffffffc077060000)
fa00: 7705fa60 ffffffc0 004cbde4 ffffffc0 7705fb40 ffffffc0 762ead98
ffffffc0
fa20: 7705fb40 ffffffc0 762eada8 ffffffc0 7705fbf8 ffffffc0 007ca368
ffffffc0
fa40: 00908810 ffffffc0 004cbdcc ffffffc0 7705fb40 ffffffc0 0033a998
0000001c
fa60: 7705fab0 ffffffc0 004c962c ffffffc0 7705fb40 ffffffc0 75c5aac8
ffffffc0
fa80: 7705fb40 ffffffc0 75c5aaac ffffffc0 75c5a810 ffffffc0 762ec800
ffffffc0
faa0: 00000000 00000000 75c5aaac ffffffc0 7705fad0 ffffffc0 004c969c
ffffffc0
fac0: 75c5a800 ffffffc0 004c9688 ffffffc0 7705fb00 ffffffc0 004c9e78
ffffffc0
fae0: 75c5a800 ffffffc0 75ccf010 ffffffc0 75c5a800 ffffffc0 7705fb90
ffffffc0
fb00: 7705fb90 ffffffc0 004c8d40 ffffffc0 75c5a800 ffffffc0 75ccf010
ffffffc0
fb20: 00000000 00000000 75c5aaac ffffffc0 00953000 ffffffc0 007ca368
ffffffc0
fb40: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
fb60: 00000000 00000000 00006374 00000000 ffffffff ffffffff 80800000
80808080
fb80: 75c5aab6 ffffffc0 feff6273 fefefefe 7705fc20 ffffffc0 004cbf74
ffffffc0
fba0: 762ead98 ffffffc0 75ccf010 ffffffc0 00000000 00000000 007ca368
ffffffc0
fbc0: 008f9000 ffffffc0 00000000 00000000 00842264 ffffffc0 008766b0
ffffffc0
fbe0: 008766e0 ffffffc0 75ccf010 ffffffc0 75ccf000 ffffffc0 007ca368
ffffffc0
fc00: 008f9000 ffffffc0 00000000 00000000 00842264 ffffffc0 008766b0
ffffffc0
fc20: 7705fc50 ffffffc0 003fb5e4 ffffffc0 75ccf010 ffffffc0 00908760
ffffffc0
fc40: 00908788 ffffffc0 00950000 ffffffc0 7705fc80 ffffffc0 003f99e4
ffffffc0
fc60: 75ccf010 ffffffc0 00000000 00000000 00908788 ffffffc0 00950000
ffffffc0
fc80: 7705fcc0 ffffffc0 003f9b90 ffffffc0 75ccf010 ffffffc0 00908788
ffffffc0
fca0: 75ccf070 ffffffc0 008f9ac0 ffffffc0 008f9000 ffffffc0 00876740
ffffffc0
fcc0: 7705fcf0 ffffffc0 003f7ce4 ffffffc0 00000000 00000000 00908788
ffffffc0
fce0: 003f9af4 ffffffc0 00876740 ffffffc0 7705fd30 ffffffc0 003f94cc
ffffffc0
fd00: 00908788 ffffffc0 76a3e3c0 ffffffc0 00000000 00000000 005f2380
ffffffc0
fd20: 77005ea8 ffffffc0 7725ade8 ffffffc0 7705fd40 ffffffc0 003f9168
ffffffc0
fd40: 7705fd80 ffffffc0 003fa460 ffffffc0 00908788 ffffffc0 008c8a60
ffffffc0
fd60: 00000000 00000000 00863990 ffffffc0 00000000 00000000 ffffffd0
00000000
fd80: 7705fdb0 ffffffc0 003fb518 ffffffc0 008c8a60 ffffffc0 008c8a60
ffffffc0
fda0: 762ec4c0 ffffffc0 00863990 ffffffc0 7705fdc0 ffffffc0 008639a8
ffffffc0
fdc0: 7705fdd0 ffffffc0 00082868 ffffffc0 7705fe50 ffffffc0 00842b14
ffffffc0
fde0: 000000bd 00000000 00930000 ffffffc0 008393d8 ffffffc0 00000006
00000000
fe00: 00930000 ffffffc0 008766b0 ffffffc0 00876600 ffffffc0 00000030
00000000
fe20: 7705fe30 ffffffc0 00793498 ffffffc0 00792d88 ffffffc0 00000006
00000006
fe40: 00000000 00000000 7e9fdbac ffffffc0 7705feb0 ffffffc0 005f43e0
ffffffc0
fe60: 005f43d0 ffffffc0 00000000 00000000 00000000 00000000 00000000
00000000
fe80: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
fea0: 00000000 00000000 00000000 00000000 00000000 00000000 00085c10
ffffffc0
fec0: 005f43d0 ffffffc0 00000000 00000000 00000000 00000000 00000000
00000000
fee0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
ff00: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
ff20: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
ff40: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
ff60: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
ff80: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
ffa0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000005
00000000
ffe0: 00000000 00000000 00000000 00000000 0c2ae48d c2569cad c4144163
8cef60fe
Call trace:
[<          (null)>]           (null)
[<ffffffc0004cbde0>] mtk_rtc_read_time+0x9c/0x134
[<ffffffc0004c9628>] __rtc_read_time.isra.3+0x40/0x7c
[<ffffffc0004c9698>] rtc_read_time+0x34/0x58
[<ffffffc0004c9e74>] __rtc_read_alarm+0x20/0x37c
[<ffffffc0004c8d3c>] rtc_device_register+0x194/0x2e0
[<ffffffc0004cbf70>] mtk_rtc_probe+0xf8/0x18c
[<ffffffc0003fb5e0>] platform_drv_probe+0x48/0xc4
[<ffffffc0003f99e0>] driver_probe_device+0x188/0x29c
[<ffffffc0003f9b8c>] __driver_attach+0x98/0xa0
[<ffffffc0003f7ce0>] bus_for_each_dev+0x54/0x98
[<ffffffc0003f94c8>] driver_attach+0x1c/0x28
[<ffffffc0003f9164>] bus_add_driver+0x1c0/0x228
[<ffffffc0003fa45c>] driver_register+0x64/0x130
[<ffffffc0003fb514>] __platform_driver_register+0x5c/0x68
[<ffffffc0008639a4>] mtk_rtc_driver_init+0x14/0x20
[<ffffffc000082864>] do_one_initcall+0x88/0x1ac
[<ffffffc000842b10>] kernel_init_freeable+0x158/0x1fc
[<ffffffc0005f43dc>] kernel_init+0xc/0xd8
Mark Brown July 21, 2015, 5:25 p.m. UTC | #3
On Tue, Jul 21, 2015 at 02:07:25PM +0800, Henry Chen wrote:

> Then in driver rtc-mt6397.c, it used regmap_bulk_read() to get the time
> of PMIC, and hit the null function of format_val(), because the
> regmap_bus was null.

> It skipped the initialization of format_val() because bus == null, but
> called the format_val() at regmap_bulk_read() if bus == null.

OK, so the issue here is that when we fall back to regmap_read() we may
do so because we have reg_read() and reg_write() functions which in turn
imply no formatting.  The expectation here is that val must be an array
of int.  The code doesn't completely take that into account though and
the user you're pointing at is assuming it's an array of 16 bit values
which isn't totally unreasonable if it did specify val_bits (we don't
check for that).

> Maybe it was not the good fix for this, but should be a problem need to
> be reported, or should I need to give the regmap_bus on mtk_pmic_wrap.c?

That file isn't in mainline...

memcpy() is definitely not a safe way to move from an unsigned int to a
u16 which is what your specific use case is trying to do.  I'll need to
do an audit of existing users (or someone else will!) to figure out what
people are doing with .val_bits in drivers using reg_read() and
reg_write() but I think what we should be doing here is probably
providing appropriate conversion functions based on val_bits on init.
Henry Chen July 22, 2015, 2:31 p.m. UTC | #4
On Tue, 2015-07-21 at 18:25 +0100, Mark Brown wrote:
> On Tue, Jul 21, 2015 at 02:07:25PM +0800, Henry Chen wrote:
> 
> > Then in driver rtc-mt6397.c, it used regmap_bulk_read() to get the time
> > of PMIC, and hit the null function of format_val(), because the
> > regmap_bus was null.
> 
> > It skipped the initialization of format_val() because bus == null, but
> > called the format_val() at regmap_bulk_read() if bus == null.
> 
> OK, so the issue here is that when we fall back to regmap_read() we may
> do so because we have reg_read() and reg_write() functions which in turn
> imply no formatting.  The expectation here is that val must be an array
> of int.  The code doesn't completely take that into account though and
> the user you're pointing at is assuming it's an array of 16 bit values
> which isn't totally unreasonable if it did specify val_bits (we don't
> check for that).
So, could I call regmap_bulk_read() on rtc-mt6307.c, should I need to
change it ?
> 
> > Maybe it was not the good fix for this, but should be a problem need to
> > be reported, or should I need to give the regmap_bus on mtk_pmic_wrap.c?
> 
> That file isn't in mainline...

oh...it's mtk-pmic-wrap.c, sorry about that.
> 
> memcpy() is definitely not a safe way to move from an unsigned int to a
> u16 which is what your specific use case is trying to do.  I'll need to
> do an audit of existing users (or someone else will!) to figure out what
> people are doing with .val_bits in drivers using reg_read() and
> reg_write() but I think what we should be doing here is probably
> providing appropriate conversion functions based on val_bits on init.

Ok, got it, memcpy() should not be used here anymore.

Thanks,
Henry
Mark Brown July 22, 2015, 5 p.m. UTC | #5
On Wed, Jul 22, 2015 at 10:31:34PM +0800, Henry Chen wrote:
> On Tue, 2015-07-21 at 18:25 +0100, Mark Brown wrote:

> > OK, so the issue here is that when we fall back to regmap_read() we may
> > do so because we have reg_read() and reg_write() functions which in turn
> > imply no formatting.  The expectation here is that val must be an array
> > of int.  The code doesn't completely take that into account though and
> > the user you're pointing at is assuming it's an array of 16 bit values
> > which isn't totally unreasonable if it did specify val_bits (we don't
> > check for that).

> So, could I call regmap_bulk_read() on rtc-mt6307.c, should I need to
> change it ?

It should be fine but you may need to change to pass an array of
unsigned int instead of an array of u16 in.

> > > Maybe it was not the good fix for this, but should be a problem need to
> > > be reported, or should I need to give the regmap_bus on mtk_pmic_wrap.c?

> > That file isn't in mainline...

> oh...it's mtk-pmic-wrap.c, sorry about that.

Found it - thanks.

> > memcpy() is definitely not a safe way to move from an unsigned int to a
> > u16 which is what your specific use case is trying to do.  I'll need to
> > do an audit of existing users (or someone else will!) to figure out what
> > people are doing with .val_bits in drivers using reg_read() and
> > reg_write() but I think what we should be doing here is probably
> > providing appropriate conversion functions based on val_bits on init.

> Ok, got it, memcpy() should not be used here anymore.

Right.  We just need to do a survey of existing users and figure out
what the least disruptive format function to provide is.  That way we
don't have to special case other code that uses formatting.
Daniel Kurtz Aug. 12, 2015, 2:20 p.m. UTC | #6
Hi Henry & Mark,

On Tue, Jul 21, 2015 at 2:07 PM, Henry Chen <HenryC.Chen@mediatek.com> wrote:
> On Mon, 2015-07-20 at 16:02 +0100, Mark Brown wrote:
>> On Mon, Jul 20, 2015 at 08:41:50PM +0800, Henry Chen wrote:
>> > The regmap_format will not be initialize since regmap_bus is not assgined
>> > on regmap_init(). It should has a function check before using
>> > format_val() to avoid null function called on regmap_bulk_read().
>>
>> > -                   map->format.format_val(val + (i * val_bytes), ival, 0);
>> > +                   if (map->format.format_val)
>> > +                           map->format.format_val(val + (i * val_bytes), ival, 0);
>> > +                   else
>> > +                           memcpy(val + (i * val_bytes), &ival, val_bytes);
>>
>> Your changelog doesn't explan why we are in this code path in the first
>> place without a format_val() and why a memcpy() is an appropriate
>> replacement.  It should, it's not clear to me that this is a good fix
>> but I don't feel I fully understand the problem.
>
> Sorry for being unclear for issue, the call flow as following,
>
> First, in drivers/mfd/mtk_pmic_wrap.c which registered regmap without
> rebmap_bus.
> devm_regmap_init(wrp->dev, NULL, wrp, &pwrap_regmap_config);
>
> It call to regmap_init() and go to "skip_format_initialization" because
> regmap_bus didn't assign by driver.
>
> if (!bus) {
>         map->reg_read  = config->reg_read;
>         map->reg_write = config->reg_write;
>
>         map->defer_caching = false;
>         goto skip_format_initialization;"
>
> Then in driver rtc-mt6397.c, it used regmap_bulk_read() to get the time
> of PMIC, and hit the null function of format_val(), because the
> regmap_bus was null.
>
> It skipped the initialization of format_val() because bus == null, but
> called the format_val() at regmap_bulk_read() if bus == null.
>
> Maybe it was not the good fix for this, but should be a problem need to
> be reported, or should I need to give the regmap_bus on mtk_pmic_wrap.c?

I ran into this bug when testing Matthias' v4.2-next/for-next branch
on mt8173.  It now crashes on boot.

Since I didn't see it elsewhere in this discussion, I'll point out
that the "regression" here was introduced by commit [0], which added
the call to map->format.format_val from regmap_bulk_read() when
map->bus == NULL.

[0] commit 15b8d2c41fe5839582029f65c5f7004db451cc2b
  Author: Arun Chandran <achandran@mvista.com>
  regmap: Fix regmap_bulk_read in BE mode

Perhaps the easiest work around to unbreak v4.2 is, as Henry mentions,
for mtk_pmic_wrap to define its own regmap_bus, with .read() &
.write() handlers.  This way they will inherit the default built-in
format_val() from the regmap core.

Making mtk_pmic-wrap into a regmap_bus makes a bit of sense
architecturally, too, since it is essentially just a bus for accessing
the registers of an off-chip PMIC.  The CPU sees a platform bus, but
the registers of the remote PMIC are accessed over a dedicated SPI
bus.

WDYT?

Henry, can you try to implement this?

Thanks,
-Dan
Henry Chen Aug. 13, 2015, 3:25 p.m. UTC | #7
On Wed, 2015-08-12 at 22:20 +0800, Daniel Kurtz wrote:

> 
> Since I didn't see it elsewhere in this discussion, I'll point out
> that the "regression" here was introduced by commit [0], which added
> the call to map->format.format_val from regmap_bulk_read() when
> map->bus == NULL.
> 
> [0] commit 15b8d2c41fe5839582029f65c5f7004db451cc2b
>   Author: Arun Chandran <achandran@mvista.com>
>   regmap: Fix regmap_bulk_read in BE mode
> 
> Perhaps the easiest work around to unbreak v4.2 is, as Henry mentions,
> for mtk_pmic_wrap to define its own regmap_bus, with .read() &
> .write() handlers.  This way they will inherit the default built-in
> format_val() from the regmap core.
> 
> Making mtk_pmic-wrap into a regmap_bus makes a bit of sense
> architecturally, too, since it is essentially just a bus for accessing
> the registers of an off-chip PMIC.  The CPU sees a platform bus, but
> the registers of the remote PMIC are accessed over a dedicated SPI
> bus.
> 
> WDYT?
> 
> Henry, can you try to implement this?

Hi Daniel,
I can try to create a regmap_bus for pmic wrap. But I'm not sure if it
was the good solution for this problem.

Hi Mark,
Sorry, I'm afraid that I cannot do this right on init as you said last
time. What do you think about regmap_bus, can you accept that way?

Thanks,
Henry
> 
> Thanks,
> -Dan
Mark Brown Aug. 14, 2015, 5:56 p.m. UTC | #8
On Thu, Aug 13, 2015 at 11:25:05PM +0800, Henry Chen wrote:
> On Wed, 2015-08-12 at 22:20 +0800, Daniel Kurtz wrote:

> > Making mtk_pmic-wrap into a regmap_bus makes a bit of sense
> > architecturally, too, since it is essentially just a bus for accessing
> > the registers of an off-chip PMIC.  The CPU sees a platform bus, but
> > the registers of the remote PMIC are accessed over a dedicated SPI
> > bus.

> Sorry, I'm afraid that I cannot do this right on init as you said last
> time. What do you think about regmap_bus, can you accept that way?

I don't have enough context from the above to understand what's going
on, sorry.  Implementing a bus for something that isn't actually a Linux
bus doesn't seem like the best idea though and in general what I'm
seeing of the discussion sounds like you're hacking around in driver
code to bodge around problems that are being seen rather than really
addressing things, the code I can see certainly looks like it just wants
to be implementing read and write operations for a single MFD.

Patch
diff mbox

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 7111d04..c1e8c32 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -2317,7 +2317,10 @@  int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
 					  &ival);
 			if (ret != 0)
 				return ret;
-			map->format.format_val(val + (i * val_bytes), ival, 0);
+			if (map->format.format_val)
+				map->format.format_val(val + (i * val_bytes), ival, 0);
+			else
+				memcpy(val + (i * val_bytes), &ival, val_bytes);
 		}
 	}