From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from greygoose-centos7.csh.rit.edu (greygoose-centos7.csh.rit.edu [129.21.49.170]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2361716EB6F; Mon, 12 Aug 2024 11:45:08 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=129.21.49.170 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1723463111; cv=none; b=lyyn3dihxDCUnkTylMSxrKxRr35zwTHDgzo9P1ZtFutn1wHeGvYdAHM+UrZUnjH6XWsK5ikCDy1R3v34mmQKben1may8+O1wrZPbl69RYJoLi1bf+HME9CSaODb1bUHn4n75MSUAeHxnLkH0BL6xX/7b/1oeP6UL1Ok0MZ+bqUg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1723463111; c=relaxed/simple; bh=uOL+xRrpDRKsfJnu12xst1gJUwPVFXoBXx7bQuZiBOU=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=SPtuvdiGVHW/xB4L0Y8RljmAD7yBIz6HS6PBu8qgDggHW5hQ1pUCn3xfeOmyososqV8wc/xbujv/IWza9NFFlm0vX8tgJe//p4yVGeSzy79GtWz3XtIxyIOdJmPSlxMCLeaHKYoEnvOGMTDdpsrfRstL+tGokQ46uAqFCz4srck= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=freedom.csh.rit.edu; spf=none smtp.mailfrom=freedom.csh.rit.edu; arc=none smtp.client-ip=129.21.49.170 Authentication-Results: smtp.subspace.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=freedom.csh.rit.edu Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=freedom.csh.rit.edu 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> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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.