[PATCH v2 0/2] Enable read timeout specification for sbefifo read

[PATCH v2 0/2] Enable read timeout specification for sbefifo read

[Amitay Isaacs]

Certain SBE operations (e.g. collecting trace information from processors)
can take long time (> 10 seconds) to finish before SBE can respond.
Such operations will currentlly timeout due to the default response
timeout of 10 seconds.  This patchset allows users to set a longer timeout
using ioctl on the sbefifo device fd, before issuing SBE operations that
are likely to take longer.

v2 changes:
  - uint32 --> u32
  - Avoid resetting the read timeout after single operation.  Use ioctl
    to set the read timeout per open fd for sbefifo device.
  - Specifying 0 value to ioctl will reset the timeout to default value
  - Document ioctl behaviour in uapi/linux/fsi.h


Amitay Isaacs (2):
  fsi: sbefifo: Use specified value of start of response timeout
  fsi: sbefifo: implement FSI_SBEFIFO_READ_TIMEOUT ioctl

 drivers/fsi/fsi-sbefifo.c | 48 ++++++++++++++++++++++++++++++++++++++-
 include/uapi/linux/fsi.h  | 14 ++++++++++++
 2 files changed, 61 insertions(+), 1 deletion(-)

-- 
2.33.1

[PATCH v2 1/2] fsi: sbefifo: Use specified value of start of response timeout

[Amitay Isaacs]

For some of the chip-ops where sbe needs to collect trace information,
sbe can take a long time (>30s) to respond.  Currently these chip-ops
will timeout as the start of response timeout defaults to 10s.

Instead of default value, use specified value.  The require timeout
value will be set using ioctl.

