diff mbox series

[v5,3/8] mfd: syscon: Add reference counting and device managed support

Message ID 20240808154658.247873-4-herve.codina@bootlin.com (mailing list archive)
State New, archived
Headers show
Series Add support for the LAN966x PCI device using a DT overlay | expand

Commit Message

Herve Codina Aug. 8, 2024, 3:46 p.m. UTC 
From: Clément Léger <clement.leger@bootlin.com>

Syscon releasing is not supported.
Without release function, unbinding a driver that uses syscon whether
explicitly or due to a module removal left the used syscon in a in-use
state.

For instance a syscon_node_to_regmap() call from a consumer retrieves a
syscon regmap instance. Internally, syscon_node_to_regmap() can create
syscon instance and add it to the existing syscon list. No API is
available to release this syscon instance, remove it from the list and
free it when it is not used anymore.

Introduce reference counting in syscon in order to keep track of syscon
usage using syscon_{get,put}() and add a device managed version of
syscon_regmap_lookup_by_phandle(), to automatically release the syscon
instance on the consumer removal.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/mfd/syscon.c       | 138 ++++++++++++++++++++++++++++++++++---
 include/linux/mfd/syscon.h |  16 +++++
 2 files changed, 144 insertions(+), 10 deletions(-)

Comments

Lee Jones Sept. 3, 2024, 3:38 p.m. UTC | #1 
On Thu, 08 Aug 2024, Herve Codina wrote:

> From: Clément Léger <clement.leger@bootlin.com>
> 
> Syscon releasing is not supported.
> Without release function, unbinding a driver that uses syscon whether
> explicitly or due to a module removal left the used syscon in a in-use
> state.
> 
> For instance a syscon_node_to_regmap() call from a consumer retrieves a
> syscon regmap instance. Internally, syscon_node_to_regmap() can create
> syscon instance and add it to the existing syscon list. No API is
> available to release this syscon instance, remove it from the list and
> free it when it is not used anymore.
> 
> Introduce reference counting in syscon in order to keep track of syscon
> usage using syscon_{get,put}() and add a device managed version of
> syscon_regmap_lookup_by_phandle(), to automatically release the syscon
> instance on the consumer removal.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/mfd/syscon.c       | 138 ++++++++++++++++++++++++++++++++++---
>  include/linux/mfd/syscon.h |  16 +++++
>  2 files changed, 144 insertions(+), 10 deletions(-)

This doesn't look very popular.

What are the potential ramifications for existing users?
Herve Codina Sept. 3, 2024, 4:01 p.m. UTC | #2 
Hi Lee,

On Tue, 3 Sep 2024 16:38:39 +0100
Lee Jones <lee@kernel.org> wrote:

> On Thu, 08 Aug 2024, Herve Codina wrote:
> 
> > From: Clément Léger <clement.leger@bootlin.com>
> > 
> > Syscon releasing is not supported.
> > Without release function, unbinding a driver that uses syscon whether
> > explicitly or due to a module removal left the used syscon in a in-use
> > state.
> > 
> > For instance a syscon_node_to_regmap() call from a consumer retrieves a
> > syscon regmap instance. Internally, syscon_node_to_regmap() can create
> > syscon instance and add it to the existing syscon list. No API is
> > available to release this syscon instance, remove it from the list and
> > free it when it is not used anymore.
> > 
> > Introduce reference counting in syscon in order to keep track of syscon
> > usage using syscon_{get,put}() and add a device managed version of
> > syscon_regmap_lookup_by_phandle(), to automatically release the syscon
> > instance on the consumer removal.
> > 
> > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  drivers/mfd/syscon.c       | 138 ++++++++++++++++++++++++++++++++++---
> >  include/linux/mfd/syscon.h |  16 +++++
> >  2 files changed, 144 insertions(+), 10 deletions(-)  
> 
> This doesn't look very popular.
> 
> What are the potential ramifications for existing users?
> 

