Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux