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: <lek2i4jpc7x257ycxeuto2op742x555a25wxdgh5lzcgtqg3lp@t7opf5nlgspt>
Date: Mon, 12 Aug 2024 21:16:47 +0100
From: Andi Shyti <andi.shyti@...nel.org>
To: Mary Strodl <mstrodl@...edom.csh.rit.edu>
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 Mary,

> > 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.

mine was a guess, actually... if you see that my comment doesn't
fit, feel free to ignore it.

As I said I didn't look in details all the structures and their
dependencies.

> > > +
> > > +	/* 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 {

you don't ened anymore for this else, at the end, but this looks
correct.

Thanks,
Andi