[RESEND,v6,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 restarts 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 1 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           | 159 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/sbe-p9.h      |   6 ++
 5 files changed, 189 insertions(+), 4 deletions(-)

On Tue, 2018-04-03 at 17:27 +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 restarts 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 1 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           | 159 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/sbe-p9.h      |   6 ++
>  5 files changed, 189 insertions(+), 4 deletions(-)
> 
> diff --git a/core/interrupts.c b/core/interrupts.c
> index b7f597c84..b82a9afd0 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 */
> @@ -470,7 +471,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 059bbf757..0da9516f8 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>
>  
> @@ -86,6 +87,24 @@ 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;
> +
> +/*
> + * Minimum timeout value for P9 is 100 microseconds. After that
> + * SBE timer can handle granularity of 1 microsecond.
> + */
> +#define SBE_TIMER_DEFAULT_US	100
> +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
>  #define SBE_FFDC_PRESENT	PPC_BIT(1)
> @@ -469,8 +488,18 @@ static void p9_sbe_handle_response(struct p9_sbe *sbe, struct p9_sbe_msg *msg)
>  	}
>  }
>  
> +/* Check whether we got ACK from SBE for timer message */
> +static inline bool p9_sbe_timer_got_ack(void)
> +{
> +	if (p9_sbe_msg_busy(timer_ctrl_msg))
> +		return false;
> +
> +	return true;
> +}
> +
>  void p9_sbe_interrupt(uint32_t chip_id)
>  {
> +	bool timer_interrupt = false;
>  	int rc;
>  	u64 data = 0, val;
>  	struct proc_chip *chip;
> @@ -504,6 +533,16 @@ void p9_sbe_interrupt(uint32_t chip_id)
>  		prd_sbe_passthrough(chip_id);
>  	}
>  
> +	/* Timer expired */
> +	if (data & SBE_HOST_TIMER_EXPIRY) {
> +		if (!p9_sbe_timer_got_ack()) {
> +			data &= ~(SBE_HOST_TIMER_EXPIRY);
> +		} else {
> +			sbe_timer_in_progress = false;
> +			timer_interrupt = true;
> +		}
> +	}

So not sure I understand the above. It's the same bit in the doorbell
that is the "ack" and the timer expiry ? That means you basically need
"2" interrupts, one for the ack, one for the expiry, when you set the
timer ?

That's racy no ? IE what if you poll a bit slowly bcs you're busy
elsewhere and get both ack and timer interrupt ? They coalesce into a
single event and your timer is gone, no ?

