| Message ID | 20211215005833.222841-2-amitay@ozlabs.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] fsi: sbefifo: Use specified value of start of response timeout | expand |
On Wed, 15 Dec 2021 at 00:58, Amitay Isaacs <amitay@ozlabs.org> wrote: > > FSI_SBEFIFO_READ_TIMEOUT ioctl sets the read timeout (in seconds) for > the response to *the next* chip-op sent to sbe. The timeout value is > reset to default after the chip-op. The timeout affects only the read() > operation on sbefifo device fd. Makes sense to me. Lets see if Greg or Andrew will review the icotl interface for us. Reviewed-by: Joel Stanley <joel@jms.id.au> > > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> > --- > drivers/fsi/fsi-sbefifo.c | 42 +++++++++++++++++++++++++++++++++++++++ > include/uapi/linux/fsi.h | 6 ++++++ > 2 files changed, 48 insertions(+) > > diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c > index 9188161f440c..b2654b143b85 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; > + uint32_t 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,11 @@ 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); > + /* Reset the read timeout after a single chip-op */ > + sbefifo->timeout_start_rsp_ms = SBEFIFO_TIMEOUT_START_RSP; > + user->read_timeout_ms = SBEFIFO_TIMEOUT_START_RSP; > mutex_unlock(&sbefifo->lock); > if (rc < 0) > goto bail; > @@ -847,6 +855,7 @@ static ssize_t sbefifo_user_read(struct file *file, char __user *buf, > rc = len - iov_iter_count(&resp_iter); > bail: > sbefifo_release_command(user); > + user->read_timeout_ms = 0; > mutex_unlock(&user->file_lock); > return rc; > } > @@ -928,12 +937,45 @@ 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) > +{ > + uint32_t timeout; > + > + if (get_user(timeout, (__u32 __user *)argp)) > + return -EFAULT; > + 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; > + void __user **argp = (void __user *)arg; > + int rc = -ENOTTY; > + > + if (!user) > + return -EINVAL; > + > + mutex_lock(&user->file_lock); > + switch (cmd) { > + case FSI_SBEFIFO_READ_TIMEOUT: > + rc = sbefifo_read_timeout(user, argp); > + 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..3e00874ace22 100644 > --- a/include/uapi/linux/fsi.h > +++ b/include/uapi/linux/fsi.h > @@ -55,4 +55,10 @@ 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 > + */ > + > +#define FSI_SBEFIFO_READ_TIMEOUT _IOW('s', 0x00, __u32) > + > #endif /* _UAPI_LINUX_FSI_H */ > -- > 2.33.1 >
Hi Amitay, On Wed, 15 Dec 2021, at 11:28, Amitay Isaacs wrote: > FSI_SBEFIFO_READ_TIMEOUT ioctl sets the read timeout (in seconds) for > the response to *the next* chip-op sent to sbe. The timeout value is > reset to default after the chip-op. For the user? Why reset it automatically? To avoid unexpected surprises in existing code? > The timeout affects only the read() > operation on sbefifo device fd. > > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> > --- > drivers/fsi/fsi-sbefifo.c | 42 +++++++++++++++++++++++++++++++++++++++ > include/uapi/linux/fsi.h | 6 ++++++ > 2 files changed, 48 insertions(+) > > diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c > index 9188161f440c..b2654b143b85 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; > + uint32_t 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,11 @@ 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); > + /* Reset the read timeout after a single chip-op */ > + sbefifo->timeout_start_rsp_ms = SBEFIFO_TIMEOUT_START_RSP; > + user->read_timeout_ms = SBEFIFO_TIMEOUT_START_RSP; I guess I was querying this one > mutex_unlock(&sbefifo->lock); > if (rc < 0) > goto bail; > @@ -847,6 +855,7 @@ static ssize_t sbefifo_user_read(struct file *file, > char __user *buf, > rc = len - iov_iter_count(&resp_iter); > bail: > sbefifo_release_command(user); > + user->read_timeout_ms = 0; > mutex_unlock(&user->file_lock); > return rc; > } > @@ -928,12 +937,45 @@ 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) > +{ > + uint32_t timeout; > + > + if (get_user(timeout, (__u32 __user *)argp)) Hmm > + return -EFAULT; > + 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; > + void __user **argp = (void __user *)arg; Why are we doing strange things with the pointer types? Andrew > + int rc = -ENOTTY; > + > + if (!user) > + return -EINVAL; > + > + mutex_lock(&user->file_lock); > + switch (cmd) { > + case FSI_SBEFIFO_READ_TIMEOUT: > + rc = sbefifo_read_timeout(user, argp); > + 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..3e00874ace22 100644 > --- a/include/uapi/linux/fsi.h > +++ b/include/uapi/linux/fsi.h > @@ -55,4 +55,10 @@ 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 > + */ > + > +#define FSI_SBEFIFO_READ_TIMEOUT _IOW('s', 0x00, __u32) > + > #endif /* _UAPI_LINUX_FSI_H */ > -- > 2.33.1
On Wed, Dec 15, 2021 at 11:58:33AM +1100, Amitay Isaacs wrote: > FSI_SBEFIFO_READ_TIMEOUT ioctl sets the read timeout (in seconds) for > the response to *the next* chip-op sent to sbe. The timeout value is > reset to default after the chip-op. The timeout affects only the read() > operation on sbefifo device fd. > > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> > --- > drivers/fsi/fsi-sbefifo.c | 42 +++++++++++++++++++++++++++++++++++++++ > include/uapi/linux/fsi.h | 6 ++++++ > 2 files changed, 48 insertions(+) > > diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c > index 9188161f440c..b2654b143b85 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; > + uint32_t read_timeout_ms; u32 please. uint32_t is a userspace thing. > }; > > 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,11 @@ 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); > + /* Reset the read timeout after a single chip-op */ > + sbefifo->timeout_start_rsp_ms = SBEFIFO_TIMEOUT_START_RSP; > + user->read_timeout_ms = SBEFIFO_TIMEOUT_START_RSP; > mutex_unlock(&sbefifo->lock); > if (rc < 0) > goto bail; > @@ -847,6 +855,7 @@ static ssize_t sbefifo_user_read(struct file *file, char __user *buf, > rc = len - iov_iter_count(&resp_iter); > bail: > sbefifo_release_command(user); > + user->read_timeout_ms = 0; > mutex_unlock(&user->file_lock); > return rc; > } > @@ -928,12 +937,45 @@ 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) > +{ > + uint32_t timeout; u32 > + > + if (get_user(timeout, (__u32 __user *)argp)) > + return -EFAULT; > + 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; > + void __user **argp = (void __user *)arg; > + int rc = -ENOTTY; > + > + if (!user) > + return -EINVAL; > + > + mutex_lock(&user->file_lock); > + switch (cmd) { > + case FSI_SBEFIFO_READ_TIMEOUT: > + rc = sbefifo_read_timeout(user, argp); > + break; > + } > + mutex_unlock(&user->file_lock); > + return rc; > +} Why do you have to have an ioctl for a single thing like this? > + > 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..3e00874ace22 100644 > --- a/include/uapi/linux/fsi.h > +++ b/include/uapi/linux/fsi.h > @@ -55,4 +55,10 @@ 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 > + */ > + > +#define FSI_SBEFIFO_READ_TIMEOUT _IOW('s', 0x00, __u32) Where have you documented this new user/kernel api? And why not just use a sysfs file for something like this? thanks, greg k-h
Hi Andrew, On Wed, 2021-12-15 at 16:13 +1030, Andrew Jeffery wrote: > Hi Amitay, > > On Wed, 15 Dec 2021, at 11:28, Amitay Isaacs wrote: > > FSI_SBEFIFO_READ_TIMEOUT ioctl sets the read timeout (in seconds) > > for > > the response to *the next* chip-op sent to sbe. The timeout value > > is > > reset to default after the chip-op. > > For the user? Why reset it automatically? To avoid unexpected > surprises in existing code? Ideally I would prefer the timeout as an argument for the read() call, so it's specific to that read call and nothing else. Of course, this simplifies the user-space code that needs to issue only a single ioctl before the "long" chip-ops. I am fine with using the ioctl to just set the timeout value also. > > > The timeout affects only the read() > > operation on sbefifo device fd. > > > > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> > > --- > > drivers/fsi/fsi-sbefifo.c | 42 > > +++++++++++++++++++++++++++++++++++++++ > > include/uapi/linux/fsi.h | 6 ++++++ > > 2 files changed, 48 insertions(+) > > > > diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c > > index 9188161f440c..b2654b143b85 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; > > + uint32_t 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,11 @@ 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); > > + /* Reset the read timeout after a single chip-op */ > > + sbefifo->timeout_start_rsp_ms = SBEFIFO_TIMEOUT_START_RSP; > > + user->read_timeout_ms = SBEFIFO_TIMEOUT_START_RSP; > > I guess I was querying this one > > > mutex_unlock(&sbefifo->lock); > > if (rc < 0) > > goto bail; > > @@ -847,6 +855,7 @@ static ssize_t sbefifo_user_read(struct file > > *file, > > char __user *buf, > > rc = len - iov_iter_count(&resp_iter); > > bail: > > sbefifo_release_command(user); > > + user->read_timeout_ms = 0; > > mutex_unlock(&user->file_lock); > > return rc; > > } > > @@ -928,12 +937,45 @@ 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) > > +{ > > + uint32_t timeout; > > + > > + if (get_user(timeout, (__u32 __user *)argp)) > > Hmm > > > + return -EFAULT; > > + 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; > > + void __user **argp = (void __user *)arg; > > Why are we doing strange things with the pointer types? That's what the ioctl implementation for fsi does. (Monkey see, monkey copy!) :-) > > Andrew > > > + int rc = -ENOTTY; > > + > > + if (!user) > > + return -EINVAL; > > + > > + mutex_lock(&user->file_lock); > > + switch (cmd) { > > + case FSI_SBEFIFO_READ_TIMEOUT: > > + rc = sbefifo_read_timeout(user, argp); > > + 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..3e00874ace22 100644 > > --- a/include/uapi/linux/fsi.h > > +++ b/include/uapi/linux/fsi.h > > @@ -55,4 +55,10 @@ 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 > > + */ > > + > > +#define FSI_SBEFIFO_READ_TIMEOUT _IOW('s', 0x00, __u32) > > + > > #endif /* _UAPI_LINUX_FSI_H */ > > -- > > 2.33.1 Amitay.
On Wed, 2021-12-15 at 06:59 +0100, Greg KH wrote: > On Wed, Dec 15, 2021 at 11:58:33AM +1100, Amitay Isaacs wrote: > > FSI_SBEFIFO_READ_TIMEOUT ioctl sets the read timeout (in seconds) > > for > > the response to *the next* chip-op sent to sbe. The timeout value > > is > > reset to default after the chip-op. The timeout affects only the > > read() > > operation on sbefifo device fd. > > > > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> > > --- > > drivers/fsi/fsi-sbefifo.c | 42 > > +++++++++++++++++++++++++++++++++++++++ > > include/uapi/linux/fsi.h | 6 ++++++ > > 2 files changed, 48 insertions(+) > > > > diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c > > index 9188161f440c..b2654b143b85 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; > > + uint32_t read_timeout_ms; > > u32 please. uint32_t is a userspace thing. Sure thing. > > > }; > > > > 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,11 @@ 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); > > + /* Reset the read timeout after a single chip-op */ > > + sbefifo->timeout_start_rsp_ms = SBEFIFO_TIMEOUT_START_RSP; > > + user->read_timeout_ms = SBEFIFO_TIMEOUT_START_RSP; > > mutex_unlock(&sbefifo->lock); > > if (rc < 0) > > goto bail; > > @@ -847,6 +855,7 @@ static ssize_t sbefifo_user_read(struct file > > *file, char __user *buf, > > rc = len - iov_iter_count(&resp_iter); > > bail: > > sbefifo_release_command(user); > > + user->read_timeout_ms = 0; > > mutex_unlock(&user->file_lock); > > return rc; > > } > > @@ -928,12 +937,45 @@ 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) > > +{ > > + uint32_t timeout; > > u32 > > > + > > + if (get_user(timeout, (__u32 __user *)argp)) > > + return -EFAULT; > > + 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; > > + void __user **argp = (void __user *)arg; > > + int rc = -ENOTTY; > > + > > + if (!user) > > + return -EINVAL; > > + > > + mutex_lock(&user->file_lock); > > + switch (cmd) { > > + case FSI_SBEFIFO_READ_TIMEOUT: > > + rc = sbefifo_read_timeout(user, argp); > > + break; > > + } > > + mutex_unlock(&user->file_lock); > > + return rc; > > +} > > Why do you have to have an ioctl for a single thing like this? This timeout needs to be set for only certain write/read operations (referred as sbe chip-ops) done via open fd for sbefifo device. There can be multiple simultaneous users of the device, and the timeout should only be applied to specific chip-ops as user requests. > > > + > > 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..3e00874ace22 100644 > > --- a/include/uapi/linux/fsi.h > > +++ b/include/uapi/linux/fsi.h > > @@ -55,4 +55,10 @@ 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 > > + */ > > + > > +#define FSI_SBEFIFO_READ_TIMEOUT _IOW('s', 0x00, __u32) > > Where have you documented this new user/kernel api? What's the best location to add the information? I would prefer to add this information along with the FSI ioctl, but could not find it in Documentation/. > > And why not just use a sysfs file for something like this? > I guess sysfs interface would be useful for setting a global property, rather than a parameter affecting individual operation. > > thanks, > > greg k-h Thanks. Amitay.
Hi Amitay,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linux/master]
[also build test WARNING on linus/master v5.16-rc5 next-20211214]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Amitay-Isaacs/fsi-sbefifo-Use-specified-value-of-start-of-response-timeout/20211215-090038
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 136057256686de39cc3a07c2e39ef6bc43003ff6
config: h8300-randconfig-s032-20211214 (https://download.01.org/0day-ci/archive/20211215/202112151543.C2qA1wIs-lkp@intel.com/config)
compiler: h8300-linux-gcc (GCC) 11.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-dirty
# https://github.com/0day-ci/linux/commit/abe81bc06079f76e9a9f4ebe6cc0a963ee5b6985
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Amitay-Isaacs/fsi-sbefifo-Use-specified-value-of-start-of-response-timeout/20211215-090038
git checkout abe81bc06079f76e9a9f4ebe6cc0a963ee5b6985
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=h8300 SHELL=/bin/bash drivers/fsi/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/fsi/fsi-sbefifo.c:956:31: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void [noderef] __user **argp @@ got void [noderef] __user * @@
drivers/fsi/fsi-sbefifo.c:956:31: sparse: expected void [noderef] __user **argp
drivers/fsi/fsi-sbefifo.c:956:31: sparse: got void [noderef] __user *
vim +956 drivers/fsi/fsi-sbefifo.c
952
953 static long sbefifo_user_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
954 {
955 struct sbefifo_user *user = file->private_data;
> 956 void __user **argp = (void __user *)arg;
957 int rc = -ENOTTY;
958
959 if (!user)
960 return -EINVAL;
961
962 mutex_lock(&user->file_lock);
963 switch (cmd) {
964 case FSI_SBEFIFO_READ_TIMEOUT:
965 rc = sbefifo_read_timeout(user, argp);
966 break;
967 }
968 mutex_unlock(&user->file_lock);
969 return rc;
970 }
971
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c index 9188161f440c..b2654b143b85 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; + uint32_t 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,11 @@ 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); + /* Reset the read timeout after a single chip-op */ + sbefifo->timeout_start_rsp_ms = SBEFIFO_TIMEOUT_START_RSP; + user->read_timeout_ms = SBEFIFO_TIMEOUT_START_RSP; mutex_unlock(&sbefifo->lock); if (rc < 0) goto bail; @@ -847,6 +855,7 @@ static ssize_t sbefifo_user_read(struct file *file, char __user *buf, rc = len - iov_iter_count(&resp_iter); bail: sbefifo_release_command(user); + user->read_timeout_ms = 0; mutex_unlock(&user->file_lock); return rc; } @@ -928,12 +937,45 @@ 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) +{ + uint32_t timeout; + + if (get_user(timeout, (__u32 __user *)argp)) + return -EFAULT; + 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; + void __user **argp = (void __user *)arg; + int rc = -ENOTTY; + + if (!user) + return -EINVAL; + + mutex_lock(&user->file_lock); + switch (cmd) { + case FSI_SBEFIFO_READ_TIMEOUT: + rc = sbefifo_read_timeout(user, argp); + 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..3e00874ace22 100644 --- a/include/uapi/linux/fsi.h +++ b/include/uapi/linux/fsi.h @@ -55,4 +55,10 @@ 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 + */ + +#define FSI_SBEFIFO_READ_TIMEOUT _IOW('s', 0x00, __u32) + #endif /* _UAPI_LINUX_FSI_H */
FSI_SBEFIFO_READ_TIMEOUT ioctl sets the read timeout (in seconds) for the response to *the next* chip-op sent to sbe. The timeout value is reset to default after the chip-op. The timeout affects only the read() operation on sbefifo device fd. Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> --- drivers/fsi/fsi-sbefifo.c | 42 +++++++++++++++++++++++++++++++++++++++ include/uapi/linux/fsi.h | 6 ++++++ 2 files changed, 48 insertions(+)