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

From: Mary Strodl
Date: Mon Aug 12 2024 - 07:45:18 EST


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.