| Message ID | 20221115111438.1639527-1-alexander.usyskin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mei: add timeout to send | expand |
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
> > 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
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
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);
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(-)