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
