From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id BCE35C52D7C for ; Mon, 12 Aug 2024 11:45:06 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1E0B66B008A; Mon, 12 Aug 2024 07:45:06 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1903E6B008C; Mon, 12 Aug 2024 07:45:06 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0582C6B0092; Mon, 12 Aug 2024 07:45:05 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id D48D66B008A for ; Mon, 12 Aug 2024 07:45:05 -0400 (EDT) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 509C2A7140 for ; Mon, 12 Aug 2024 11:45:05 +0000 (UTC) X-FDA: 82443412170.25.E10D014 Received: from greygoose-centos7.csh.rit.edu (greygoose-centos7.csh.rit.edu [129.21.49.170]) by imf10.hostedemail.com (Postfix) with ESMTP id 57087C0024 for ; Mon, 12 Aug 2024 11:45:03 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=none; dmarc=fail reason="No valid SPF, No valid DKIM" header.from=rit.edu (policy=quarantine); spf=none (imf10.hostedemail.com: domain of mstrodl@freedom.csh.rit.edu has no SPF policy when checking 129.21.49.170) smtp.mailfrom=mstrodl@freedom.csh.rit.edu ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1723463047; a=rsa-sha256; cv=none; b=1sR9ExaV0K0PXGcl9jlmBstbVK1Ay9+YOhysPXGEdCOwiYAno9J55IHazSiukBqUCtf5ou 4jIUHvy7HS7NcDFEZMsZbR2MBjYj5TERCC2vD7aiEt+tVIes/cT2dqWeSPuDE+Qojrbmvy gWv9REapEwaa73zLkhEBskRZykMFTLg= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=none; dmarc=fail reason="No valid SPF, No valid DKIM" header.from=rit.edu (policy=quarantine); spf=none (imf10.hostedemail.com: domain of mstrodl@freedom.csh.rit.edu has no SPF policy when checking 129.21.49.170) smtp.mailfrom=mstrodl@freedom.csh.rit.edu ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1723463047; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=7ljtJ4ijbeiwAJtjiwi7K4Bd/sKWvCNTssokGof7j00=; b=nlVm1CoE/IM3x0ckLyccHSOSWWsBdJWuYjnyZ0WcEKuQacWty7cxDvkkqba55meiZUez15 bAdrSbyTxdcKhXMPgNy5oy4KxO1tdcZiATQRdg3JPsyZU7KxhqosmlLPY4ROCyDSlmYTBu n6IijpHh0bR6sZ6If7p8wp2ChJycpz4= Received: from localhost (localhost [127.0.0.1]) by greygoose-centos7.csh.rit.edu (Postfix) with ESMTP id 325F2457363D; Mon, 12 Aug 2024 07:45:02 -0400 (EDT) X-Virus-Scanned: amavisd-new at csh.rit.edu Received: from greygoose-centos7.csh.rit.edu ([127.0.0.1]) by localhost (mail.csh.rit.edu [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id rPLnRiNy7L2I; Mon, 12 Aug 2024 07:45:01 -0400 (EDT) Received: from freedom.csh.rit.edu (freedom.csh.rit.edu [129.21.49.182]) by greygoose-centos7.csh.rit.edu (Postfix) with ESMTPS id A9B50457328D; Mon, 12 Aug 2024 07:45:01 -0400 (EDT) Date: Mon, 12 Aug 2024 07:45:00 -0400 From: Mary Strodl To: Andi Shyti Cc: Mary Strodl , linux-kernel@vger.kernel.org, akpm@linux-foundation.org, urezki@gmail.com, hch@infradead.org, linux-mm@kvack.org, lee@kernel.org, linux-i2c@vger.kernel.org, s.hauer@pengutronix.de, christian.gmeiner@gmail.com Subject: Re: [PATCH v3 1/2] x86: Add basic support for the Congatec CGEB BIOS interface Message-ID: References: <20240808183527.3950120-1-mstrodl@csh.rit.edu> <20240808183527.3950120-2-mstrodl@csh.rit.edu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 57087C0024 X-Stat-Signature: pif31395o11o7ah86fi8tkxaq4xfyb1a X-Rspamd-Pre-Result: action=add header; module=dmarc; Action set by DMARC X-Rspam-User: X-Rspam: Yes X-HE-Tag: 1723463103-269348 X-HE-Meta: U2FsdGVkX1/TyuIHwI3EitGEggEDNIuHA3LkkCmg/YRJeESRVMy62g/BMXXH/gI8XczJqg4UY3Q4+eWMiu/K5ZjPYT3+uzHSZVScWjzju3VNzGQ8PJ3Twe6Z6gOqg3TSfG+Ch3up7PEwXzXwQwSRgIMO2Id4sm8lG8AvqBXvrmFwcwKspZ935MzGRYIoiy2/KP5crw0CTAIGfcsjaW7fF9Vk8TcKGAnGqe4omNhINYhtiJM42hB9kfTe32DXSH07ByT5EMGIs3DhNslmf6V3eZw9QD04y1kGIM/GrOPFJKAsxaFRyPCC/DU/DJFTMqs8b04O6+xFC/+orshB9o8Fj3sf3g43eJNFeu60x7a9k6RtZM/CaE5pfF/ZUpnuSejgMDsmeab4roTdgiclvMakC9Ws/0jCdk6sIq5bcSKc6X1eDR/43lHG79ss93E5cdjONAcpxC/l8ixJmKptVdwo9/LMwt21lek1qKihXH/Pa5LyMJYfpZ9C9L6XTBkX472DdMVnIGOpvgK80MtR7xaJwQV2AkLn7HsAepTVKe8fDRFW2evzuKQ17bPmIh7cOAiH4zO+JBVCM1D99ggqPXhTpPm5g02mn6SDxV77uiJd/ClrB5wqChkwIgiyRL/Aap5jA+gxZBZTD4bEnwTIfLtmdIAn30fBPQIJ43DUUWnK7mADO4LjjJQr1UcUy4JJV/Oi1CRrycRhCAY+Jl3d9ptEhGDf0xjsvub5R0EMPLE9qkVG5BUeJsSUEs62jabO77gIXev9FhIIWU4lzHrstaiUmGr1xgcgXjJ4inVdHTuP6ApPSM/Ef8ca3/M2gLsPlE0RPa3EARt5FG0xqMBAbXMF8DOqk0TGabsE+XXOIJ/f4J5D47HPvnrrP1inpExMYUG5/pkQPDE5o1YnOQ+zU/JZquFdnlMsm+dE84/6wZC/Szvl5lx1b8HLivHIjVI7zSpt8mUfQvUiaFBZ5fBOCf3 02h1Q28D dzyU3IfSf5AJKPW+sJmkr1ZVmAcXNRb1QtVWHMT56WIfNvL7j1WtXuWr3/hywyEwtjqhaYUBeW9g1zxGwILXwvBHrvR9eR0qbZ6USYuzKnGdumg0nvBCszjkPRA3YZQ9+uBSxoTWgfy3Z/AuLUTkSEdNKphfUE8jEv10p1HXI0jB/yOkN3unEtBz4bNCPI1YJ7TOVehjy6O+6Yd/MCiVyVbh34Q== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: Hi Andi, On Fri, Aug 09, 2024 at 01:46:44AM +0100, Andi Shyti wrote: > If you use the SPDX identifier you don't need to mention the GPL > parts here. Gotcha, will remove the GPL blurb in next series > the include files are normally alphabetically sorted. Will-do! > I'm counting too many global variables here and too many global > structure definitions. It might get a bit hard to follow. > > I'd suggest to simplify this part as much as possible and keep > everything in as less structures as possible (ideally just one). > > Please make local as many variables as you can. I don't disagree with you here. Unfortunately everything between the pack(push, 4) and pack(pop) needs to stay, since those are the BIOS's ABI. I'd be open to putting them in a header file or something though, but I wasn't sure what convention was on stuff like that. What do you think? I think moving the BIOS ABI and maybe the over-the-wire types (struct cgeb_request and friends) out would probably make it much easier to follow. > just return -EBUSY Will-do! > when you use constants like "5", you need to give it an > explanation, either as a define (the name speaks by itself) or a > comment. We don't want people asking "why 5?" :-) I genuinely don't remember how this loop got into it. I'll see if it behaves well without the loop. > You either use brackets for every if/else/else fi or for no one. Gotcha. I think I fixed all of them, will be in next series. > just return err Will-do! > > > + > > + /* Wait for a response to the request */ > > + err = wait_for_completion_interruptible_timeout( > > + &req->done, msecs_to_jiffies(20000)); > > + if (err == 0) { > > + pr_err("CGEB: Timed out running request of type %d!\n", > > + msg.type); > > + err = -ETIMEDOUT; > > just "return -ETIMEDOUT;" and you can cleanup the code below Here's where I landed on that: /* Wait for a response to the request */ err = wait_for_completion_interruptible_timeout( &req->done, msecs_to_jiffies(20000)); if (err == 0) { pr_err("CGEB: Timed out running request of type %d!\n", msg.type); return -ETIMEDOUT; } else if (err < 0) { return err; } else { err = 0; } Unfortunately, I do still need to handle the err < 0 case, but this is a little cleaner than it was. > brackets! Will-do > I don't see any need for the out: label Removed > here it can be useful to have a goto statement: > > goto out; > ... > out: > mutex_unlock(...); > return; Will-do! Thanks again for taking the time to review.