RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
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@2000/dcsr-atbrepl@3a8000
> > > >create child: /dcsr@2000/dcsr-tsgen-ctrl@3a9000
> > > >create child: /dcsr@2000/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_x
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 (!(>dev))
+ return ERR_PTR(-ENODEV);
+
+ regmap = regmap_init_mmio(>dev, base,
_regmap_config);
+ } else {
+ /* for early users let's create dummy syscon device and use
it */
+ device_initialize(_pdev.dev);
+ dummy_pdev.dev.of_node = np;
+ regmap = regmap_init_mmio(_pdev.dev, base,
_regmap_config);
+ }
- regmap = regmap_init_mmio(NULL, base, _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,
_regmap_config);
> > > > > > + pdev = of_find_device_by_node(np);
> > > > > > + if (!(>dev))
> > > > > > + return
Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
On Fri, Sep 19, 2014 at 01:20:18PM +0800, Xiubo Li-B47053 wrote:
> [...]
> > >create child: /dcsr@2000/dcsr-atbrepl@3a8000
> > >create child: /dcsr@2000/dcsr-tsgen-ctrl@3a9000
> > >create child: /dcsr@2000/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, _regmap_config);
> > > > > + pdev = of_find_device_by_node(np);
> > > > > + if (!(>dev))
> > > > > + return ERR_PTR(-ENODEV);
> > > > > +
> > > > > + regmap = regmap_init_mmio(>dev, base,
> > _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
> > > > > > > ---
> > > > > > > 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"))
> > > > >
Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
On Fri, Sep 19, 2014 at 01:20:18PM +0800, Xiubo Li-B47053 wrote:
[...]
create child: /dcsr@2000/dcsr-atbrepl@3a8000
create child: /dcsr@2000/dcsr-tsgen-ctrl@3a9000
create child: /dcsr@2000/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.du...@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
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
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@2000/dcsr-atbrepl@3a8000
create child: /dcsr@2000/dcsr-tsgen-ctrl@3a9000
create child: /dcsr@2000/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_x
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);
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
[...]
> >create child: /dcsr@2000/dcsr-atbrepl@3a8000
> >create child: /dcsr@2000/dcsr-tsgen-ctrl@3a9000
> >create child: /dcsr@2000/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, _regmap_config);
> > > > + pdev = of_find_device_by_node(np);
> > > > + if (!(>dev))
> > > > + return ERR_PTR(-ENODEV);
> > > > +
> > > > + regmap = regmap_init_mmio(>dev, base,
> _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
> > > > > > ---
> > > > > > 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 !=
Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
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@120 >create child: /soc/smmu@128 >create child: /soc/smmu@130 >create child: /soc/smmu@138 >create child: /soc/interrupt-controller@140 >create child: /soc/tzasc@150 > of_platform_bus_create() - skipping /soc/tzasc@150, no compatible prop >create child: /soc/ifc@153 >create child: /soc/ifc@153/nor@0,0 >create child: /soc/ifc@153/board-control@2,0 >create child: /soc/dcfg@1ee > syscon 1ee.dcfg: regmap [mem 0x01ee-0x01ee] registered >create child: /soc/quadspi@155 >create child: /soc/esdhc@156 >create child: /soc/scfg@157 >create child: /soc/crypto@170 >create child: /soc/sfp@1e8 > of_platform_bus_create() - skipping /soc/sfp@1e8, no compatible prop >create child: /soc/snvs@1e9 > of_platform_bus_create() - skipping /soc/snvs@1e9, no compatible prop >create child: /soc/serdes1@1ea > of_platform_bus_create() - skipping /soc/serdes1@1ea, no compatible prop >create child: /soc/clocking@1ee1000 >create child: /soc/rcpm@1ee2000 >create child: /soc/dspi@210 >create child: /soc/dspi@211 >create child: /soc/i2c@218 >create child: /soc/i2c@219 >create child: /soc/i2c@21a >create child: /soc/serial@21c0500 >create child: /soc/serial@21c0600 >create child: /soc/serial@21d0500 >create child: /soc/serial@21d0600 >create child: /soc/gpio@230 >create child: /soc/gpio@231 >create child: /soc/gpio@232 >create child: /soc/gpio@233 >create child: /soc/uqe@240 >create child: /soc/uqe@240/qeic@80 >create child: /soc/uqe@240/ucc@2000 > irq: no irq domain found for /soc/uqe@240/qeic@80 ! >create child: /soc/uqe@240/ucc@2200 > irq: no irq domain found for /soc/uqe@240/qeic@80 ! >create child: /soc/uqe@240/muram@1 >create child: /soc/uqe@240/si@700 >create child: /soc/uqe@240/siram@1000 >create child: /soc/serial@295 >create child: /soc/serial@296 >create child: /soc/serial@297 >create child: /soc/serial@298 >create child: /soc/serial@299 >create child: /soc/serial@29a >create child: /soc/ftm0_1@29d >create child: /soc/ftm@29f > of_platform_bus_create() - skipping /soc/ftm@29f, no compatible prop >create child: /soc/ftm@2a0 >create child: /soc/ftm@2a1 > of_platform_bus_create() - skipping /soc/ftm@2a1, no compatible prop >create child: /soc/ftm@2a2 > of_platform_bus_create() - skipping /soc/ftm@2a2, no compatible prop >create child: /soc/ftm@2a3 >create child: /soc/ftm@2a4 >create child: /soc/wdog@2ad >create child: /soc/sai@2b6 >create child: /soc/edma@2c0 >create child:
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
> > 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@120 create child: /soc/smmu@128 create child: /soc/smmu@130 create child: /soc/smmu@138 create child: /soc/interrupt-controller@140 create child: /soc/tzasc@150 of_platform_bus_create() - skipping /soc/tzasc@150, no compatible prop create child: /soc/ifc@153 create child: /soc/ifc@153/nor@0,0 create child: /soc/ifc@153/board-control@2,0 create child: /soc/dcfg@1ee syscon 1ee.dcfg: regmap [mem 0x01ee-0x01ee] registered create child: /soc/quadspi@155 create child: /soc/esdhc@156 create child: /soc/scfg@157 create child: /soc/crypto@170 create child: /soc/sfp@1e8 of_platform_bus_create() - skipping /soc/sfp@1e8, no compatible prop create child: /soc/snvs@1e9 of_platform_bus_create() - skipping /soc/snvs@1e9, no compatible prop create child: /soc/serdes1@1ea of_platform_bus_create() - skipping /soc/serdes1@1ea, no compatible prop create child: /soc/clocking@1ee1000 create child: /soc/rcpm@1ee2000 create child: /soc/dspi@210 create child: /soc/dspi@211 create child: /soc/i2c@218 create child: /soc/i2c@219 create child: /soc/i2c@21a create child: /soc/serial@21c0500 create child: /soc/serial@21c0600 create child: /soc/serial@21d0500 create child: /soc/serial@21d0600 create child: /soc/gpio@230 create child: /soc/gpio@231 create child: /soc/gpio@232 create child: /soc/gpio@233 create child: /soc/uqe@240 create child: /soc/uqe@240/qeic@80 create child: /soc/uqe@240/ucc@2000 irq: no irq domain found for /soc/uqe@240/qeic@80 ! create child: /soc/uqe@240/ucc@2200 irq: no irq domain found for /soc/uqe@240/qeic@80 ! create child: /soc/uqe@240/muram@1 create child: /soc/uqe@240/si@700 create child: /soc/uqe@240/siram@1000 create child: /soc/serial@295 create child: /soc/serial@296 create child: /soc/serial@297 create child: /soc/serial@298 create child: /soc/serial@299 create child: /soc/serial@29a create child: /soc/ftm0_1@29d create child: /soc/ftm@29f of_platform_bus_create() - skipping /soc/ftm@29f, no compatible prop create child: /soc/ftm@2a0 create child: /soc/ftm@2a1 of_platform_bus_create() - skipping /soc/ftm@2a1, no compatible prop create child: /soc/ftm@2a2 of_platform_bus_create() - skipping /soc/ftm@2a2, no compatible prop create child: /soc/ftm@2a3 create child: /soc/ftm@2a4 create child: /soc/wdog@2ad create child: /soc/sai@2b6 create child: /soc/edma@2c0 create child: /soc/dcu@2ce create child: /soc/mdio@2d24000 create child: /soc/ptp_clock@2d10e00 create child: /soc/ethernet@2d1 create child: /soc/ethernet@2d5 create child: /soc/ethernet@2d9 create child: /soc/usb@860 create child: /soc/usb3@310 create child: /soc/can@2a7 create child:
Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
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,
> > > _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,
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
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
> > > ---
> > > 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-ker...@lists.infradead.org
> > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > >
> > >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
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,
> > _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, _regmap_config);
+ pdev = of_find_device_by_node(np);
+ if (!(>dev))
+ return ERR_PTR(-ENODEV);
+
+ regmap = regmap_init_mmio(>dev, base, _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
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
> > ---
> > 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-ker...@lists.infradead.org
> > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > >
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
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,
> _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
> ---
> 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
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
Hi,
[...]
> > 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.
>
>
> 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.
>
Another question:
Then the regmap core cannot deal with the following issue:
Like the patch adding the endianness support for syscon:
[PATCH] mfd: syscon: binding: Add syscon endianness support.
So here just register one dummy syscon device is a good choice I do
think.
:)
Thanks,
BRs
Xiubo
> Signed-off-by: Pankaj Dubey
> ---
> 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-ker...@lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > >
--
To unsubscribe from this list: send the line "unsubscribe
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
Hi Pankaj,
One more question:
For example:
regmap_read()
> _regmap_read()
> 2112 #ifdef LOG_DEVICE
> 2113 if (strcmp(dev_name(map->dev), LOG_DEVICE)
== 0)
> 2114 dev_info(map->dev, "%x => %x\n",
reg, *val);
> 2115 #endif
In dev_name(map->dev) will also encounter the same crash core trace like in
regmap_get_val_endian(dev, ...).
Could there be another method, such as using 'dummy dev' instead of 'NULL dev' ?
Just one suggestion.
BRs
Xiubo
> -Original Message-
> From: Pankaj Dubey [mailto:pankaj.du...@samsung.com]
> Sent: Thursday, September 18, 2014 2:03 PM
> To: Dong Aisheng-B29396
> Cc: linux-arm-ker...@lists.infradead.org; linux-samsung-...@vger.kernel.org;
> linux-kernel@vger.kernel.org; kgene@samsung.com; li...@arm.linux.org.uk;
> a...@arndb.de; naus...@samsung.com; tomasz.f...@gmail.com; jo...@samsung.com;
> thomas...@samsung.com; vikas.saj...@samsung.com; chow@samsung.com;
> lee.jo...@linaro.org; 'Boris BREZILLON'; Xiubo Li-B47053; 'Geert
> Uytterhoeven';
> 'Stephen Warren'
> Subject: RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform
> devices
>
> 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,
> _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.
>
>
> 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
> ---
> 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
>
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
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,
_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.
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
---
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
> > >
> > > >
> > > >
> > >
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
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@120 create child: /soc/smmu@128 create child: /soc/smmu@130 create child: /soc/smmu@138 create child: /soc/interrupt-controller@140 create child: /soc/tzasc@150 of_platform_bus_create() - skipping /soc/tzasc@150, no compatible prop create child: /soc/ifc@153 create child: /soc/ifc@153/nor@0,0 create child: /soc/ifc@153/board-control@2,0 create child: /soc/dcfg@1ee syscon 1ee.dcfg: regmap [mem 0x01ee-0x01ee] registered create child: /soc/quadspi@155 create child: /soc/esdhc@156 create child: /soc/scfg@157 create child: /soc/crypto@170 create child: /soc/sfp@1e8 of_platform_bus_create() - skipping /soc/sfp@1e8, no compatible prop create child: /soc/snvs@1e9 of_platform_bus_create() - skipping /soc/snvs@1e9, no compatible prop create child: /soc/serdes1@1ea of_platform_bus_create() - skipping /soc/serdes1@1ea, no compatible prop create child: /soc/clocking@1ee1000 create child: /soc/rcpm@1ee2000 create child: /soc/dspi@210 create child: /soc/dspi@211 create child: /soc/i2c@218 create child: /soc/i2c@219 create child: /soc/i2c@21a create child: /soc/serial@21c0500 create child: /soc/serial@21c0600 create child: /soc/serial@21d0500 create child: /soc/serial@21d0600 create child: /soc/gpio@230 create child: /soc/gpio@231 create child: /soc/gpio@232 create child: /soc/gpio@233 create child: /soc/uqe@240 create child: /soc/uqe@240/qeic@80 create child: /soc/uqe@240/ucc@2000 irq: no irq domain found for /soc/uqe@240/qeic@80 ! create child: /soc/uqe@240/ucc@2200 irq: no irq domain found for /soc/uqe@240/qeic@80 ! create child: /soc/uqe@240/muram@1 create child: /soc/uqe@240/si@700 create child: /soc/uqe@240/siram@1000 create child: /soc/serial@295 create child: /soc/serial@296 create child: /soc/serial@297 create child: /soc/serial@298 create child: /soc/serial@299 create child: /soc/serial@29a create child: /soc/ftm0_1@29d create child: /soc/ftm@29f of_platform_bus_create() - skipping /soc/ftm@29f, no compatible prop create child: /soc/ftm@2a0 create child: /soc/ftm@2a1 of_platform_bus_create() - skipping /soc/ftm@2a1, no compatible prop create child: /soc/ftm@2a2 of_platform_bus_create() - skipping /soc/ftm@2a2, no compatible prop create child: /soc/ftm@2a3 create child: /soc/ftm@2a4 create child: /soc/wdog@2ad create child: /soc/sai@2b6 create child: /soc/edma@2c0 create child: /soc/dcu@2ce create child: /soc/mdio@2d24000 create child: /soc/ptp_clock@2d10e00 create child: /soc/ethernet@2d1 create child: /soc/ethernet@2d5 create child: /soc/ethernet@2d9 create child: /soc/usb@860 create child: /soc/usb3@310 create child: /soc/can@2a7 create child: /soc/can@2a8 create child: /soc/can@2a9 create child: /soc/can@2aa
Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
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@120 create child: /soc/smmu@128 create child: /soc/smmu@130 create child: /soc/smmu@138 create child: /soc/interrupt-controller@140 create child: /soc/tzasc@150 of_platform_bus_create() - skipping /soc/tzasc@150, no compatible prop create child: /soc/ifc@153 create child: /soc/ifc@153/nor@0,0 create child: /soc/ifc@153/board-control@2,0 create child: /soc/dcfg@1ee syscon 1ee.dcfg: regmap [mem 0x01ee-0x01ee] registered create child: /soc/quadspi@155 create child: /soc/esdhc@156 create child: /soc/scfg@157 create child: /soc/crypto@170 create child: /soc/sfp@1e8 of_platform_bus_create() - skipping /soc/sfp@1e8, no compatible prop create child: /soc/snvs@1e9 of_platform_bus_create() - skipping /soc/snvs@1e9, no compatible prop create child: /soc/serdes1@1ea of_platform_bus_create() - skipping /soc/serdes1@1ea, no compatible prop create child: /soc/clocking@1ee1000 create child: /soc/rcpm@1ee2000 create child: /soc/dspi@210 create child: /soc/dspi@211 create child: /soc/i2c@218 create child: /soc/i2c@219 create child: /soc/i2c@21a create child: /soc/serial@21c0500 create child: /soc/serial@21c0600 create child: /soc/serial@21d0500 create child: /soc/serial@21d0600 create child: /soc/gpio@230 create child: /soc/gpio@231 create child: /soc/gpio@232 create child: /soc/gpio@233 create child: /soc/uqe@240 create child: /soc/uqe@240/qeic@80 create child: /soc/uqe@240/ucc@2000 irq: no irq domain found for /soc/uqe@240/qeic@80 ! create child: /soc/uqe@240/ucc@2200 irq: no irq domain found for /soc/uqe@240/qeic@80 ! create child: /soc/uqe@240/muram@1 create child: /soc/uqe@240/si@700 create child: /soc/uqe@240/siram@1000 create child: /soc/serial@295 create child: /soc/serial@296 create child: /soc/serial@297 create child: /soc/serial@298 create child: /soc/serial@299 create child: /soc/serial@29a create child: /soc/ftm0_1@29d create child: /soc/ftm@29f of_platform_bus_create() - skipping /soc/ftm@29f, no compatible prop create child: /soc/ftm@2a0 create child: /soc/ftm@2a1 of_platform_bus_create() - skipping /soc/ftm@2a1, no compatible prop create child: /soc/ftm@2a2 of_platform_bus_create() - skipping /soc/ftm@2a2, no compatible prop create child: /soc/ftm@2a3 create child: /soc/ftm@2a4 create child: /soc/wdog@2ad create child: /soc/sai@2b6 create child: /soc/edma@2c0 create child: /soc/dcu@2ce create child: /soc/mdio@2d24000 create child: /soc/ptp_clock@2d10e00 create child: /soc/ethernet@2d1 create child: /soc/ethernet@2d5 create child: /soc/ethernet@2d9
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
[...]
create child: /dcsr@2000/dcsr-atbrepl@3a8000
create child: /dcsr@2000/dcsr-tsgen-ctrl@3a9000
create child: /dcsr@2000/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.du...@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 !=
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
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.
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.du...@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
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
Hi Pankaj,
One more question:
For example:
regmap_read()
_regmap_read()
2112 #ifdef LOG_DEVICE
2113 if (strcmp(dev_name(map-dev), LOG_DEVICE)
== 0)
2114 dev_info(map-dev, %x = %x\n,
reg, *val);
2115 #endif
In dev_name(map-dev) will also encounter the same crash core trace like in
regmap_get_val_endian(dev, ...).
Could there be another method, such as using 'dummy dev' instead of 'NULL dev' ?
Just one suggestion.
BRs
Xiubo
-Original Message-
From: Pankaj Dubey [mailto:pankaj.du...@samsung.com]
Sent: Thursday, September 18, 2014 2:03 PM
To: Dong Aisheng-B29396
Cc: linux-arm-ker...@lists.infradead.org; linux-samsung-...@vger.kernel.org;
linux-kernel@vger.kernel.org; kgene@samsung.com; li...@arm.linux.org.uk;
a...@arndb.de; naus...@samsung.com; tomasz.f...@gmail.com; jo...@samsung.com;
thomas...@samsung.com; vikas.saj...@samsung.com; chow@samsung.com;
lee.jo...@linaro.org; 'Boris BREZILLON'; Xiubo Li-B47053; 'Geert
Uytterhoeven';
'Stephen Warren'
Subject: RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform
devices
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.
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.du...@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
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
Hi,
[...]
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.
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.
Another question:
Then the regmap core cannot deal with the following issue:
Like the patch adding the endianness support for syscon:
[PATCH] mfd: syscon: binding: Add syscon endianness support.
So here just register one dummy syscon device is a good choice I do
think.
:)
Thanks,
BRs
Xiubo
Signed-off-by: Pankaj Dubey pankaj.du...@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-ker...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
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.du...@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
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.du...@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-ker...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
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.
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
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.du...@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-ker...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
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);
---
Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
On Thu, Sep 18, 2014 at 08:59:32AM +0530, Pankaj Dubey wrote: > +CC: Dong Aisheng > > Hi Arnd, > > On Wednesday, September 17, 2014, Arnd Bergmann wrote, > > > V2 of this patchset and related discussion can be found here [1]. > > > > > > Changes since v2: > > > - Added back platform device support from syscon, with one change that > > >syscon will not be probed for DT based platform. > > > - Added back syscon_regmap_lookup_by_pdevname API so that non-DT base > > >users of syscon will not be broken. > > > - Removed unwanted change in syscon.h. > > > - Modified Signed-off-by list, added Suggested-by of Tomasz Figa and > > >Arnd Bergmann. > > > - Added Tested-by of Vivek Gautam for testing on Exynos platform. > > > > Looks fine. Provided you can figure out the problem that Dong Aisheng > reported, > > please add my > > > > Acked-by: Arnd Bergmann > > > > Thanks. > After he reported I have again done code walk-through of regmap_init_mmio > and > could not see any such fatal error. At the same time I have replied to Dong > Aisheng, > asking for more details and waiting for his reply. > I just replied and gave the log. Regards Dong Aisheng > > > -} > > > +static struct syscon *of_syscon_register(struct device_node *np); > > > > > > > One minor comment: please avoid doing forward declarations of local > functions. It > > took me a while to understand what is going on because I expect all > functions to be > > ordered such that they only get called by functions below, and don't need > this. > > > > Just move of_syscon_register() here directly. > > > > OK, I will remove forward declaration of "of_syscon_register" and respin > patch once again. > > > Arnd > > Thanks, > Pankaj Dubey > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
On Wed, Sep 17, 2014 at 04:50:50PM +0530, Pankaj Dubey wrote:
> Hi,
>
> On Wednesday, September 17, 2014, Dong Aisheng Wrote,
> > >
> > > +static struct syscon *of_syscon_register(struct device_node *np) {
> > > + struct syscon *syscon;
> > > + struct regmap *regmap;
> > > + void __iomem *base;
> > > +
> > > + if (!of_device_is_compatible(np, "syscon"))
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
> > > + if (!syscon)
> > > + return ERR_PTR(-ENOMEM);
> > > +
> > > + base = of_iomap(np, 0);
> > > + if (!base)
> > > + return ERR_PTR(-ENOMEM);
> > > +
> > > + regmap = regmap_init_mmio(NULL, base, _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?
> > I just tested on MX6SX SDB board and it seemed crashed at here in
> regmap_init
> > function.
> >
>
> Can you please provide crash log which can give more information about the
> crash?
>
My crash log is:
[0.225148] Unable to handle kernel NULL pointer dereference at virtual
address 01d4
[0.233383] pgd = 80004000
[0.236185] [01d4] *pgd=
[0.239873] Internal error: Oops: 5 [#1] SMP ARM
[0.244588] Modules linked in:
[0.247753] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
3.17.0-rc4-next-20140915-6-g8ba2dd0-dirty #421
[0.257342] task: bd878000 ti: bd88 task.ti: bd88
[0.262848] PC is at regmap_init+0x21c/0xab4
[0.267221] LR is at vprintk_emit+0x254/0x5e0
[0.271677] pc : [<80389420>]lr : [<8006a4f8>]psr: 6153
[0.271677] sp : bd881ae0 ip : bd881a48 fp : bd881b1c
[0.283354] r10: r9 : bd8e1850 r8 : 0003
[0.288678] r7 : r6 : 8098fccc r5 : 8098ee0c r4 : bd8f4e00
[0.295307] r3 : bd878000 r2 : 01e5 r1 : 806ee8f0 r0 : 808762c0
[0.301938] Flags: nZCv IRQs on FIQs off Mode SVC_32 ISA ARM Segment
kernel
[0.309438] Control: 10c5387d Table: 8000404a DAC: 0015
[0.315284] Process swapper/0 (pid: 1, stack limit = 0xbd880240)
[0.321391] Stack: (0xbd881ae0 to 0xbd882000)
[0.325851] 1ae0: bd8e1850 bd881b1c bd881af8 8038f580 8098fccc
be7d49b8
[0.334136] 1b00: 8098fca4 bd917000 bd8e1850 bd881b34 bd881b20
8038f68c 80389210
[0.342422] 1b20: bd91b480 8098fcc4 bd881b5c bd881b38 80398b24 8038f664
bd91ac10
[0.350708] 1b40: be7d4bc0 bd917010 bd917000 bd8e1850 bd881bac bd881b60
803185a4 80398a54
[0.358993] 1b60: bd881b94 bd881b70
[0.367277] 1b80: bd917010 80983a64 bd916c10
80983a64
[0.375563] 1ba0: bd881bc4 bd881bb0 8037bb44 80318510 811c65f0 bd917010
bd881bec bd881bc8
[0.383848] 1bc0: 8037a104 8037bb1c 80983a64 bd917010 8037a23c bd916c10
[0.392134] 1be0: bd881c04 bd881bf0 8037a284 8037a004 bd917010
bd881c2c bd881c08
[0.400419] 1c00: 80378614 8037a248 bd8038d8 bd8af7d4 bd916c10 bd917010
bd917044 8098e308
[0.408704] 1c20: bd881c4c bd881c30 80379fb8 803785c0 bd803800 bd917018
bd917010 8098e308
[0.416990] 1c40: bd881c6c bd881c50 8037961c 80379f48 bd917018
bd917010
[0.425275] 1c60: bd881ca4 bd881c70 80377904 80379598 bd881cc8 bd881cc0
bd881cb8 bd881c88
[0.433560] 1c80: be7d4bc0 bd916c10 0001 bd917000 be7d4c10
bd881cb4 bd881ca8
[0.441845] 1ca0: 804f1708 803774e8 bd881cfc bd881cb8 804f1f2c 804f16dc
bd881cdc bd881cc8
[0.450130] 1cc0: 806b6d7c 80060478 809b0220 6153 bd881cfc
be7d4bc0
[0.458415] 1ce0: 0001 8070bb80 bd916c10 bd881d5c bd881d00
804f2068 804f1e5c
[0.466700] 1d00: bd878000 a153 809b0220 bd881d2c bd881d20
80060480 80060284
[0.474985] 1d20: bd881d44 bd881d30 806b6d7c 80060478 be7d4bc0
be7d49b8
[0.483270] 1d40: 0001 8070bb80 bd916c10 bd881dbc bd881d60
804f20c4 804f1f8c
[0.491556] 1d60: 0001 6153 809b0220 bd881d8c bd881d80
80060480 80060284
[0.499841] 1d80: bd881da4 bd881d90 806b6d7c 80060478 be7d46f8 be7d49b8
be7cf764
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
+CC: Dong Aisheng Hi Arnd, On Wednesday, September 17, 2014, Arnd Bergmann wrote, > > V2 of this patchset and related discussion can be found here [1]. > > > > Changes since v2: > > - Added back platform device support from syscon, with one change that > >syscon will not be probed for DT based platform. > > - Added back syscon_regmap_lookup_by_pdevname API so that non-DT base > >users of syscon will not be broken. > > - Removed unwanted change in syscon.h. > > - Modified Signed-off-by list, added Suggested-by of Tomasz Figa and > >Arnd Bergmann. > > - Added Tested-by of Vivek Gautam for testing on Exynos platform. > > Looks fine. Provided you can figure out the problem that Dong Aisheng reported, > please add my > > Acked-by: Arnd Bergmann > Thanks. After he reported I have again done code walk-through of regmap_init_mmio and could not see any such fatal error. At the same time I have replied to Dong Aisheng, asking for more details and waiting for his reply. > > -} > > +static struct syscon *of_syscon_register(struct device_node *np); > > > > One minor comment: please avoid doing forward declarations of local functions. It > took me a while to understand what is going on because I expect all functions to be > ordered such that they only get called by functions below, and don't need this. > > Just move of_syscon_register() here directly. > OK, I will remove forward declaration of "of_syscon_register" and respin patch once again. > Arnd Thanks, Pankaj Dubey -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
On Wednesday 17 September 2014, Pankaj Dubey wrote: > --- > V2 of this patchset and related discussion can be found here [1]. > > Changes since v2: > - Added back platform device support from syscon, with one change that >syscon will not be probed for DT based platform. > - Added back syscon_regmap_lookup_by_pdevname API so that non-DT base >users of syscon will not be broken. > - Removed unwanted change in syscon.h. > - Modified Signed-off-by list, added Suggested-by of Tomasz Figa and >Arnd Bergmann. > - Added Tested-by of Vivek Gautam for testing on Exynos platform. Looks fine. Provided you can figure out the problem that Dong Aisheng reported, please add my Acked-by: Arnd Bergmann > -} > +static struct syscon *of_syscon_register(struct device_node *np); > One minor comment: please avoid doing forward declarations of local functions. It took me a while to understand what is going on because I expect all functions to be ordered such that they only get called by functions below, and don't need this. Just move of_syscon_register() here directly. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
Hi,
On Wednesday, September 17, 2014, Dong Aisheng Wrote,
> >
> > +static struct syscon *of_syscon_register(struct device_node *np) {
> > + struct syscon *syscon;
> > + struct regmap *regmap;
> > + void __iomem *base;
> > +
> > + if (!of_device_is_compatible(np, "syscon"))
> > + return ERR_PTR(-EINVAL);
> > +
> > + syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
> > + if (!syscon)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + base = of_iomap(np, 0);
> > + if (!base)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + regmap = regmap_init_mmio(NULL, base, _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.
> I just tested on MX6SX SDB board and it seemed crashed at here in
regmap_init
> function.
>
Can you please provide crash log which can give more information about the
crash?
Thanks,
Pankaj Dubey
> Regards
> Dong Aisheng
>
> > + if (IS_ERR(regmap)) {
> > + pr_err("regmap init failed\n");
> > + return ERR_CAST(regmap);
> > + }
> > +
> > + syscon->regmap = regmap;
> > + syscon->np = np;
> > +
> > + spin_lock(_list_slock);
> > + list_add_tail(>list, _list);
> > + spin_unlock(_list_slock);
> > +
> > + return syscon;
> > +}
> > +
> > static int syscon_probe(struct platform_device *pdev) {
> > struct device *dev = >dev;
> > @@ -167,7 +204,6 @@ static struct platform_driver syscon_driver = {
> > .driver = {
> > .name = "syscon",
> > .owner = THIS_MODULE,
> > - .of_match_table = of_syscon_match,
> > },
> > .probe = syscon_probe,
> > .id_table = syscon_ids,
> > --
> > 1.7.9.5
> >
> >
> > ___
> > linux-arm-kernel mailing list
> > linux-arm-ker...@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
On Wed, Sep 17, 2014 at 12:01:50PM +0530, Pankaj Dubey wrote:
> Currently a syscon entity can be only registered directly through a
> platform device that binds to a dedicated syscon driver. However in
> certain use cases it is desirable to make a device used with another
> driver a syscon interface provider.
>
> For example, certain SoCs (e.g. Exynos) contain system controller
> blocks which perform various functions such as power domain control,
> CPU power management, low power mode control, but in addition contain
> certain IP integration glue, such as various signal masks,
> coprocessor power control, etc. In such case, there is a need to have
> a dedicated driver for such system controller but also share registers
> with other drivers. The latter is where the syscon interface is helpful.
>
> In case of DT based platforms, this patch decouples syscon object from
> syscon platform driver, and allows to create syscon objects first time
> when it is required by calling of syscon_regmap_lookup_by APIs and keep
> a list of such syscon objects along with syscon provider device_nodes
> and regmap handles.
>
> For non-DT based platforms, this patch keeps syscon platform driver
> structure where is can be probed and such non-DT based drivers can use
> syscon_regmap_lookup_by_pdev API and get access to regmap handles.
> Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT based,
> we can completly remove platform driver of syscon, and keep only helper
> functions to get regmap handles.
>
> Suggested-by: Arnd Bergmann
> Suggested-by: Tomasz Figa
> Tested-by: Vivek Gautam
> Signed-off-by: Pankaj Dubey
> ---
> V2 of this patchset and related discussion can be found here [1].
>
> Changes since v2:
> - Added back platform device support from syscon, with one change that
>syscon will not be probed for DT based platform.
> - Added back syscon_regmap_lookup_by_pdevname API so that non-DT base
>users of syscon will not be broken.
> - Removed unwanted change in syscon.h.
> - Modified Signed-off-by list, added Suggested-by of Tomasz Figa and
>Arnd Bergmann.
> - Added Tested-by of Vivek Gautam for testing on Exynos platform.
>
> Changes since v1:
> - Removed of_syscon_unregister function.
> - Modified of_syscon_register function and it will be used by syscon.c
>to create syscon objects whenever required.
> - Removed platform device support from syscon.
> - Removed syscon_regmap_lookup_by_pdevname API support.
> - As there are significant changes w.r.t patchset v1, I am taking over
>author for this patchset from Tomasz Figa.
>
> [1]: https://lkml.org/lkml/2014/9/2/299
>
> drivers/mfd/syscon.c | 78
> --
> 1 file changed, 57 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index ca15878..4a4bad9 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -15,40 +15,49 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> -#include
> #include
> #include
> #include
> #include
> +#include
>
> static struct platform_driver syscon_driver;
>
> +static DEFINE_SPINLOCK(syscon_list_slock);
> +static LIST_HEAD(syscon_list);
> +
> struct syscon {
> + struct device_node *np;
> struct regmap *regmap;
> + struct list_head list;
> };
>
> -static int syscon_match_node(struct device *dev, void *data)
> -{
> - struct device_node *dn = data;
> -
> - return (dev->of_node == dn) ? 1 : 0;
> -}
> +static struct syscon *of_syscon_register(struct device_node *np);
>
> struct regmap *syscon_node_to_regmap(struct device_node *np)
> {
> - struct syscon *syscon;
> - struct device *dev;
> + struct syscon *entry, *syscon = NULL;
>
> - dev = driver_find_device(_driver.driver, NULL, np,
> - syscon_match_node);
> - if (!dev)
> - return ERR_PTR(-EPROBE_DEFER);
> + spin_lock(_list_slock);
>
> - syscon = dev_get_drvdata(dev);
> + list_for_each_entry(entry, _list, list)
> + if (entry->np == np) {
> + syscon = entry;
> + break;
> + }
>
> - return syscon->regmap;
> + spin_unlock(_list_slock);
> +
> + if (!syscon)
> + syscon = of_syscon_register(np);
> +
> + if (!IS_ERR(syscon))
> + return syscon->regmap;
> +
> + return ERR_CAST(syscon);
> }
> EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
>
> @@ -110,17 +119,45 @@ struct regmap *syscon_regmap_lookup_by_phandle(struct
> device_node *np,
> }
> EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle);
>
> -static const struct of_device_id of_syscon_match[] = {
> - { .compatible = "syscon", },
> - { },
> -};
> -
> static struct regmap_config syscon_regmap_config = {
> .reg_bits = 32,
> .val_bits = 32,
> .reg_stride = 4,
> };
>
> +static struct syscon
[PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
Currently a syscon entity can be only registered directly through a
platform device that binds to a dedicated syscon driver. However in
certain use cases it is desirable to make a device used with another
driver a syscon interface provider.
For example, certain SoCs (e.g. Exynos) contain system controller
blocks which perform various functions such as power domain control,
CPU power management, low power mode control, but in addition contain
certain IP integration glue, such as various signal masks,
coprocessor power control, etc. In such case, there is a need to have
a dedicated driver for such system controller but also share registers
with other drivers. The latter is where the syscon interface is helpful.
In case of DT based platforms, this patch decouples syscon object from
syscon platform driver, and allows to create syscon objects first time
when it is required by calling of syscon_regmap_lookup_by APIs and keep
a list of such syscon objects along with syscon provider device_nodes
and regmap handles.
For non-DT based platforms, this patch keeps syscon platform driver
structure where is can be probed and such non-DT based drivers can use
syscon_regmap_lookup_by_pdev API and get access to regmap handles.
Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT based,
we can completly remove platform driver of syscon, and keep only helper
functions to get regmap handles.
Suggested-by: Arnd Bergmann
Suggested-by: Tomasz Figa
Tested-by: Vivek Gautam
Signed-off-by: Pankaj Dubey
---
V2 of this patchset and related discussion can be found here [1].
Changes since v2:
- Added back platform device support from syscon, with one change that
syscon will not be probed for DT based platform.
- Added back syscon_regmap_lookup_by_pdevname API so that non-DT base
users of syscon will not be broken.
- Removed unwanted change in syscon.h.
- Modified Signed-off-by list, added Suggested-by of Tomasz Figa and
Arnd Bergmann.
- Added Tested-by of Vivek Gautam for testing on Exynos platform.
Changes since v1:
- Removed of_syscon_unregister function.
- Modified of_syscon_register function and it will be used by syscon.c
to create syscon objects whenever required.
- Removed platform device support from syscon.
- Removed syscon_regmap_lookup_by_pdevname API support.
- As there are significant changes w.r.t patchset v1, I am taking over
author for this patchset from Tomasz Figa.
[1]: https://lkml.org/lkml/2014/9/2/299
drivers/mfd/syscon.c | 78 --
1 file changed, 57 insertions(+), 21 deletions(-)
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index ca15878..4a4bad9 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -15,40 +15,49 @@
#include
#include
#include
+#include
#include
#include
-#include
#include
#include
#include
#include
+#include
static struct platform_driver syscon_driver;
+static DEFINE_SPINLOCK(syscon_list_slock);
+static LIST_HEAD(syscon_list);
+
struct syscon {
+ struct device_node *np;
struct regmap *regmap;
+ struct list_head list;
};
-static int syscon_match_node(struct device *dev, void *data)
-{
- struct device_node *dn = data;
-
- return (dev->of_node == dn) ? 1 : 0;
-}
+static struct syscon *of_syscon_register(struct device_node *np);
struct regmap *syscon_node_to_regmap(struct device_node *np)
{
- struct syscon *syscon;
- struct device *dev;
+ struct syscon *entry, *syscon = NULL;
- dev = driver_find_device(_driver.driver, NULL, np,
-syscon_match_node);
- if (!dev)
- return ERR_PTR(-EPROBE_DEFER);
+ spin_lock(_list_slock);
- syscon = dev_get_drvdata(dev);
+ list_for_each_entry(entry, _list, list)
+ if (entry->np == np) {
+ syscon = entry;
+ break;
+ }
- return syscon->regmap;
+ spin_unlock(_list_slock);
+
+ if (!syscon)
+ syscon = of_syscon_register(np);
+
+ if (!IS_ERR(syscon))
+ return syscon->regmap;
+
+ return ERR_CAST(syscon);
}
EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
@@ -110,17 +119,45 @@ struct regmap *syscon_regmap_lookup_by_phandle(struct
device_node *np,
}
EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle);
-static const struct of_device_id of_syscon_match[] = {
- { .compatible = "syscon", },
- { },
-};
-
static struct regmap_config syscon_regmap_config = {
.reg_bits = 32,
.val_bits = 32,
.reg_stride = 4,
};
+static struct syscon *of_syscon_register(struct device_node *np)
+{
+ struct syscon *syscon;
+ struct regmap *regmap;
+ void __iomem *base;
+
+ if (!of_device_is_compatible(np, "syscon"))
+ return ERR_PTR(-EINVAL);
+
+ syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
+ if (!syscon)
+
[PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
Currently a syscon entity can be only registered directly through a
platform device that binds to a dedicated syscon driver. However in
certain use cases it is desirable to make a device used with another
driver a syscon interface provider.
For example, certain SoCs (e.g. Exynos) contain system controller
blocks which perform various functions such as power domain control,
CPU power management, low power mode control, but in addition contain
certain IP integration glue, such as various signal masks,
coprocessor power control, etc. In such case, there is a need to have
a dedicated driver for such system controller but also share registers
with other drivers. The latter is where the syscon interface is helpful.
In case of DT based platforms, this patch decouples syscon object from
syscon platform driver, and allows to create syscon objects first time
when it is required by calling of syscon_regmap_lookup_by APIs and keep
a list of such syscon objects along with syscon provider device_nodes
and regmap handles.
For non-DT based platforms, this patch keeps syscon platform driver
structure where is can be probed and such non-DT based drivers can use
syscon_regmap_lookup_by_pdev API and get access to regmap handles.
Once all users of syscon_regmap_lookup_by_pdev migrated to DT based,
we can completly remove platform driver of syscon, and keep only helper
functions to get regmap handles.
Suggested-by: Arnd Bergmann a...@arndb.de
Suggested-by: Tomasz Figa tomasz.f...@gmail.com
Tested-by: Vivek Gautam gautam.vi...@samsung.com
Signed-off-by: Pankaj Dubey pankaj.du...@samsung.com
---
V2 of this patchset and related discussion can be found here [1].
Changes since v2:
- Added back platform device support from syscon, with one change that
syscon will not be probed for DT based platform.
- Added back syscon_regmap_lookup_by_pdevname API so that non-DT base
users of syscon will not be broken.
- Removed unwanted change in syscon.h.
- Modified Signed-off-by list, added Suggested-by of Tomasz Figa and
Arnd Bergmann.
- Added Tested-by of Vivek Gautam for testing on Exynos platform.
Changes since v1:
- Removed of_syscon_unregister function.
- Modified of_syscon_register function and it will be used by syscon.c
to create syscon objects whenever required.
- Removed platform device support from syscon.
- Removed syscon_regmap_lookup_by_pdevname API support.
- As there are significant changes w.r.t patchset v1, I am taking over
author for this patchset from Tomasz Figa.
[1]: https://lkml.org/lkml/2014/9/2/299
drivers/mfd/syscon.c | 78 --
1 file changed, 57 insertions(+), 21 deletions(-)
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index ca15878..4a4bad9 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -15,40 +15,49 @@
#include linux/err.h
#include linux/io.h
#include linux/module.h
+#include linux/list.h
#include linux/of.h
#include linux/of_address.h
-#include linux/of_platform.h
#include linux/platform_data/syscon.h
#include linux/platform_device.h
#include linux/regmap.h
#include linux/mfd/syscon.h
+#include linux/slab.h
static struct platform_driver syscon_driver;
+static DEFINE_SPINLOCK(syscon_list_slock);
+static LIST_HEAD(syscon_list);
+
struct syscon {
+ struct device_node *np;
struct regmap *regmap;
+ struct list_head list;
};
-static int syscon_match_node(struct device *dev, void *data)
-{
- struct device_node *dn = data;
-
- return (dev-of_node == dn) ? 1 : 0;
-}
+static struct syscon *of_syscon_register(struct device_node *np);
struct regmap *syscon_node_to_regmap(struct device_node *np)
{
- struct syscon *syscon;
- struct device *dev;
+ struct syscon *entry, *syscon = NULL;
- dev = driver_find_device(syscon_driver.driver, NULL, np,
-syscon_match_node);
- if (!dev)
- return ERR_PTR(-EPROBE_DEFER);
+ spin_lock(syscon_list_slock);
- syscon = dev_get_drvdata(dev);
+ list_for_each_entry(entry, syscon_list, list)
+ if (entry-np == np) {
+ syscon = entry;
+ break;
+ }
- return syscon-regmap;
+ spin_unlock(syscon_list_slock);
+
+ if (!syscon)
+ syscon = of_syscon_register(np);
+
+ if (!IS_ERR(syscon))
+ return syscon-regmap;
+
+ return ERR_CAST(syscon);
}
EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
@@ -110,17 +119,45 @@ struct regmap *syscon_regmap_lookup_by_phandle(struct
device_node *np,
}
EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle);
-static const struct of_device_id of_syscon_match[] = {
- { .compatible = syscon, },
- { },
-};
-
static struct regmap_config syscon_regmap_config = {
.reg_bits = 32,
.val_bits = 32,
.reg_stride = 4,
};
+static struct syscon
Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
On Wed, Sep 17, 2014 at 12:01:50PM +0530, Pankaj Dubey wrote:
Currently a syscon entity can be only registered directly through a
platform device that binds to a dedicated syscon driver. However in
certain use cases it is desirable to make a device used with another
driver a syscon interface provider.
For example, certain SoCs (e.g. Exynos) contain system controller
blocks which perform various functions such as power domain control,
CPU power management, low power mode control, but in addition contain
certain IP integration glue, such as various signal masks,
coprocessor power control, etc. In such case, there is a need to have
a dedicated driver for such system controller but also share registers
with other drivers. The latter is where the syscon interface is helpful.
In case of DT based platforms, this patch decouples syscon object from
syscon platform driver, and allows to create syscon objects first time
when it is required by calling of syscon_regmap_lookup_by APIs and keep
a list of such syscon objects along with syscon provider device_nodes
and regmap handles.
For non-DT based platforms, this patch keeps syscon platform driver
structure where is can be probed and such non-DT based drivers can use
syscon_regmap_lookup_by_pdev API and get access to regmap handles.
Once all users of syscon_regmap_lookup_by_pdev migrated to DT based,
we can completly remove platform driver of syscon, and keep only helper
functions to get regmap handles.
Suggested-by: Arnd Bergmann a...@arndb.de
Suggested-by: Tomasz Figa tomasz.f...@gmail.com
Tested-by: Vivek Gautam gautam.vi...@samsung.com
Signed-off-by: Pankaj Dubey pankaj.du...@samsung.com
---
V2 of this patchset and related discussion can be found here [1].
Changes since v2:
- Added back platform device support from syscon, with one change that
syscon will not be probed for DT based platform.
- Added back syscon_regmap_lookup_by_pdevname API so that non-DT base
users of syscon will not be broken.
- Removed unwanted change in syscon.h.
- Modified Signed-off-by list, added Suggested-by of Tomasz Figa and
Arnd Bergmann.
- Added Tested-by of Vivek Gautam for testing on Exynos platform.
Changes since v1:
- Removed of_syscon_unregister function.
- Modified of_syscon_register function and it will be used by syscon.c
to create syscon objects whenever required.
- Removed platform device support from syscon.
- Removed syscon_regmap_lookup_by_pdevname API support.
- As there are significant changes w.r.t patchset v1, I am taking over
author for this patchset from Tomasz Figa.
[1]: https://lkml.org/lkml/2014/9/2/299
drivers/mfd/syscon.c | 78
--
1 file changed, 57 insertions(+), 21 deletions(-)
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index ca15878..4a4bad9 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -15,40 +15,49 @@
#include linux/err.h
#include linux/io.h
#include linux/module.h
+#include linux/list.h
#include linux/of.h
#include linux/of_address.h
-#include linux/of_platform.h
#include linux/platform_data/syscon.h
#include linux/platform_device.h
#include linux/regmap.h
#include linux/mfd/syscon.h
+#include linux/slab.h
static struct platform_driver syscon_driver;
+static DEFINE_SPINLOCK(syscon_list_slock);
+static LIST_HEAD(syscon_list);
+
struct syscon {
+ struct device_node *np;
struct regmap *regmap;
+ struct list_head list;
};
-static int syscon_match_node(struct device *dev, void *data)
-{
- struct device_node *dn = data;
-
- return (dev-of_node == dn) ? 1 : 0;
-}
+static struct syscon *of_syscon_register(struct device_node *np);
struct regmap *syscon_node_to_regmap(struct device_node *np)
{
- struct syscon *syscon;
- struct device *dev;
+ struct syscon *entry, *syscon = NULL;
- dev = driver_find_device(syscon_driver.driver, NULL, np,
- syscon_match_node);
- if (!dev)
- return ERR_PTR(-EPROBE_DEFER);
+ spin_lock(syscon_list_slock);
- syscon = dev_get_drvdata(dev);
+ list_for_each_entry(entry, syscon_list, list)
+ if (entry-np == np) {
+ syscon = entry;
+ break;
+ }
- return syscon-regmap;
+ spin_unlock(syscon_list_slock);
+
+ if (!syscon)
+ syscon = of_syscon_register(np);
+
+ if (!IS_ERR(syscon))
+ return syscon-regmap;
+
+ return ERR_CAST(syscon);
}
EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
@@ -110,17 +119,45 @@ struct regmap *syscon_regmap_lookup_by_phandle(struct
device_node *np,
}
EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle);
-static const struct of_device_id of_syscon_match[] = {
- { .compatible = syscon, },
- { },
-};
-
static struct regmap_config
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
Hi,
On Wednesday, September 17, 2014, Dong Aisheng Wrote,
+static struct syscon *of_syscon_register(struct device_node *np) {
+ struct syscon *syscon;
+ struct regmap *regmap;
+ void __iomem *base;
+
+ if (!of_device_is_compatible(np, syscon))
+ return ERR_PTR(-EINVAL);
+
+ syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
+ if (!syscon)
+ return ERR_PTR(-ENOMEM);
+
+ base = of_iomap(np, 0);
+ if (!base)
+ return ERR_PTR(-ENOMEM);
+
+ 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.
I just tested on MX6SX SDB board and it seemed crashed at here in
regmap_init
function.
Can you please provide crash log which can give more information about the
crash?
Thanks,
Pankaj Dubey
Regards
Dong Aisheng
+ if (IS_ERR(regmap)) {
+ pr_err(regmap init failed\n);
+ return ERR_CAST(regmap);
+ }
+
+ syscon-regmap = regmap;
+ syscon-np = np;
+
+ spin_lock(syscon_list_slock);
+ list_add_tail(syscon-list, syscon_list);
+ spin_unlock(syscon_list_slock);
+
+ return syscon;
+}
+
static int syscon_probe(struct platform_device *pdev) {
struct device *dev = pdev-dev;
@@ -167,7 +204,6 @@ static struct platform_driver syscon_driver = {
.driver = {
.name = syscon,
.owner = THIS_MODULE,
- .of_match_table = of_syscon_match,
},
.probe = syscon_probe,
.id_table = syscon_ids,
--
1.7.9.5
___
linux-arm-kernel mailing list
linux-arm-ker...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
On Wednesday 17 September 2014, Pankaj Dubey wrote: --- V2 of this patchset and related discussion can be found here [1]. Changes since v2: - Added back platform device support from syscon, with one change that syscon will not be probed for DT based platform. - Added back syscon_regmap_lookup_by_pdevname API so that non-DT base users of syscon will not be broken. - Removed unwanted change in syscon.h. - Modified Signed-off-by list, added Suggested-by of Tomasz Figa and Arnd Bergmann. - Added Tested-by of Vivek Gautam for testing on Exynos platform. Looks fine. Provided you can figure out the problem that Dong Aisheng reported, please add my Acked-by: Arnd Bergmann a...@arndb.de -} +static struct syscon *of_syscon_register(struct device_node *np); One minor comment: please avoid doing forward declarations of local functions. It took me a while to understand what is going on because I expect all functions to be ordered such that they only get called by functions below, and don't need this. Just move of_syscon_register() here directly. Arnd -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
+CC: Dong Aisheng Hi Arnd, On Wednesday, September 17, 2014, Arnd Bergmann wrote, V2 of this patchset and related discussion can be found here [1]. Changes since v2: - Added back platform device support from syscon, with one change that syscon will not be probed for DT based platform. - Added back syscon_regmap_lookup_by_pdevname API so that non-DT base users of syscon will not be broken. - Removed unwanted change in syscon.h. - Modified Signed-off-by list, added Suggested-by of Tomasz Figa and Arnd Bergmann. - Added Tested-by of Vivek Gautam for testing on Exynos platform. Looks fine. Provided you can figure out the problem that Dong Aisheng reported, please add my Acked-by: Arnd Bergmann a...@arndb.de Thanks. After he reported I have again done code walk-through of regmap_init_mmio and could not see any such fatal error. At the same time I have replied to Dong Aisheng, asking for more details and waiting for his reply. -} +static struct syscon *of_syscon_register(struct device_node *np); One minor comment: please avoid doing forward declarations of local functions. It took me a while to understand what is going on because I expect all functions to be ordered such that they only get called by functions below, and don't need this. Just move of_syscon_register() here directly. OK, I will remove forward declaration of of_syscon_register and respin patch once again. Arnd Thanks, Pankaj Dubey -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
On Wed, Sep 17, 2014 at 04:50:50PM +0530, Pankaj Dubey wrote:
Hi,
On Wednesday, September 17, 2014, Dong Aisheng Wrote,
+static struct syscon *of_syscon_register(struct device_node *np) {
+ struct syscon *syscon;
+ struct regmap *regmap;
+ void __iomem *base;
+
+ if (!of_device_is_compatible(np, syscon))
+ return ERR_PTR(-EINVAL);
+
+ syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
+ if (!syscon)
+ return ERR_PTR(-ENOMEM);
+
+ base = of_iomap(np, 0);
+ if (!base)
+ return ERR_PTR(-ENOMEM);
+
+ 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?
I just tested on MX6SX SDB board and it seemed crashed at here in
regmap_init
function.
Can you please provide crash log which can give more information about the
crash?
My crash log is:
[0.225148] Unable to handle kernel NULL pointer dereference at virtual
address 01d4
[0.233383] pgd = 80004000
[0.236185] [01d4] *pgd=
[0.239873] Internal error: Oops: 5 [#1] SMP ARM
[0.244588] Modules linked in:
[0.247753] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
3.17.0-rc4-next-20140915-6-g8ba2dd0-dirty #421
[0.257342] task: bd878000 ti: bd88 task.ti: bd88
[0.262848] PC is at regmap_init+0x21c/0xab4
[0.267221] LR is at vprintk_emit+0x254/0x5e0
[0.271677] pc : [80389420]lr : [8006a4f8]psr: 6153
[0.271677] sp : bd881ae0 ip : bd881a48 fp : bd881b1c
[0.283354] r10: r9 : bd8e1850 r8 : 0003
[0.288678] r7 : r6 : 8098fccc r5 : 8098ee0c r4 : bd8f4e00
[0.295307] r3 : bd878000 r2 : 01e5 r1 : 806ee8f0 r0 : 808762c0
[0.301938] Flags: nZCv IRQs on FIQs off Mode SVC_32 ISA ARM Segment
kernel
[0.309438] Control: 10c5387d Table: 8000404a DAC: 0015
[0.315284] Process swapper/0 (pid: 1, stack limit = 0xbd880240)
[0.321391] Stack: (0xbd881ae0 to 0xbd882000)
[0.325851] 1ae0: bd8e1850 bd881b1c bd881af8 8038f580 8098fccc
be7d49b8
[0.334136] 1b00: 8098fca4 bd917000 bd8e1850 bd881b34 bd881b20
8038f68c 80389210
[0.342422] 1b20: bd91b480 8098fcc4 bd881b5c bd881b38 80398b24 8038f664
bd91ac10
[0.350708] 1b40: be7d4bc0 bd917010 bd917000 bd8e1850 bd881bac bd881b60
803185a4 80398a54
[0.358993] 1b60: bd881b94 bd881b70
[0.367277] 1b80: bd917010 80983a64 bd916c10
80983a64
[0.375563] 1ba0: bd881bc4 bd881bb0 8037bb44 80318510 811c65f0 bd917010
bd881bec bd881bc8
[0.383848] 1bc0: 8037a104 8037bb1c 80983a64 bd917010 8037a23c bd916c10
[0.392134] 1be0: bd881c04 bd881bf0 8037a284 8037a004 bd917010
bd881c2c bd881c08
[0.400419] 1c00: 80378614 8037a248 bd8038d8 bd8af7d4 bd916c10 bd917010
bd917044 8098e308
[0.408704] 1c20: bd881c4c bd881c30 80379fb8 803785c0 bd803800 bd917018
bd917010 8098e308
[0.416990] 1c40: bd881c6c bd881c50 8037961c 80379f48 bd917018
bd917010
[0.425275] 1c60: bd881ca4 bd881c70 80377904 80379598 bd881cc8 bd881cc0
bd881cb8 bd881c88
[0.433560] 1c80: be7d4bc0 bd916c10 0001 bd917000 be7d4c10
bd881cb4 bd881ca8
[0.441845] 1ca0: 804f1708 803774e8 bd881cfc bd881cb8 804f1f2c 804f16dc
bd881cdc bd881cc8
[0.450130] 1cc0: 806b6d7c 80060478 809b0220 6153 bd881cfc
be7d4bc0
[0.458415] 1ce0: 0001 8070bb80 bd916c10 bd881d5c bd881d00
804f2068 804f1e5c
[0.466700] 1d00: bd878000 a153 809b0220 bd881d2c bd881d20
80060480 80060284
[0.474985] 1d20: bd881d44 bd881d30 806b6d7c 80060478 be7d4bc0
be7d49b8
[0.483270] 1d40: 0001 8070bb80 bd916c10 bd881dbc bd881d60
804f20c4 804f1f8c
[0.491556] 1d60: 0001 6153 809b0220 bd881d8c bd881d80
80060480 80060284
[0.499841] 1d80: bd881da4 bd881d90 806b6d7c 80060478 be7d46f8 be7d49b8
be7cf764
[0.508126] 1da0: 0001 8070bb80 bd910810 bd881e1c bd881dc0
Re: [PATCH v3] mfd: syscon: Decouple syscon interface from platform devices
On Thu, Sep 18, 2014 at 08:59:32AM +0530, Pankaj Dubey wrote: +CC: Dong Aisheng Hi Arnd, On Wednesday, September 17, 2014, Arnd Bergmann wrote, V2 of this patchset and related discussion can be found here [1]. Changes since v2: - Added back platform device support from syscon, with one change that syscon will not be probed for DT based platform. - Added back syscon_regmap_lookup_by_pdevname API so that non-DT base users of syscon will not be broken. - Removed unwanted change in syscon.h. - Modified Signed-off-by list, added Suggested-by of Tomasz Figa and Arnd Bergmann. - Added Tested-by of Vivek Gautam for testing on Exynos platform. Looks fine. Provided you can figure out the problem that Dong Aisheng reported, please add my Acked-by: Arnd Bergmann a...@arndb.de Thanks. After he reported I have again done code walk-through of regmap_init_mmio and could not see any such fatal error. At the same time I have replied to Dong Aisheng, asking for more details and waiting for his reply. I just replied and gave the log. Regards Dong Aisheng -} +static struct syscon *of_syscon_register(struct device_node *np); One minor comment: please avoid doing forward declarations of local functions. It took me a while to understand what is going on because I expect all functions to be ordered such that they only get called by functions below, and don't need this. Just move of_syscon_register() here directly. OK, I will remove forward declaration of of_syscon_register and respin patch once again. Arnd Thanks, Pankaj Dubey -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
