> > 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 <[email protected]> > > --- > > 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 -- Thanks, Sasha
