[PATCH linux dev-4.10 v2] drivers (fsi): sbefifo: Add timeout for waiting for data

[Brad Bishop]

> On Oct 26, 2017, at 6:34 PM, Eddie James <eajames at linux.vnet.ibm.com> wrote:
> 
> From: "Edward A. James" <eajames at us.ibm.com>
> 
> During a transfer, in the event that the SBE isn't running, the driver
> will wait for data to appear in the FIFO forever. This doesn't work at
> BMC boot time if any transfers are attempted (by OCC driver probing, for
> example), as it will just hang the boot. So add a simple timeout
> mechanism when the driver reschdules waiting for data to show up in the
> FIFO.
> 
> Tested on witherspoon by rebooting the BMC successfully with chassis
> power on.
> 
> Signed-off-by: Edward A. James <eajames at us.ibm.com>
> ---
> Changes since v1:
> * Reset the wait_data_timeout field if we get data. If we get data, the SBE
>   is running and we really shouldn't timeout.
> 
> drivers/fsi/fsi-sbefifo.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index f756822..41b838e 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -54,6 +54,7 @@
> #define SBEFIFO_EOT_ACK		0x14
> 
> #define SBEFIFO_RESCHEDULE	msecs_to_jiffies(500)
> +#define SBEFIFO_MAX_RESCHDULE	msecs_to_jiffies(5000)
> 
> struct sbefifo {
> 	struct timer_list poll_timer;
> @@ -77,6 +78,7 @@ struct sbefifo_buf {
> };
> 
> struct sbefifo_xfr {
> +	unsigned long wait_data_timeout;
> 	struct sbefifo_buf *rbuf;
> 	struct sbefifo_buf *wbuf;
> 	struct list_head client;
> @@ -450,11 +452,27 @@ static void sbefifo_poll_timer(unsigned long data)
> 
> 		devn = sbefifo_dev_nwreadable(sts);
> 		if (devn == 0) {
> +			/*
> +			 * Limit the maximum waiting period for data in the
> +			 * FIFO. If the SBE isn't running, we will wait
> +			 * forever.
> +			 */
> +			if (!xfr->wait_data_timeout) {
> +				xfr->wait_data_timeout =
> +					jiffies + SBEFIFO_MAX_RESCHDULE;
> +			} else if (time_after(jiffies,
> +					      xfr->wait_data_timeout)) {
> +				ret = -ETIME;
> +				goto out;
> +			}
> +
> 			/* No data yet.  Reschedule. */
> 			sbefifo->poll_timer.expires = jiffies +
> 				SBEFIFO_RESCHEDULE;
> 			add_timer(&sbefifo->poll_timer);
> 			goto out_unlock;
> +		} else {
> +			xfr->wait_data_timeout = 0;
> 		}
> 
> 		/* Fill.  The EOT word is discarded.  */
> -- 
> 1.8.3.1

I think I chose this wait forever approach because I figured user space could
use nonblock + epoll if they didn’t want to wait forever.  Rationale being the
kernel should provide access and not dictate policy.

But userspace isn’t the problem here, its the in-kernel api being used by
other drivers.

The only other idea I have here if we don’t like this patch is to make the in-kernel
API never block (or provide a non-blocking version of it), but I vaguely remember
that already being discussed and deemed un-optimal for other reasons.

-brad