[Skiboot] [RFC PATCH] FSP: Solidify timeout poll code

[Ananth N Mavinakayanahalli]

On Mon, Dec 08, 2014 at 11:36:20PM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2014-12-08 at 17:41 +0530, Ananth N Mavinakayanahalli wrote:
> > Prevent multiple callers to the timeout poller.
> > Add more error checking while there.
> > 
> > Signed-off-by: Ananth N Mavinakayanahalli <ananth at in.ibm.com>
> > ---
> >  hw/fsp/fsp.c |   13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/fsp/fsp.c b/hw/fsp/fsp.c
> > index 8a73f2c..bc5e3e7 100644
> > --- a/hw/fsp/fsp.c
> > +++ b/hw/fsp/fsp.c
> > @@ -91,6 +91,7 @@ static void *fsp_inbound_buf = NULL;
> >  static u32 fsp_inbound_off;
> >  
> >  static struct lock fsp_lock = LOCK_UNLOCKED;
> > +static struct lock fsp_timeout_poller_lock = LOCK_UNLOCKED;
> >  
> >  static u64 fsp_cmdclass_resp_bitmask;
> >  static u64 timeout_timer;
> > @@ -1904,7 +1905,7 @@ bool fsp_present(void)
> >  	return first_fsp != NULL;
> >  }
> >  
> > -static void fsp_timeout_poll(void *data __unused)
> > +static void __fsp_timeout_poll(void *data __unused)
> >  {
> >  	u64 now = mftb();
> >  	u64 timeout_val = 0;
> > @@ -1952,6 +1953,8 @@ static void fsp_timeout_poll(void *data __unused)
> >  				goto next_bit;
> >  			}
> >  			req = list_top(&cmdclass->msgq,	struct fsp_msg, link);
> > +			if (!req)
> > +				goto next_bit;
> >  			w0 = req->word0;
> >  			w1 = req->word1;
> >  			mstate = req->state;
> > @@ -1976,6 +1979,14 @@ static void fsp_timeout_poll(void *data __unused)
> >  	}
> >  }
> >  
> > +static void fsp_timeout_poll(void *data __unused)
> > +{
> > +	if (try_lock(&fsp_timeout_poller_lock)) {
> > +		__fsp_timeout_poll(data);
> > +		unlock(&fsp_timeout_poller_lock);
> > +	}
> > +}
> 
> I don't like that, we sometimes call poll in a tight loop , I'd rather
> be a little bit smarter and:
> 
> 
> 	/* The lowest granularity for a message timeout is 30 secs.
> 	 * So every 30secs, check if there is any message
> 	 * waiting for a response from the FSP
> 	 */
> 	if ((tb_compare(now, timeout_timer) == TB_AAFTERB) ||
> 		(tb_compare(now, timeout_timer) == TB_AEQUALB))
> 
>       ---> take it here
> 
> 		timeout_timer = now + secs_to_tb(30);
> 
> 
> 	else
> 		return;
> 
> (We might need to re-check we have expired before updating timeout_timer
> so we don't have two racing)

Certainly makes sense.. will work on this...

Ananth