[v2,7/9] EDAC: Add devres helpers for edac_mc_alloc/edac_mc_add_mc(_with_groups)

Commit Message

Jan Lübbe Aug. 2, 2017, 12:39 p.m. UTC

These helpers simplify error handling in the _probe functions and automate
deallocation in the _remove functions.

Signed-off-by: Jan Luebbe <[e-mail]>
---
 drivers/edac/edac_mc.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/edac/edac_mc.h | 26 ++++++++++++++++++++++++
 2 files changed, 80 insertions(+)

Comments

Borislav Petkov Aug. 14, 2017, 11:01 a.m. UTC | #1

On Wed, Aug 02, 2017 at 02:39:20PM +0200, Jan Luebbe wrote:
> These helpers simplify error handling in the _probe functions and automate
> deallocation in the _remove functions.
> 
> Signed-off-by: Jan Luebbe <[e-mail]>
> ---
>  drivers/edac/edac_mc.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/edac/edac_mc.h | 26 ++++++++++++++++++++++++
>  2 files changed, 80 insertions(+)
> 
> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> index 480072139b7a..d3ee2b851329 100644
> --- a/drivers/edac/edac_mc.c
> +++ b/drivers/edac/edac_mc.c
> @@ -517,6 +517,34 @@ void edac_mc_free(struct mem_ctl_info *mci)
>  }
>  EXPORT_SYMBOL_GPL(edac_mc_free);
>  
> +/**
> + * devm_edac_mc_free() - Internal helper to call edac_mc_free from a devres
> + *	action.
> + */
> +static void devm_edac_mc_free(void *mci)
> +{
> +	edac_mc_free((struct mem_ctl_info *)mci);
> +}
> +
> +struct mem_ctl_info *devm_edac_mc_alloc(struct device *dev,
> +					unsigned int mc_num,
> +					unsigned int n_layers,
> +					struct edac_mc_layer *layers,
> +					unsigned int sz_pvt)
> +{
> +	struct mem_ctl_info *mci;
> +
> +	mci = edac_mc_alloc(mc_num, n_layers, layers, sz_pvt);
> +	if (!mci)
> +		return mci;
> +
> +	if (devm_add_action_or_reset(dev, devm_edac_mc_free, mci))
> +		return NULL;
> +
> +	return mci;
> +}
> +EXPORT_SYMBOL_GPL(devm_edac_mc_alloc);

Ok, I see what you want to do here but I'm not excited.

Rather, what I've been telling other EDAC driver writers is to first
probe the hardware as much as possible until they *know* they're all
ready to init and only then call the EDAC core to allocate structures.

Because, with this lazy method, the pointless memory allocation
and freeing would still happen if the late probing fails. But that
allocation and freeing is completely pointless.

So looking at armada_xp_mc_edac_probe(), for example, you should do
edac_mc_alloc() *after* armada_xp_mc_edac_read_config() has succeeded.
The same idea holds true for aurora_l2_edac_probe(): do all probing
and gathering of info from the hardware first and then call EDAC core
functions.

Thanks.

Patch

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 480072139b7a..d3ee2b851329 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -517,6 +517,34 @@  void edac_mc_free(struct mem_ctl_info *mci)
 }
 EXPORT_SYMBOL_GPL(edac_mc_free);
 
+/**
+ * devm_edac_mc_free() - Internal helper to call edac_mc_free from a devres
+ *	action.
+ */
+static void devm_edac_mc_free(void *mci)
+{
+	edac_mc_free((struct mem_ctl_info *)mci);
+}
+
+struct mem_ctl_info *devm_edac_mc_alloc(struct device *dev,
+					unsigned int mc_num,
+					unsigned int n_layers,
+					struct edac_mc_layer *layers,
+					unsigned int sz_pvt)
+{
+	struct mem_ctl_info *mci;
+
+	mci = edac_mc_alloc(mc_num, n_layers, layers, sz_pvt);
+	if (!mci)
+		return mci;
+
+	if (devm_add_action_or_reset(dev, devm_edac_mc_free, mci))
+		return NULL;
+
+	return mci;
+}
+EXPORT_SYMBOL_GPL(devm_edac_mc_alloc);
+
 bool edac_has_mcs(void)
 {
 	bool ret;
@@ -828,6 +856,32 @@  struct mem_ctl_info *edac_mc_del_mc(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(edac_mc_del_mc);
 
+/**
+ * devm_edac_mc_del_mc() - Internal helper to call edac_mc_del_mc from a devres
+ *	action.
+ */
+static void devm_edac_mc_del_mc(void *dev)
+{
+	edac_mc_del_mc((struct device *)dev);
+}
+
+int devm_edac_mc_add_mc_with_groups(struct device *dev,
+				    struct mem_ctl_info *mci,
+				    const struct attribute_group **groups)
+{
+	int ret;
+
+	ret = edac_mc_add_mc_with_groups(mci, groups);
+	if (ret)
+		return ret;
+
+	if (devm_add_action_or_reset(dev, devm_edac_mc_del_mc, dev))
+		return 1;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_edac_mc_add_mc_with_groups);
+
 static void edac_mc_scrub_block(unsigned long page, unsigned long offset,
 				u32 size)
 {
diff --git a/drivers/edac/edac_mc.h b/drivers/edac/edac_mc.h
index 5357800e418d..32f88cc4a65b 100644
--- a/drivers/edac/edac_mc.h
+++ b/drivers/edac/edac_mc.h
@@ -149,6 +149,32 @@  extern int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
 extern void edac_mc_free(struct mem_ctl_info *mci);
 
 /**
+ * devm_edac_mc_alloc() - Helper to call edac_mc_alloc() and register it for
+ *	cleanup with devres.
+ *
+ * Returns:
+ *	On success, return a pointer to struct mem_ctl_info pointer;
+ *	%NULL otherwise
+ */
+struct mem_ctl_info *devm_edac_mc_alloc(struct device *dev,
+					unsigned int mc_num,
+					unsigned int n_layers,
+					struct edac_mc_layer *layers,
+					unsigned int sz_pvt);
+
+/**
+ * devm_edac_mc_add_mc_with_groups() - Helper to call
+ *	edac_mc_add_mc_with_groups() and register it for cleanup with devres.
+ *
+ * Returns:
+ *	0 on Success, or an error code on failure
+ */
+int devm_edac_mc_add_mc_with_groups(struct device *dev,
+				    struct mem_ctl_info *mci,
+				    const struct attribute_group **groups);
+#define devm_edac_mc_add_mc(dev, mci)	devm_edac_mc_add_mc_with_groups(dev, mci, NULL)
+
+/**
  * edac_has_mcs() - Check if any MCs have been allocated.
  *
  * Returns: