[v7,2/2] SBE: Add timer support

SBE on P9 provides one shot programmable timer facility. We can use this
to implement OPAL timers and hence limit the reliance on the Linux
heartbeat (similar to HW timer facility provided by SLW on P8).

Design:
  - We will continue to run Linux heartbeat.
  - Each chip has SBE. This patch always schedules timer on SBE on master chip.
  - Start timer option starts new timer or modifies an active timer for the
    specified timeout.
  - SBE expects timeout value in microseconds. We track timeout value in TB.
    Hence we convert tb to microseconds before sending request to SBE.
  - We are requesting ack from SBE for timer message. It gaurantees that
    SBE has scheduled timer.
  - Disabling SBE timer
    We expect SBE to send timer expiry interrupt whenever timer expires. We
    wait for 10 more ms before disabling timer.
    In future we can consider below alternative approaches:
      - Presently SBE timer disable is permanent (until we reboot system).
        SBE sends "I'm back" interrupt after reset. We can consider restarting
        timer after SBE reset.
      - Reset SBE and start timer again.
      - Each chip has SBE. On multi chip system we can try to schedule timer
        on different chip.

Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 core/interrupts.c     |   3 +-
 core/test/run-timer.c |   8 ++
 core/timer.c          |  17 ++++-
 hw/sbe-p9.c           | 205 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/sbe-p9.h      |   6 ++
 5 files changed, 235 insertions(+), 4 deletions(-)

