| Message ID | 1437396110-5192-1-git-send-email-henryc.chen@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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
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.
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
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
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.
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); } }
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(-)