Existing user don't use devm_syscon_regmap_lookup_by_phandle() nor
syscon_put_regmap().

So refcount is incremented but never decremented. syscon is never
released. Exactly the same as current implementation.
Nothing change for existing users.

Best regards,
Hervé
Herve Codina Sept. 9, 2024, 7:52 a.m. UTC | #3 
Hi Lee, Arnd,

On Tue, 3 Sep 2024 18:01:16 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> Hi Lee,
> 
> On Tue, 3 Sep 2024 16:38:39 +0100
> Lee Jones <lee@kernel.org> wrote:
> 
> > On Thu, 08 Aug 2024, Herve Codina wrote:
> >   
> > > From: Clément Léger <clement.leger@bootlin.com>
> > > 
> > > Syscon releasing is not supported.
> > > Without release function, unbinding a driver that uses syscon whether
> > > explicitly or due to a module removal left the used syscon in a in-use
> > > state.
> > > 
> > > For instance a syscon_node_to_regmap() call from a consumer retrieves a
> > > syscon regmap instance. Internally, syscon_node_to_regmap() can create
> > > syscon instance and add it to the existing syscon list. No API is
> > > available to release this syscon instance, remove it from the list and
> > > free it when it is not used anymore.
> > > 
> > > Introduce reference counting in syscon in order to keep track of syscon
> > > usage using syscon_{get,put}() and add a device managed version of
> > > syscon_regmap_lookup_by_phandle(), to automatically release the syscon
> > > instance on the consumer removal.
> > > 
> > > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > > ---
> > >  drivers/mfd/syscon.c       | 138 ++++++++++++++++++++++++++++++++++---
> > >  include/linux/mfd/syscon.h |  16 +++++
> > >  2 files changed, 144 insertions(+), 10 deletions(-)    
> > 
> > This doesn't look very popular.
> > 
> > What are the potential ramifications for existing users?
> >   
> 
> Existing user don't use devm_syscon_regmap_lookup_by_phandle() nor
> syscon_put_regmap().
> 
> So refcount is incremented but never decremented. syscon is never
> released. Exactly the same as current implementation.
> Nothing change for existing users.
> 
> Best regards,
> Hervé

I hope I answered to Lee's question related to possible impacts on
existing drivers.

Is there anything else that blocks this patch from being applied ?

Best regards,
Hervé
Lee Jones Sept. 12, 2024, 2:37 p.m. UTC | #4 
On Mon, 09 Sep 2024, Herve Codina wrote:

> Hi Lee, Arnd,
> 
> On Tue, 3 Sep 2024 18:01:16 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
> 
> > Hi Lee,
> > 
> > On Tue, 3 Sep 2024 16:38:39 +0100
> > Lee Jones <lee@kernel.org> wrote:
> > 
> > > On Thu, 08 Aug 2024, Herve Codina wrote:
> > >   
> > > > From: Clément Léger <clement.leger@bootlin.com>
> > > > 
> > > > Syscon releasing is not supported.
> > > > Without release function, unbinding a driver that uses syscon whether
> > > > explicitly or due to a module removal left the used syscon in a in-use
> > > > state.
> > > > 
> > > > For instance a syscon_node_to_regmap() call from a consumer retrieves a
> > > > syscon regmap instance. Internally, syscon_node_to_regmap() can create
> > > > syscon instance and add it to the existing syscon list. No API is
> > > > available to release this syscon instance, remove it from the list and
> > > > free it when it is not used anymore.
> > > > 
> > > > Introduce reference counting in syscon in order to keep track of syscon
> > > > usage using syscon_{get,put}() and add a device managed version of
> > > > syscon_regmap_lookup_by_phandle(), to automatically release the syscon
> > > > instance on the consumer removal.
> > > > 
> > > > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > > > ---
> > > >  drivers/mfd/syscon.c       | 138 ++++++++++++++++++++++++++++++++++---
> > > >  include/linux/mfd/syscon.h |  16 +++++
> > > >  2 files changed, 144 insertions(+), 10 deletions(-)    
> > > 
> > > This doesn't look very popular.
> > > 
> > > What are the potential ramifications for existing users?
> > >   
> > 
> > Existing user don't use devm_syscon_regmap_lookup_by_phandle() nor
> > syscon_put_regmap().
> > 
> > So refcount is incremented but never decremented. syscon is never
> > released. Exactly the same as current implementation.
> > Nothing change for existing users.
> > 
> > Best regards,
> > Hervé
> 
> I hope I answered to Lee's question related to possible impacts on
> existing drivers.
> 
> Is there anything else that blocks this patch from being applied ?

