On Fri, Sep 19, 2014 at 11:38:03AM +0800, Xiubo Li-B47053 wrote: > > > Thanks for testing. In that case I will post this change, as I feel this > > > should be > > > fixed irrespective of my syscon patch. > > > > > > > But as Xiubo pointed in another mail, it may still cause other issues. > > > > Looking at regmap.c, there're still some other places using the device > > > pointer, e.g. > > > > dev_xxx debug information and some tracepoints also take device pointer as > > > > parameter(not sure if it will break if dev is NULL). > > > > Another thing is that if dev is NULL, we may not be able to use regmap > > > debugfs > > > > feature which seems also not as our expected. > > > > > > > > > > I would have preferred to check dev for NULL, as it's only at two places and > > > we could > > > still have debug prints for NULL dev, as normal pr_info instead of dev_info. > > > > > > But Xiubo also pointed out that his patch [1] which updates syscon binding > > > information > > > will be useless if we pass NULL dev in regmap_init_mmio, which he posted > > > today, > > > and it requires dev pointer in "regmap_get_val_endian" function to read DT > > > property. > > > > > > [1]: [PATCH] mfd: syscon: binding: Add syscon endianness support > > > https://lkml.org/lkml/2014/9/18/67 > > > > > > So instead of adding dummy device or creating device structure, I would > > > prefer to get actual > > > device pointer corresponding to "np" passed in of_syscon_register function > > > as shown below: > > > > > > > I wonder this may not work at early stage before the devices are populated > > from device tree. > > My initial understanding that one important thing for your patch is > > to address this issue, isn't it? > > Many people have asked for this feature before. > > > > My boot log: > > ... > of_platform_bus_create() - skipping /chosen, no compatible prop > of_platform_bus_create() - skipping /aliases, no compatible prop > of_platform_bus_create() - skipping /memory, no compatible prop > of_platform_bus_create() - skipping /cpus, no compatible prop > create child: /soc/smmu@1200000 > create child: /soc/smmu@1280000 > create child: /soc/smmu@1300000 > create child: /soc/smmu@1380000 > create child: /soc/interrupt-controller@1400000 > create child: /soc/tzasc@1500000 > of_platform_bus_create() - skipping /soc/tzasc@1500000, no compatible prop > create child: /soc/ifc@1530000 > create child: /soc/ifc@1530000/nor@0,0 > create child: /soc/ifc@1530000/board-control@2,0 > create child: /soc/dcfg@1ee0000 > syscon 1ee0000.dcfg: regmap [mem 0x01ee0000-0x01eeffff] registered > create child: /soc/quadspi@1550000 > create child: /soc/esdhc@1560000 > create child: /soc/scfg@1570000 > create child: /soc/crypto@1700000 > create child: /soc/sfp@1e80000 > of_platform_bus_create() - skipping /soc/sfp@1e80000, no compatible prop > create child: /soc/snvs@1e90000 > of_platform_bus_create() - skipping /soc/snvs@1e90000, no compatible prop > create child: /soc/serdes1@1ea0000 > of_platform_bus_create() - skipping /soc/serdes1@1ea0000, no compatible prop > create child: /soc/clocking@1ee1000 > create child: /soc/rcpm@1ee2000 > create child: /soc/dspi@2100000 > create child: /soc/dspi@2110000 > create child: /soc/i2c@2180000 > create child: /soc/i2c@2190000 > create child: /soc/i2c@21a0000 > create child: /soc/serial@21c0500 > create child: /soc/serial@21c0600 > create child: /soc/serial@21d0500 > create child: /soc/serial@21d0600 > create child: /soc/gpio@2300000 > create child: /soc/gpio@2310000 > create child: /soc/gpio@2320000 > create child: /soc/gpio@2330000 > create child: /soc/uqe@2400000 > create child: /soc/uqe@2400000/qeic@80 > create child: /soc/uqe@2400000/ucc@2000 > irq: no irq domain found for /soc/uqe@2400000/qeic@80 ! > create child: /soc/uqe@2400000/ucc@2200 > irq: no irq domain found for /soc/uqe@2400000/qeic@80 ! > create child: /soc/uqe@2400000/muram@10000 > create child: /soc/uqe@2400000/si@700 > create child: /soc/uqe@2400000/siram@1000 > create child: /soc/serial@2950000 > create child: /soc/serial@2960000 > create child: /soc/serial@2970000 > create child: /soc/serial@2980000 > create child: /soc/serial@2990000 > create child: /soc/serial@29a0000 > create child: /soc/ftm0_1@29d0000 > create child: /soc/ftm@29f0000 > of_platform_bus_create() - skipping /soc/ftm@29f0000, no compatible prop > create child: /soc/ftm@2a00000 > create child: /soc/ftm@2a10000 > of_platform_bus_create() - skipping /soc/ftm@2a10000, no compatible prop > create child: /soc/ftm@2a20000 > of_platform_bus_create() - skipping /soc/ftm@2a20000, no compatible prop > create child: /soc/ftm@2a30000 > create child: /soc/ftm@2a40000 > create child: /soc/wdog@2ad0000 > create child: /soc/sai@2b60000 > create child: /soc/edma@2c00000 > create child: /soc/dcu@2ce0000 > create child: /soc/mdio@2d24000 > create child: /soc/ptp_clock@2d10e00 > create child: /soc/ethernet@2d10000 > create child: /soc/ethernet@2d50000 > create child: /soc/ethernet@2d90000 > create child: /soc/usb@8600000 > create child: /soc/usb3@3100000 > create child: /soc/can@2a70000 > create child: /soc/can@2a80000 > create child: /soc/can@2a90000 > create child: /soc/can@2aa0000 > create child: /soc/pcie@3400000 > create child: /soc/pcie@3500000 > create child: /dcsr@20000000/dcsr-epu@0 > create child: /dcsr@20000000/dcsr-gdi@100000 > create child: /dcsr@20000000/dcsr-dddi@120000 > create child: /dcsr@20000000/dcsr-dcfg@220000 > create child: /dcsr@20000000/dcsr-clock@221000 > create child: /dcsr@20000000/dcsr-rcpm@222000 > create child: /dcsr@20000000/dcsr-ccp@225000 > create child: /dcsr@20000000/dcsr-fusectrl@226000 > create child: /dcsr@20000000/dcsr-dap@300000 > create child: /dcsr@20000000/dcsr-cstf@350000 > create child: /dcsr@20000000/dcsr-a7rom@360000 > create child: /dcsr@20000000/dcsr-a7cpu@370000 > create child: /dcsr@20000000/dcsr-a7cti@378000 > create child: /dcsr@20000000/dcsr-etm@37c000 > create child: /dcsr@20000000/dcsr-hugorom@3a0000 > create child: /dcsr@20000000/dcsr-etf@3a1000 > create child: /dcsr@20000000/dcsr-etr@3a3000 > create child: /dcsr@20000000/dcsr-cti@3a4000 > create child: /dcsr@20000000/dcsr-atbrepl@3a8000 > create child: /dcsr@20000000/dcsr-tsgen-ctrl@3a9000 > create child: /dcsr@20000000/dcsr-tsgen-read@3aa000 > create child: /regulators/regulator@0 > ... > > As default the Linux will create all the platform device for each DT node, which > Can be found from "drivers/of/platform.c". > > So we can get the pdev node using the specified DT node, and feel safe to use > it as Pankaj's patch does. > I mean before the devices are populated from device tree. For example, we usually call of_platform_populate in .init_machine. Before it, we may not be able to get it's device, isn't it? Regards Dong Aisheng > And also we must make sure that the 'syscon' DT nodes has the compatible prop. > > Thanks, > > BRs > Xiubo > > > > Regards > > Dong Aisheng > > > > > ---- > > > static struct syscon *of_syscon_register(struct device_node *np) > > > { > > > + struct platform_device *pdev; > > > struct syscon *syscon; > > > struct regmap *regmap; > > > void __iomem *base; > > > @@ -142,7 +144,11 @@ static struct syscon *of_syscon_register(struct > > > device_node *np) > > > if (!base) > > > return ERR_PTR(-ENOMEM); > > > > > > - regmap = regmap_init_mmio(NULL, base, &syscon_regmap_config); > > > + pdev = of_find_device_by_node(np); > > > + if (!(&pdev->dev)) > > > + return ERR_PTR(-ENODEV); > > > + > > > + regmap = regmap_init_mmio(&pdev->dev, base, &syscon_regmap_config); > > > if (IS_ERR(regmap)) { > > > pr_err("regmap init failed\n"); > > > return ERR_CAST(regmap); > > > ------- > > > > > > I have tested this in linux-next and it works well. In this way there won't > > > be any issues of > > > dereferencing NULL pointer in regmap.c and at the same time, if DT has > > > {big,little}-endian > > > optional property in syscon device node, it will be taken care. > > > > > > So I would wait for Arnd's opinion about above mentioned changes and then > > > post a new > > > change after addressing Arnd's minor comment along with this fix in next > > > revision. > > > > > > > > > Thanks, > > > Pankaj Dubey > > > > Maybe we could consider create device structure for each syscon compatible > > > device in > > > > syscon driver in of_syscon_register in first time which seems to be > > > reasonable. > > > > > > > > Regards > > > > Dong Aisheng > > > > > > > > > -------------------------------------------- > > > > > Subject: [PATCH] regmap: fix NULL pointer dereference in > > > > > regmap_get_val_endian > > > > > > > > > > Recent commits for getting reg endianess causing NULL pointer > > > > > dereference if dev is passed NULL in regmap_init_mmio. This patch > > > > > fixes this issue, and allows to parse reg endianess only if dev and > > > > > dev->of_node exist. > > > > > > > > > > Signed-off-by: Pankaj Dubey <pankaj.dubey@xxxxxxxxxxx> > > > > > --- > > > > > drivers/base/regmap/regmap.c | 23 ++++++++++++++--------- > > > > > 1 file changed, 14 insertions(+), 9 deletions(-) > > > > > > > > > > diff --git a/drivers/base/regmap/regmap.c > > > > > b/drivers/base/regmap/regmap.c index f2281af..455a877 100644 > > > > > --- a/drivers/base/regmap/regmap.c > > > > > +++ b/drivers/base/regmap/regmap.c > > > > > @@ -477,7 +477,7 @@ static enum regmap_endian > > > > > regmap_get_val_endian(struct device *dev, > > > > > const struct regmap_bus *bus, > > > > > const struct regmap_config *config) > > > { > > > > > - struct device_node *np = dev->of_node; > > > > > + struct device_node *np; > > > > > enum regmap_endian endian; > > > > > > > > > > /* Retrieve the endianness specification from the regmap config > > */ > > > > > @@ -487,15 +487,20 @@ static enum regmap_endian > > > > > regmap_get_val_endian(struct device *dev, > > > > > if (endian != REGMAP_ENDIAN_DEFAULT) > > > > > return endian; > > > > > > > > > > - /* Parse the device's DT node for an endianness specification */ > > > > > - if (of_property_read_bool(np, "big-endian")) > > > > > - endian = REGMAP_ENDIAN_BIG; > > > > > - else if (of_property_read_bool(np, "little-endian")) > > > > > - endian = REGMAP_ENDIAN_LITTLE; > > > > > + /* If the dev and dev->of_node exist try to get endianness from > > DT > > > > > */ > > > > > + if (dev && dev->of_node) { > > > > > + np = dev->of_node; > > > > > > > > > > - /* If the endianness was specified in DT, use that */ > > > > > - if (endian != REGMAP_ENDIAN_DEFAULT) > > > > > - return endian; > > > > > + /* Parse the device's DT node for an endianness > > > > > specification */ > > > > > + if (of_property_read_bool(np, "big-endian")) > > > > > + endian = REGMAP_ENDIAN_BIG; > > > > > + else if (of_property_read_bool(np, "little-endian")) > > > > > + endian = REGMAP_ENDIAN_LITTLE; > > > > > + > > > > > + /* If the endianness was specified in DT, use that */ > > > > > + if (endian != REGMAP_ENDIAN_DEFAULT) > > > > > + return endian; > > > > > + } > > > > > > > > > > /* Retrieve the endianness specification from the bus config */ > > > > > if (bus && bus->val_format_endian_default) > > > > > -- > > > > > > > > > > Thanks, > > > > > Pankaj Dubey > > > > > > > > > > > Regards > > > > > > Dong Aisheng > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > Pankaj Dubey > > > > > > > > > > > > > > > Regards > > > > > > > > Dong Aisheng > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > > > > > > linux-arm-kernel mailing list > > > > > > > > > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > > > > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > > > > > > > > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |