[PATCH v1 0/2] i2c: Stop i2c modules being unloaded while in use.

[PATCH v1 0/2] i2c: Stop i2c modules being unloaded while in use.

[jim_baxter]

From: Jim Baxter <jim_baxter@mentor.com>

This patchset adds a new i2c_mux_add_reparented_adapter API to the i2c
that allows owning modules to use module_get/module_put and stop the
i2c bus module being removed whilst in use.

This was tested on an ARM i.MX6 Sabre board with the pca953x gpio module.

Joshua Frkuska (2):
  i2c-mux: add i2c_mux_add_reparented_adapter api
  i2c-mux-gpio: call i2c_add_reparented_mux_adapter

 drivers/i2c/i2c-mux.c            | 14 ++++++++++++--
 drivers/i2c/muxes/i2c-mux-gpio.c |  6 +++++-
 include/linux/i2c-mux.h          | 15 +++++++++++++++
 3 files changed, 32 insertions(+), 3 deletions(-)

-- 
1.9.1

[PATCH v1 1/2] i2c-mux: add i2c_mux_add_reparented_adapter api

[jim_baxter]

From: Joshua Frkuska <joshua_frkuska@mentor.com>

This new api allows the i2c adapter created by i2c-mux to be owned by
the module that calls this api. This allows module removal chaining to
be done at a higher level.

Signed-off-by: Joshua Frkuska <joshua_frkuska@mentor.com>
Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
 drivers/i2c/i2c-mux.c   | 14 ++++++++++++--
 include/linux/i2c-mux.h | 15 +++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 8eee986..191816b 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -263,7 +263,8 @@ struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent,
 }
 EXPORT_SYMBOL_GPL(i2c_mux_alloc);
 
-int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
+int i2c_mux_add_reparented_adapter(struct module *owner,
+			struct i2c_mux_core *muxc,
 			u32 force_nr, u32 chan_id,
 			unsigned int class)
 {
@@ -305,7 +306,7 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
 	/* Now fill out new adapter structure */
 	snprintf(priv->adap.name, sizeof(priv->adap.name),
 		 "i2c-%d-mux (chan_id %d)", i2c_adapter_id(parent), chan_id);
-	priv->adap.owner = THIS_MODULE;
+	priv->adap.owner = owner;
 	priv->adap.algo = &priv->algo;
 	priv->adap.algo_data = priv;
 	priv->adap.dev.parent = &parent->dev;
@@ -385,6 +386,15 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
 	muxc->adapter[muxc->num_adapters++] = &priv->adap;
 	return 0;
 }
+EXPORT_SYMBOL_GPL(i2c_mux_add_reparented_adapter);
+
+int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
+			u32 force_nr, u32 chan_id,
+			unsigned int class)
+{
+	return i2c_mux_add_reparented_adapter(THIS_MODULE, muxc, force_nr,
+			chan_id, class);
+}
 EXPORT_SYMBOL_GPL(i2c_mux_add_adapter);
 
 void i2c_mux_del_adapters(struct i2c_mux_core *muxc)
diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h
index d4c1d12..d4f8610 100644
--- a/include/linux/i2c-mux.h
+++ b/include/linux/i2c-mux.h
@@ -64,6 +64,21 @@ struct i2c_adapter *i2c_root_adapter(struct device *dev);
  * Called to create an i2c bus on a multiplexed bus segment.
  * The chan_id parameter is passed to the select and deselect
  * callback functions to perform hardware-specific mux control.
+ *
+ * Unlike the simple i2c_add_mux_adapter, this is passed in the caller's
+ * module reference in order to reparent the adapter from the perspective of
+ * the i2c hierarchy. This allows the caller to be reference locked while the
+ * i2c adapter is in use.
+ */
+int i2c_mux_add_reparented_adapter(struct module *owner,
+				   struct i2c_mux_core *muxc,
+				   u32 force_nr, u32 chan_id,
+				   unsigned int class);
+
+/*
+ * Called to create an i2c bus on a multiplexed bus segment.
+ * The chan_id parameter is passed to the select and deselect
+ * callback functions to perform hardware-specific mux control.
  */
 int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
 			u32 force_nr, u32 chan_id,
-- 
1.9.1

[PATCH v1 2/2] i2c-mux-gpio: call i2c_add_reparented_mux_adapter

[jim_baxter]

From: Joshua Frkuska <joshua_frkuska@mentor.com>

This reparents the adapter created in i2c-mux to this module for
module unloading and chaining purposes.

Signed-off-by: Joshua Frkuska <joshua_frkuska@mentor.com>
Signed-off-by: Jim Baxter <jim_baxter@mentor.com>
---
 drivers/i2c/muxes/i2c-mux-gpio.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index e5cf26e..9f92535 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -234,7 +234,11 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
 		u32 nr = mux->data.base_nr ? (mux->data.base_nr + i) : 0;
 		unsigned int class = mux->data.classes ? mux->data.classes[i] : 0;
 
-		ret = i2c_mux_add_adapter(muxc, nr, mux->data.values[i], class);
+		ret = i2c_mux_add_reparented_adapter(THIS_MODULE,
+						     muxc,
+						     nr,
+						     mux->data.values[i],
+						     class);
 		if (ret) {
 			dev_err(&pdev->dev, "Failed to add adapter %d\n", i);
 			goto add_adapter_failed;
-- 
1.9.1

Re: [PATCH v1 0/2] i2c: Stop i2c modules being unloaded while in use.

[Peter Rosin]

On 2016-09-07 22:05, jim_baxter@mentor.com wrote:
> From: Jim Baxter <jim_baxter@mentor.com>
> 
> This patchset adds a new i2c_mux_add_reparented_adapter API to the i2c
> that allows owning modules to use module_get/module_put and stop the
> i2c bus module being removed whilst in use.
> 
> This was tested on an ARM i.MX6 Sabre board with the pca953x gpio module.
> 
> Joshua Frkuska (2):
>   i2c-mux: add i2c_mux_add_reparented_adapter api
>   i2c-mux-gpio: call i2c_add_reparented_mux_adapter

nitpick: Patch subjects for the second patch is wrong.

"reparented" is a bit dual when dealing with i2c adapter trees.
i2c_mux_add_owned_adapter is perhaps clearer?

Aside from that, I'm not using modules much and need some enlightenment
as to why the i2c_del_mux_adapter() call in i2c_mux_gpio_remove() is not
sufficient and what exactly the problem is? Why would someone/something
unload the i2c-mux module prematurely?

Would it be an alternative to make i2c-mux a proper kernel object of
some kind? I mean, why do not all other mux users also need to modify
the owner? Why is i2c-mux-gpio special?

CHeers,
Peter

Re: [PATCH v1 0/2] i2c: Stop i2c modules being unloaded while in use.

[Baxter, Jim]

Hi Peter,

> nitpick: Patch subjects for the second patch is wrong.
>
> "reparented" is a bit dual when dealing with i2c adapter trees.
> i2c_mux_add_owned_adapter is perhaps clearer?

Agreed, I will update that.

>
>
> Aside from that, I'm not using modules much and need some enlightenment
> as to why the i2c_del_mux_adapter() call in i2c_mux_gpio_remove() is not
> sufficient and what exactly the problem is? Why would someone/something
> unload the i2c-mux module prematurely?

It is not a normal operation to remove the i2c gpio mux, however systemd
could unload modules out of order if users are restarting services
incorrectly and cause unintended side-effects. This change would stop an
i2c-mux that maybe controlling a voltage regulator from being unloaded
and disabling power to parts of the system unexpectedly.

>
>
> Would it be an alternative to make i2c-mux a proper kernel object of
> some kind? I mean, why do not all other mux users also need to modify
> the owner? Why is i2c-mux-gpio special?
>

i2c-mux-gpio is not special, the code inserted by
[PATCH v1 1/2] i2c-mux: add i2c_mux_add_reparented_adapter api could be
used by other muxes if required.

Best regards
Jim

Re: [PATCH v1 0/2] i2c: Stop i2c modules being unloaded while in use.

[Peter Rosin]

Hi Jim!

On 2016-09-13 18:55, Baxter, Jim wrote:
> Hi Peter,
> 
>> nitpick: Patch subjects for the second patch is wrong.
>>
>> "reparented" is a bit dual when dealing with i2c adapter trees.
>> i2c_mux_add_owned_adapter is perhaps clearer?
> 
> Agreed, I will update that.
> 
>>
>>
>> Aside from that, I'm not using modules much and need some enlightenment
>> as to why the i2c_del_mux_adapter() call in i2c_mux_gpio_remove() is not
>> sufficient and what exactly the problem is? Why would someone/something
>> unload the i2c-mux module prematurely?
> 
> It is not a normal operation to remove the i2c gpio mux, however systemd
> could unload modules out of order if users are restarting services
> incorrectly and cause unintended side-effects. This change would stop an
> i2c-mux that maybe controlling a voltage regulator from being unloaded
> and disabling power to parts of the system unexpectedly.

I don't see how that can happen. The voltage regulator that sits behind
the mux should be a user of one of the child adapters on the mux, so that
child adapter can't be removed as long as the voltage regulator is there.
And the i2c-mux can't be unloaded as long as the child adapter is there,
since it is the owner. So what's going on?

Simply changing the owner of the child adapter will break this chain
leaving the i2c-mux as owner of nothing, thus *making* it a target for
premature unloading.

I must be fundamentally misunderstanding something.

BTW, was this solving a real issue, or is it all theory?

Cheers,
Peter

>> Would it be an alternative to make i2c-mux a proper kernel object of
>> some kind? I mean, why do not all other mux users also need to modify
>> the owner? Why is i2c-mux-gpio special?
>>
> 
> i2c-mux-gpio is not special, the code inserted by
> [PATCH v1 1/2] i2c-mux: add i2c_mux_add_reparented_adapter api could be
> used by other muxes if required.