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

[Mary Strodl <mstrodl@freedom.csh.rit.edu>]

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.