Arnd usually takes care of Syscon reviews.

Perhaps he's out on vacation.

Let's wait a little longer, since it's too late for this cycle anyway.
Herve Codina Sept. 23, 2024, 8:07 a.m. UTC | #5 
Hi,

On Thu, 12 Sep 2024 15:37:40 +0100
Lee Jones <lee@kernel.org> wrote:

> On Mon, 09 Sep 2024, Herve Codina wrote:
> 
> > Hi Lee, Arnd,
> > 
> > On Tue, 3 Sep 2024 18:01:16 +0200
> > Herve Codina <herve.codina@bootlin.com> wrote:
> >   
> > > Hi Lee,
> > > 
> > > On Tue, 3 Sep 2024 16:38:39 +0100
> > > Lee Jones <lee@kernel.org> wrote:
> > >   
> > > > On Thu, 08 Aug 2024, Herve Codina wrote:
> > > >     
> > > > > From: Clément Léger <clement.leger@bootlin.com>
> > > > > 
> > > > > Syscon releasing is not supported.
> > > > > Without release function, unbinding a driver that uses syscon whether
> > > > > explicitly or due to a module removal left the used syscon in a in-use
> > > > > state.
> > > > > 
> > > > > For instance a syscon_node_to_regmap() call from a consumer retrieves a
> > > > > syscon regmap instance. Internally, syscon_node_to_regmap() can create
> > > > > syscon instance and add it to the existing syscon list. No API is
> > > > > available to release this syscon instance, remove it from the list and
> > > > > free it when it is not used anymore.
> > > > > 
> > > > > Introduce reference counting in syscon in order to keep track of syscon
> > > > > usage using syscon_{get,put}() and add a device managed version of
> > > > > syscon_regmap_lookup_by_phandle(), to automatically release the syscon
> > > > > instance on the consumer removal.
> > > > > 
> > > > > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> > > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > > > > ---
> > > > >  drivers/mfd/syscon.c       | 138 ++++++++++++++++++++++++++++++++++---
> > > > >  include/linux/mfd/syscon.h |  16 +++++
> > > > >  2 files changed, 144 insertions(+), 10 deletions(-)      
> > > > 
> > > > This doesn't look very popular.
> > > > 
> > > > What are the potential ramifications for existing users?
> > > >     
> > > 
> > > Existing user don't use devm_syscon_regmap_lookup_by_phandle() nor
> > > syscon_put_regmap().
> > > 
> > > So refcount is incremented but never decremented. syscon is never
> > > released. Exactly the same as current implementation.
> > > Nothing change for existing users.
> > > 
> > > Best regards,
> > > Hervé  
> > 
> > I hope I answered to Lee's question related to possible impacts on
> > existing drivers.
> > 
> > Is there anything else that blocks this patch from being applied ?  
> 
> Arnd usually takes care of Syscon reviews.
> 
> Perhaps he's out on vacation.
> 
> Let's wait a little longer, since it's too late for this cycle anyway.
> 

Discussed the topic with Arnd Bergmann at Linux Plumbers Conference.
Adding ref-counting and support for removal in syscon is rejected by Arnd.

