Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1188177 > unrolled thread

[PATCH] regmap: Add function check before called format_val

Back to article view | Back to linux.kernel

Contents

  [PATCH] regmap: Add function check before called format_val Henry Chen <henryc.chen@mediatek.com> - 2015-07-20 14:50 +0200
    Re: [PATCH] regmap: Add function check before called format_val Henry Chen <HenryC.Chen@mediatek.com> - 2015-07-21 08:10 +0200
      Re: [PATCH] regmap: Add function check before called format_val Henry Chen <HenryC.Chen@mediatek.com> - 2015-07-22 16:40 +0200
        Re: [PATCH] regmap: Add function check before called format_val Mark Brown <broonie@kernel.org> - 2015-07-22 19:10 +0200

#1188177 — [PATCH] regmap: Add function check before called format_val

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

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);
 		}
 	}
 
-- 
1.8.1.1.dirty

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1188688

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1190020

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1190119

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web