Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 16 Mar 2020 13:12:14 +0100
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>,
        <linux-edac@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 06/11] EDAC/ghes: Carve out MC device handling into
 separate functions
Message-ID: <20200316121214.qb3nq22xsbiqaz3e@rric.localdomain>
References: <20200306151318.17422-1-rrichter@marvell.com>
 <20200306151318.17422-7-rrichter@marvell.com>
 <20200316093134.GB26126@zn.tnic>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200316093134.GB26126@zn.tnic>
User-Agent: NeoMutt/20170113 (1.7.2)
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from rric.localdomain (31.208.96.227) by HE1PR06CA0142.eurprd06.prod.outlook.com (2603:10a6:7:16::29) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2814.19 via Frontend Transport; Mon, 16 Mar 2020 12:12:20 +0000
X-Originating-IP: [31.208.96.227]
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: adbd3704-3b8b-4b85-2260-08d7c9a347b7
X-MS-TrafficTypeDiagnostic: MN2PR18MB3054:
X-Microsoft-Antispam-PRVS: <MN2PR18MB3054A7F65E40499E2A2CD8C9D9F90@MN2PR18MB3054.namprd18.prod.outlook.com>
X-MS-Oob-TLC-OOBClassifiers: OLM:10000;
X-Forefront-PRVS: 03449D5DD1
X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(4636009)(376002)(136003)(396003)(366004)(39860400002)(346002)(199004)(52116002)(7696005)(8936002)(6506007)(8676002)(6916009)(1076003)(16526019)(6666004)(5660300002)(186003)(316002)(26005)(478600001)(66556008)(2906002)(66476007)(53546011)(54906003)(66946007)(9686003)(86362001)(81156014)(81166006)(55016002)(956004)(4326008);DIR:OUT;SFP:1101;SCL:1;SRVR:MN2PR18MB3054;H:MN2PR18MB3408.namprd18.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;A:1;
Received-SPF: None (protection.outlook.com: marvell.com does not designate
 permitted sender hosts)
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: hifFB5vKd3a0izdniwFmtaL+Xr569KufWh832HgK2eup7cKqBPKFm9iB+V84eVPYuFWl4Eryaaf9i7JpAj6cFWP5biDBF40ewRKj8GfM+QI8+V4TvvxzH5ECgRS4wVflVtRRm3LulwfQxouU+HPlkkKCgk8JHpkaElPfQgOpq+ntuVvsat/ZLLxG8/aYonn0uhjzn/LuUcVc9Yav1gbBsbw8jT6K4c9GCAC8szGnFih2gLaLjqNq9AdwchkW9mU5gDBlPQk4ZlACL/2s2VogInl+Rt3GI+hlIwEGIUZ/xSSyD0WWZkNIgOqOkvU3YKgeSWfh9IppXo8n52az5suV33jBkxUM5ke4n6hz3W6rNbcCtx1syfAsnzAcCPjP70CKrpGnP68SWdcsTlS0feqqRGetB+s0ITcU1Wl7/iDbtu1YLsPKc5Qe/YqrPCYbdcRK
X-MS-Exchange-AntiSpam-MessageData: OlRGKyz/cM/BdKBY3zYbcRNCqISwQ8vILXuek2MAo5A2U80IAyiqfB2cPIzxsKlnzbP44gMBe+nfgXNe3tIRkFRrjpDJtIM0W0kqwEdDRdroSv1OHTEEOCrc3TrbZ1fRsDeK2RW/dFz2+ZQJaaHS1Q==
X-MS-Exchange-CrossTenant-Network-Message-Id: adbd3704-3b8b-4b85-2260-08d7c9a347b7
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Mar 2020 12:12:22.0648
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: kNEfK73CcIcJq1OqkWkCodgyrjyjL9MC74G4pyhl6LPeb6CJfpqUI8dSu29orJRN0PWvVwrlL3WngxKKjexybg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR18MB3054
X-OriginatorOrg: marvell.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138,18.0.572
 definitions=2020-03-16_03:2020-03-12,2020-03-16 signatures=0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 16.03.20 10:31:34, Borislav Petkov wrote:
> On Fri, Mar 06, 2020 at 04:13:13PM +0100, Robert Richter wrote:
> > The functions are too long, carve out code that handles MC devices
> > into the new functions ghes_mc_create(), ghes_mc_add_or_free() and
> > ghes_mc_free(). Apart from better code readability the functions can
> > be reused and the implementation of the error paths becomes easier.
> > 
> > Signed-off-by: Robert Richter <rrichter@marvell.com>
> > ---
> >  drivers/edac/ghes_edac.c | 133 +++++++++++++++++++++++----------------
> >  1 file changed, 79 insertions(+), 54 deletions(-)
> > 
> > diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> > index 358519e8c2e9..5a4c9694bbff 100644
> > --- a/drivers/edac/ghes_edac.c
> > +++ b/drivers/edac/ghes_edac.c
> > @@ -462,16 +462,81 @@ static struct acpi_platform_list plat_list[] = {
> >  	{ } /* End */
> >  };
> >  
> > -int ghes_edac_register(struct ghes *ghes, struct device *dev)
> > +static struct mem_ctl_info *ghes_mc_create(struct device *dev, int mc_idx,
> > +					int num_dimm)
> >  {
> > -	bool fake = false;
> > -	int rc = 0, num_dimm = 0;
> > +	struct edac_mc_layer layers[1];
> >  	struct mem_ctl_info *mci;
> >  	struct ghes_mci *pvt;
> > -	struct edac_mc_layer layers[1];
> > -	struct ghes_dimm_fill dimm_fill;
> > +
> > +	layers[0].type = EDAC_MC_LAYER_ALL_MEM;
> > +	layers[0].size = num_dimm;
> > +	layers[0].is_virt_csrow = true;
> > +
> > +	mci = edac_mc_alloc(mc_idx, ARRAY_SIZE(layers), layers, sizeof(*pvt));
> > +	if (!mci)
> > +		return NULL;
> > +
> > +	pvt		= mci->pvt_info;
> > +	pvt->mci	= mci;
> > +
> > +	mci->pdev = dev;
> > +	mci->mtype_cap = MEM_FLAG_EMPTY;
> > +	mci->edac_ctl_cap = EDAC_FLAG_NONE;
> > +	mci->edac_cap = EDAC_FLAG_NONE;
> > +	mci->mod_name = "ghes_edac.c";
> > +	mci->ctl_name = "ghes_edac";
> > +	mci->dev_name = "ghes";
> > +
> > +	return mci;
> > +}
> > +
> > +static int ghes_mc_add_or_free(struct mem_ctl_info *mci)
> 
> ghes_mc_add() is good enough. The fact that the function has error
> handling doesn't need to be in the name.

It's not just error handling here. I choose the name as the mci is
freed if it could not be added, which is different to just return an
error if it fails. Otherwise the flow would be something like:

	mci = ghes_mc_create(...);
	rc = ghes_mc_add(mci);
	if (rc) {
		ghes_mc_free(mci);
		/* do something */
	}

But we do not need mci any longer on error, so we can free it directly
which makes the code much easier, but in fact it does 2 things at a
time then:

	mci = ghes_mc_create(...);
	rc = ghes_mc_add_or_free(mci);
	if (rc)
        	/* do something */

I would rather prefer ghes_mc_add_or_free().

-Robert