For my LAN966x use case (syscon is used only by the reset controller), the
solution is to remove the syscon device and handle directly the reset protect
register in the reset controller itself.

I will propose modifications in that way in the next iteration.

Regards,
Hervé
diff mbox series

Patch

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 33f1e07ab24d..a0f8737b8d63 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -34,6 +34,7 @@  struct syscon {
 	struct regmap *regmap;
 	struct reset_control *reset;
 	struct list_head list;
+	struct kref refcount;
 };
 
 static const struct regmap_config syscon_regmap_config = {
@@ -147,6 +148,8 @@  static struct syscon *of_syscon_register(struct device_node *np, bool check_res)
 
 	syscon->regmap = regmap;
 	syscon->np = np;
+	of_node_get(syscon->np);
+	kref_init(&syscon->refcount);
 
 	spin_lock(&syscon_list_slock);
 	list_add_tail(&syscon->list, &syscon_list);
@@ -168,7 +171,31 @@  static struct syscon *of_syscon_register(struct device_node *np, bool check_res)
 	return ERR_PTR(ret);
 }
 
-static struct regmap *device_node_get_regmap(struct device_node *np,
+static void syscon_free(struct kref *kref)
+{
+	struct syscon *syscon = container_of(kref, struct syscon, refcount);
+
+	spin_lock(&syscon_list_slock);
+	list_del(&syscon->list);
+	spin_unlock(&syscon_list_slock);
+
+	regmap_exit(syscon->regmap);
+	of_node_put(syscon->np);
+	kfree(syscon);
+}
+
+static struct syscon *syscon_get(struct syscon *syscon)
+{
+	kref_get(&syscon->refcount);
+	return syscon;
+}
+
+static void syscon_put(struct syscon *syscon)
+{
+	kref_put(&syscon->refcount, syscon_free);
+}
+
+static struct syscon *device_node_get_syscon(struct device_node *np,
 					     bool check_res)
 {
 	struct syscon *entry, *syscon = NULL;
@@ -183,9 +210,18 @@  static struct regmap *device_node_get_regmap(struct device_node *np,
 
 	spin_unlock(&syscon_list_slock);
 
-	if (!syscon)
-		syscon = of_syscon_register(np, check_res);
+	if (syscon)
+		return syscon_get(syscon);
+
+	return of_syscon_register(np, check_res);
+}
+
+static struct regmap *device_node_get_regmap(struct device_node *np,
+					     bool check_res)
+{
+	struct syscon *syscon;
 
+	syscon = device_node_get_syscon(np, check_res);
 	if (IS_ERR(syscon))
 		return ERR_CAST(syscon);
 
@@ -246,12 +282,23 @@  struct regmap *device_node_to_regmap(struct device_node *np)
 }
 EXPORT_SYMBOL_GPL(device_node_to_regmap);
 
-struct regmap *syscon_node_to_regmap(struct device_node *np)
+static struct syscon *syscon_node_to_syscon(struct device_node *np)
 {
 	if (!of_device_is_compatible(np, "syscon"))
 		return ERR_PTR(-EINVAL);
 
-	return device_node_get_regmap(np, true);
+	return device_node_get_syscon(np, true);
+}
+
+struct regmap *syscon_node_to_regmap(struct device_node *np)
+{
+	struct syscon *syscon;
+
+	syscon = syscon_node_to_syscon(np);
+	if (IS_ERR(syscon))
+		return ERR_CAST(syscon);
+
+	return syscon->regmap;
 }
 EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
 
@@ -271,11 +318,11 @@  struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
 }
 EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
 
-struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
-					const char *property)
+static struct syscon *syscon_lookup_by_phandle(struct device_node *np,
+					       const char *property)
 {
 	struct device_node *syscon_np;
-	struct regmap *regmap;
+	struct syscon *syscon;
 
 	if (property)
 		syscon_np = of_parse_phandle(np, property, 0);
@@ -285,12 +332,24 @@  struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
 	if (!syscon_np)
 		return ERR_PTR(-ENODEV);
 
-	regmap = syscon_node_to_regmap(syscon_np);
+	syscon = syscon_node_to_syscon(syscon_np);
 
 	if (property)
 		of_node_put(syscon_np);
 
-	return regmap;
+	return syscon;
+}
+
+struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
+					       const char *property)
+{
+	struct syscon *syscon;
+
+	syscon = syscon_lookup_by_phandle(np, property);
+	if (IS_ERR(syscon))
+		return ERR_CAST(syscon);
+
+	return syscon->regmap;
 }
 EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle);
 
