[PATCH] mfd: twl-core: export twl_get_regmap

[PATCH] mfd: twl-core: export twl_get_regmap

[Nicolae Rosia]

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

[PATCH] mfd: twl-core: export twl_get_regmap

[Lee Jones]

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

[PATCH] mfd: twl-core: export twl_get_regmap

[Nicolae Rosia]

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

[PATCH] mfd: twl-core: export twl_get_regmap

[Lee Jones]

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

[PATCH] mfd: twl-core: export twl_get_regmap

[Russell King - ARM Linux]

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.

[PATCH] mfd: twl-core: export twl_get_regmap

[Nicolae Rosia]

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*.

[PATCH] mfd: twl-core: export twl_get_regmap

[Russell King - ARM Linux]

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.

[PATCH] mfd: twl-core: export twl_get_regmap

[Russell King - ARM Linux]

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.

[PATCH] mfd: twl-core: export twl_get_regmap

[Rosia, Nicolae]

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

[PATCH] mfd: twl-core: export twl_get_regmap

[Nicolae Rosia]

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