| Message ID | 20240808154658.247873-4-herve.codina@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for the LAN966x PCI device using a DT overlay | expand |
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?
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é
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é
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.
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