>  	/* SBE came back from reset */
>  	if (data & SBE_HOST_RESET) {
>  		prlog(PR_NOTICE, "Back from reset [chip id = %x]\n", chip_id);
> @@ -547,6 +586,9 @@ clr_interrupt:
>  
>  	p9_sbe_process_queue(sbe);
>  	unlock(&sbe->lock);
> +	/* Drop lock and call timers */
> +	if (timer_interrupt)
> +		check_timers(true);

If for some reason the timer fired early, are you confident it will be
re-armed ?

>  }
>  
>  static void p9_sbe_timeout_poll(void *user_data __unused)
> @@ -598,6 +640,120 @@ static void p9_sbe_timeout_poll(void *user_data __unused)
>  		p9_sbe_process_queue(sbe);
>  		unlock(&sbe->lock);
>  	}
> +
> +	/*
> +	 * Check if the timer is working. If at least 1ms elapsed since
> +	 * last scheduled timer expiry.
> +	 */
> +	if (sbe_has_timer && sbe_timer_in_progress) {
> +		chip = get_chip(sbe_default_chip_id);
> +		sbe = chip->sbe;
> +		if (tb_compare(mftb(), sbe_last_gen_stamp + msecs_to_tb(1))
> +		    != TB_AAFTERB)
> +			return;

So I would have put that inside the above lock section, basically, if
the chip is the default chip, add the timeout logic (and move it to a
separate function).

> +		/*
> +		 * 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_default_chip_id);


Otherwise you call the above twice in the same function and the
function becomes a mess.

Also you keep taking & releasing the lock, it's inefficient. Follow my
advice in the other patch, have the lock taken once in a per-chip thing
and everything else underneath is done with that one lock instance. 

> +
> +		lock(&sbe->lock);
> +		if (!sbe_timer_in_progress) {
> +			unlock(&sbe->lock);
> +			return;
> +		}
> +
> +		if (tb_compare(mftb(), sbe_last_gen_stamp + msecs_to_tb(1))
> +		    != TB_AAFTERB) {
> +			unlock(&sbe->lock);
> +			return;
> +		}
> +
> +		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;
> +		unlock(&sbe->lock);
> +	}
> +}
> +
> +/*
> + * 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)
> +{
> +	int rc;
> +	u32 tick_us = SBE_TIMER_DEFAULT_US;
> +	u64 tb_cnt, now = mftb();
> +
> +	if (!sbe_has_timer || new_target == sbe_timer_target)
> +		return;
> +
> +	sbe_timer_target = new_target;
> +
> +	/* Modify timer */
> +	if (sbe_timer_in_progress) {
> +		if (sbe_timer_target < now)
> +			return;
> +
> +		/* remaining time <= sbe_timer_def_tb */
> +		if ((sbe_last_gen_stamp - now) <= sbe_timer_def_tb)
> +			return;
> +
> +		if (sbe_timer_target > sbe_last_gen_stamp)
> +			return;
> +
> +		sbe_timer_in_progress = false;

So aren't you potentially re-using or modifying a message that is in
the middle of being sent ?

IE a previous call to p9_sbe_update_timer_expiry has queued the
message, it's now in the process of being sent, while this gets called,
cleares sbe_timer_in_progress, ad move on with hacking the "active"
message ... that isn't right.

You need to flag that a new target is needed and prep a new message
from the completion of the previous one.

> +	}
> +
> +	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 */
> +	timer_ctrl_msg->reg[0] &= ~(PPC_BITMASK(32, 47));

Why ?

> +	/* Update timeout value */
> +	timer_ctrl_msg->reg[1] = tick_us;
> +
> +	rc = p9_sbe_sync_msg(sbe_default_chip_id, timer_ctrl_msg, false);
> +	if (rc != OPAL_SUCCESS) {
> +		prlog(PR_ERR, "Failed to start timer [chip id = %x]\n",
> +		      sbe_default_chip_id);
> +		return;
> +	}
> +
> +	/* Update last scheduled timer value */
> +	sbe_last_gen_stamp = mftb() + usecs_to_tb(tick_us);
> +	sbe_timer_in_progress = true;
> +}
> +
> +/* 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);
> +
> +	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)
> @@ -635,6 +791,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 892be4b85..c68894f11 100644
> --- a/include/sbe-p9.h
> +++ b/include/sbe-p9.h
> @@ -230,4 +230,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/13/2018 03:05 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2018-04-03 at 17:27 +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).

.../...

>>   
>> +	/* Timer expired */
>> +	if (data & SBE_HOST_TIMER_EXPIRY) {
>> +		if (!p9_sbe_timer_got_ack()) {
>> +			data &= ~(SBE_HOST_TIMER_EXPIRY);
>> +		} else {
>> +			sbe_timer_in_progress = false;
>> +			timer_interrupt = true;
>> +		}
>> +	}
> 
> So not sure I understand the above. It's the same bit in the doorbell
> that is the "ack" and the timer expiry ? That means you basically need
> "2" interrupts, one for the ack, one for the expiry, when you set the
> timer ?

Its same doorbell register, but different bits. In some cases there will be 
delay in processing interrupt and we endup getting both bits set simultaneously. 
  So I added get_ack() to make sure we process ack before processing timer 
interrupt. In such cases we endup having two interrupt to process timer.

I'm changing from sync_msg() to async_msg(). With that we don't need about stuff 
anyway.

