linux-kernel - Re: [PATCH v3 1/2] x86: Add basic support for the Congatec CGEB BIOS interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zrn1vNgn7q5npxU7@freedom.csh.rit.edu>
Date: Mon, 12 Aug 2024 07:45:00 -0400
From: Mary Strodl <mstrodl@...edom.csh.rit.edu>
To: Andi Shyti <andi.shyti@...nel.org>
Cc: Mary Strodl <mstrodl@....rit.edu>, linux-kernel@...r.kernel.org,
	akpm@...ux-foundation.org, urezki@...il.com, hch@...radead.org,
	linux-mm@...ck.org, lee@...nel.org, linux-i2c@...r.kernel.org,
	s.hauer@...gutronix.de, christian.gmeiner@...il.com
Subject: Re: [PATCH v3 1/2] x86: Add basic support for the Congatec CGEB BIOS
 interface

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.