[v2] mei: add timeout to send

Message ID 20221115111438.1639527-1-alexander.usyskin@intel.com
State New
Headers
Series [v2] mei: add timeout to send |

Commit Message

Usyskin, Alexander Nov. 15, 2022, 11:14 a.m. UTC
  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.

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.usyskin@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(-)
  

Comments

Greg KH Nov. 15, 2022, 1:45 p.m. UTC | #1
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.usyskin@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
  
Usyskin, Alexander Nov. 15, 2022, 2:27 p.m. UTC | #2
> > 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 :(

Odd == not balanced? Will try to do better in V3
> 
> >
> > 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.usyskin@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?

Will add here too, sure.

> 
> > +
> >  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?
With timeout the mei_gfx_memory_ready may now fail gracefully
and we should not move state if message is not sent.

Should I split this fix into another patch or document in this one?

> 
> > +		}
> >  		/* 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.
Will drop

> 
> > +{
> > +	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"?

The timeout has no effect for non-blocking writes (bit in mode parameter),
as they are returning immediately and are not waiting at all.

> 
> And what do you use here to wait "for forever"?

The MAX_SCHEDULE_TIMEOUT indicates 'forever' - will add it in KDoc
This is implementation of Rodrigo's suggestion to use only wait with timeout but with
maximum one if 'forever' is required.

> 
> > + *
> > + * 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"?

There are only two callers for mei_cl_write now, one with timeout and another without.
Don't think that we need wrapper that has one caller, and no additions are planned.

The __mei_cl_write_timeout may be called by more than one user.
This patch converts one caller, but it is open for other callers in future.

> 
> >  {
> >  	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?

Very big one, rouge equivalent for 'forever'

> 
> thanks,
> 
> greg k-h
  
Greg KH Nov. 15, 2022, 2:59 p.m. UTC | #3
On Tue, Nov 15, 2022 at 02:27:02PM +0000, Usyskin, Alexander wrote:
> > > -		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?
> With timeout the mei_gfx_memory_ready may now fail gracefully
> and we should not move state if message is not sent.
> 
> Should I split this fix into another patch or document in this one?

Split it into a different patch please.


> > > +/**
> > > + * __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"?
> 
> The timeout has no effect for non-blocking writes (bit in mode parameter),
> as they are returning immediately and are not waiting at all.

That's not obvious, please say that the timeout is affected by the mode
and how it is affected.

thanks,

greg k-h
  

Patch

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
+
 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;
+		}
 		/* 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)
+{
+	return __mei_cl_send_timeout(cl, buf, length, vtag, mode, MAX_SCHEDULE_TIMEOUT);
+}
+
+/**
+ * __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
+ *
+ * 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
  *
  * 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)
 {
 	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);
 out:
 	mutex_unlock(&dev->device_lock);
 	return rets;
diff --git a/drivers/misc/mei/mei_dev.h b/drivers/misc/mei/mei_dev.h
index 8d8018428d9d..996b70a988be 100644
--- a/drivers/misc/mei/mei_dev.h
+++ b/drivers/misc/mei/mei_dev.h
@@ -379,6 +379,8 @@  void mei_cl_bus_rescan_work(struct work_struct *work);
 void mei_cl_bus_dev_fixup(struct mei_cl_device *dev);
 ssize_t __mei_cl_send(struct mei_cl *cl, const u8 *buf, size_t length, u8 vtag,
 		      unsigned int mode);
+ssize_t __mei_cl_send_timeout(struct mei_cl *cl, const u8 *buf, size_t length, u8 vtag,
+			      unsigned int mode, unsigned long timeout);
 ssize_t __mei_cl_recv(struct mei_cl *cl, u8 *buf, size_t length, u8 *vtag,
 		      unsigned int mode, unsigned long timeout);
 bool mei_cl_bus_rx_event(struct mei_cl *cl);