On Mon, 2018-04-16 at 17:18 +0530, Vasant Hegde wrote:
> SBE on P9 provides one shot programmable timer facility. We can use this
> to implement OPAL timers and hence limit the reliance on the Linux
> heartbeat (similar to HW timer facility provided by SLW on P8).
> 
> Design:
>   - We will continue to run Linux heartbeat.
>   - Each chip has SBE. This patch always schedules timer on SBE on master chip.
>   - Start timer option starts new timer or modifies an active timer for the
>     specified timeout.
>   - SBE expects timeout value in microseconds. We track timeout value in TB.
>     Hence we convert tb to microseconds before sending request to SBE.
>   - We are requesting ack from SBE for timer message. It gaurantees that
>     SBE has scheduled timer.
>   - Disabling SBE timer
>     We expect SBE to send timer expiry interrupt whenever timer expires. We
>     wait for 10 more ms before disabling timer.
>     In future we can consider below alternative approaches:
>       - Presently SBE timer disable is permanent (until we reboot system).
>         SBE sends "I'm back" interrupt after reset. We can consider restarting
>         timer after SBE reset.
>       - Reset SBE and start timer again.
>       - Each chip has SBE. On multi chip system we can try to schedule timer
>         on different chip.
> 
> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  core/interrupts.c     |   3 +-
>  core/test/run-timer.c |   8 ++
>  core/timer.c          |  17 ++++-
>  hw/sbe-p9.c           | 205 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/sbe-p9.h      |   6 ++
>  5 files changed, 235 insertions(+), 4 deletions(-)
> 
> diff --git a/core/interrupts.c b/core/interrupts.c
> index 4452511f0..5d7a68cd5 100644
> --- a/core/interrupts.c
> +++ b/core/interrupts.c
> @@ -26,6 +26,7 @@
>  #include <ccan/str/str.h>
>  #include <timer.h>
>  #include <sbe-p8.h>
> +#include <sbe-p9.h>
>  
>  /* ICP registers */
>  #define ICP_XIRR		0x4	/* 32-bit access */
> @@ -489,7 +490,7 @@ static int64_t opal_handle_interrupt(uint32_t isn, __be64 *outstanding_event_mas
>  	is->ops->interrupt(is, isn);
>  
>  	/* Check timers if SBE timer isn't working */
> -	if (!p8_sbe_timer_ok())
> +	if (!p8_sbe_timer_ok() && !p9_sbe_timer_ok())
>  		check_timers(true);
>  
>  	/* Update output events */
> diff --git a/core/test/run-timer.c b/core/test/run-timer.c
> index 986af28d8..159e007ac 100644
> --- a/core/test/run-timer.c
> +++ b/core/test/run-timer.c
> @@ -4,12 +4,15 @@
>  
>  #define __TEST__
>  #include <timer.h>
> +#include <skiboot.h>
>  
>  #define mftb()	(stamp)
>  #define sync()
>  #define smt_lowest()
>  #define smt_medium()
>  
> +enum proc_gen proc_gen = proc_gen_p9;
> +
>  static uint64_t stamp, last;
>  struct lock;
>  static inline void lock_caller(struct lock *l, const char *caller)
> @@ -53,6 +56,11 @@ void p8_sbe_update_timer_expiry(uint64_t new_target)
>  	/* FIXME: do intersting SLW timer sim */
>  }
>  
> +void p9_sbe_update_timer_expiry(uint64_t new_target)
> +{
> +	(void)new_target;
> +}
> +
>  int main(void)
>  {
>  	unsigned int i;
> diff --git a/core/timer.c b/core/timer.c
> index 21f62a492..8eefb74be 100644
> --- a/core/timer.c
> +++ b/core/timer.c
> @@ -5,6 +5,7 @@
>  #include <device.h>
>  #include <opal.h>
>  #include <sbe-p8.h>
> +#include <sbe-p9.h>
>  
>  #ifdef __TEST__
>  #define this_cpu()	((void *)-1)
> @@ -109,8 +110,12 @@ static void __schedule_timer_at(struct timer *t, uint64_t when)
>   bail:
>  	/* Pick up the next timer and upddate the SBE HW timer */
>  	lt = list_top(&timer_list, struct timer, link);
> -	if (lt)
> -		p8_sbe_update_timer_expiry(lt->target);
> +	if (lt) {
> +		if (proc_gen < proc_gen_p9)
> +			p8_sbe_update_timer_expiry(lt->target);
> +		else
> +			p9_sbe_update_timer_expiry(lt->target);
> +	}
>  }
>  
>  void schedule_timer_at(struct timer *t, uint64_t when)
> @@ -167,7 +172,11 @@ static void __check_poll_timers(uint64_t now)
>  		 * arbitrarily 1us.
>  		 */
>  		if (t->running) {
> -			p8_sbe_update_timer_expiry(now + usecs_to_tb(1));
> +			if (proc_gen < proc_gen_p9)
> +				p8_sbe_update_timer_expiry(now + usecs_to_tb(1));
> +			else
> +				p9_sbe_update_timer_expiry(now + usecs_to_tb(1));
> +
>  			break;
>  		}
>  
> @@ -266,6 +275,8 @@ void late_init_timers(void)
>  	 */
>  	if (platform.heartbeat_time) {
>  		heartbeat = platform.heartbeat_time();
> +	} else if (p9_sbe_timer_ok()) {
> +		heartbeat = HEARTBEAT_DEFAULT_MS * 10;
>  	} else if (p8_sbe_timer_ok() || fsp_present()) {
>  		heartbeat = HEARTBEAT_DEFAULT_MS * 10;
>  	}
> diff --git a/hw/sbe-p9.c b/hw/sbe-p9.c
> index 9dc487ba2..a5b0706ef 100644
> --- a/hw/sbe-p9.c
> +++ b/hw/sbe-p9.c
> @@ -53,6 +53,7 @@
>  #include <sbe-p9.h>
>  #include <skiboot.h>
>  #include <timebase.h>
> +#include <timer.h>
>  #include <trace.h>
>  #include <xscom.h>
>  
> @@ -80,11 +81,36 @@ struct p9_sbe {
>  /* Default SBE chip ID */
>  static int sbe_default_chip_id = -1;
>  
> +/* Is SBE timer running? */
> +static bool sbe_has_timer = false;
> +static bool sbe_timer_in_progress = false;
> +
> +/* Inflight and next timer in TB */
> +static uint64_t sbe_last_gen_stamp;
> +static uint64_t sbe_timer_target;
> +
> +static bool has_new_target = false;
> +static bool got_timer_interrupt = false;
> +
> +/* Timer lock */
> +struct lock sbe_timer_lock;
> +
> +/*
> + * Minimum timeout value for P9 is 500 microseconds. After that
> + * SBE timer can handle granularity of 1 microsecond.
> + */
> +#define SBE_TIMER_DEFAULT_US	500
> +static uint64_t sbe_timer_def_tb;
> +
> +/* Timer control message */
> +static struct p9_sbe_msg *timer_ctrl_msg;
> +
>  #define SBE_STATUS_PRI_SHIFT	0x30
>  #define SBE_STATUS_SEC_SHIFT	0x20
>  
>  /* Forward declaration */
>  static void p9_sbe_timeout_poll_one(struct p9_sbe *sbe);
> +static void p9_sbe_timer_schedule(void);
>  
>  /* bit 0-15 : Primary status code */
>  static inline u16 p9_sbe_get_primary_rc(struct p9_sbe_msg *resp)
> @@ -562,6 +588,14 @@ static void __p9_sbe_interrupt(struct p9_sbe *sbe)
>  		p9_sbe_msg_complete(sbe, msg);
>  	}
>  
> +	/* Timer expired */
> +	if (data & SBE_HOST_TIMER_EXPIRY) {
> +		if (sbe->chip_id == sbe_default_chip_id) {
> +			sbe_timer_in_progress = false;
> +			got_timer_interrupt = true;
> +		}
> +	}
> +
>  next_msg:
>  	p9_sbe_process_queue(sbe);
>  }
> @@ -578,6 +612,15 @@ void p9_sbe_interrupt(uint32_t chip_id)
>  	sbe = chip->sbe;
>  	lock(&sbe->lock);
>  	__p9_sbe_interrupt(sbe);
> +
> +	if (got_timer_interrupt) {
> +		got_timer_interrupt = false;
> +		/* Drop lock and call timers */
> +		unlock(&sbe->lock);
> +		check_timers(true);
> +		return;
> +	}
> +
>  	unlock(&sbe->lock);
>  }
>  
> @@ -634,6 +677,55 @@ out:
>  	unlock(&sbe->lock);
>  }
>  
> +/*
> + * Check if the timer is working. If at least 10ms elapsed since
> + * last scheduled timer expiry.
> + */
> +static void p9_sbe_timer_poll(struct p9_sbe *sbe)
> +{
> +	if (!sbe_has_timer || !sbe_timer_in_progress)
> +		return;
> +
> +	lock(&sbe->lock);
> +	if (tb_compare(mftb(), sbe_last_gen_stamp + msecs_to_tb(10))
> +	    != TB_AAFTERB)
> +		goto out;

Do you need the lock for the above ?

> +	/*
> +	 * In some cases there will be a delay in calling OPAL interrupt
> +	 * handler routine (opal_handle_interrupt). In such cases its
> +	 * possible that SBE has responded, but OPAL didn't act on that.
> +	 * Hence check for SBE response before disabling timer.
> +	 */
> +	__p9_sbe_interrupt(sbe);
> +	if (got_timer_interrupt)
> +		goto call_timer;

Again, why do you call that here ?

You just called p9_sbe_timeout_poll_one() for that same SBE, which
already took the lock *and* called __p9_sbe_interrupt.

I would instead of a call-out from p9_sbe_timeout_poll_one() to check
timers.

> +	if (!sbe_timer_in_progress)
> +		goto out;
> +
> +	if (tb_compare(mftb(), sbe_last_gen_stamp + msecs_to_tb(10))
> +	    != TB_AAFTERB)
> +		goto out;
> +
> +	prlog(PR_ERR, "Timer stuck, falling back to OPAL pollers.\n");
> +	prlog(PR_ERR, "You will likely have slower I2C and may have "
> +	      "experienced increased jitter.\n");
> +	p9_sbe_reg_dump(sbe_default_chip_id);
> +	sbe_has_timer = false;
> +	sbe_timer_in_progress = false;
> +
> +out:
> +	unlock(&sbe->lock);
> +	return;
> +
> +call_timer:
> +	got_timer_interrupt = false;
> +	/* Drop lock and call timers */
> +	unlock(&sbe->lock);
> +	check_timers(true);
> +}
> +
>  static void p9_sbe_timeout_poll(void *user_data __unused)
>  {
>  	struct p9_sbe *sbe;
> @@ -644,9 +736,119 @@ static void p9_sbe_timeout_poll(void *user_data __unused)
>  			continue;
>  		sbe = chip->sbe;
>  		p9_sbe_timeout_poll_one(sbe);
> +		if (sbe->chip_id == sbe_default_chip_id)
> +			p9_sbe_timer_poll(sbe);
>  	}
>  }
>  
> +static void p9_sbe_timer_resp(struct p9_sbe_msg *msg)
> +{
> +	if (msg->state != sbe_msg_done) {
> +		prlog(PR_DEBUG, "Failed to schedule timer [chip id %x]\n",
> +		      sbe_default_chip_id);
> +
> +		if (!has_new_target)
> +			return;
> +	} else {
> +		/* Update last scheduled timer value */
> +		sbe_last_gen_stamp = mftb() +
> +			usecs_to_tb(timer_ctrl_msg->reg[1]);
> +		sbe_timer_in_progress = true;
> +	}

The common path should be that we don't have a new target,
I would suggest movign the !has_new_target down here, before
the lock. To be race-free, we need to order it with the msg state, so
you do need a sync.

What about the lockless updates to sbe_last_gen_stamp and
sbe_timer_in_progress ? Is that safe? 

> +	lock(&sbe_timer_lock);
> +	if (has_new_target) {
> +		has_new_target = false;
> +		p9_sbe_timer_schedule();
> +	}
> +	unlock(&sbe_timer_lock);
> +}
> +
> +static void p9_sbe_timer_schedule(void)
> +{
> +	int rc;
> +	u32 tick_us = SBE_TIMER_DEFAULT_US;
> +	u64 tb_cnt, now = mftb();
> +
> +	if (sbe_timer_in_progress) {
> +		/* Remaining time of inflight timer <= sbe_timer_def_tb */
> +		if ((sbe_last_gen_stamp - now) <= sbe_timer_def_tb)
> +			return;
> +	}
> +
> +	if (now < sbe_timer_target) {
> +		/* Calculate how many microseconds from now, rounded up */
> +		if ((sbe_timer_target - now) > sbe_timer_def_tb) {
> +			tb_cnt = sbe_timer_target - now + usecs_to_tb(1) - 1;
> +			tick_us = tb_to_usecs(tb_cnt);
> +		}
> +	}
> +
> +	/* Clear sequence number. p9_sbe_queue_msg will add new sequene ID */
> +	timer_ctrl_msg->reg[0] &= ~(PPC_BITMASK(32, 47));
> +	/* Update timeout value */
> +	timer_ctrl_msg->reg[1] = tick_us;
> +	rc = p9_sbe_queue_msg(sbe_default_chip_id, timer_ctrl_msg,
> +			      p9_sbe_timer_resp);
> +	if (rc != OPAL_SUCCESS) {
> +		prlog(PR_ERR, "Failed to start timer [chip id = %x]\n",
> +		      sbe_default_chip_id);
> +		return;
> +	}
> +}
> +
> +/*
> + * This is called with the timer lock held, so there is no
> + * issue with re-entrancy or concurrence
> + */
> +void p9_sbe_update_timer_expiry(uint64_t new_target)
> +{
> +	u64 now = mftb();
> +
> +	if (!sbe_has_timer || new_target == sbe_timer_target)
> +		return;
> +
> +	lock(&sbe_timer_lock);
> +	/* Timer message is in flight. Record new timer and schedule later */
> +	if (p9_sbe_msg_busy(timer_ctrl_msg) || has_new_target) {
> +		if (new_target < now)
> +			goto out;

Will the caller handle the case where we just passed the target ? Will
it re-check timers after calling us ?

> +		if (new_target > sbe_timer_target)
> +			goto out;
> +
> +		sbe_timer_target = new_target;
> +		has_new_target = true;
> +		goto out;
> +	}
> +
> +	sbe_timer_target = new_target;
> +	p9_sbe_timer_schedule();
> +
> +out:
> +	unlock(&sbe_timer_lock);
> +}
> +
> +/* Initialize SBE timer */
> +static void p9_sbe_timer_init(void)
> +{
> +	timer_ctrl_msg = p9_sbe_mkmsg(SBE_CMD_CONTROL_TIMER,
> +				      CONTROL_TIMER_START, 0, 0, 0);
> +	assert(timer_ctrl_msg);
> +
> +	init_lock(&sbe_timer_lock);
> +	sbe_has_timer = true;
> +	sbe_timer_target = mftb();
> +	sbe_last_gen_stamp = ~0ull;
> +	sbe_timer_def_tb = usecs_to_tb(SBE_TIMER_DEFAULT_US);
> +	prlog(PR_INFO, "Timer facility on chip %x\n", sbe_default_chip_id);
> +}
> +
> +bool p9_sbe_timer_ok(void)
> +{
> +	return sbe_has_timer;
> +}
> +
>  void p9_sbe_init(void)
>  {
>  	struct dt_node *xn;
> @@ -680,6 +882,9 @@ void p9_sbe_init(void)
>  		return;
>  	}
>  
> +	/* Initiate SBE timer */
> +	p9_sbe_timer_init();
> +
>  	/* Initiate SBE timeout poller */
>  	opal_add_poller(p9_sbe_timeout_poll, NULL);
>  }
> diff --git a/include/sbe-p9.h b/include/sbe-p9.h
> index 329afff80..4b839d8ba 100644
> --- a/include/sbe-p9.h
> +++ b/include/sbe-p9.h
> @@ -231,4 +231,10 @@ extern void p9_sbe_init(void);
>  /* SBE interrupt */
>  extern void p9_sbe_interrupt(uint32_t chip_id);
>  
> +/* Is SBE timer available ? */
> +extern bool p9_sbe_timer_ok(void);
> +
> +/* Update SBE timer expiry */
> +extern void p9_sbe_update_timer_expiry(uint64_t new_target);
> +
>  #endif	/* __SBE_P9_H */

