* [PATCH] mfd: twl-core: export twl_get_regmap
@ 2016-11-12 11:38 ` Nicolae Rosia
0 siblings, 0 replies; 20+ messages in thread
From: Nicolae Rosia @ 2016-11-12 11:38 UTC (permalink / raw
To: linux-arm-kernel
We want to get rid of global twl_i2c_{write/read}.
As a first step, allow clients to get the regmap and write directly
Signed-off-by: Nicolae Rosia <Nicolae_Rosia@mentor.com>
---
drivers/mfd/twl-core.c | 3 ++-
include/linux/i2c/twl.h | 2 ++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index c64615d..49e6a4b 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -421,7 +421,7 @@ EXPORT_SYMBOL(twl_rev);
*
* Returns the regmap pointer or NULL in case of failure.
*/
-static struct regmap *twl_get_regmap(u8 mod_no)
+struct regmap *twl_get_regmap(u8 mod_no)
{
int sid;
struct twl_client *twl;
@@ -440,6 +440,7 @@ static struct regmap *twl_get_regmap(u8 mod_no)
return twl->regmap;
}
+EXPORT_SYMBOL(twl_get_regmap);
/**
* twl_i2c_write - Writes a n bit register in TWL4030/TWL5030/TWL60X0
diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
index 9ad7828..4c43cdb3 100644
--- a/include/linux/i2c/twl.h
+++ b/include/linux/i2c/twl.h
@@ -174,6 +174,8 @@ static inline int twl_class_is_ ##class(void) \
TWL_CLASS_IS(4030, TWL4030_CLASS_ID)
TWL_CLASS_IS(6030, TWL6030_CLASS_ID)
+struct regmap *twl_get_regmap(u8 mod_no);
+
/* Set the regcache bypass for the regmap associated with the nodule */
int twl_set_regcache_bypass(u8 mod_no, bool enable);
--
2.5.5
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH] mfd: twl-core: export twl_get_regmap
@ 2016-11-12 11:38 ` Nicolae Rosia
0 siblings, 0 replies; 20+ messages in thread
From: Nicolae Rosia @ 2016-11-12 11:38 UTC (permalink / raw
To: Tony Lindgren, Lee Jones; +Cc: linux-omap, Nicolae Rosia, linux-arm-kernel
We want to get rid of global twl_i2c_{write/read}.
As a first step, allow clients to get the regmap and write directly
Signed-off-by: Nicolae Rosia <Nicolae_Rosia@mentor.com>
---
drivers/mfd/twl-core.c | 3 ++-
include/linux/i2c/twl.h | 2 ++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index c64615d..49e6a4b 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -421,7 +421,7 @@ EXPORT_SYMBOL(twl_rev);
*
* Returns the regmap pointer or NULL in case of failure.
*/
-static struct regmap *twl_get_regmap(u8 mod_no)
+struct regmap *twl_get_regmap(u8 mod_no)
{
int sid;
struct twl_client *twl;
@@ -440,6 +440,7 @@ static struct regmap *twl_get_regmap(u8 mod_no)
return twl->regmap;
}
+EXPORT_SYMBOL(twl_get_regmap);
/**
* twl_i2c_write - Writes a n bit register in TWL4030/TWL5030/TWL60X0
diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
index 9ad7828..4c43cdb3 100644
--- a/include/linux/i2c/twl.h
+++ b/include/linux/i2c/twl.h
@@ -174,6 +174,8 @@ static inline int twl_class_is_ ##class(void) \
TWL_CLASS_IS(4030, TWL4030_CLASS_ID)
TWL_CLASS_IS(6030, TWL6030_CLASS_ID)
+struct regmap *twl_get_regmap(u8 mod_no);
+
/* Set the regcache bypass for the regmap associated with the nodule */
int twl_set_regcache_bypass(u8 mod_no, bool enable);
--
2.5.5
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH] mfd: twl-core: export twl_get_regmap
2016-11-12 11:38 ` Nicolae Rosia
@ 2016-11-18 18:55 ` Lee Jones
-1 siblings, 0 replies; 20+ messages in thread
From: Lee Jones @ 2016-11-18 18:55 UTC (permalink / raw
To: linux-arm-kernel
On Sat, 12 Nov 2016, Nicolae Rosia wrote:
> We want to get rid of global twl_i2c_{write/read}.
> As a first step, allow clients to get the regmap and write directly
What's stopping you from passing it through device data?
> Signed-off-by: Nicolae Rosia <Nicolae_Rosia@mentor.com>
> ---
> drivers/mfd/twl-core.c | 3 ++-
> include/linux/i2c/twl.h | 2 ++
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> index c64615d..49e6a4b 100644
> --- a/drivers/mfd/twl-core.c
> +++ b/drivers/mfd/twl-core.c
> @@ -421,7 +421,7 @@ EXPORT_SYMBOL(twl_rev);
> *
> * Returns the regmap pointer or NULL in case of failure.
> */
> -static struct regmap *twl_get_regmap(u8 mod_no)
> +struct regmap *twl_get_regmap(u8 mod_no)
> {
> int sid;
> struct twl_client *twl;
> @@ -440,6 +440,7 @@ static struct regmap *twl_get_regmap(u8 mod_no)
>
> return twl->regmap;
> }
> +EXPORT_SYMBOL(twl_get_regmap);
>
> /**
> * twl_i2c_write - Writes a n bit register in TWL4030/TWL5030/TWL60X0
> diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
> index 9ad7828..4c43cdb3 100644
> --- a/include/linux/i2c/twl.h
> +++ b/include/linux/i2c/twl.h
> @@ -174,6 +174,8 @@ static inline int twl_class_is_ ##class(void) \
> TWL_CLASS_IS(4030, TWL4030_CLASS_ID)
> TWL_CLASS_IS(6030, TWL6030_CLASS_ID)
>
> +struct regmap *twl_get_regmap(u8 mod_no);
> +
> /* Set the regcache bypass for the regmap associated with the nodule */
> int twl_set_regcache_bypass(u8 mod_no, bool enable);
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] mfd: twl-core: export twl_get_regmap
@ 2016-11-18 18:55 ` Lee Jones
0 siblings, 0 replies; 20+ messages in thread
From: Lee Jones @ 2016-11-18 18:55 UTC (permalink / raw
To: Nicolae Rosia; +Cc: Tony Lindgren, linux-omap, linux-arm-kernel
On Sat, 12 Nov 2016, Nicolae Rosia wrote:
> We want to get rid of global twl_i2c_{write/read}.
> As a first step, allow clients to get the regmap and write directly
What's stopping you from passing it through device data?
> Signed-off-by: Nicolae Rosia <Nicolae_Rosia@mentor.com>
> ---
> drivers/mfd/twl-core.c | 3 ++-
> include/linux/i2c/twl.h | 2 ++
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> index c64615d..49e6a4b 100644
> --- a/drivers/mfd/twl-core.c
> +++ b/drivers/mfd/twl-core.c
> @@ -421,7 +421,7 @@ EXPORT_SYMBOL(twl_rev);
> *
> * Returns the regmap pointer or NULL in case of failure.
> */
> -static struct regmap *twl_get_regmap(u8 mod_no)
> +struct regmap *twl_get_regmap(u8 mod_no)
> {
> int sid;
> struct twl_client *twl;
> @@ -440,6 +440,7 @@ static struct regmap *twl_get_regmap(u8 mod_no)
>
> return twl->regmap;
> }
> +EXPORT_SYMBOL(twl_get_regmap);
>
> /**
> * twl_i2c_write - Writes a n bit register in TWL4030/TWL5030/TWL60X0
> diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
> index 9ad7828..4c43cdb3 100644
> --- a/include/linux/i2c/twl.h
> +++ b/include/linux/i2c/twl.h
> @@ -174,6 +174,8 @@ static inline int twl_class_is_ ##class(void) \
> TWL_CLASS_IS(4030, TWL4030_CLASS_ID)
> TWL_CLASS_IS(6030, TWL6030_CLASS_ID)
>
> +struct regmap *twl_get_regmap(u8 mod_no);
> +
> /* Set the regcache bypass for the regmap associated with the nodule */
> int twl_set_regcache_bypass(u8 mod_no, bool enable);
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] mfd: twl-core: export twl_get_regmap
2016-11-18 18:55 ` Lee Jones
@ 2016-11-20 12:54 ` Nicolae Rosia
-1 siblings, 0 replies; 20+ messages in thread
From: Nicolae Rosia @ 2016-11-20 12:54 UTC (permalink / raw
To: linux-arm-kernel
On Fri, Nov 18, 2016 at 8:55 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Sat, 12 Nov 2016, Nicolae Rosia wrote:
>
>> We want to get rid of global twl_i2c_{write/read}.
>> As a first step, allow clients to get the regmap and write directly
>
> What's stopping you from passing it through device data?
>
Could you elaborate a bit?
The regmaps are stored in struct twl_client [0], stored in struct
twl_private [1], both structs are defined in the source file, not in
header.
I could however just fix the problem by reworking the struct, exposing
it and use mfd_add_device as real mfd drivers do.
[0] http://lxr.free-electrons.com/source/drivers/mfd/twl-core.c#L152
[1] http://lxr.free-electrons.com/source/drivers/mfd/twl-core.c#L163
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] mfd: twl-core: export twl_get_regmap
@ 2016-11-20 12:54 ` Nicolae Rosia
0 siblings, 0 replies; 20+ messages in thread
From: Nicolae Rosia @ 2016-11-20 12:54 UTC (permalink / raw
To: Lee Jones; +Cc: Tony Lindgren, linux-omap, Nicolae Rosia, linux-arm-kernel
On Fri, Nov 18, 2016 at 8:55 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Sat, 12 Nov 2016, Nicolae Rosia wrote:
>
>> We want to get rid of global twl_i2c_{write/read}.
>> As a first step, allow clients to get the regmap and write directly
>
> What's stopping you from passing it through device data?
>
Could you elaborate a bit?
The regmaps are stored in struct twl_client [0], stored in struct
twl_private [1], both structs are defined in the source file, not in
header.
I could however just fix the problem by reworking the struct, exposing
it and use mfd_add_device as real mfd drivers do.
[0] http://lxr.free-electrons.com/source/drivers/mfd/twl-core.c#L152
[1] http://lxr.free-electrons.com/source/drivers/mfd/twl-core.c#L163
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] mfd: twl-core: export twl_get_regmap
2016-11-20 12:54 ` Nicolae Rosia
@ 2016-11-21 9:23 ` Lee Jones
-1 siblings, 0 replies; 20+ messages in thread
From: Lee Jones @ 2016-11-21 9:23 UTC (permalink / raw
To: linux-arm-kernel
On Sun, 20 Nov 2016, Nicolae Rosia wrote:
> On Fri, Nov 18, 2016 at 8:55 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Sat, 12 Nov 2016, Nicolae Rosia wrote:
> >
> >> We want to get rid of global twl_i2c_{write/read}.
> >> As a first step, allow clients to get the regmap and write directly
> >
> > What's stopping you from passing it through device data?
> >
> Could you elaborate a bit?
> The regmaps are stored in struct twl_client [0], stored in struct
> twl_private [1], both structs are defined in the source file, not in
> header.
> I could however just fix the problem by reworking the struct, exposing
> it and use mfd_add_device as real mfd drivers do.
Woah! Thanks for prompting me to read this driver. It's a bit of a
mess isn't it? I think it would be best to convert it to use the MFD
API, yes.
It's common place to pass shared resources such as 'regmap' though
device data. You can find many examples of *__set_drvdata throughout
the kernel.
> [0] http://lxr.free-electrons.com/source/drivers/mfd/twl-core.c#L152
> [1] http://lxr.free-electrons.com/source/drivers/mfd/twl-core.c#L163
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] mfd: twl-core: export twl_get_regmap
@ 2016-11-21 9:23 ` Lee Jones
0 siblings, 0 replies; 20+ messages in thread
From: Lee Jones @ 2016-11-21 9:23 UTC (permalink / raw
To: Nicolae Rosia; +Cc: Tony Lindgren, linux-omap, Nicolae Rosia, linux-arm-kernel
On Sun, 20 Nov 2016, Nicolae Rosia wrote:
> On Fri, Nov 18, 2016 at 8:55 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Sat, 12 Nov 2016, Nicolae Rosia wrote:
> >
> >> We want to get rid of global twl_i2c_{write/read}.
> >> As a first step, allow clients to get the regmap and write directly
> >
> > What's stopping you from passing it through device data?
> >
> Could you elaborate a bit?
> The regmaps are stored in struct twl_client [0], stored in struct
> twl_private [1], both structs are defined in the source file, not in
> header.
> I could however just fix the problem by reworking the struct, exposing
> it and use mfd_add_device as real mfd drivers do.
Woah! Thanks for prompting me to read this driver. It's a bit of a
mess isn't it? I think it would be best to convert it to use the MFD
API, yes.
It's common place to pass shared resources such as 'regmap' though
device data. You can find many examples of *__set_drvdata throughout
the kernel.
> [0] http://lxr.free-electrons.com/source/drivers/mfd/twl-core.c#L152
> [1] http://lxr.free-electrons.com/source/drivers/mfd/twl-core.c#L163
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] mfd: twl-core: export twl_get_regmap
2016-11-21 9:23 ` Lee Jones
@ 2016-11-21 9:31 ` Russell King - ARM Linux
-1 siblings, 0 replies; 20+ messages in thread
From: Russell King - ARM Linux @ 2016-11-21 9:31 UTC (permalink / raw
To: linux-arm-kernel
On Mon, Nov 21, 2016 at 09:23:39AM +0000, Lee Jones wrote:
> It's common place to pass shared resources such as 'regmap' though
> device data. You can find many examples of *__set_drvdata throughout
> the kernel.
Passing data between drivers using *_set_drvdata() is a layering
violation:
1. Driver data is supposed to be driver private data associated with
the currently bound driver.
2. The driver data pointer is NULL'd when the driver unbinds from the
device. See __device_release_driver() and the
dev_set_drvdata(dev, NULL).
3. It will break with CONFIG_DEBUG_TEST_DRIVER_REMOVE enabled for a
similar reason to (2).
So, do not pass data between drivers using *_set_drvdata() - any
examples in the kernel already are founded on bad practice, are
fragile, and are already broken for some kernel configurations.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] mfd: twl-core: export twl_get_regmap
@ 2016-11-21 9:31 ` Russell King - ARM Linux
0 siblings, 0 replies; 20+ messages in thread
From: Russell King - ARM Linux @ 2016-11-21 9:31 UTC (permalink / raw
To: Lee Jones
Cc: Tony Lindgren, Nicolae Rosia, linux-omap, Nicolae Rosia,
linux-arm-kernel
On Mon, Nov 21, 2016 at 09:23:39AM +0000, Lee Jones wrote:
> It's common place to pass shared resources such as 'regmap' though
> device data. You can find many examples of *__set_drvdata throughout
> the kernel.
Passing data between drivers using *_set_drvdata() is a layering
violation:
1. Driver data is supposed to be driver private data associated with
the currently bound driver.
2. The driver data pointer is NULL'd when the driver unbinds from the
device. See __device_release_driver() and the
dev_set_drvdata(dev, NULL).
3. It will break with CONFIG_DEBUG_TEST_DRIVER_REMOVE enabled for a
similar reason to (2).
So, do not pass data between drivers using *_set_drvdata() - any
examples in the kernel already are founded on bad practice, are
fragile, and are already broken for some kernel configurations.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] mfd: twl-core: export twl_get_regmap
2016-11-21 9:31 ` Russell King - ARM Linux
@ 2016-11-21 10:03 ` Nicolae Rosia
-1 siblings, 0 replies; 20+ messages in thread
From: Nicolae Rosia @ 2016-11-21 10:03 UTC (permalink / raw
To: linux-arm-kernel
On Mon, Nov 21, 2016 at 11:31 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> Passing data between drivers using *_set_drvdata() is a layering
> violation:
>
> 1. Driver data is supposed to be driver private data associated with
> the currently bound driver.
> 2. The driver data pointer is NULL'd when the driver unbinds from the
> device. See __device_release_driver() and the
> dev_set_drvdata(dev, NULL).
> 3. It will break with CONFIG_DEBUG_TEST_DRIVER_REMOVE enabled for a
> similar reason to (2).
>
> So, do not pass data between drivers using *_set_drvdata() - any
> examples in the kernel already are founded on bad practice, are
> fragile, and are already broken for some kernel configurations.
After inspecting mfd_add_device, it seems that it creates a
platform_device which has the parent set to the driver calling the
function.
Isn't module unloading forbidden if there is a parent->child
relationship in place and you're removing the parent?
What should be the best practice to share data between drivers?
Reference counted data?
In the case of TWL, the twl-core is just a simple container for
regmaps - all other "sub devices" are using those regmaps to access
the I2C device's registers, it makes no sense to remove the parent
driver since it does *nothing*.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] mfd: twl-core: export twl_get_regmap
@ 2016-11-21 10:03 ` Nicolae Rosia
0 siblings, 0 replies; 20+ messages in thread
From: Nicolae Rosia @ 2016-11-21 10:03 UTC (permalink / raw
To: Russell King - ARM Linux
Cc: Tony Lindgren, Nicolae Rosia, linux-omap, Lee Jones,
linux-arm-kernel
On Mon, Nov 21, 2016 at 11:31 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> Passing data between drivers using *_set_drvdata() is a layering
> violation:
>
> 1. Driver data is supposed to be driver private data associated with
> the currently bound driver.
> 2. The driver data pointer is NULL'd when the driver unbinds from the
> device. See __device_release_driver() and the
> dev_set_drvdata(dev, NULL).
> 3. It will break with CONFIG_DEBUG_TEST_DRIVER_REMOVE enabled for a
> similar reason to (2).
>
> So, do not pass data between drivers using *_set_drvdata() - any
> examples in the kernel already are founded on bad practice, are
> fragile, and are already broken for some kernel configurations.
After inspecting mfd_add_device, it seems that it creates a
platform_device which has the parent set to the driver calling the
function.
Isn't module unloading forbidden if there is a parent->child
relationship in place and you're removing the parent?
What should be the best practice to share data between drivers?
Reference counted data?
In the case of TWL, the twl-core is just a simple container for
regmaps - all other "sub devices" are using those regmaps to access
the I2C device's registers, it makes no sense to remove the parent
driver since it does *nothing*.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] mfd: twl-core: export twl_get_regmap
2016-11-21 10:03 ` Nicolae Rosia
@ 2016-11-21 13:37 ` Russell King - ARM Linux
-1 siblings, 0 replies; 20+ messages in thread
From: Russell King - ARM Linux @ 2016-11-21 13:37 UTC (permalink / raw
To: linux-arm-kernel
On Mon, Nov 21, 2016 at 12:03:03PM +0200, Nicolae Rosia wrote:
> On Mon, Nov 21, 2016 at 11:31 AM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > Passing data between drivers using *_set_drvdata() is a layering
> > violation:
> >
> > 1. Driver data is supposed to be driver private data associated with
> > the currently bound driver.
> > 2. The driver data pointer is NULL'd when the driver unbinds from the
> > device. See __device_release_driver() and the
> > dev_set_drvdata(dev, NULL).
> > 3. It will break with CONFIG_DEBUG_TEST_DRIVER_REMOVE enabled for a
> > similar reason to (2).
> >
> > So, do not pass data between drivers using *_set_drvdata() - any
> > examples in the kernel already are founded on bad practice, are
> > fragile, and are already broken for some kernel configurations.
>
> After inspecting mfd_add_device, it seems that it creates a
> platform_device which has the parent set to the driver calling the
> function.
> Isn't module unloading forbidden if there is a parent->child
> relationship in place and you're removing the parent?
Forget this idea that there's any connection between modules and
the struct device relationships - there isn't anything of the kind!
Each struct device is refcounted, and child devices will hold a
reference to their parent device, so the parent device doesn't get
freed before its children are all gone.
That's a completely separate issue to when a struct device is bound
to a struct device_driver - it's entirely possible for parent drivers
to be unbound at any time, even when there are child drivers in place.
There are cases where we want that to happen - think of any driver
which is a bus driver in itself - eg, PCMCIA, MMC, USB, etc. These
drivers enumerate their children, and destroy their children when
the driver is unbound - but the driver has to be in the process of
being unbound for that to happen. That process may very well start
with the child devices being bound to their drivers.
What makes the child drivers unbind is when the bus driver deletes
the child struct devices.
> What should be the best practice to share data between drivers?
> Reference counted data?
I guess so, but you will still have a race if you do something like:
struct parent_private_data *parent_priv = dev_get_drvdata(dev->parent);
Yes, that'll get the parent's driver private data, but what you don't
know is whether the pointer remains valid, and even if you do as the
very next step:
kref_get(&parent_priv->kref);
you don't know whether parent_priv was kfree()d between these two
statements.
However, if the parent driver creates the struct device that you're
using and deletes the struct device before it frees its private data,
then you can be sure that parent_priv will be valid, because the child
drivers will be unbound during the parent driver's ->remove function,
_before_ the private data is freed.
> In the case of TWL, the twl-core is just a simple container for
> regmaps - all other "sub devices" are using those regmaps to access
> the I2C device's registers, it makes no sense to remove the parent
> driver since it does *nothing*.
I can't comment on what twl-core is doing, I haven't looked at it in
ages, but most MFD drivers have the parent device creating and destroying
their children, so it should be fine.
My original comment was more along the lines of a parent device poking
driver-private data into the child devices it was creating for the
child drivers to pick up. However, it's worth discussing the validity
cases of the parent's driver data too, as per the above.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] mfd: twl-core: export twl_get_regmap
@ 2016-11-21 13:37 ` Russell King - ARM Linux
0 siblings, 0 replies; 20+ messages in thread
From: Russell King - ARM Linux @ 2016-11-21 13:37 UTC (permalink / raw
To: Nicolae Rosia
Cc: Tony Lindgren, Nicolae Rosia, linux-omap, Lee Jones,
linux-arm-kernel
On Mon, Nov 21, 2016 at 12:03:03PM +0200, Nicolae Rosia wrote:
> On Mon, Nov 21, 2016 at 11:31 AM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > Passing data between drivers using *_set_drvdata() is a layering
> > violation:
> >
> > 1. Driver data is supposed to be driver private data associated with
> > the currently bound driver.
> > 2. The driver data pointer is NULL'd when the driver unbinds from the
> > device. See __device_release_driver() and the
> > dev_set_drvdata(dev, NULL).
> > 3. It will break with CONFIG_DEBUG_TEST_DRIVER_REMOVE enabled for a
> > similar reason to (2).
> >
> > So, do not pass data between drivers using *_set_drvdata() - any
> > examples in the kernel already are founded on bad practice, are
> > fragile, and are already broken for some kernel configurations.
>
> After inspecting mfd_add_device, it seems that it creates a
> platform_device which has the parent set to the driver calling the
> function.
> Isn't module unloading forbidden if there is a parent->child
> relationship in place and you're removing the parent?
Forget this idea that there's any connection between modules and
the struct device relationships - there isn't anything of the kind!
Each struct device is refcounted, and child devices will hold a
reference to their parent device, so the parent device doesn't get
freed before its children are all gone.
That's a completely separate issue to when a struct device is bound
to a struct device_driver - it's entirely possible for parent drivers
to be unbound at any time, even when there are child drivers in place.
There are cases where we want that to happen - think of any driver
which is a bus driver in itself - eg, PCMCIA, MMC, USB, etc. These
drivers enumerate their children, and destroy their children when
the driver is unbound - but the driver has to be in the process of
being unbound for that to happen. That process may very well start
with the child devices being bound to their drivers.
What makes the child drivers unbind is when the bus driver deletes
the child struct devices.
> What should be the best practice to share data between drivers?
> Reference counted data?
I guess so, but you will still have a race if you do something like:
struct parent_private_data *parent_priv = dev_get_drvdata(dev->parent);
Yes, that'll get the parent's driver private data, but what you don't
know is whether the pointer remains valid, and even if you do as the
very next step:
kref_get(&parent_priv->kref);
you don't know whether parent_priv was kfree()d between these two
statements.
However, if the parent driver creates the struct device that you're
using and deletes the struct device before it frees its private data,
then you can be sure that parent_priv will be valid, because the child
drivers will be unbound during the parent driver's ->remove function,
_before_ the private data is freed.
> In the case of TWL, the twl-core is just a simple container for
> regmaps - all other "sub devices" are using those regmaps to access
> the I2C device's registers, it makes no sense to remove the parent
> driver since it does *nothing*.
I can't comment on what twl-core is doing, I haven't looked at it in
ages, but most MFD drivers have the parent device creating and destroying
their children, so it should be fine.
My original comment was more along the lines of a parent device poking
driver-private data into the child devices it was creating for the
child drivers to pick up. However, it's worth discussing the validity
cases of the parent's driver data too, as per the above.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] mfd: twl-core: export twl_get_regmap
2016-11-21 13:37 ` Russell King - ARM Linux
@ 2016-11-23 11:12 ` Russell King - ARM Linux
-1 siblings, 0 replies; 20+ messages in thread
From: Russell King - ARM Linux @ 2016-11-23 11:12 UTC (permalink / raw
To: linux-arm-kernel
On Mon, Nov 21, 2016 at 01:37:55PM +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 12:03:03PM +0200, Nicolae Rosia wrote:
> > On Mon, Nov 21, 2016 at 11:31 AM, Russell King - ARM Linux
> > <linux@armlinux.org.uk> wrote:
> > > Passing data between drivers using *_set_drvdata() is a layering
> > > violation:
> > >
> > > 1. Driver data is supposed to be driver private data associated with
> > > the currently bound driver.
> > > 2. The driver data pointer is NULL'd when the driver unbinds from the
> > > device. See __device_release_driver() and the
> > > dev_set_drvdata(dev, NULL).
> > > 3. It will break with CONFIG_DEBUG_TEST_DRIVER_REMOVE enabled for a
> > > similar reason to (2).
> > >
> > > So, do not pass data between drivers using *_set_drvdata() - any
> > > examples in the kernel already are founded on bad practice, are
> > > fragile, and are already broken for some kernel configurations.
> >
> > After inspecting mfd_add_device, it seems that it creates a
> > platform_device which has the parent set to the driver calling the
> > function.
> > Isn't module unloading forbidden if there is a parent->child
> > relationship in place and you're removing the parent?
>
> Forget this idea that there's any connection between modules and
> the struct device relationships - there isn't anything of the kind!
>
> Each struct device is refcounted, and child devices will hold a
> reference to their parent device, so the parent device doesn't get
> freed before its children are all gone.
>
> That's a completely separate issue to when a struct device is bound
> to a struct device_driver - it's entirely possible for parent drivers
> to be unbound at any time, even when there are child drivers in place.
>
> There are cases where we want that to happen - think of any driver
> which is a bus driver in itself - eg, PCMCIA, MMC, USB, etc. These
> drivers enumerate their children, and destroy their children when
> the driver is unbound - but the driver has to be in the process of
> being unbound for that to happen. That process may very well start
> with the child devices being bound to their drivers.
>
> What makes the child drivers unbind is when the bus driver deletes
> the child struct devices.
>
> > What should be the best practice to share data between drivers?
> > Reference counted data?
>
> I guess so, but you will still have a race if you do something like:
>
> struct parent_private_data *parent_priv = dev_get_drvdata(dev->parent);
>
> Yes, that'll get the parent's driver private data, but what you don't
> know is whether the pointer remains valid, and even if you do as the
> very next step:
>
> kref_get(&parent_priv->kref);
>
> you don't know whether parent_priv was kfree()d between these two
> statements.
>
> However, if the parent driver creates the struct device that you're
> using and deletes the struct device before it frees its private data,
> then you can be sure that parent_priv will be valid, because the child
> drivers will be unbound during the parent driver's ->remove function,
> _before_ the private data is freed.
>
> > In the case of TWL, the twl-core is just a simple container for
> > regmaps - all other "sub devices" are using those regmaps to access
> > the I2C device's registers, it makes no sense to remove the parent
> > driver since it does *nothing*.
>
> I can't comment on what twl-core is doing, I haven't looked at it in
> ages, but most MFD drivers have the parent device creating and destroying
> their children, so it should be fine.
>
> My original comment was more along the lines of a parent device poking
> driver-private data into the child devices it was creating for the
> child drivers to pick up. However, it's worth discussing the validity
> cases of the parent's driver data too, as per the above.
I was just curious, and I took a peek at the OMAP/TWL DT files, and
I see that it's left to DT to create the children.
So, there is already _no_ lifetime relationship between the children
and the parent device drivers being probed.
What's even more fun is this:
static int
twl_probe(struct i2c_client *client, const struct i2c_device_id *id)
{
...
if (twl_priv) {
dev_dbg(&client->dev, "only one instance of %s allowed\n",
DRIVER_NAME);
return -EBUSY;
}
...
twl_priv = devm_kzalloc(&client->dev, sizeof(struct twl_private),
GFP_KERNEL);
if (!twl_priv) {
status = -ENOMEM;
goto free;
}
...
twl->regmap = devm_regmap_init_i2c(twl->client,
&twl_regmap_config[i]);
if (IS_ERR(twl->regmap)) {
status = PTR_ERR(twl->regmap);
dev_err(&client->dev,
"Failed to allocate regmap %d, err: %d\n", i,
status);
goto fail;
}
...
So, if we get a failure after successfully allocating twl_priv, then
the driver and device are dead - it can't ever be retried. What's
more is that twl_priv contains a stale pointer - and use of it would
be a use-after-free bug, even to inspect twl_priv->ready.
That brings us on to the remove path:
static int twl_remove(struct i2c_client *client)
{
...
twl_priv->ready = false;
return 0;
}
which is pretty much useless - twl_priv will be kfree()d after this
function returns, so dereferencing twl_priv is again a use-after-free
bug. What's more is that the memory pointed to by twl_priv can be
reallocated, and ->ready could contain any value.
Now, there's a bunch of sub-nodes declared in DT which cause drivers
to be probed (eg, the twl-pwmled driver). These make use of
twl_i2c_read_u8() etc to read/write registers on the device. These
call through to twl_i2c_read() and twl_i2c_write(), both of which
use twl_get_regmap().
twl_get_regmap() dereferences twl_priv, which as established above may
have been kfree()d if the twl-core driver has been unbound. Even if
twl_priv survives with its stale data, the regmap in twl->regmap will
also have been freed, so the regmap accesses are likely to screw up.
In any case, the result is likely not going to be nice.
Note that you can't fail in a driver's remove method, so you can't stop
the twl-core driver being unbound by returning an error there: the
return value is ignored.
One possible approach to this would be to make twl-core built-in only,
remove the .remove method from the driver, and set suppress_bind_attrs
in the driver structure, so userspace can't bind/unbind the I2C
driver. However, that's just papering over the problem - if the I2C
_bus_ driver gets unbound, exactly the same problem exists - I2C will
delete the clients on the bus which will cause drivers to be unbound.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] mfd: twl-core: export twl_get_regmap
@ 2016-11-23 11:12 ` Russell King - ARM Linux
0 siblings, 0 replies; 20+ messages in thread
From: Russell King - ARM Linux @ 2016-11-23 11:12 UTC (permalink / raw
To: Nicolae Rosia
Cc: Tony Lindgren, Lee Jones, linux-omap, Nicolae Rosia,
linux-arm-kernel
On Mon, Nov 21, 2016 at 01:37:55PM +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 12:03:03PM +0200, Nicolae Rosia wrote:
> > On Mon, Nov 21, 2016 at 11:31 AM, Russell King - ARM Linux
> > <linux@armlinux.org.uk> wrote:
> > > Passing data between drivers using *_set_drvdata() is a layering
> > > violation:
> > >
> > > 1. Driver data is supposed to be driver private data associated with
> > > the currently bound driver.
> > > 2. The driver data pointer is NULL'd when the driver unbinds from the
> > > device. See __device_release_driver() and the
> > > dev_set_drvdata(dev, NULL).
> > > 3. It will break with CONFIG_DEBUG_TEST_DRIVER_REMOVE enabled for a
> > > similar reason to (2).
> > >
> > > So, do not pass data between drivers using *_set_drvdata() - any
> > > examples in the kernel already are founded on bad practice, are
> > > fragile, and are already broken for some kernel configurations.
> >
> > After inspecting mfd_add_device, it seems that it creates a
> > platform_device which has the parent set to the driver calling the
> > function.
> > Isn't module unloading forbidden if there is a parent->child
> > relationship in place and you're removing the parent?
>
> Forget this idea that there's any connection between modules and
> the struct device relationships - there isn't anything of the kind!
>
> Each struct device is refcounted, and child devices will hold a
> reference to their parent device, so the parent device doesn't get
> freed before its children are all gone.
>
> That's a completely separate issue to when a struct device is bound
> to a struct device_driver - it's entirely possible for parent drivers
> to be unbound at any time, even when there are child drivers in place.
>
> There are cases where we want that to happen - think of any driver
> which is a bus driver in itself - eg, PCMCIA, MMC, USB, etc. These
> drivers enumerate their children, and destroy their children when
> the driver is unbound - but the driver has to be in the process of
> being unbound for that to happen. That process may very well start
> with the child devices being bound to their drivers.
>
> What makes the child drivers unbind is when the bus driver deletes
> the child struct devices.
>
> > What should be the best practice to share data between drivers?
> > Reference counted data?
>
> I guess so, but you will still have a race if you do something like:
>
> struct parent_private_data *parent_priv = dev_get_drvdata(dev->parent);
>
> Yes, that'll get the parent's driver private data, but what you don't
> know is whether the pointer remains valid, and even if you do as the
> very next step:
>
> kref_get(&parent_priv->kref);
>
> you don't know whether parent_priv was kfree()d between these two
> statements.
>
> However, if the parent driver creates the struct device that you're
> using and deletes the struct device before it frees its private data,
> then you can be sure that parent_priv will be valid, because the child
> drivers will be unbound during the parent driver's ->remove function,
> _before_ the private data is freed.
>
> > In the case of TWL, the twl-core is just a simple container for
> > regmaps - all other "sub devices" are using those regmaps to access
> > the I2C device's registers, it makes no sense to remove the parent
> > driver since it does *nothing*.
>
> I can't comment on what twl-core is doing, I haven't looked at it in
> ages, but most MFD drivers have the parent device creating and destroying
> their children, so it should be fine.
>
> My original comment was more along the lines of a parent device poking
> driver-private data into the child devices it was creating for the
> child drivers to pick up. However, it's worth discussing the validity
> cases of the parent's driver data too, as per the above.
I was just curious, and I took a peek at the OMAP/TWL DT files, and
I see that it's left to DT to create the children.
So, there is already _no_ lifetime relationship between the children
and the parent device drivers being probed.
What's even more fun is this:
static int
twl_probe(struct i2c_client *client, const struct i2c_device_id *id)
{
...
if (twl_priv) {
dev_dbg(&client->dev, "only one instance of %s allowed\n",
DRIVER_NAME);
return -EBUSY;
}
...
twl_priv = devm_kzalloc(&client->dev, sizeof(struct twl_private),
GFP_KERNEL);
if (!twl_priv) {
status = -ENOMEM;
goto free;
}
...
twl->regmap = devm_regmap_init_i2c(twl->client,
&twl_regmap_config[i]);
if (IS_ERR(twl->regmap)) {
status = PTR_ERR(twl->regmap);
dev_err(&client->dev,
"Failed to allocate regmap %d, err: %d\n", i,
status);
goto fail;
}
...
So, if we get a failure after successfully allocating twl_priv, then
the driver and device are dead - it can't ever be retried. What's
more is that twl_priv contains a stale pointer - and use of it would
be a use-after-free bug, even to inspect twl_priv->ready.
That brings us on to the remove path:
static int twl_remove(struct i2c_client *client)
{
...
twl_priv->ready = false;
return 0;
}
which is pretty much useless - twl_priv will be kfree()d after this
function returns, so dereferencing twl_priv is again a use-after-free
bug. What's more is that the memory pointed to by twl_priv can be
reallocated, and ->ready could contain any value.
Now, there's a bunch of sub-nodes declared in DT which cause drivers
to be probed (eg, the twl-pwmled driver). These make use of
twl_i2c_read_u8() etc to read/write registers on the device. These
call through to twl_i2c_read() and twl_i2c_write(), both of which
use twl_get_regmap().
twl_get_regmap() dereferences twl_priv, which as established above may
have been kfree()d if the twl-core driver has been unbound. Even if
twl_priv survives with its stale data, the regmap in twl->regmap will
also have been freed, so the regmap accesses are likely to screw up.
In any case, the result is likely not going to be nice.
Note that you can't fail in a driver's remove method, so you can't stop
the twl-core driver being unbound by returning an error there: the
return value is ignored.
One possible approach to this would be to make twl-core built-in only,
remove the .remove method from the driver, and set suppress_bind_attrs
in the driver structure, so userspace can't bind/unbind the I2C
driver. However, that's just papering over the problem - if the I2C
_bus_ driver gets unbound, exactly the same problem exists - I2C will
delete the clients on the bus which will cause drivers to be unbound.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] mfd: twl-core: export twl_get_regmap
2016-11-23 11:12 ` Russell King - ARM Linux
@ 2016-11-23 11:22 ` Rosia, Nicolae
-1 siblings, 0 replies; 20+ messages in thread
From: Rosia, Nicolae @ 2016-11-23 11:22 UTC (permalink / raw
To: linux-arm-kernel
Hello,
On Wed, 2016-11-23 at 11:12 +0000, Russell King - ARM Linux wrote:
> I was just curious, and I took a peek at the OMAP/TWL DT files, and
> I see that it's left to DT to create the children.
I'm converting the driver to use mfd_add_devices and
mfd_remove_devices, the subdrivers will access the parent's private
data which will remain valid since in the remove method we will be
calling mfd_remove_devices first.
After removing all global calls to twl-core.c methods, I will also get
rid of the "only one instance" of twl_priv and the "ready" flag.
Thanks for your input,
Nicolae
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] mfd: twl-core: export twl_get_regmap
@ 2016-11-23 11:22 ` Rosia, Nicolae
0 siblings, 0 replies; 20+ messages in thread
From: Rosia, Nicolae @ 2016-11-23 11:22 UTC (permalink / raw
To: linux@armlinux.org.uk
Cc: tony@atomide.com, nicolae.rosia.oss@gmail.com,
linux-omap@vger.kernel.org, lee.jones@linaro.org,
linux-arm-kernel@lists.infradead.org
Hello,
On Wed, 2016-11-23 at 11:12 +0000, Russell King - ARM Linux wrote:
> I was just curious, and I took a peek at the OMAP/TWL DT files, and
> I see that it's left to DT to create the children.
I'm converting the driver to use mfd_add_devices and
mfd_remove_devices, the subdrivers will access the parent's private
data which will remain valid since in the remove method we will be
calling mfd_remove_devices first.
After removing all global calls to twl-core.c methods, I will also get
rid of the "only one instance" of twl_priv and the "ready" flag.
Thanks for your input,
Nicolae
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] mfd: twl-core: export twl_get_regmap
2016-11-23 11:22 ` Rosia, Nicolae
@ 2016-11-26 18:23 ` Nicolae Rosia
-1 siblings, 0 replies; 20+ messages in thread
From: Nicolae Rosia @ 2016-11-26 18:23 UTC (permalink / raw
To: linux-arm-kernel
On Wed, Nov 23, 2016 at 1:22 PM, Rosia, Nicolae
<Nicolae_Rosia@mentor.com> wrote:
> I'm converting the driver to use mfd_add_devices and
> mfd_remove_devices, the subdrivers will access the parent's private
> data which will remain valid since in the remove method we will be
> calling mfd_remove_devices first.
>
> After removing all global calls to twl-core.c methods, I will also get
> rid of the "only one instance" of twl_priv and the "ready" flag.
>
> Thanks for your input,
> Nicolae
Please ignore this patch, I've sent it in a patch series here [0]
Thanks,
Nicolae
[0] https://www.spinics.net/lists/kernel/msg2392364.html
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] mfd: twl-core: export twl_get_regmap
@ 2016-11-26 18:23 ` Nicolae Rosia
0 siblings, 0 replies; 20+ messages in thread
From: Nicolae Rosia @ 2016-11-26 18:23 UTC (permalink / raw
To: Rosia, Nicolae
Cc: tony@atomide.com, linux-omap@vger.kernel.org,
lee.jones@linaro.org, linux@armlinux.org.uk,
linux-arm-kernel@lists.infradead.org
On Wed, Nov 23, 2016 at 1:22 PM, Rosia, Nicolae
<Nicolae_Rosia@mentor.com> wrote:
> I'm converting the driver to use mfd_add_devices and
> mfd_remove_devices, the subdrivers will access the parent's private
> data which will remain valid since in the remove method we will be
> calling mfd_remove_devices first.
>
> After removing all global calls to twl-core.c methods, I will also get
> rid of the "only one instance" of twl_priv and the "ready" flag.
>
> Thanks for your input,
> Nicolae
Please ignore this patch, I've sent it in a patch series here [0]
Thanks,
Nicolae
[0] https://www.spinics.net/lists/kernel/msg2392364.html
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2016-11-26 18:23 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-12 11:38 [PATCH] mfd: twl-core: export twl_get_regmap Nicolae Rosia
2016-11-12 11:38 ` Nicolae Rosia
2016-11-18 18:55 ` Lee Jones
2016-11-18 18:55 ` Lee Jones
2016-11-20 12:54 ` Nicolae Rosia
2016-11-20 12:54 ` Nicolae Rosia
2016-11-21 9:23 ` Lee Jones
2016-11-21 9:23 ` Lee Jones
2016-11-21 9:31 ` Russell King - ARM Linux
2016-11-21 9:31 ` Russell King - ARM Linux
2016-11-21 10:03 ` Nicolae Rosia
2016-11-21 10:03 ` Nicolae Rosia
2016-11-21 13:37 ` Russell King - ARM Linux
2016-11-21 13:37 ` Russell King - ARM Linux
2016-11-23 11:12 ` Russell King - ARM Linux
2016-11-23 11:12 ` Russell King - ARM Linux
2016-11-23 11:22 ` Rosia, Nicolae
2016-11-23 11:22 ` Rosia, Nicolae
2016-11-26 18:23 ` Nicolae Rosia
2016-11-26 18:23 ` Nicolae Rosia
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.