Signed-off-by: Amitay Isaacs <amitay@ozlabs.org>
---
 drivers/fsi/fsi-sbefifo.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 52328adef643..1e9b326e8f67 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -125,6 +125,7 @@ struct sbefifo {
 	bool			dead;
 	bool			async_ffdc;
 	bool			timed_out;
+	u32			timeout_start_rsp_ms;
 };
 
 struct sbefifo_user {
@@ -549,7 +550,7 @@ static int sbefifo_read_response(struct sbefifo *sbefifo, struct iov_iter *respo
 
 	dev_vdbg(dev, "reading response, buflen = %zd\n", iov_iter_count(response));
 
-	timeout = msecs_to_jiffies(SBEFIFO_TIMEOUT_START_RSP);
+	timeout = msecs_to_jiffies(sbefifo->timeout_start_rsp_ms);
 	for (;;) {
 		/* Grab FIFO status (this will handle parity errors) */
 		rc = sbefifo_wait(sbefifo, false, &status, timeout);
@@ -972,6 +973,7 @@ static int sbefifo_probe(struct device *dev)
 	sbefifo->fsi_dev = fsi_dev;
 	dev_set_drvdata(dev, sbefifo);
 	mutex_init(&sbefifo->lock);
+	sbefifo->timeout_start_rsp_ms = SBEFIFO_TIMEOUT_START_RSP;
 
 	/*
 	 * Try cleaning up the FIFO. If this fails, we still register the
-- 
2.33.1

[PATCH v2 2/2] fsi: sbefifo: implement FSI_SBEFIFO_READ_TIMEOUT ioctl

[Amitay Isaacs]

FSI_SBEFIFO_READ_TIMEOUT ioctl sets the read timeout (in seconds) for
the response received by sbefifo device from sbe.  The timeout affects
only the read operation on sbefifo device fd.

Certain SBE operations can take long time to complete and the default
timeout of 10 seconds might not be sufficient to start receiving
response from SBE.  In such cases, allow the timeout to be set to the
maximum of 120 seconds.

Signed-off-by: Amitay Isaacs <amitay@ozlabs.org>
---
 drivers/fsi/fsi-sbefifo.c | 44 +++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/fsi.h  | 14 +++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 1e9b326e8f67..771b1a44a61c 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -32,6 +32,8 @@
 #include <linux/vmalloc.h>
 #include <linux/mm.h>
 
+#include <uapi/linux/fsi.h>
+
 /*
  * The SBEFIFO is a pipe-like FSI device for communicating with
  * the self boot engine on POWER processors.
@@ -134,6 +136,7 @@ struct sbefifo_user {
 	void			*cmd_page;
 	void			*pending_cmd;
 	size_t			pending_len;
+	u32			read_timeout_ms;
 };
 
 static DEFINE_MUTEX(sbefifo_ffdc_mutex);
@@ -796,6 +799,7 @@ static int sbefifo_user_open(struct inode *inode, struct file *file)
 		return -ENOMEM;
 	}
 	mutex_init(&user->file_lock);
+	user->read_timeout_ms = SBEFIFO_TIMEOUT_START_RSP;
 
 	return 0;
 }
@@ -838,7 +842,9 @@ static ssize_t sbefifo_user_read(struct file *file, char __user *buf,
 	rc = mutex_lock_interruptible(&sbefifo->lock);
 	if (rc)
 		goto bail;
+	sbefifo->timeout_start_rsp_ms = user->read_timeout_ms;
 	rc = __sbefifo_submit(sbefifo, user->pending_cmd, cmd_len, &resp_iter);
+	sbefifo->timeout_start_rsp_ms = SBEFIFO_TIMEOUT_START_RSP;
 	mutex_unlock(&sbefifo->lock);
 	if (rc < 0)
 		goto bail;
@@ -928,12 +934,50 @@ static int sbefifo_user_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static int sbefifo_read_timeout(struct sbefifo_user *user, void __user *argp)
+{
+	u32 timeout;
+
+	if (get_user(timeout, (__u32 __user *)argp))
+		return -EFAULT;
+
+	if (timeout == 0) {
+		user->read_timeout_ms = SBEFIFO_TIMEOUT_START_RSP;
+		return 0;
+	}
+
+	if (timeout < 10 || timeout > 120)
+		return -EINVAL;
+
+	user->read_timeout_ms = timeout * 1000; /* user timeout is in sec */
+	return 0;
+}
+
+static long sbefifo_user_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct sbefifo_user *user = file->private_data;
+	int rc = -ENOTTY;
+
+	if (!user)
+		return -EINVAL;
+
+	mutex_lock(&user->file_lock);
+	switch (cmd) {
+	case FSI_SBEFIFO_READ_TIMEOUT:
+		rc = sbefifo_read_timeout(user, (void __user *)arg);
+		break;
+	}
+	mutex_unlock(&user->file_lock);
+	return rc;
+}
+
 static const struct file_operations sbefifo_fops = {
 	.owner		= THIS_MODULE,
 	.open		= sbefifo_user_open,
 	.read		= sbefifo_user_read,
 	.write		= sbefifo_user_write,
 	.release	= sbefifo_user_release,
+	.unlocked_ioctl = sbefifo_user_ioctl,
 };
 
 static void sbefifo_free(struct device *dev)
diff --git a/include/uapi/linux/fsi.h b/include/uapi/linux/fsi.h
index da577ecd90e7..a4c886832df5 100644
--- a/include/uapi/linux/fsi.h
+++ b/include/uapi/linux/fsi.h
@@ -55,4 +55,18 @@ struct scom_access {
 #define FSI_SCOM_WRITE	_IOWR('s', 0x02, struct scom_access)
 #define FSI_SCOM_RESET	_IOW('s', 0x03, __u32)
 
+/*
+ * /dev/sbefifo* ioctl interface
+ */
+
+/**
+ * FSI_SBEFIFO_READ_TIMEOUT sets the read timeout for response from SBE.
+ *
+ * The read timeout is specified in seconds.  The minimum value of read
+ * timeout is 10 seconds (default) and the maximum value of read timeout is
+ * 120 seconds.  A read timeout of 0 will reset the value to the default of
+ * (10 seconds).
+ */
+#define FSI_SBEFIFO_READ_TIMEOUT	_IOW('s', 0x00, __u32)
+
 #endif /* _UAPI_LINUX_FSI_H */
-- 
2.33.1

Re: [PATCH v2 2/2] fsi: sbefifo: implement FSI_SBEFIFO_READ_TIMEOUT ioctl

[Greg KH]

On Thu, Dec 16, 2021 at 05:24:05PM +1100, Amitay Isaacs wrote:
> FSI_SBEFIFO_READ_TIMEOUT ioctl sets the read timeout (in seconds) for
> the response received by sbefifo device from sbe.  The timeout affects
> only the read operation on sbefifo device fd.
> 
> Certain SBE operations can take long time to complete and the default
> timeout of 10 seconds might not be sufficient to start receiving
> response from SBE.  In such cases, allow the timeout to be set to the
> maximum of 120 seconds.
> 
> Signed-off-by: Amitay Isaacs <amitay@ozlabs.org>
> ---
>  drivers/fsi/fsi-sbefifo.c | 44 +++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/fsi.h  | 14 +++++++++++++
>  2 files changed, 58 insertions(+)

Where is the userspace tool that uses this new ioctl?

And again, why does it have to be an ioctl?  Where is this now
documented?  What are the units of the value?

greg k-h

Re: [PATCH v2 2/2] fsi: sbefifo: implement FSI_SBEFIFO_READ_TIMEOUT ioctl

[Joel Stanley]

Hi Greg,

On Thu, 16 Dec 2021 at 08:04, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Dec 16, 2021 at 05:24:05PM +1100, Amitay Isaacs wrote:
> > FSI_SBEFIFO_READ_TIMEOUT ioctl sets the read timeout (in seconds) for
> > the response received by sbefifo device from sbe.  The timeout affects
> > only the read operation on sbefifo device fd.
> >
> > Certain SBE operations can take long time to complete and the default
> > timeout of 10 seconds might not be sufficient to start receiving
> > response from SBE.  In such cases, allow the timeout to be set to the
> > maximum of 120 seconds.
> >
> > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org>
> > ---
> >  drivers/fsi/fsi-sbefifo.c | 44 +++++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/fsi.h  | 14 +++++++++++++
> >  2 files changed, 58 insertions(+)

I'm taking over this patch series on behalf of Amitay.

> Where is the userspace tool that uses this new ioctl?

The userspace is a library called 'libpdbg'. Amitay has some patches
prepared that use this new ioctl here:

 https://github.com/amitay/pdbg/commits/ioctl

> And again, why does it have to be an ioctl?  Where is this now
> documented?  What are the units of the value?

We need a way for userspace to tell the driver that subsequent
operations (writes to the chardev, that cause the driver to send data
to a microcontroller over a fsi link) are expected to take a longer
time, so the kernel should wait longer before declaring the operation
timed out.

The kernel driver doesn't know about the specific operation, and we
don't want to have that policy in the kernel, so userspace will set
the timeout appropriately.

I'll send a new version, as Amitay wants to remove the one-shot nature
of the configuration, and instead have it persist. I'll update the
patch to make it clear what the units are.

Where do we usually stick documentation for ioctls such as this one?

Cheers,

Joel