@@ -341,6 +400,65 @@  struct regmap *syscon_regmap_lookup_by_phandle_optional(struct device_node *np,
 }
 EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle_optional);
 
+static struct syscon *syscon_from_regmap(struct regmap *regmap)
+{
+	struct syscon *entry, *syscon = NULL;
+
+	spin_lock(&syscon_list_slock);
+
+	list_for_each_entry(entry, &syscon_list, list)
+		if (entry->regmap == regmap) {
+			syscon = entry;
+			break;
+		}
+
+	spin_unlock(&syscon_list_slock);
+
+	return syscon;
+}
+
+void syscon_put_regmap(struct regmap *regmap)
+{
+	struct syscon *syscon;
+
+	syscon = syscon_from_regmap(regmap);
+	if (!syscon)
+		return;
+
+	syscon_put(syscon);
+}
+EXPORT_SYMBOL_GPL(syscon_put_regmap);
+
+static void devm_syscon_release(void *syscon)
+{
+	syscon_put(syscon);
+}
+
+static struct regmap *__devm_syscon_get(struct device *dev,
+					struct syscon *syscon)
+{
+	int ret;
+
+	if (IS_ERR(syscon))
+		return ERR_CAST(syscon);
+
+	ret = devm_add_action_or_reset(dev, devm_syscon_release, syscon);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return syscon->regmap;
+}
+
+struct regmap *devm_syscon_regmap_lookup_by_phandle(struct device *dev,
+						    struct device_node *np,
+						    const char *property)
+{
+	struct syscon *syscon = syscon_lookup_by_phandle(np, property);
+
+	return __devm_syscon_get(dev, syscon);
+}
+EXPORT_SYMBOL_GPL(devm_syscon_regmap_lookup_by_phandle);
+
 static int syscon_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h
index aad9c6b50463..a3025b4efb4a 100644
--- a/include/linux/mfd/syscon.h
+++ b/include/linux/mfd/syscon.h
@@ -15,6 +15,7 @@ 
 #include <linux/errno.h>
 
 struct device_node;
+struct device;
 
 #ifdef CONFIG_MFD_SYSCON
 struct regmap *device_node_to_regmap(struct device_node *np);
@@ -30,6 +31,10 @@  struct regmap *syscon_regmap_lookup_by_phandle_optional(struct device_node *np,
 							const char *property);
 int of_syscon_register_regmap(struct device_node *np,
 			      struct regmap *regmap);
+void syscon_put_regmap(struct regmap *regmap);
+struct regmap *devm_syscon_regmap_lookup_by_phandle(struct device *dev,
+						    struct device_node *np,
+						    const char *property);
 #else
 static inline struct regmap *device_node_to_regmap(struct device_node *np)
 {
@@ -73,6 +78,17 @@  static inline int of_syscon_register_regmap(struct device_node *np,
 					struct regmap *regmap)
 {
 	return -EOPNOTSUPP;
+
+static inline void syscon_put_regmap(struct regmap *regmap)
+{
+}
+
+static inline
+struct regmap *devm_syscon_regmap_lookup_by_phandle(struct device *dev,
+						    struct device_node *np,
+						    const char *property)
+{
+	return NULL;
 }
 
 #endif