On 04/17/2018 09:48 AM, Benjamin Herrenschmidt wrote:
> On Mon, 2018-04-16 at 17:18 +0530, Vasant Hegde wrote:
>> SBE on P9 provides one shot programmable timer facility. We can use this
>> to implement OPAL timers and hence limit the reliance on the Linux
>> heartbeat (similar to HW timer facility provided by SLW on P8).
>>

.../...


>>   
>> +/*
>> + * Check if the timer is working. If at least 10ms elapsed since
>> + * last scheduled timer expiry.
>> + */
>> +static void p9_sbe_timer_poll(struct p9_sbe *sbe)
>> +{
>> +	if (!sbe_has_timer || !sbe_timer_in_progress)
>> +		return;
>> +
>> +	lock(&sbe->lock);
>> +	if (tb_compare(mftb(), sbe_last_gen_stamp + msecs_to_tb(10))
>> +	    != TB_AAFTERB)
>> +		goto out;
> 
> Do you need the lock for the above ?

Not required.

> 
>> +	/*
>> +	 * In some cases there will be a delay in calling OPAL interrupt
>> +	 * handler routine (opal_handle_interrupt). In such cases its
>> +	 * possible that SBE has responded, but OPAL didn't act on that.
>> +	 * Hence check for SBE response before disabling timer.
>> +	 */
>> +	__p9_sbe_interrupt(sbe);
>> +	if (got_timer_interrupt)
>> +		goto call_timer;
> 
> Again, why do you call that here ?
> 
> You just called p9_sbe_timeout_poll_one() for that same SBE, which
> already took the lock *and* called __p9_sbe_interrupt.
> 
> I would instead of a call-out from p9_sbe_timeout_poll_one() to check
> timers.

Yeah. We can move p9_sbe_timeout_poll inside p9_sbe_timeout_poll_one(). That way 
we can avoid lock-unlock. But we still need to call __p9_sbe_interrupt() twice.. 
as timer interrupt will come after removing msg from sbe->msg_list.

> 
>> +	if (!sbe_timer_in_progress)
>> +		goto out;
>> +
>> +	if (tb_compare(mftb(), sbe_last_gen_stamp + msecs_to_tb(10))
>> +	    != TB_AAFTERB)
>> +		goto out;
>> +
>> +	prlog(PR_ERR, "Timer stuck, falling back to OPAL pollers.\n");
>> +	prlog(PR_ERR, "You will likely have slower I2C and may have "
>> +	      "experienced increased jitter.\n");
>> +	p9_sbe_reg_dump(sbe_default_chip_id);
>> +	sbe_has_timer = false;
>> +	sbe_timer_in_progress = false;
>> +
>> +out:
>> +	unlock(&sbe->lock);
>> +	return;
>> +
>> +call_timer:
>> +	got_timer_interrupt = false;
>> +	/* Drop lock and call timers */
>> +	unlock(&sbe->lock);
>> +	check_timers(true);
>> +}
>> +
>>   static void p9_sbe_timeout_poll(void *user_data __unused)
>>   {
>>   	struct p9_sbe *sbe;
>> @@ -644,9 +736,119 @@ static void p9_sbe_timeout_poll(void *user_data __unused)
>>   			continue;
>>   		sbe = chip->sbe;
>>   		p9_sbe_timeout_poll_one(sbe);
>> +		if (sbe->chip_id == sbe_default_chip_id)
>> +			p9_sbe_timer_poll(sbe);
>>   	}
>>   }
>>   
>> +static void p9_sbe_timer_resp(struct p9_sbe_msg *msg)
>> +{
>> +	if (msg->state != sbe_msg_done) {
>> +		prlog(PR_DEBUG, "Failed to schedule timer [chip id %x]\n",
>> +		      sbe_default_chip_id);
>> +
>> +		if (!has_new_target)
>> +			return;
>> +	} else {
>> +		/* Update last scheduled timer value */
>> +		sbe_last_gen_stamp = mftb() +
>> +			usecs_to_tb(timer_ctrl_msg->reg[1]);
>> +		sbe_timer_in_progress = true;
>> +	}
> 
> The common path should be that we don't have a new target,
> I would suggest movign the !has_new_target down here, before
> the lock. To be race-free, we need to order it with the msg state, so
> you do need a sync.

Oh yes. I should have moved has_new_target to down.

> 
> What about the lockless updates to sbe_last_gen_stamp and
> sbe_timer_in_progress ? Is that safe?

I think its safe.

> 
>> +	lock(&sbe_timer_lock);
>> +	if (has_new_target) {
>> +		has_new_target = false;
>> +		p9_sbe_timer_schedule();
>> +	}
>> +	unlock(&sbe_timer_lock);
>> +}
>> +
>> +static void p9_sbe_timer_schedule(void)
>> +{
>> +	int rc;
>> +	u32 tick_us = SBE_TIMER_DEFAULT_US;
>> +	u64 tb_cnt, now = mftb();
>> +
>> +	if (sbe_timer_in_progress) {
>> +		/* Remaining time of inflight timer <= sbe_timer_def_tb */
>> +		if ((sbe_last_gen_stamp - now) <= sbe_timer_def_tb)
>> +			return;
>> +	}
>> +
>> +	if (now < sbe_timer_target) {
>> +		/* Calculate how many microseconds from now, rounded up */
>> +		if ((sbe_timer_target - now) > sbe_timer_def_tb) {
>> +			tb_cnt = sbe_timer_target - now + usecs_to_tb(1) - 1;
>> +			tick_us = tb_to_usecs(tb_cnt);
>> +		}
>> +	}
>> +
>> +	/* Clear sequence number. p9_sbe_queue_msg will add new sequene ID */
>> +	timer_ctrl_msg->reg[0] &= ~(PPC_BITMASK(32, 47));
>> +	/* Update timeout value */
>> +	timer_ctrl_msg->reg[1] = tick_us;
>> +	rc = p9_sbe_queue_msg(sbe_default_chip_id, timer_ctrl_msg,
>> +			      p9_sbe_timer_resp);
>> +	if (rc != OPAL_SUCCESS) {
>> +		prlog(PR_ERR, "Failed to start timer [chip id = %x]\n",
>> +		      sbe_default_chip_id);
>> +		return;
>> +	}
>> +}
>> +
>> +/*
>> + * This is called with the timer lock held, so there is no
>> + * issue with re-entrancy or concurrence
>> + */
>> +void p9_sbe_update_timer_expiry(uint64_t new_target)
>> +{
>> +	u64 now = mftb();
>> +
>> +	if (!sbe_has_timer || new_target == sbe_timer_target)
>> +		return;
>> +
>> +	lock(&sbe_timer_lock);
>> +	/* Timer message is in flight. Record new timer and schedule later */
>> +	if (p9_sbe_msg_busy(timer_ctrl_msg) || has_new_target) {
>> +		if (new_target < now)
>> +			goto out;
> 
> Will the caller handle the case where we just passed the target ? Will
> it re-check timers after calling us ?

Sorry. I'm not sure I understood your question.

So we enter this block only when new_target is < inflight timer. So we are good 
in source.
Now on callback path if SBE triggers early we have problem (..as check_timer 
doesn't re-arm).

May be fix check_timer to re-arm timer? -OR- do you want me to take care in 
driver itself?
We can take care here by doing something like below in p9_sbe_interrupt () function.
         if (got_timer_interrupt) {
                 got_timer_interrupt = false;
                 /* Drop lock and call timers */
                 unlock(&sbe->lock);
                 check_timers(true);

		/* Early interrupt , re-arm timer */
		if (sbe_last_gen_timestamp > mftb())
			p9_sbe_update_timer_expiry(sbe_last_gen_timestamp);

	}


-Vasant

On Tue, 2018-04-17 at 12:31 +0530, Vasant Hegde wrote:
> 
> > Again, why do you call that here ?
> > 
> > You just called p9_sbe_timeout_poll_one() for that same SBE, which
> > already took the lock *and* called __p9_sbe_interrupt.
> > 
> > I would instead of a call-out from p9_sbe_timeout_poll_one() to check
> > timers.
> 
> Yeah. We can move p9_sbe_timeout_poll inside p9_sbe_timeout_poll_one(). That way 
> we can avoid lock-unlock. But we still need to call __p9_sbe_interrupt() twice.. 
> as timer interrupt will come after removing msg from sbe->msg_list.

I'm not sure I understand why we need to call it twice, worst cast, if
the timer was really very very short, we'll just hit it on the next
call to poll no ? Esp. timeouts, we don't need to be paricularly
precise.

   .../...

+ * This is called with the timer lock held, so there is no
> > > + * issue with re-entrancy or concurrence
> > > + */
> > > +void p9_sbe_update_timer_expiry(uint64_t new_target)
> > > +{
> > > +	u64 now = mftb();
> > > +
> > > +	if (!sbe_has_timer || new_target == sbe_timer_target)
> > > +		return;
> > > +
> > > +	lock(&sbe_timer_lock);
> > > +	/* Timer message is in flight. Record new timer and schedule later */
> > > +	if (p9_sbe_msg_busy(timer_ctrl_msg) || has_new_target) {
> > > +		if (new_target < now)
> > > +			goto out;
> > 
> > Will the caller handle the case where we just passed the target ? Will
> > it re-check timers after calling us ?
> 
> Sorry. I'm not sure I understood your question.
> 
> So we enter this block only when new_target is < inflight timer. So we are good 
> in source.

How so ? We re-snapshot "now", meaning we could have moved beyond
"new_target" already. If that happens, we will hit your "goto out"
above and not program any timer.

Unless the caller checks, which it doesn't ...

> Now on callback path if SBE triggers early we have problem (..as check_timer 
> doesn't re-arm).
> 
> May be fix check_timer to re-arm timer? -OR- do you want me to take care in 
> driver itself?

We should be smarter inside __check_timers(). We should probably pass
down the "from_interrupt" argument from check_timers() and handle that
case, along with the t->running case which currently will cause us to
delay until the next timer interrupt, which isn't a great idea, so we
should probably reconfigure a short delay or something...

> We can take care here by doing something like below in p9_sbe_interrupt () function.
>          if (got_timer_interrupt) {
>                  got_timer_interrupt = false;
>                  /* Drop lock and call timers */
>                  unlock(&sbe->lock);
>                  check_timers(true);
> 
> 		/* Early interrupt , re-arm timer */
> 		if (sbe_last_gen_timestamp > mftb())
> 			p9_sbe_update_timer_expiry(sbe_last_gen_timestamp);
> 
> 	}
> 
> 
> -Vasant
>