* Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
@ 2014-09-18 7:55 ` Dong Aisheng
0 siblings, 0 replies; 48+ messages in thread
From: Dong Aisheng @ 2014-09-18 7:55 UTC (permalink / raw)
To: Pankaj Dubey
Cc: linux-arm-kernel, linux-samsung-soc, linux-kernel, kgene.kim,
linux, arnd, naushad, tomasz.figa, joshi, thomas.ab, vikas.sajjan,
chow.kim, lee.jones, 'Boris BREZILLON',
'Xiubo Li', 'Geert Uytterhoeven',
'Stephen Warren'
On Thu, Sep 18, 2014 at 11:33:26AM +0530, Pankaj Dubey wrote:
> Hi,
>
> Adding CC to Xiubo Li, Geert Uytterhoeven and Stephen Warren.
>
> On Thursday, September 18, 2014, Dong Aisheng wrote,
> > On Wed, Sep 17, 2014 at 04:50:50PM +0530, Pankaj Dubey wrote:
> > > Hi,
> > >
> > > On Wednesday, September 17, 2014, Dong Aisheng Wrote,
> > > > >
> > > > > + regmap = regmap_init_mmio(NULL, base,
> &syscon_regmap_config);
> > > >
> > > > Does a NULL device pointer work?
> > >
> > > Yes, it is safe, at least we are able to test on Exynos based SoC.
> > > I have tested it with kgene/for-next kernel on Exynos3250.
> > > Also it has been tested on Exynos5250 based Snow board with 3.17-rc5
> > > based kernel by Vivek Gautam.
> > >
> > > Patch V2 also has been tested by "Borris Brezillon" on AT91 platform.
> > >
> > >
> >
> > The kernel i tested was next-20140915 of linux-next.
> >
> > please see regmap_get_val_endian called in regmap_init function.
> > 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;
> > enum regmap_endian endian;
> > ...
> > }
> > It will crash at the first line of dev->of_node if dev is NULL.
> >
> > Can you check if you're using the same code as mine?
>
> No, it's not same.
> My bad that I was not using linux-next for testing this patch.
> We tested on kgene/for-next where these changes still have not come.
> Just now I checked linux-next and found that it will crash at first line of
> "regmap_get_val_endian" as there is no check for NULL on dev.
>
> I checked git history of regmap.c file and found recently this file has been
> modified
> for adding DT endianness binding support. Following are set of patches gone
> for this
>
> cf673fb regmap: Split regmap_get_endian() in two functions
> 5844a8b regmap: Fix handling of volatile registers for format_write() chips
> 45e1a27 regmap: of_regmap_get_endian() cleanup
> ba1b53f regmap: Fix DT endianess parsing logic
> d647c19 regmap: add DT endianness binding support.
>
> I think there should have been a check for NULL on "dev" in
> "regmap_get_val_endian", so that if dev pointer exist then only it makes
> sense to get
> endianness property from DT.
>
> I will suggest following fix in regmap.c for this. With following fix I
> tested it and it works well
> on linux-next also. So if you can confirm following fix is working for you
> then I can post this
> patch.
>
I tested the patch work.
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.
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@samsung.com>
> ---
> 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@lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > >
>
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
@ 2014-09-18 7:55 ` Dong Aisheng
0 siblings, 0 replies; 48+ messages in thread
From: Dong Aisheng @ 2014-09-18 7:55 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Sep 18, 2014 at 11:33:26AM +0530, Pankaj Dubey wrote:
> Hi,
>
> Adding CC to Xiubo Li, Geert Uytterhoeven and Stephen Warren.
>
> On Thursday, September 18, 2014, Dong Aisheng wrote,
> > On Wed, Sep 17, 2014 at 04:50:50PM +0530, Pankaj Dubey wrote:
> > > Hi,
> > >
> > > On Wednesday, September 17, 2014, Dong Aisheng Wrote,
> > > > >
> > > > > + regmap = regmap_init_mmio(NULL, base,
> &syscon_regmap_config);
> > > >
> > > > Does a NULL device pointer work?
> > >
> > > Yes, it is safe, at least we are able to test on Exynos based SoC.
> > > I have tested it with kgene/for-next kernel on Exynos3250.
> > > Also it has been tested on Exynos5250 based Snow board with 3.17-rc5
> > > based kernel by Vivek Gautam.
> > >
> > > Patch V2 also has been tested by "Borris Brezillon" on AT91 platform.
> > >
> > >
> >
> > The kernel i tested was next-20140915 of linux-next.
> >
> > please see regmap_get_val_endian called in regmap_init function.
> > 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;
> > enum regmap_endian endian;
> > ...
> > }
> > It will crash at the first line of dev->of_node if dev is NULL.
> >
> > Can you check if you're using the same code as mine?
>
> No, it's not same.
> My bad that I was not using linux-next for testing this patch.
> We tested on kgene/for-next where these changes still have not come.
> Just now I checked linux-next and found that it will crash at first line of
> "regmap_get_val_endian" as there is no check for NULL on dev.
>
> I checked git history of regmap.c file and found recently this file has been
> modified
> for adding DT endianness binding support. Following are set of patches gone
> for this
>
> cf673fb regmap: Split regmap_get_endian() in two functions
> 5844a8b regmap: Fix handling of volatile registers for format_write() chips
> 45e1a27 regmap: of_regmap_get_endian() cleanup
> ba1b53f regmap: Fix DT endianess parsing logic
> d647c19 regmap: add DT endianness binding support.
>
> I think there should have been a check for NULL on "dev" in
> "regmap_get_val_endian", so that if dev pointer exist then only it makes
> sense to get
> endianness property from DT.
>
> I will suggest following fix in regmap.c for this. With following fix I
> tested it and it works well
> on linux-next also. So if you can confirm following fix is working for you
> then I can post this
> patch.
>
I tested the patch work.
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.
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@samsung.com>
> ---
> 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 at lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > >
>
^ permalink raw reply [flat|nested] 48+ messages in thread* RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
2014-09-18 7:55 ` Dong Aisheng
@ 2014-09-18 8:58 ` Li.Xiubo at freescale.com
-1 siblings, 0 replies; 48+ messages in thread
From: Li.Xiubo @ 2014-09-18 8:58 UTC (permalink / raw)
To: Aisheng.Dong@freescale.com, Pankaj Dubey
Cc: linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
kgene.kim@samsung.com, linux@arm.linux.org.uk, arnd@arndb.de,
naushad@samsung.com, tomasz.figa@gmail.com, joshi@samsung.com,
thomas.ab@samsung.com, vikas.sajjan@samsung.com,
chow.kim@samsung.com, lee.jones@linaro.org,
'Boris BREZILLON', 'Geert Uytterhoeven',
'Stephen Warren'
[...]
> > I think there should have been a check for NULL on "dev" in
> > "regmap_get_val_endian", so that if dev pointer exist then only it makes
> > sense to get
> > endianness property from DT.
> >
> > I will suggest following fix in regmap.c for this. With following fix I
> > tested it and it works well
> > on linux-next also. So if you can confirm following fix is working for you
> > then I can post this
> > patch.
> >
>
> I tested the patch work.
> 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.
>
> 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@samsung.com>
> > ---
> > 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;
And the 'np' must be NULL as default.
Isn't it ?
Thanks,
BRs
Xiubo
> > 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@lists.infradead.org
> > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > >
> >
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
@ 2014-09-18 8:58 ` Li.Xiubo at freescale.com
0 siblings, 0 replies; 48+ messages in thread
From: Li.Xiubo at freescale.com @ 2014-09-18 8:58 UTC (permalink / raw)
To: linux-arm-kernel
[...]
> > I think there should have been a check for NULL on "dev" in
> > "regmap_get_val_endian", so that if dev pointer exist then only it makes
> > sense to get
> > endianness property from DT.
> >
> > I will suggest following fix in regmap.c for this. With following fix I
> > tested it and it works well
> > on linux-next also. So if you can confirm following fix is working for you
> > then I can post this
> > patch.
> >
>
> I tested the patch work.
> 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.
>
> 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@samsung.com>
> > ---
> > 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;
And the 'np' must be NULL as default.
Isn't it ?
Thanks,
BRs
Xiubo
> > 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 at lists.infradead.org
> > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > >
> >
^ permalink raw reply [flat|nested] 48+ messages in thread* RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
2014-09-18 8:58 ` Li.Xiubo at freescale.com
@ 2014-09-18 9:40 ` Pankaj Dubey
-1 siblings, 0 replies; 48+ messages in thread
From: Pankaj Dubey @ 2014-09-18 9:40 UTC (permalink / raw)
To: Li.Xiubo, Aisheng.Dong
Cc: linux-arm-kernel, linux-samsung-soc, linux-kernel, kgene.kim,
linux, arnd, naushad, tomasz.figa, joshi, thomas.ab, vikas.sajjan,
chow.kim, lee.jones, 'Boris BREZILLON',
'Geert Uytterhoeven', 'Stephen Warren'
Hi,
On September 18, 2014, Li.Xiubo wrote,
> Subject: RE: [PATCH v3] mfd: syscon: Decouple syscon interface from
platform
> devices
>
> [...]
> > > I think there should have been a check for NULL on "dev" in
> > > "regmap_get_val_endian", so that if dev pointer exist then only it
> > > makes sense to get endianness property from DT.
> > >
> > > I will suggest following fix in regmap.c for this. With following
> > > fix I tested it and it works well on linux-next also. So if you can
> > > confirm following fix is working for you then I can post this patch.
> > >
> >
> > I tested the patch work.
> > 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.
> >
> > 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@samsung.com>
> > > ---
> > > 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;
>
> And the 'np' must be NULL as default.
>
> Isn't it ?
>
Yes.
Thanks,
Pankaj Dubey
> Thanks,
>
> BRs
> Xiubo
>
> > > 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@lists.infradead.org
> > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > >
> > >
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
@ 2014-09-18 9:40 ` Pankaj Dubey
0 siblings, 0 replies; 48+ messages in thread
From: Pankaj Dubey @ 2014-09-18 9:40 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On September 18, 2014, Li.Xiubo wrote,
> Subject: RE: [PATCH v3] mfd: syscon: Decouple syscon interface from
platform
> devices
>
> [...]
> > > I think there should have been a check for NULL on "dev" in
> > > "regmap_get_val_endian", so that if dev pointer exist then only it
> > > makes sense to get endianness property from DT.
> > >
> > > I will suggest following fix in regmap.c for this. With following
> > > fix I tested it and it works well on linux-next also. So if you can
> > > confirm following fix is working for you then I can post this patch.
> > >
> >
> > I tested the patch work.
> > 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.
> >
> > 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@samsung.com>
> > > ---
> > > 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;
>
> And the 'np' must be NULL as default.
>
> Isn't it ?
>
Yes.
Thanks,
Pankaj Dubey
> Thanks,
>
> BRs
> Xiubo
>
> > > 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 at lists.infradead.org
> > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > >
> > >
^ permalink raw reply [flat|nested] 48+ messages in thread
* RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
2014-09-18 7:55 ` Dong Aisheng
@ 2014-09-18 9:36 ` Pankaj Dubey
-1 siblings, 0 replies; 48+ messages in thread
From: Pankaj Dubey @ 2014-09-18 9:36 UTC (permalink / raw)
To: 'Dong Aisheng', 'Xiubo Li', arnd
Cc: linux-arm-kernel, linux-samsung-soc, linux-kernel, kgene.kim,
linux, naushad, tomasz.figa, joshi, thomas.ab, vikas.sajjan,
chow.kim, lee.jones, 'Boris BREZILLON',
'Geert Uytterhoeven', 'Stephen Warren'
Hi,
On September 18, 2014 1:26, Dong Aisheng wrote
> On Thu, Sep 18, 2014 at 11:33:26AM +0530, Pankaj Dubey wrote:
> > Hi,
> >
> > Adding CC to Xiubo Li, Geert Uytterhoeven and Stephen Warren.
> >
> > On Thursday, September 18, 2014, Dong Aisheng wrote,
> > > On Wed, Sep 17, 2014 at 04:50:50PM +0530, Pankaj Dubey wrote:
> > > > Hi,
> > > >
> > > > On Wednesday, September 17, 2014, Dong Aisheng Wrote,
> > > > > >
> > > > > > + regmap = regmap_init_mmio(NULL, base,
> > &syscon_regmap_config);
> > > > >
> > > > > Does a NULL device pointer work?
> > > >
> > > > Yes, it is safe, at least we are able to test on Exynos based SoC.
> > > > I have tested it with kgene/for-next kernel on Exynos3250.
> > > > Also it has been tested on Exynos5250 based Snow board with
> > > > 3.17-rc5 based kernel by Vivek Gautam.
> > > >
> > > > Patch V2 also has been tested by "Borris Brezillon" on AT91
platform.
> > > >
> > > >
> > >
> > > The kernel i tested was next-20140915 of linux-next.
> > >
> > > please see regmap_get_val_endian called in regmap_init function.
> > > 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;
> > > enum regmap_endian endian;
> > > ...
> > > }
> > > It will crash at the first line of dev->of_node if dev is NULL.
> > >
> > > Can you check if you're using the same code as mine?
> >
> > No, it's not same.
> > My bad that I was not using linux-next for testing this patch.
> > We tested on kgene/for-next where these changes still have not come.
> > Just now I checked linux-next and found that it will crash at first
> > line of "regmap_get_val_endian" as there is no check for NULL on dev.
> >
> > I checked git history of regmap.c file and found recently this file
> > has been modified for adding DT endianness binding support. Following
> > are set of patches gone for this
> >
> > cf673fb regmap: Split regmap_get_endian() in two functions 5844a8b
> > regmap: Fix handling of volatile registers for format_write() chips
> > 45e1a27 regmap: of_regmap_get_endian() cleanup ba1b53f regmap: Fix DT
> > endianess parsing logic
> > d647c19 regmap: add DT endianness binding support.
> >
> > I think there should have been a check for NULL on "dev" in
> > "regmap_get_val_endian", so that if dev pointer exist then only it
> > makes sense to get endianness property from DT.
> >
> > I will suggest following fix in regmap.c for this. With following fix
> > I tested it and it works well on linux-next also. So if you can
> > confirm following fix is working for you then I can post this patch.
> >
>
> I tested the patch work.
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:
----
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@samsung.com>
> > ---
> > 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@lists.infradead.org
> > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > >
> >
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
@ 2014-09-18 9:36 ` Pankaj Dubey
0 siblings, 0 replies; 48+ messages in thread
From: Pankaj Dubey @ 2014-09-18 9:36 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On September 18, 2014 1:26, Dong Aisheng wrote
> On Thu, Sep 18, 2014 at 11:33:26AM +0530, Pankaj Dubey wrote:
> > Hi,
> >
> > Adding CC to Xiubo Li, Geert Uytterhoeven and Stephen Warren.
> >
> > On Thursday, September 18, 2014, Dong Aisheng wrote,
> > > On Wed, Sep 17, 2014 at 04:50:50PM +0530, Pankaj Dubey wrote:
> > > > Hi,
> > > >
> > > > On Wednesday, September 17, 2014, Dong Aisheng Wrote,
> > > > > >
> > > > > > + regmap = regmap_init_mmio(NULL, base,
> > &syscon_regmap_config);
> > > > >
> > > > > Does a NULL device pointer work?
> > > >
> > > > Yes, it is safe, at least we are able to test on Exynos based SoC.
> > > > I have tested it with kgene/for-next kernel on Exynos3250.
> > > > Also it has been tested on Exynos5250 based Snow board with
> > > > 3.17-rc5 based kernel by Vivek Gautam.
> > > >
> > > > Patch V2 also has been tested by "Borris Brezillon" on AT91
platform.
> > > >
> > > >
> > >
> > > The kernel i tested was next-20140915 of linux-next.
> > >
> > > please see regmap_get_val_endian called in regmap_init function.
> > > 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;
> > > enum regmap_endian endian;
> > > ...
> > > }
> > > It will crash at the first line of dev->of_node if dev is NULL.
> > >
> > > Can you check if you're using the same code as mine?
> >
> > No, it's not same.
> > My bad that I was not using linux-next for testing this patch.
> > We tested on kgene/for-next where these changes still have not come.
> > Just now I checked linux-next and found that it will crash at first
> > line of "regmap_get_val_endian" as there is no check for NULL on dev.
> >
> > I checked git history of regmap.c file and found recently this file
> > has been modified for adding DT endianness binding support. Following
> > are set of patches gone for this
> >
> > cf673fb regmap: Split regmap_get_endian() in two functions 5844a8b
> > regmap: Fix handling of volatile registers for format_write() chips
> > 45e1a27 regmap: of_regmap_get_endian() cleanup ba1b53f regmap: Fix DT
> > endianess parsing logic
> > d647c19 regmap: add DT endianness binding support.
> >
> > I think there should have been a check for NULL on "dev" in
> > "regmap_get_val_endian", so that if dev pointer exist then only it
> > makes sense to get endianness property from DT.
> >
> > I will suggest following fix in regmap.c for this. With following fix
> > I tested it and it works well on linux-next also. So if you can
> > confirm following fix is working for you then I can post this patch.
> >
>
> I tested the patch work.
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:
----
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@samsung.com>
> > ---
> > 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 at lists.infradead.org
> > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > >
> >
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
2014-09-18 9:36 ` Pankaj Dubey
(?)
@ 2014-09-18 10:05 ` Dong Aisheng
-1 siblings, 0 replies; 48+ messages in thread
From: Dong Aisheng @ 2014-09-18 10:05 UTC (permalink / raw)
To: Pankaj Dubey
Cc: 'Boris BREZILLON', kgene.kim, linux,
'Geert Uytterhoeven', arnd, 'Stephen Warren',
naushad, 'Xiubo Li', linux-kernel, tomasz.figa,
linux-samsung-soc, thomas.ab, vikas.sajjan, chow.kim, joshi,
lee.jones, linux-arm-kernel
On Thu, Sep 18, 2014 at 03:06:59PM +0530, Pankaj Dubey wrote:
> Hi,
>
> On September 18, 2014 1:26, Dong Aisheng wrote
> > On Thu, Sep 18, 2014 at 11:33:26AM +0530, Pankaj Dubey wrote:
> > > Hi,
> > >
> > > Adding CC to Xiubo Li, Geert Uytterhoeven and Stephen Warren.
> > >
> > > On Thursday, September 18, 2014, Dong Aisheng wrote,
> > > > On Wed, Sep 17, 2014 at 04:50:50PM +0530, Pankaj Dubey wrote:
> > > > > Hi,
> > > > >
> > > > > On Wednesday, September 17, 2014, Dong Aisheng Wrote,
> > > > > > >
> > > > > > > + regmap = regmap_init_mmio(NULL, base,
> > > &syscon_regmap_config);
> > > > > >
> > > > > > Does a NULL device pointer work?
> > > > >
> > > > > Yes, it is safe, at least we are able to test on Exynos based SoC.
> > > > > I have tested it with kgene/for-next kernel on Exynos3250.
> > > > > Also it has been tested on Exynos5250 based Snow board with
> > > > > 3.17-rc5 based kernel by Vivek Gautam.
> > > > >
> > > > > Patch V2 also has been tested by "Borris Brezillon" on AT91
> platform.
> > > > >
> > > > >
> > > >
> > > > The kernel i tested was next-20140915 of linux-next.
> > > >
> > > > please see regmap_get_val_endian called in regmap_init function.
> > > > 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;
> > > > enum regmap_endian endian;
> > > > ...
> > > > }
> > > > It will crash at the first line of dev->of_node if dev is NULL.
> > > >
> > > > Can you check if you're using the same code as mine?
> > >
> > > No, it's not same.
> > > My bad that I was not using linux-next for testing this patch.
> > > We tested on kgene/for-next where these changes still have not come.
> > > Just now I checked linux-next and found that it will crash at first
> > > line of "regmap_get_val_endian" as there is no check for NULL on dev.
> > >
> > > I checked git history of regmap.c file and found recently this file
> > > has been modified for adding DT endianness binding support. Following
> > > are set of patches gone for this
> > >
> > > cf673fb regmap: Split regmap_get_endian() in two functions 5844a8b
> > > regmap: Fix handling of volatile registers for format_write() chips
> > > 45e1a27 regmap: of_regmap_get_endian() cleanup ba1b53f regmap: Fix DT
> > > endianess parsing logic
> > > d647c19 regmap: add DT endianness binding support.
> > >
> > > I think there should have been a check for NULL on "dev" in
> > > "regmap_get_val_endian", so that if dev pointer exist then only it
> > > makes sense to get endianness property from DT.
> > >
> > > I will suggest following fix in regmap.c for this. With following fix
> > > I tested it and it works well on linux-next also. So if you can
> > > confirm following fix is working for you then I can post this patch.
> > >
> >
> > I tested the patch work.
>
> 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.
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@samsung.com>
> > > ---
> > > 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@lists.infradead.org
> > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > >
> > >
>
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
@ 2014-09-18 10:05 ` Dong Aisheng
0 siblings, 0 replies; 48+ messages in thread
From: Dong Aisheng @ 2014-09-18 10:05 UTC (permalink / raw)
To: Pankaj Dubey
Cc: 'Xiubo Li', arnd, linux-arm-kernel, linux-samsung-soc,
linux-kernel, kgene.kim, linux, naushad, tomasz.figa, joshi,
thomas.ab, vikas.sajjan, chow.kim, lee.jones,
'Boris BREZILLON', 'Geert Uytterhoeven',
'Stephen Warren'
On Thu, Sep 18, 2014 at 03:06:59PM +0530, Pankaj Dubey wrote:
> Hi,
>
> On September 18, 2014 1:26, Dong Aisheng wrote
> > On Thu, Sep 18, 2014 at 11:33:26AM +0530, Pankaj Dubey wrote:
> > > Hi,
> > >
> > > Adding CC to Xiubo Li, Geert Uytterhoeven and Stephen Warren.
> > >
> > > On Thursday, September 18, 2014, Dong Aisheng wrote,
> > > > On Wed, Sep 17, 2014 at 04:50:50PM +0530, Pankaj Dubey wrote:
> > > > > Hi,
> > > > >
> > > > > On Wednesday, September 17, 2014, Dong Aisheng Wrote,
> > > > > > >
> > > > > > > + regmap = regmap_init_mmio(NULL, base,
> > > &syscon_regmap_config);
> > > > > >
> > > > > > Does a NULL device pointer work?
> > > > >
> > > > > Yes, it is safe, at least we are able to test on Exynos based SoC.
> > > > > I have tested it with kgene/for-next kernel on Exynos3250.
> > > > > Also it has been tested on Exynos5250 based Snow board with
> > > > > 3.17-rc5 based kernel by Vivek Gautam.
> > > > >
> > > > > Patch V2 also has been tested by "Borris Brezillon" on AT91
> platform.
> > > > >
> > > > >
> > > >
> > > > The kernel i tested was next-20140915 of linux-next.
> > > >
> > > > please see regmap_get_val_endian called in regmap_init function.
> > > > 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;
> > > > enum regmap_endian endian;
> > > > ...
> > > > }
> > > > It will crash at the first line of dev->of_node if dev is NULL.
> > > >
> > > > Can you check if you're using the same code as mine?
> > >
> > > No, it's not same.
> > > My bad that I was not using linux-next for testing this patch.
> > > We tested on kgene/for-next where these changes still have not come.
> > > Just now I checked linux-next and found that it will crash at first
> > > line of "regmap_get_val_endian" as there is no check for NULL on dev.
> > >
> > > I checked git history of regmap.c file and found recently this file
> > > has been modified for adding DT endianness binding support. Following
> > > are set of patches gone for this
> > >
> > > cf673fb regmap: Split regmap_get_endian() in two functions 5844a8b
> > > regmap: Fix handling of volatile registers for format_write() chips
> > > 45e1a27 regmap: of_regmap_get_endian() cleanup ba1b53f regmap: Fix DT
> > > endianess parsing logic
> > > d647c19 regmap: add DT endianness binding support.
> > >
> > > I think there should have been a check for NULL on "dev" in
> > > "regmap_get_val_endian", so that if dev pointer exist then only it
> > > makes sense to get endianness property from DT.
> > >
> > > I will suggest following fix in regmap.c for this. With following fix
> > > I tested it and it works well on linux-next also. So if you can
> > > confirm following fix is working for you then I can post this patch.
> > >
> >
> > I tested the patch work.
>
> 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.
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@samsung.com>
> > > ---
> > > 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@lists.infradead.org
> > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > >
> > >
>
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
@ 2014-09-18 10:05 ` Dong Aisheng
0 siblings, 0 replies; 48+ messages in thread
From: Dong Aisheng @ 2014-09-18 10:05 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Sep 18, 2014 at 03:06:59PM +0530, Pankaj Dubey wrote:
> Hi,
>
> On September 18, 2014 1:26, Dong Aisheng wrote
> > On Thu, Sep 18, 2014 at 11:33:26AM +0530, Pankaj Dubey wrote:
> > > Hi,
> > >
> > > Adding CC to Xiubo Li, Geert Uytterhoeven and Stephen Warren.
> > >
> > > On Thursday, September 18, 2014, Dong Aisheng wrote,
> > > > On Wed, Sep 17, 2014 at 04:50:50PM +0530, Pankaj Dubey wrote:
> > > > > Hi,
> > > > >
> > > > > On Wednesday, September 17, 2014, Dong Aisheng Wrote,
> > > > > > >
> > > > > > > + regmap = regmap_init_mmio(NULL, base,
> > > &syscon_regmap_config);
> > > > > >
> > > > > > Does a NULL device pointer work?
> > > > >
> > > > > Yes, it is safe, at least we are able to test on Exynos based SoC.
> > > > > I have tested it with kgene/for-next kernel on Exynos3250.
> > > > > Also it has been tested on Exynos5250 based Snow board with
> > > > > 3.17-rc5 based kernel by Vivek Gautam.
> > > > >
> > > > > Patch V2 also has been tested by "Borris Brezillon" on AT91
> platform.
> > > > >
> > > > >
> > > >
> > > > The kernel i tested was next-20140915 of linux-next.
> > > >
> > > > please see regmap_get_val_endian called in regmap_init function.
> > > > 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;
> > > > enum regmap_endian endian;
> > > > ...
> > > > }
> > > > It will crash at the first line of dev->of_node if dev is NULL.
> > > >
> > > > Can you check if you're using the same code as mine?
> > >
> > > No, it's not same.
> > > My bad that I was not using linux-next for testing this patch.
> > > We tested on kgene/for-next where these changes still have not come.
> > > Just now I checked linux-next and found that it will crash at first
> > > line of "regmap_get_val_endian" as there is no check for NULL on dev.
> > >
> > > I checked git history of regmap.c file and found recently this file
> > > has been modified for adding DT endianness binding support. Following
> > > are set of patches gone for this
> > >
> > > cf673fb regmap: Split regmap_get_endian() in two functions 5844a8b
> > > regmap: Fix handling of volatile registers for format_write() chips
> > > 45e1a27 regmap: of_regmap_get_endian() cleanup ba1b53f regmap: Fix DT
> > > endianess parsing logic
> > > d647c19 regmap: add DT endianness binding support.
> > >
> > > I think there should have been a check for NULL on "dev" in
> > > "regmap_get_val_endian", so that if dev pointer exist then only it
> > > makes sense to get endianness property from DT.
> > >
> > > I will suggest following fix in regmap.c for this. With following fix
> > > I tested it and it works well on linux-next also. So if you can
> > > confirm following fix is working for you then I can post this patch.
> > >
> >
> > I tested the patch work.
>
> 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.
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@samsung.com>
> > > ---
> > > 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 at lists.infradead.org
> > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > >
> > >
>
^ permalink raw reply [flat|nested] 48+ messages in thread* RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
2014-09-18 10:05 ` Dong Aisheng
@ 2014-09-19 3:38 ` Li.Xiubo at freescale.com
-1 siblings, 0 replies; 48+ messages in thread
From: Li.Xiubo @ 2014-09-19 3:38 UTC (permalink / raw)
To: Aisheng.Dong@freescale.com, Pankaj Dubey
Cc: arnd@arndb.de, linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
kgene.kim@samsung.com, linux@arm.linux.org.uk,
naushad@samsung.com, tomasz.figa@gmail.com, joshi@samsung.com,
thomas.ab@samsung.com, vikas.sajjan@samsung.com,
chow.kim@samsung.com, lee.jones@linaro.org,
'Boris BREZILLON', 'Geert Uytterhoeven',
'Stephen Warren'
> > 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.
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@samsung.com>
> > > > ---
> > > > 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@lists.infradead.org
> > > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > > >
> > > >
> >
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
@ 2014-09-19 3:38 ` Li.Xiubo at freescale.com
0 siblings, 0 replies; 48+ messages in thread
From: Li.Xiubo at freescale.com @ 2014-09-19 3:38 UTC (permalink / raw)
To: linux-arm-kernel
> > 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 at 1200000
create child: /soc/smmu at 1280000
create child: /soc/smmu at 1300000
create child: /soc/smmu at 1380000
create child: /soc/interrupt-controller at 1400000
create child: /soc/tzasc at 1500000
of_platform_bus_create() - skipping /soc/tzasc at 1500000, no compatible prop
create child: /soc/ifc at 1530000
create child: /soc/ifc at 1530000/nor at 0,0
create child: /soc/ifc at 1530000/board-control at 2,0
create child: /soc/dcfg at 1ee0000
syscon 1ee0000.dcfg: regmap [mem 0x01ee0000-0x01eeffff] registered
create child: /soc/quadspi at 1550000
create child: /soc/esdhc at 1560000
create child: /soc/scfg at 1570000
create child: /soc/crypto at 1700000
create child: /soc/sfp at 1e80000
of_platform_bus_create() - skipping /soc/sfp at 1e80000, no compatible prop
create child: /soc/snvs at 1e90000
of_platform_bus_create() - skipping /soc/snvs at 1e90000, no compatible prop
create child: /soc/serdes1 at 1ea0000
of_platform_bus_create() - skipping /soc/serdes1 at 1ea0000, no compatible prop
create child: /soc/clocking at 1ee1000
create child: /soc/rcpm at 1ee2000
create child: /soc/dspi at 2100000
create child: /soc/dspi at 2110000
create child: /soc/i2c at 2180000
create child: /soc/i2c at 2190000
create child: /soc/i2c at 21a0000
create child: /soc/serial at 21c0500
create child: /soc/serial at 21c0600
create child: /soc/serial at 21d0500
create child: /soc/serial at 21d0600
create child: /soc/gpio at 2300000
create child: /soc/gpio at 2310000
create child: /soc/gpio at 2320000
create child: /soc/gpio at 2330000
create child: /soc/uqe at 2400000
create child: /soc/uqe at 2400000/qeic at 80
create child: /soc/uqe at 2400000/ucc at 2000
irq: no irq domain found for /soc/uqe at 2400000/qeic at 80 !
create child: /soc/uqe at 2400000/ucc at 2200
irq: no irq domain found for /soc/uqe at 2400000/qeic at 80 !
create child: /soc/uqe at 2400000/muram at 10000
create child: /soc/uqe at 2400000/si at 700
create child: /soc/uqe at 2400000/siram at 1000
create child: /soc/serial at 2950000
create child: /soc/serial at 2960000
create child: /soc/serial at 2970000
create child: /soc/serial at 2980000
create child: /soc/serial at 2990000
create child: /soc/serial at 29a0000
create child: /soc/ftm0_1 at 29d0000
create child: /soc/ftm at 29f0000
of_platform_bus_create() - skipping /soc/ftm at 29f0000, no compatible prop
create child: /soc/ftm at 2a00000
create child: /soc/ftm at 2a10000
of_platform_bus_create() - skipping /soc/ftm at 2a10000, no compatible prop
create child: /soc/ftm at 2a20000
of_platform_bus_create() - skipping /soc/ftm at 2a20000, no compatible prop
create child: /soc/ftm at 2a30000
create child: /soc/ftm at 2a40000
create child: /soc/wdog at 2ad0000
create child: /soc/sai at 2b60000
create child: /soc/edma at 2c00000
create child: /soc/dcu at 2ce0000
create child: /soc/mdio at 2d24000
create child: /soc/ptp_clock at 2d10e00
create child: /soc/ethernet at 2d10000
create child: /soc/ethernet at 2d50000
create child: /soc/ethernet at 2d90000
create child: /soc/usb at 8600000
create child: /soc/usb3 at 3100000
create child: /soc/can at 2a70000
create child: /soc/can at 2a80000
create child: /soc/can at 2a90000
create child: /soc/can at 2aa0000
create child: /soc/pcie at 3400000
create child: /soc/pcie at 3500000
create child: /dcsr at 20000000/dcsr-epu at 0
create child: /dcsr at 20000000/dcsr-gdi at 100000
create child: /dcsr at 20000000/dcsr-dddi at 120000
create child: /dcsr at 20000000/dcsr-dcfg at 220000
create child: /dcsr at 20000000/dcsr-clock at 221000
create child: /dcsr at 20000000/dcsr-rcpm at 222000
create child: /dcsr at 20000000/dcsr-ccp at 225000
create child: /dcsr at 20000000/dcsr-fusectrl at 226000
create child: /dcsr at 20000000/dcsr-dap at 300000
create child: /dcsr at 20000000/dcsr-cstf at 350000
create child: /dcsr at 20000000/dcsr-a7rom at 360000
create child: /dcsr at 20000000/dcsr-a7cpu at 370000
create child: /dcsr at 20000000/dcsr-a7cti at 378000
create child: /dcsr at 20000000/dcsr-etm at 37c000
create child: /dcsr at 20000000/dcsr-hugorom at 3a0000
create child: /dcsr at 20000000/dcsr-etf at 3a1000
create child: /dcsr at 20000000/dcsr-etr at 3a3000
create child: /dcsr at 20000000/dcsr-cti at 3a4000
create child: /dcsr at 20000000/dcsr-atbrepl at 3a8000
create child: /dcsr at 20000000/dcsr-tsgen-ctrl at 3a9000
create child: /dcsr at 20000000/dcsr-tsgen-read at 3aa000
create child: /regulators/regulator at 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.
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@samsung.com>
> > > > ---
> > > > 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 at lists.infradead.org
> > > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > > >
> > > >
> >
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
2014-09-19 3:38 ` Li.Xiubo at freescale.com
(?)
@ 2014-09-19 4:19 ` Dong Aisheng
-1 siblings, 0 replies; 48+ messages in thread
From: Dong Aisheng @ 2014-09-19 4:19 UTC (permalink / raw)
To: Xiubo Li-B47053
Cc: Dong Aisheng-B29396, Pankaj Dubey, arnd@arndb.de,
linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
kgene.kim@samsung.com, linux@arm.linux.org.uk,
naushad@samsung.com, tomasz.figa@gmail.com, joshi@samsung.com,
thomas.ab@samsung.com, vikas.sajjan@samsung.com,
chow.kim@samsung.com, lee.jones@linaro.org,
'Boris BREZILLON', 'Geert Uytterhoeven'
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@samsung.com>
> > > > > ---
> > > > > 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@lists.infradead.org
> > > > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > > > >
> > > > >
> > >
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
@ 2014-09-19 4:19 ` Dong Aisheng
0 siblings, 0 replies; 48+ messages in thread
From: Dong Aisheng @ 2014-09-19 4:19 UTC (permalink / raw)
To: Xiubo Li-B47053
Cc: Dong Aisheng-B29396, Pankaj Dubey, arnd@arndb.de,
linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
kgene.kim@samsung.com, linux@arm.linux.org.uk,
naushad@samsung.com, tomasz.figa@gmail.com, joshi@samsung.com,
thomas.ab@samsung.com, vikas.sajjan@samsung.com,
chow.kim@samsung.com, lee.jones@linaro.org,
'Boris BREZILLON', 'Geert Uytterhoeven',
'Stephen Warren'
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@samsung.com>
> > > > > ---
> > > > > 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@lists.infradead.org
> > > > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > > > >
> > > > >
> > >
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
@ 2014-09-19 4:19 ` Dong Aisheng
0 siblings, 0 replies; 48+ messages in thread
From: Dong Aisheng @ 2014-09-19 4:19 UTC (permalink / raw)
To: linux-arm-kernel
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 at 1200000
> create child: /soc/smmu at 1280000
> create child: /soc/smmu at 1300000
> create child: /soc/smmu at 1380000
> create child: /soc/interrupt-controller at 1400000
> create child: /soc/tzasc at 1500000
> of_platform_bus_create() - skipping /soc/tzasc at 1500000, no compatible prop
> create child: /soc/ifc at 1530000
> create child: /soc/ifc at 1530000/nor at 0,0
> create child: /soc/ifc at 1530000/board-control at 2,0
> create child: /soc/dcfg at 1ee0000
> syscon 1ee0000.dcfg: regmap [mem 0x01ee0000-0x01eeffff] registered
> create child: /soc/quadspi at 1550000
> create child: /soc/esdhc at 1560000
> create child: /soc/scfg at 1570000
> create child: /soc/crypto at 1700000
> create child: /soc/sfp at 1e80000
> of_platform_bus_create() - skipping /soc/sfp at 1e80000, no compatible prop
> create child: /soc/snvs at 1e90000
> of_platform_bus_create() - skipping /soc/snvs at 1e90000, no compatible prop
> create child: /soc/serdes1 at 1ea0000
> of_platform_bus_create() - skipping /soc/serdes1 at 1ea0000, no compatible prop
> create child: /soc/clocking at 1ee1000
> create child: /soc/rcpm at 1ee2000
> create child: /soc/dspi at 2100000
> create child: /soc/dspi at 2110000
> create child: /soc/i2c at 2180000
> create child: /soc/i2c at 2190000
> create child: /soc/i2c at 21a0000
> create child: /soc/serial at 21c0500
> create child: /soc/serial at 21c0600
> create child: /soc/serial at 21d0500
> create child: /soc/serial at 21d0600
> create child: /soc/gpio at 2300000
> create child: /soc/gpio at 2310000
> create child: /soc/gpio at 2320000
> create child: /soc/gpio at 2330000
> create child: /soc/uqe at 2400000
> create child: /soc/uqe at 2400000/qeic at 80
> create child: /soc/uqe at 2400000/ucc at 2000
> irq: no irq domain found for /soc/uqe at 2400000/qeic at 80 !
> create child: /soc/uqe at 2400000/ucc at 2200
> irq: no irq domain found for /soc/uqe at 2400000/qeic at 80 !
> create child: /soc/uqe at 2400000/muram at 10000
> create child: /soc/uqe at 2400000/si at 700
> create child: /soc/uqe at 2400000/siram at 1000
> create child: /soc/serial at 2950000
> create child: /soc/serial at 2960000
> create child: /soc/serial at 2970000
> create child: /soc/serial at 2980000
> create child: /soc/serial at 2990000
> create child: /soc/serial at 29a0000
> create child: /soc/ftm0_1 at 29d0000
> create child: /soc/ftm at 29f0000
> of_platform_bus_create() - skipping /soc/ftm at 29f0000, no compatible prop
> create child: /soc/ftm at 2a00000
> create child: /soc/ftm at 2a10000
> of_platform_bus_create() - skipping /soc/ftm at 2a10000, no compatible prop
> create child: /soc/ftm at 2a20000
> of_platform_bus_create() - skipping /soc/ftm at 2a20000, no compatible prop
> create child: /soc/ftm at 2a30000
> create child: /soc/ftm at 2a40000
> create child: /soc/wdog at 2ad0000
> create child: /soc/sai at 2b60000
> create child: /soc/edma at 2c00000
> create child: /soc/dcu at 2ce0000
> create child: /soc/mdio at 2d24000
> create child: /soc/ptp_clock at 2d10e00
> create child: /soc/ethernet at 2d10000
> create child: /soc/ethernet at 2d50000
> create child: /soc/ethernet at 2d90000
> create child: /soc/usb at 8600000
> create child: /soc/usb3 at 3100000
> create child: /soc/can at 2a70000
> create child: /soc/can at 2a80000
> create child: /soc/can at 2a90000
> create child: /soc/can at 2aa0000
> create child: /soc/pcie at 3400000
> create child: /soc/pcie at 3500000
> create child: /dcsr at 20000000/dcsr-epu at 0
> create child: /dcsr at 20000000/dcsr-gdi at 100000
> create child: /dcsr at 20000000/dcsr-dddi at 120000
> create child: /dcsr at 20000000/dcsr-dcfg at 220000
> create child: /dcsr at 20000000/dcsr-clock at 221000
> create child: /dcsr at 20000000/dcsr-rcpm at 222000
> create child: /dcsr at 20000000/dcsr-ccp at 225000
> create child: /dcsr at 20000000/dcsr-fusectrl at 226000
> create child: /dcsr at 20000000/dcsr-dap at 300000
> create child: /dcsr at 20000000/dcsr-cstf at 350000
> create child: /dcsr at 20000000/dcsr-a7rom at 360000
> create child: /dcsr at 20000000/dcsr-a7cpu at 370000
> create child: /dcsr at 20000000/dcsr-a7cti at 378000
> create child: /dcsr at 20000000/dcsr-etm at 37c000
> create child: /dcsr at 20000000/dcsr-hugorom at 3a0000
> create child: /dcsr at 20000000/dcsr-etf at 3a1000
> create child: /dcsr at 20000000/dcsr-etr at 3a3000
> create child: /dcsr at 20000000/dcsr-cti at 3a4000
> create child: /dcsr at 20000000/dcsr-atbrepl at 3a8000
> create child: /dcsr at 20000000/dcsr-tsgen-ctrl at 3a9000
> create child: /dcsr at 20000000/dcsr-tsgen-read at 3aa000
> create child: /regulators/regulator at 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@samsung.com>
> > > > > ---
> > > > > 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 at lists.infradead.org
> > > > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > > > >
> > > > >
> > >
^ permalink raw reply [flat|nested] 48+ messages in thread* RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
2014-09-19 4:19 ` Dong Aisheng
(?)
@ 2014-09-19 5:20 ` Li.Xiubo at freescale.com
-1 siblings, 0 replies; 48+ messages in thread
From: Li.Xiubo @ 2014-09-19 5:20 UTC (permalink / raw)
Cc: Aisheng.Dong@freescale.com, Pankaj Dubey, arnd@arndb.de,
linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
kgene.kim@samsung.com, linux@arm.linux.org.uk,
naushad@samsung.com, tomasz.figa@gmail.com, joshi@samsung.com,
thomas.ab@samsung.com, vikas.sajjan@samsung.com,
chow.kim@samsung.com, lee.jones@linaro.org,
'Boris BREZILLON', 'Geert Uytterhoeven'
[...]
> > 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?
>
Yes, right.
For this case, we'd better create the pdev or dev manually for the first time
We use it, right ?
Thanks,
BRs
Xiubo
> 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@samsung.com>
> > > > > > ---
> > > > > > 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@lists.infradead.org
> > > > > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > > > > >
> > > > > >
> > > >
^ permalink raw reply [flat|nested] 48+ messages in thread* RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
@ 2014-09-19 5:20 ` Li.Xiubo at freescale.com
0 siblings, 0 replies; 48+ messages in thread
From: Li.Xiubo @ 2014-09-19 5:20 UTC (permalink / raw)
To: Aisheng.Dong@freescale.com
Cc: Aisheng.Dong@freescale.com, Pankaj Dubey, arnd@arndb.de,
linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
kgene.kim@samsung.com, linux@arm.linux.org.uk,
naushad@samsung.com, tomasz.figa@gmail.com, joshi@samsung.com,
thomas.ab@samsung.com, vikas.sajjan@samsung.com,
chow.kim@samsung.com, lee.jones@linaro.org,
'Boris BREZILLON', 'Geert Uytterhoeven',
'Stephen Warren'
[...]
> > 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?
>
Yes, right.
For this case, we'd better create the pdev or dev manually for the first time
We use it, right ?
Thanks,
BRs
Xiubo
> 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@samsung.com>
> > > > > > ---
> > > > > > 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@lists.infradead.org
> > > > > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > > > > >
> > > > > >
> > > >
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
@ 2014-09-19 5:20 ` Li.Xiubo at freescale.com
0 siblings, 0 replies; 48+ messages in thread
From: Li.Xiubo at freescale.com @ 2014-09-19 5:20 UTC (permalink / raw)
To: linux-arm-kernel
[...]
> > create child: /dcsr at 20000000/dcsr-atbrepl at 3a8000
> > create child: /dcsr at 20000000/dcsr-tsgen-ctrl at 3a9000
> > create child: /dcsr at 20000000/dcsr-tsgen-read at 3aa000
> > create child: /regulators/regulator at 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?
>
Yes, right.
For this case, we'd better create the pdev or dev manually for the first time
We use it, right ?
Thanks,
BRs
Xiubo
> 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@samsung.com>
> > > > > > ---
> > > > > > 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 at lists.infradead.org
> > > > > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > > > > >
> > > > > >
> > > >
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
2014-09-19 5:20 ` Li.Xiubo at freescale.com
(?)
@ 2014-09-19 5:46 ` Dong Aisheng
-1 siblings, 0 replies; 48+ messages in thread
From: Dong Aisheng @ 2014-09-19 5:46 UTC (permalink / raw)
To: Xiubo Li-B47053
Cc: Dong Aisheng-B29396, Pankaj Dubey, arnd@arndb.de,
linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
kgene.kim@samsung.com, linux@arm.linux.org.uk,
naushad@samsung.com, tomasz.figa@gmail.com, joshi@samsung.com,
thomas.ab@samsung.com, vikas.sajjan@samsung.com,
chow.kim@samsung.com, lee.jones@linaro.org,
'Boris BREZILLON', 'Geert Uytterhoeven'
On Fri, Sep 19, 2014 at 01:20:18PM +0800, Xiubo Li-B47053 wrote:
> [...]
> > > 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?
> >
>
> Yes, right.
>
> For this case, we'd better create the pdev or dev manually for the first time
> We use it, right ?
Yes, that's my understanding.
Regards
Dong Aisheng
>
> Thanks,
>
> BRs
> Xiubo
>
>
> > 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@samsung.com>
> > > > > > > ---
> > > > > > > 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@lists.infradead.org
> > > > > > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > > > > > >
> > > > > > >
> > > > >
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
@ 2014-09-19 5:46 ` Dong Aisheng
0 siblings, 0 replies; 48+ messages in thread
From: Dong Aisheng @ 2014-09-19 5:46 UTC (permalink / raw)
To: Xiubo Li-B47053
Cc: Dong Aisheng-B29396, Pankaj Dubey, arnd@arndb.de,
linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
kgene.kim@samsung.com, linux@arm.linux.org.uk,
naushad@samsung.com, tomasz.figa@gmail.com, joshi@samsung.com,
thomas.ab@samsung.com, vikas.sajjan@samsung.com,
chow.kim@samsung.com, lee.jones@linaro.org,
'Boris BREZILLON', 'Geert Uytterhoeven',
'Stephen Warren'
On Fri, Sep 19, 2014 at 01:20:18PM +0800, Xiubo Li-B47053 wrote:
> [...]
> > > 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?
> >
>
> Yes, right.
>
> For this case, we'd better create the pdev or dev manually for the first time
> We use it, right ?
Yes, that's my understanding.
Regards
Dong Aisheng
>
> Thanks,
>
> BRs
> Xiubo
>
>
> > 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@samsung.com>
> > > > > > > ---
> > > > > > > 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@lists.infradead.org
> > > > > > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > > > > > >
> > > > > > >
> > > > >
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
@ 2014-09-19 5:46 ` Dong Aisheng
0 siblings, 0 replies; 48+ messages in thread
From: Dong Aisheng @ 2014-09-19 5:46 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Sep 19, 2014 at 01:20:18PM +0800, Xiubo Li-B47053 wrote:
> [...]
> > > create child: /dcsr at 20000000/dcsr-atbrepl at 3a8000
> > > create child: /dcsr at 20000000/dcsr-tsgen-ctrl at 3a9000
> > > create child: /dcsr at 20000000/dcsr-tsgen-read at 3aa000
> > > create child: /regulators/regulator at 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?
> >
>
> Yes, right.
>
> For this case, we'd better create the pdev or dev manually for the first time
> We use it, right ?
Yes, that's my understanding.
Regards
Dong Aisheng
>
> Thanks,
>
> BRs
> Xiubo
>
>
> > 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@samsung.com>
> > > > > > > ---
> > > > > > > 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 at lists.infradead.org
> > > > > > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > > > > > >
> > > > > > >
> > > > >
^ permalink raw reply [flat|nested] 48+ messages in thread* RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
2014-09-19 5:46 ` Dong Aisheng
@ 2014-09-19 9:20 ` Pankaj Dubey
-1 siblings, 0 replies; 48+ messages in thread
From: Pankaj Dubey @ 2014-09-19 9:20 UTC (permalink / raw)
To: 'Dong Aisheng', 'Xiubo Li-B47053'
Cc: 'Dong Aisheng-B29396', arnd, linux-arm-kernel,
linux-samsung-soc, linux-kernel, kgene.kim, linux, naushad,
tomasz.figa, joshi, thomas.ab, vikas.sajjan, chow.kim, lee.jones,
'Boris BREZILLON', 'Geert Uytterhoeven',
'Stephen Warren'
Hi Dong and Xiubo,
On Friday, September 19, 2014, Dong Aisheng wrote,
> On Fri, Sep 19, 2014 at 01:20:18PM +0800, Xiubo Li-B47053 wrote:
> > [...]
> > > > 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?
> > >
> >
> > Yes, right.
> >
> > For this case, we'd better create the pdev or dev manually for the
> > first time We use it, right ?
>
> Yes, that's my understanding.
>
Thanks for all your inputs.
First let me clarify that the main purpose of this patch, we introduced this
patch mainly because
if some HW IP is having secondary compatibility as "syscon", syscon driver
was getting probed
and since there can be only on platform_device corresponding to one
device_node, actual driver
corresponding to that HW IP was not getting probed. So we wanted to decouple
syscon from platform
device, and any such driver should be able to become syscon provider. Please
see discussion here [1].
[1]: https://lkml.org/lkml/2014/6/17/331
But while doing so eventually this patch was also able to cover the feature
of early availability of syscon
lookup APIs.
As both of you pointed out that if we use "of_find_device_by_node" and it
won't work for early users
of syscon. I also agree with that, but it just skipped from my mind while
suggesting that approach.
I reconsidered again and made following changes and tested it. Now I am able
to use syscon_lookup_by_xxxxx
APIs, very early (before dt_machine_init) as well as after
of_platform_populate. So please review and if
it is acceptable I will post next version of this patch.
Basic idea is, check for device_node pointer in of_syscon_register, if
device corresponding to that device_node
is already populated and available use "of_find_device_by_node" else create
a dummy platform device and then
use it.
----------
static struct syscon *of_syscon_register(struct device_node *np)
{
+ struct platform_device *pdev = NULL;
struct syscon *syscon;
struct regmap *regmap;
void __iomem *base;
+ struct platform_device dummy_pdev = {
+ .name = "dummy-syscon",
+ .id = -1,
+ };
+
if (!of_device_is_compatible(np, "syscon"))
return ERR_PTR(-EINVAL);
@@ -141,8 +149,22 @@ static struct syscon *of_syscon_register(struct
device_node *np)
base = of_iomap(np, 0);
if (!base)
return ERR_PTR(-ENOMEM);
+
+ if (!of_device_is_available(np) ||
+ of_node_test_and_set_flag(np, OF_POPULATED)) {
+ /* if device is already populated and avaiable then use it
*/
+ pdev = of_find_device_by_node(np);
+ if (!(&pdev->dev))
+ return ERR_PTR(-ENODEV);
+
+ regmap = regmap_init_mmio(&pdev->dev, base,
&syscon_regmap_config);
+ } else {
+ /* for early users let's create dummy syscon device and use
it */
+ device_initialize(&dummy_pdev.dev);
+ dummy_pdev.dev.of_node = np;
+ regmap = regmap_init_mmio(&dummy_pdev.dev, base,
&syscon_regmap_config);
+ }
- regmap = regmap_init_mmio(NULL, base, &syscon_regmap_config);
if (IS_ERR(regmap)) {
pr_err("regmap init failed\n");
return ERR_CAST(regmap);
--
Thanks,
Pankaj Dubey
> Regards
> Dong Aisheng
>
> >
> > Thanks,
> >
> > BRs
> > Xiubo
> >
> >
> > > 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@samsung.com>
> > > > > > > > ---
> > > > > > > > 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@lists.infradead.org
> > > > > > > > > > > > http://lists.infradead.org/mailman/listinfo/linux-
> > > > > > > > > > > > arm-kernel
> > > > > > > > > >
> > > > > > > >
> > > > > >
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
@ 2014-09-19 9:20 ` Pankaj Dubey
0 siblings, 0 replies; 48+ messages in thread
From: Pankaj Dubey @ 2014-09-19 9:20 UTC (permalink / raw)
To: linux-arm-kernel
Hi Dong and Xiubo,
On Friday, September 19, 2014, Dong Aisheng wrote,
> On Fri, Sep 19, 2014 at 01:20:18PM +0800, Xiubo Li-B47053 wrote:
> > [...]
> > > > create child: /dcsr at 20000000/dcsr-atbrepl at 3a8000
> > > > create child: /dcsr at 20000000/dcsr-tsgen-ctrl at 3a9000
> > > > create child: /dcsr at 20000000/dcsr-tsgen-read at 3aa000
> > > > create child: /regulators/regulator at 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?
> > >
> >
> > Yes, right.
> >
> > For this case, we'd better create the pdev or dev manually for the
> > first time We use it, right ?
>
> Yes, that's my understanding.
>
Thanks for all your inputs.
First let me clarify that the main purpose of this patch, we introduced this
patch mainly because
if some HW IP is having secondary compatibility as "syscon", syscon driver
was getting probed
and since there can be only on platform_device corresponding to one
device_node, actual driver
corresponding to that HW IP was not getting probed. So we wanted to decouple
syscon from platform
device, and any such driver should be able to become syscon provider. Please
see discussion here [1].
[1]: https://lkml.org/lkml/2014/6/17/331
But while doing so eventually this patch was also able to cover the feature
of early availability of syscon
lookup APIs.
As both of you pointed out that if we use "of_find_device_by_node" and it
won't work for early users
of syscon. I also agree with that, but it just skipped from my mind while
suggesting that approach.
I reconsidered again and made following changes and tested it. Now I am able
to use syscon_lookup_by_xxxxx
APIs, very early (before dt_machine_init) as well as after
of_platform_populate. So please review and if
it is acceptable I will post next version of this patch.
Basic idea is, check for device_node pointer in of_syscon_register, if
device corresponding to that device_node
is already populated and available use "of_find_device_by_node" else create
a dummy platform device and then
use it.
----------
static struct syscon *of_syscon_register(struct device_node *np)
{
+ struct platform_device *pdev = NULL;
struct syscon *syscon;
struct regmap *regmap;
void __iomem *base;
+ struct platform_device dummy_pdev = {
+ .name = "dummy-syscon",
+ .id = -1,
+ };
+
if (!of_device_is_compatible(np, "syscon"))
return ERR_PTR(-EINVAL);
@@ -141,8 +149,22 @@ static struct syscon *of_syscon_register(struct
device_node *np)
base = of_iomap(np, 0);
if (!base)
return ERR_PTR(-ENOMEM);
+
+ if (!of_device_is_available(np) ||
+ of_node_test_and_set_flag(np, OF_POPULATED)) {
+ /* if device is already populated and avaiable then use it
*/
+ pdev = of_find_device_by_node(np);
+ if (!(&pdev->dev))
+ return ERR_PTR(-ENODEV);
+
+ regmap = regmap_init_mmio(&pdev->dev, base,
&syscon_regmap_config);
+ } else {
+ /* for early users let's create dummy syscon device and use
it */
+ device_initialize(&dummy_pdev.dev);
+ dummy_pdev.dev.of_node = np;
+ regmap = regmap_init_mmio(&dummy_pdev.dev, base,
&syscon_regmap_config);
+ }
- regmap = regmap_init_mmio(NULL, base, &syscon_regmap_config);
if (IS_ERR(regmap)) {
pr_err("regmap init failed\n");
return ERR_CAST(regmap);
--
Thanks,
Pankaj Dubey
> Regards
> Dong Aisheng
>
> >
> > Thanks,
> >
> > BRs
> > Xiubo
> >
> >
> > > 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@samsung.com>
> > > > > > > > ---
> > > > > > > > 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 at lists.infradead.org
> > > > > > > > > > > > http://lists.infradead.org/mailman/listinfo/linux-
> > > > > > > > > > > > arm-kernel
> > > > > > > > > >
> > > > > > > >
> > > > > >
^ permalink raw reply [flat|nested] 48+ messages in thread