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.