> 
> That's racy no ? IE what if you poll a bit slowly bcs you're busy
> elsewhere and get both ack and timer interrupt ? They coalesce into a
> single event and your timer is gone, no ?
> 
>>   	/* SBE came back from reset */
>>   	if (data & SBE_HOST_RESET) {
>>   		prlog(PR_NOTICE, "Back from reset [chip id = %x]\n", chip_id);
>> @@ -547,6 +586,9 @@ clr_interrupt:
>>   
>>   	p9_sbe_process_queue(sbe);
>>   	unlock(&sbe->lock);
>> +	/* Drop lock and call timers */
>> +	if (timer_interrupt)
>> +		check_timers(true);
> 
> If for some reason the timer fired early, are you confident it will be
> re-armed ?

Our understanding is SBE will never get back to us early. They will always come 
after timeout.
So we are good here.

> 
>>   }
>>   
>>   static void p9_sbe_timeout_poll(void *user_data __unused)
>> @@ -598,6 +640,120 @@ static void p9_sbe_timeout_poll(void *user_data __unused)
>>   		p9_sbe_process_queue(sbe);
>>   		unlock(&sbe->lock);
>>   	}
>> +
>> +	/*
>> +	 * Check if the timer is working. If at least 1ms elapsed since
>> +	 * last scheduled timer expiry.
>> +	 */
>> +	if (sbe_has_timer && sbe_timer_in_progress) {
>> +		chip = get_chip(sbe_default_chip_id);
>> +		sbe = chip->sbe;
>> +		if (tb_compare(mftb(), sbe_last_gen_stamp + msecs_to_tb(1))
>> +		    != TB_AAFTERB)
>> +			return;
> 
> So I would have put that inside the above lock section, basically, if
> the chip is the default chip, add the timeout logic (and move it to a
> separate function).

Yeah. I will have separate p9_sbe_timer_poll() to timer timeout checking. Also I 
will move above block  inside lock.

> 
>> +		/*
>> +		 * 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_default_chip_id);
> 
> 
> Otherwise you call the above twice in the same function and the
> function becomes a mess.
> 
> Also you keep taking & releasing the lock, it's inefficient. Follow my
> advice in the other patch, have the lock taken once in a per-chip thing
> and everything else underneath is done with that one lock instance.

Agree. Moved to separate function.


> 
>> +
>> +		lock(&sbe->lock);
>> +		if (!sbe_timer_in_progress) {
>> +			unlock(&sbe->lock);
>> +			return;
>> +		}
>> +
>> +		if (tb_compare(mftb(), sbe_last_gen_stamp + msecs_to_tb(1))
>> +		    != TB_AAFTERB) {
>> +			unlock(&sbe->lock);
>> +			return;
>> +		}
>> +
>> +		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;
>> +		unlock(&sbe->lock);
>> +	}
>> +}
>> +
>> +/*
>> + * 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)
>> +{
>> +	int rc;
>> +	u32 tick_us = SBE_TIMER_DEFAULT_US;
>> +	u64 tb_cnt, now = mftb();
>> +
>> +	if (!sbe_has_timer || new_target == sbe_timer_target)
>> +		return;
>> +
>> +	sbe_timer_target = new_target;
>> +
>> +	/* Modify timer */
>> +	if (sbe_timer_in_progress) {
>> +		if (sbe_timer_target < now)
>> +			return;
>> +
>> +		/* remaining time <= sbe_timer_def_tb */
>> +		if ((sbe_last_gen_stamp - now) <= sbe_timer_def_tb)
>> +			return;
>> +
>> +		if (sbe_timer_target > sbe_last_gen_stamp)
>> +			return;
>> +
>> +		sbe_timer_in_progress = false;
> 
> So aren't you potentially re-using or modifying a message that is in
> the middle of being sent ?
> 
> IE a previous call to p9_sbe_update_timer_expiry has queued the
> message, it's now in the process of being sent, while this gets called,
> cleares sbe_timer_in_progress, ad move on with hacking the "active"
> message ... that isn't right.
> 
> You need to flag that a new target is needed and prep a new message
> from the completion of the previous one.

Done.

I'm moving from sync msg to async message. Also I've "has_new_target" note down 
new timer value while we are processing current timer.

Thanks
-Vasant

On Mon, 2018-04-16 at 15:26 +0530, Vasant Hegde wrote:
> 
> > >   	/* SBE came back from reset */
> > >   	if (data & SBE_HOST_RESET) {
> > >   		prlog(PR_NOTICE, "Back from reset [chip id = %x]\n", chip_id);
> > > @@ -547,6 +586,9 @@ clr_interrupt:
> > >   
> > >   	p9_sbe_process_queue(sbe);
> > >   	unlock(&sbe->lock);
> > > +	/* Drop lock and call timers */
> > > +	if (timer_interrupt)
> > > +		check_timers(true);
> > 
> > If for some reason the timer fired early, are you confident it will be
> > re-armed ?
> 
> Our understanding is SBE will never get back to us early. They will always come 
> after timeout.
> So we are good here.

No, you need to ensure you are robust. It could be something as simple
as the clock drifting between the SBE and the CPU for example.

> > 
> > >   }
> > >   
> > >   static void p9_sbe_timeout_poll(void *user_data __unused)
> > > @@ -598,6 +640,120 @@ static void p9_sbe_timeout_poll(void *user_data __unused)
> > >   		p9_sbe_process_queue(sbe);
> > >   		unlock(&sbe->lock);
> > >   	}
> > > +
> > > +	/*
> > > +	 * Check if the timer is working. If at least 1ms elapsed since
> > > +	 * last scheduled timer expiry.
> > > +	 */
> > > +	if (sbe_has_timer && sbe_timer_in_progress) {
> > > +		chip = get_chip(sbe_default_chip_id);
> > > +		sbe = chip->sbe;
> > > +		if (tb_compare(mftb(), sbe_last_gen_stamp + msecs_to_tb(1))
> > > +		    != TB_AAFTERB)
> > > +			return;
> > 
> > So I would have put that inside the above lock section, basically, if
> > the chip is the default chip, add the timeout logic (and move it to a
> > separate function).
> 
> Yeah. I will have separate p9_sbe_timer_poll() to timer timeout checking. Also I 
> will move above block  inside lock.
> 
> > 
> > > +		/*
> > > +		 * 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_default_chip_id);
> > 
> > 
> > Otherwise you call the above twice in the same function and the
> > function becomes a mess.
> > 
> > Also you keep taking & releasing the lock, it's inefficient. Follow my
> > advice in the other patch, have the lock taken once in a per-chip thing
> > and everything else underneath is done with that one lock instance.
> 
> Agree. Moved to separate function.
> 
> 
> > 
> > > +
> > > +		lock(&sbe->lock);
> > > +		if (!sbe_timer_in_progress) {
> > > +			unlock(&sbe->lock);
> > > +			return;
> > > +		}
> > > +
> > > +		if (tb_compare(mftb(), sbe_last_gen_stamp + msecs_to_tb(1))
> > > +		    != TB_AAFTERB) {
> > > +			unlock(&sbe->lock);
> > > +			return;
> > > +		}
> > > +
> > > +		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;
> > > +		unlock(&sbe->lock);
> > > +	}
> > > +}
> > > +
> > > +/*
> > > + * 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)
> > > +{
> > > +	int rc;
> > > +	u32 tick_us = SBE_TIMER_DEFAULT_US;
> > > +	u64 tb_cnt, now = mftb();
> > > +
> > > +	if (!sbe_has_timer || new_target == sbe_timer_target)
> > > +		return;
> > > +
> > > +	sbe_timer_target = new_target;
> > > +
> > > +	/* Modify timer */
> > > +	if (sbe_timer_in_progress) {
> > > +		if (sbe_timer_target < now)
> > > +			return;
> > > +
> > > +		/* remaining time <= sbe_timer_def_tb */
> > > +		if ((sbe_last_gen_stamp - now) <= sbe_timer_def_tb)
> > > +			return;
> > > +
> > > +		if (sbe_timer_target > sbe_last_gen_stamp)
> > > +			return;
> > > +
> > > +		sbe_timer_in_progress = false;
> > 
> > So aren't you potentially re-using or modifying a message that is in
> > the middle of being sent ?
> > 
> > IE a previous call to p9_sbe_update_timer_expiry has queued the
> > message, it's now in the process of being sent, while this gets called,
> > cleares sbe_timer_in_progress, ad move on with hacking the "active"
> > message ... that isn't right.
> > 
> > You need to flag that a new target is needed and prep a new message
> > from the completion of the previous one.
> 
> Done.
> 
> I'm moving from sync msg to async message. Also I've "has_new_target" note down 
> new timer value while we are processing current timer.
> 
> Thanks
> -Vasant

On 04/17/2018 09:10 AM, Benjamin Herrenschmidt wrote:
> On Mon, 2018-04-16 at 15:26 +0530, Vasant Hegde wrote:
>>
>>>>    	/* SBE came back from reset */
>>>>    	if (data & SBE_HOST_RESET) {
>>>>    		prlog(PR_NOTICE, "Back from reset [chip id = %x]\n", chip_id);
>>>> @@ -547,6 +586,9 @@ clr_interrupt:
>>>>    
>>>>    	p9_sbe_process_queue(sbe);
>>>>    	unlock(&sbe->lock);
>>>> +	/* Drop lock and call timers */
>>>> +	if (timer_interrupt)
>>>> +		check_timers(true);
>>>
>>> If for some reason the timer fired early, are you confident it will be
>>> re-armed ?
>>
>> Our understanding is SBE will never get back to us early. They will always come
>> after timeout.
>> So we are good here.
> 
> No, you need to ensure you are robust. It could be something as simple
> as the clock drifting between the SBE and the CPU for example.

Hmmm. Yeah. That's possible. We have two option here.
Fix check_timers code.. something like below

--- a/core/timer.c
+++ b/core/timer.c
@@ -203,8 +203,13 @@ static void __check_timers(uint64_t now)
                 t = list_top(&timer_list, struct timer, link);

                 /* Top of list not expired ? that's it ... */
-               if (!t || t->target > now)
+               if (!t || t->target > now) {
+                       if (proc_gen < proc_gen_p9)
+                               p8_sbe_update_timer_expiry(t->target);
+                       else
+                               p9_sbe_update_timer_expiry(t->target);
                         break;
+               }


.. we are already holding lock.. so should be fine. Of course I haven't tested 
this one. If you are fine  with this approach I will test and send patch.

-OR-

Take care if SBE driver itself. Fairly straight forward. In sbe_interrupt 
processing path, we just need to compare sbe_last_gen_timestamp with mftb() and 
re-arm it.

Thanks
-Vasant