On 27.04.20 18:38:56, Borislav Petkov wrote:
> On Wed, Apr 22, 2020 at 01:58:12PM +0200, Robert Richter wrote:
> > +static int ghes_mc_add_or_free(struct mem_ctl_info *mci,
> > + struct list_head *dimm_list)
>
> No, I think we talked about this already. This function should be
> called:
>
> ghes_mc_add()
>
> and should do one thing and one thing only in good old unix tradition:
> add the MC.
>
> > +{
> > unsigned long flags;
> > - int idx = -1;
> > + int rc;
> > +
> > + rc = edac_mc_add_mc(mci);
> > + if (rc < 0) {
>
> > + ghes_dimm_release(dimm_list);
> > + edac_mc_free(mci);
> > + return rc;
>
> Those last three lines should be called by the *caller* of
> ghes_mc_add(), when latter returns an error value.
These direct operations are nothing a caller should deal with.
The caller does now:
mci = ghes_mc_create(...);
... /* prepare dimms */
return ghes_mc_add_or_free(...);
To shut it down we just use:
ghes_mc_free();
Pretty simple.
Now, lets look at your suggestion to put it out of the function. A
caller always needs to free the mci and dimms, so we will get:
int rc;
mci = ghes_mc_create(...);
... /* prepare dimms */
rc = ghes_mc_add(...);
if (rc < 0) {
/* free mci */
/* free dimms */
...
}
return rc;
We loose the tail call and simplicity here. Note this duplicates code
as there are 2 users of ghes_mc_add().
Now, the caller does not know the implementation details, so we need
to provide another release function (let's call it *_release() here):
mci = ghes_mc_create(...);
... /* prepare dimms */
rc = ghes_mc_add(...);
if (rc < 0) {
ghes_mc_release(mci);
ghes_dimm_release(dimm_list);
}
return rc;
Ok, now there is another function needed to release everything.
This design also impacts ghes_mc_free(). So the shutdown
implementation would turn to:
struct mem_ctl_info *mci;
...
mci = ghes_mc_del();
ghes_mc_release(mci);
...
I don't see any benefit. See also below for the delta of an
implementation of the suggested changes.
>
> > + }
> > +
> > + spin_lock_irqsave(&ghes_lock, flags);
> > + ghes_pvt = mci->pvt_info;
> > + list_splice_tail(dimm_list, &ghes_dimm_list);
> > + spin_unlock_irqrestore(&ghes_lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static void ghes_mc_free(void)
> > +{
> > + struct mem_ctl_info *mci;
> > + unsigned long flags;
> > + LIST_HEAD(dimm_list);
> > +
> > + /*
> > + * Wait for the irq handler being finished.
> > + */
> > + spin_lock_irqsave(&ghes_lock, flags);
> > + mci = ghes_pvt ? ghes_pvt->mci : NULL;
> > + ghes_pvt = NULL;
> > + list_splice_init(&ghes_dimm_list, &dimm_list);
> > + spin_unlock_irqrestore(&ghes_lock, flags);
> > +
> > + ghes_dimm_release(&dimm_list);
> > +
> > + if (!mci)
> > + return;
> > +
> > + mci = edac_mc_del_mc(mci->pdev);
> > + if (mci)
> > + edac_mc_free(mci);
> > +}
>
> This function needs to do only freeing of the mc. The list splicing and
> dimm releasing needs to be done by its caller, before calling it.
ghes_mc_free() is the counterpart to ghes_mc_add() and thus needs to
also handle the dimm_list here. This cannot be left to the caller.
Considering all the above, I don't see how your suggestions to the
interface could improve the code. Hmm...
Below an implementation that illustrates the changes.
Thanks,
-Robert
---
drivers/edac/ghes_edac.c | 41 +++++++++++++++++++++++++++-------------
1 file changed, 28 insertions(+), 13 deletions(-)
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 7f39346d895b..896d7b488fc2 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -576,18 +576,14 @@ static struct mem_ctl_info *ghes_mc_create(struct device
*dev, int mc_idx,
return mci;
}
-static int ghes_mc_add_or_free(struct mem_ctl_info *mci,
- struct list_head *dimms)
+static int ghes_mc_add(struct mem_ctl_info *mci, struct list_head *dimms)
{
unsigned long flags;
int rc;
rc = edac_mc_add_mc(mci);
- if (rc < 0) {
- dimm_release(dimms);
- edac_mc_free(mci);
+ if (rc < 0)
return rc;
- }
spin_lock_irqsave(&ghes_lock, flags);
ghes_pvt = mci->pvt_info;
@@ -597,7 +593,7 @@ static int ghes_mc_add_or_free(struct mem_ctl_info *mci,
return 0;
}
-static void ghes_mc_free(void)
+static struct mem_ctl_info *ghes_mc_del(void)
{
struct mem_ctl_info *mci;
unsigned long flags;
@@ -614,10 +610,14 @@ static void ghes_mc_free(void)
dimm_release(&dimms);
- if (!mci)
- return;
+ if (mci)
+ mci = edac_mc_del_mc(mci->pdev);
- mci = edac_mc_del_mc(mci->pdev);
+ return mci;
+}
+
+static void ghes_mc_release(struct mem_ctl_info *mci)
+{
if (mci)
edac_mc_free(mci);
}
@@ -627,6 +627,7 @@ static int ghes_edac_register_fake(struct device *dev)
struct mem_ctl_info *mci;
struct dimm_info *dimm;
LIST_HEAD(empty);
+ int rc;
mci = ghes_mc_create(dev, 0, 1);
if (!mci)
@@ -642,13 +643,18 @@ static int ghes_edac_register_fake(struct device *dev)
snprintf(dimm->label, sizeof(dimm->label), "unknown memory");
- return ghes_mc_add_or_free(mci, &empty);
+ rc = ghes_mc_add(mci, &empty);
+ if (rc < 0)
+ ghes_mc_free(mci);
+
+ return rc;
}
static int ghes_edac_register_one(struct device *dev, int mc_idx, int num_dimm)
{
struct dimm_fill dimm_fill;
struct mem_ctl_info *mci;
+ int rc;
mci = ghes_mc_create(dev, mc_idx, num_dimm);
if (!mci)
@@ -660,7 +666,13 @@ static int ghes_edac_register_one(struct device *dev, int
mc_idx, int num_dimm)
dmi_walk(ghes_edac_dmidecode, &dimm_fill);
- return ghes_mc_add_or_free(mci, &dimm_fill.dimms);
+ rc = ghes_mc_add(mci, &dimm_fill.dimms);
+ if (rc < 0) {
+ dimm_release(&dimm_fill.dimms);
+ ghes_mc_release(mci);
+ }
+
+ return rc;
}
int ghes_edac_register(struct ghes *ghes, struct device *dev)
@@ -740,10 +752,13 @@ int ghes_edac_register(struct ghes *ghes, struct device
*dev)
void ghes_edac_unregister(struct ghes *ghes)
{
+ struct mem_ctl_info *mci;
+
mutex_lock(&ghes_reg_mutex);
if (refcount_dec_and_test(&ghes_refcount)) {
- ghes_mc_free();
+ mci = ghes_mc_del();
+ ghes_mc_release(mci);
dimm_pool_destroy();
}
--
2.20.1
