From: Robert Richter <rrichter@marvell.com>
To: Borislav Petkov <bp@alien8.de>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
Tony Luck <tony.luck@intel.com>,
James Morse <james.morse@arm.com>,
Aristeu Rozanski <aris@redhat.com>,
Matthias Brugger <mbrugger@suse.com>,
<linux-edac@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 08/10] EDAC/ghes: Carve out MC device handling into separate functions
Date: Wed, 6 May 2020 10:45:59 +0200 [thread overview]
Message-ID: <20200506084558.tcayd2fuzoe6rsfm@rric.localdomain> (raw)
In-Reply-To: <20200427163856.GG11036@zn.tnic>
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
next prev parent reply other threads:[~2020-05-06 8:47 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-22 11:58 [PATCH v2 00/10] EDAC/mc/ghes: Fixes, cleanup and reworks Robert Richter
2020-04-22 11:58 ` [PATCH v2 01/10] EDAC/mc: Fix usage of snprintf() and dimm location setup Robert Richter
2020-04-22 20:52 ` Borislav Petkov
2020-05-19 9:27 ` Robert Richter
2020-04-22 11:58 ` [PATCH v2 02/10] EDAC/mc: Use int type for parameters of edac_mc_alloc() Robert Richter
2020-04-23 17:49 ` Borislav Petkov
2020-05-19 9:33 ` Robert Richter
2020-04-22 11:58 ` [PATCH v2 03/10] EDAC/ghes: Remove unused members of struct ghes_edac_pvt, rename it to ghes_mci Robert Richter
2020-04-23 17:55 ` Borislav Petkov
2020-05-05 7:50 ` Robert Richter
2020-04-22 11:58 ` [PATCH v2 04/10] EDAC/ghes: Make SMBIOS handle private data to ghes Robert Richter
2020-04-24 12:12 ` kbuild test robot
2020-04-24 16:21 ` Borislav Petkov
2020-05-05 12:48 ` Robert Richter
2020-04-22 11:58 ` [PATCH v2 05/10] EDAC/ghes: Setup DIMM label from DMI and use it in error reports Robert Richter
2020-04-22 11:58 ` [PATCH v2 06/10] EDAC/ghes: Remove local variable rdr_mask in ghes_edac_dmidecode() Robert Richter
2020-04-27 7:08 ` Borislav Petkov
2020-04-27 17:24 ` Luck, Tony
2020-04-27 17:34 ` Borislav Petkov
2020-05-19 9:34 ` Robert Richter
2020-04-22 11:58 ` [PATCH v2 07/10] EDAC/ghes: Cleanup struct ghes_edac_dimm_fill, rename it to ghes_dimm_fill Robert Richter
2020-04-27 14:00 ` Borislav Petkov
2020-05-19 9:35 ` Robert Richter
2020-04-22 11:58 ` [PATCH v2 08/10] EDAC/ghes: Carve out MC device handling into separate functions Robert Richter
2020-04-27 16:38 ` Borislav Petkov
2020-05-06 8:45 ` Robert Richter [this message]
2020-05-11 13:32 ` Borislav Petkov
2020-05-19 9:57 ` Robert Richter
2020-04-22 11:58 ` [PATCH v2 09/10] EDAC/ghes: Have a separate code path for creating the fake MC Robert Richter
2020-04-22 11:58 ` [PATCH v2 10/10] EDAC/ghes: Carve out code into ghes_edac_register_{one,fake}() Robert Richter
2020-05-06 8:53 ` [PATCH v2 00/10] EDAC/mc/ghes: Fixes, cleanup and reworks Robert Richter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200506084558.tcayd2fuzoe6rsfm@rric.localdomain \
--to=rrichter@marvell.com \
--cc=aris@redhat.com \
--cc=bp@alien8.de \
--cc=james.morse@arm.com \
--cc=linux-edac@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mbrugger@suse.com \
--cc=mchehab@kernel.org \
--cc=tony.luck@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).