On Tue, Nov 15, 2022 at 01:14:38PM +0200, Alexander Usyskin wrote:
> When driver wakes up the firmware from the low power state, it is sending
> a memory ready message.
> The send is done via synchronous/blocking function to ensure that firmware
> is in ready state. However, in case of firmware undergoing reset send
> might be block forever.
> To address this issue a timeout is added to blocking write command on
> the internal bus.
Odd formatting of the text :(
>
> Introduce the __mei_cl_send_timeout function to use instead of
> __mei_cl_send in cases where timeout is required.
> The mei_cl_write has only two callers and there is no need to split
> it into two functions.
>
> Signed-off-by: Alexander Usyskin <alexander.usys...@intel.com>
> ---
> V2: address review comments:
> - split __mei_cl_send and __mei_cl_send_timeout
> - add units to timeout KDoc
> - use MAX_SCHEDULE_TIMEOUT to squash wait to one macro
>
> drivers/misc/mei/bus-fixup.c | 14 +++++++++-----
> drivers/misc/mei/bus.c | 22 ++++++++++++++++++++--
> drivers/misc/mei/client.c | 18 ++++++++++++++----
> drivers/misc/mei/client.h | 2 +-
> drivers/misc/mei/main.c | 2 +-
> drivers/misc/mei/mei_dev.h | 2 ++
> 6 files changed, 47 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/misc/mei/bus-fixup.c b/drivers/misc/mei/bus-fixup.c
> index 71fbf0bc8453..9959b8e8e91d 100644
> --- a/drivers/misc/mei/bus-fixup.c
> +++ b/drivers/misc/mei/bus-fixup.c
> @@ -188,17 +188,20 @@ static int mei_fwver(struct mei_cl_device *cldev)
> return ret;
> }
>
> +#define GFX_MEMORY_READY_TIMEOUT 200
units?
> +
> static int mei_gfx_memory_ready(struct mei_cl_device *cldev)
> {
> struct mkhi_gfx_mem_ready req = {0};
> - unsigned int mode = MEI_CL_IO_TX_INTERNAL;
> + unsigned int mode = MEI_CL_IO_TX_INTERNAL | MEI_CL_IO_TX_BLOCKING;
>
> req.hdr.group_id = MKHI_GROUP_ID_GFX;
> req.hdr.command = MKHI_GFX_MEMORY_READY_CMD_REQ;
> req.flags = MKHI_GFX_MEM_READY_PXP_ALLOWED;
>
> dev_dbg(&cldev->dev, "Sending memory ready command\n");
> - return __mei_cl_send(cldev->cl, (u8 *)&req, sizeof(req), 0, mode);
> + return __mei_cl_send_timeout(cldev->cl, (u8 *)&req, sizeof(req), 0,
> + mode, GFX_MEMORY_READY_TIMEOUT);
> }
>
> static void mei_mkhi_fix(struct mei_cl_device *cldev)
> @@ -263,12 +266,13 @@ static void mei_gsc_mkhi_fix_ver(struct mei_cl_device
> *cldev)
>
> if (cldev->bus->pxp_mode == MEI_DEV_PXP_INIT) {
> ret = mei_gfx_memory_ready(cldev);
> - if (ret < 0)
> + if (ret < 0) {
> dev_err(&cldev->dev, "memory ready command failed
> %d\n", ret);
> - else
> + } else {
> dev_dbg(&cldev->dev, "memory ready command sent\n");
> + cldev->bus->pxp_mode = MEI_DEV_PXP_SETUP;
What does the mode change have to do with a timeout?
> + }
> /* we go to reset after that */
> - cldev->bus->pxp_mode = MEI_DEV_PXP_SETUP;
> goto out;
> }
>
> diff --git a/drivers/misc/mei/bus.c b/drivers/misc/mei/bus.c
> index 1fbe127ff633..63043e8df980 100644
> --- a/drivers/misc/mei/bus.c
> +++ b/drivers/misc/mei/bus.c
> @@ -32,8 +32,26 @@
> *
> * Return: written size bytes or < 0 on error
> */
> -ssize_t __mei_cl_send(struct mei_cl *cl, const u8 *buf, size_t length, u8
> vtag,
> +inline ssize_t __mei_cl_send(struct mei_cl *cl, const u8 *buf, size_t
> length, u8 vtag,
> unsigned int mode)
Why inline? The compiler is smart enough.
> +{
> + return __mei_cl_send_timeout(cl, buf, length, vtag, mode,
> MAX_SCHEDULE_TIMEOUT);
So this will block for how long? Please document this.
> +}
> +
> +/**
> + * __mei_cl_send_timeout - internal client send (write)
> + *
> + * @cl: host client
> + * @buf: buffer to send
> + * @length: buffer length
> + * @vtag: virtual tag
> + * @mode: sending mode
> + * @timeout: send timeout in milliseconds for blocking writes
What do you mean "for blocking writes"?
And what do you use here to wait "for forever"?
> + *
> + * Return: written size bytes or < 0 on error
> + */
> +ssize_t __mei_cl_send_timeout(struct mei_cl *cl, const u8 *buf, size_t
> length, u8 vtag,
> + unsigned int mode, unsigned long timeout)
> {
> struct mei_device *bus;
> struct mei_cl_cb *cb;
> @@ -108,7 +126,7 @@ ssize_t __mei_cl_send(struct mei_cl *cl, const u8 *buf,
> size_t length, u8 vtag,
> cb->buf.size = 0;
> }
>
> - rets = mei_cl_write(cl, cb);
> + rets = mei_cl_write(cl, cb, timeout);
>
> if (mode & MEI_CL_IO_SGL && rets == 0)
> rets = length;
> diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
> index 6c8b71ae32c8..02c278202ad7 100644
> --- a/drivers/misc/mei/client.c
> +++ b/drivers/misc/mei/client.c
> @@ -1954,10 +1954,11 @@ int mei_cl_irq_write(struct mei_cl *cl, struct
> mei_cl_cb *cb,
> *
> * @cl: host client
> * @cb: write callback with filled data
> + * @timeout: send timeout in milliseconds for blocking writes
Again, what is the max, and what do you mean "for blocking writes"?
> *
> * Return: number of bytes sent on success, <0 on failure.
> */
> -ssize_t mei_cl_write(struct mei_cl *cl, struct mei_cl_cb *cb)
> +ssize_t mei_cl_write(struct mei_cl *cl, struct mei_cl_cb *cb, unsigned long
> timeout)
Why not have mei_cl_write() and mei_cl_write_timeout() like you do for
"send"?
> {
> struct mei_device *dev;
> struct mei_msg_data *buf;
> @@ -2081,11 +2082,20 @@ ssize_t mei_cl_write(struct mei_cl *cl, struct
> mei_cl_cb *cb)
> if (blocking && cl->writing_state != MEI_WRITE_COMPLETE) {
>
> mutex_unlock(&dev->device_lock);
> - rets = wait_event_interruptible(cl->tx_wait,
> - cl->writing_state == MEI_WRITE_COMPLETE ||
> - (!mei_cl_is_connected(cl)));
> + rets = wait_event_interruptible_timeout(cl->tx_wait,
> + cl->writing_state ==
> MEI_WRITE_COMPLETE ||
> +
> (!mei_cl_is_connected(cl)),
> +
> msecs_to_jiffies(timeout));
> mutex_lock(&dev->device_lock);
> + /* clean all queue on timeout as something fatal happened */
> + if (rets == 0) {
> + rets = -ETIME;
> + mei_io_tx_list_free_cl(&dev->write_list, cl, NULL);
> + mei_io_tx_list_free_cl(&dev->write_waiting_list, cl,
> NULL);
> + }
> /* wait_event_interruptible returns -ERESTARTSYS */
> + if (rets > 0)
> + rets = 0;
> if (rets) {
> if (signal_pending(current))
> rets = -EINTR;
> diff --git a/drivers/misc/mei/client.h b/drivers/misc/mei/client.h
> index 418056fb1489..9052860bcfe0 100644
> --- a/drivers/misc/mei/client.h
> +++ b/drivers/misc/mei/client.h
> @@ -246,7 +246,7 @@ int mei_cl_connect(struct mei_cl *cl, struct
> mei_me_client *me_cl,
> int mei_cl_irq_connect(struct mei_cl *cl, struct mei_cl_cb *cb,
> struct list_head *cmpl_list);
> int mei_cl_read_start(struct mei_cl *cl, size_t length, const struct file
> *fp);
> -ssize_t mei_cl_write(struct mei_cl *cl, struct mei_cl_cb *cb);
> +ssize_t mei_cl_write(struct mei_cl *cl, struct mei_cl_cb *cb, unsigned long
> timeout);
> int mei_cl_irq_write(struct mei_cl *cl, struct mei_cl_cb *cb,
> struct list_head *cmpl_list);
>
> diff --git a/drivers/misc/mei/main.c b/drivers/misc/mei/main.c
> index 930887e7e38d..632d4ae21e46 100644
> --- a/drivers/misc/mei/main.c
> +++ b/drivers/misc/mei/main.c
> @@ -383,7 +383,7 @@ static ssize_t mei_write(struct file *file, const char
> __user *ubuf,
> goto out;
> }
>
> - rets = mei_cl_write(cl, cb);
> + rets = mei_cl_write(cl, cb, MAX_SCHEDULE_TIMEOUT);
Why this specific value?
thanks,
greg k-h
