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

[Andi Shyti]

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