Re: [PATCH] mei: add timeout to send
From: Tvrtko Ursulin
Date: Fri Nov 04 2022 - 04:25:34 EST
Hi,
Not really driver I looked at before but since you copied me some
comments below.
On 03/11/2022 15:55, Alexander Usyskin wrote:
When driver wakes up the firmware from the low power stand,
it is sending a memory ready message.
The send is done via synchronous/blocking function to ensure
that firmware is in ready state. However firmware might be
in unstable state and send might be block forever.
To address this issue a timeout is added to blocking write command on
the internal bus.
It would be preferrable to consistently wrap at 75 as per kernel coding
style so it looks tidy.
Signed-off-by: Alexander Usyskin <alexander.usyskin@xxxxxxxxx>
---
drivers/misc/mei/bus-fixup.c | 19 +++++++++++--------
drivers/misc/mei/bus.c | 9 +++++----
drivers/misc/mei/client.c | 21 +++++++++++++++++----
drivers/misc/mei/client.h | 2 +-
drivers/misc/mei/main.c | 2 +-
drivers/misc/mei/mei_dev.h | 2 +-
6 files changed, 36 insertions(+), 19 deletions(-)
diff --git a/drivers/misc/mei/bus-fixup.c b/drivers/misc/mei/bus-fixup.c
index 71fbf0bc8453..3174cad8a5cc 100644
--- a/drivers/misc/mei/bus-fixup.c
+++ b/drivers/misc/mei/bus-fixup.c
@@ -128,7 +128,7 @@ static int mei_osver(struct mei_cl_device *cldev)
os_ver = (struct mei_os_ver *)fwcaps->data;
os_ver->os_type = OSTYPE_LINUX;
- return __mei_cl_send(cldev->cl, buf, size, 0, mode);
+ return __mei_cl_send(cldev->cl, buf, size, 0, mode, 0);
}
#define MKHI_FWVER_BUF_LEN (sizeof(struct mkhi_msg_hdr) + \
@@ -149,7 +149,7 @@ static int mei_fwver(struct mei_cl_device *cldev)
req.hdr.command = MKHI_GEN_GET_FW_VERSION_CMD;
ret = __mei_cl_send(cldev->cl, (u8 *)&req, sizeof(req), 0,
- MEI_CL_IO_TX_BLOCKING);
+ MEI_CL_IO_TX_BLOCKING, 0);
if (ret < 0) {
dev_err(&cldev->dev, "Could not send ReqFWVersion cmd\n");
return ret;
@@ -188,17 +188,19 @@ 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(cldev->cl, (u8 *)&req, sizeof(req), 0, mode, GFX_MEMORY_READY_TIMEOUT);
}
static void mei_mkhi_fix(struct mei_cl_device *cldev)
@@ -263,12 +265,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;
}
@@ -374,7 +377,7 @@ static int mei_nfc_if_version(struct mei_cl *cl,
WARN_ON(mutex_is_locked(&bus->device_lock));
ret = __mei_cl_send(cl, (u8 *)&cmd, sizeof(cmd), 0,
- MEI_CL_IO_TX_BLOCKING);
+ MEI_CL_IO_TX_BLOCKING, 0);
if (ret < 0) {
dev_err(bus->dev, "Could not send IF version cmd\n");
return ret;
diff --git a/drivers/misc/mei/bus.c b/drivers/misc/mei/bus.c
index 1fbe127ff633..136b45192904 100644
--- a/drivers/misc/mei/bus.c
+++ b/drivers/misc/mei/bus.c
@@ -29,11 +29,12 @@
* @length: buffer length
* @vtag: virtual tag
* @mode: sending mode
+ * @timeout: send timeout for blocking writes, 0 for infinite timeout
*
* 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,
- unsigned int mode)
+ unsigned int mode, unsigned long timeout)
{
struct mei_device *bus;
struct mei_cl_cb *cb;
@@ -108,7 +109,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;
@@ -254,7 +255,7 @@ ssize_t mei_cldev_send_vtag(struct mei_cl_device *cldev, const u8 *buf,
{
struct mei_cl *cl = cldev->cl;
- return __mei_cl_send(cl, buf, length, vtag, MEI_CL_IO_TX_BLOCKING);
+ return __mei_cl_send(cl, buf, length, vtag, MEI_CL_IO_TX_BLOCKING, 0);
}
EXPORT_SYMBOL_GPL(mei_cldev_send_vtag);
@@ -924,7 +925,7 @@ ssize_t mei_cldev_send_gsc_command(struct mei_cl_device *cldev,
}
/* send the message to GSC */
- ret = __mei_cl_send(cl, (u8 *)ext_hdr, buf_sz, 0, MEI_CL_IO_SGL);
+ ret = __mei_cl_send(cl, (u8 *)ext_hdr, buf_sz, 0, MEI_CL_IO_SGL, 0);
if (ret < 0) {
dev_err(bus->dev, "__mei_cl_send failed, returned %zd\n", ret);
goto end;
diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
index 6c8b71ae32c8..68f2e23b8ae5 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 for blocking writes, 0 for infinite timeout
*
* 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,9 +2082,21 @@ 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)));
+ if (timeout) {
+ rets = wait_event_interruptible_timeout(cl->tx_wait,
+ cl->writing_state == MEI_WRITE_COMPLETE ||
+ (!mei_cl_is_connected(cl)),
+ msecs_to_jiffies(timeout));
+ if (rets == 0)
+ rets = -ETIME;
+ if (rets > 0)
+ rets = 0;
+ } else {
+ rets = wait_event_interruptible(cl->tx_wait,
+ cl->writing_state == MEI_WRITE_COMPLETE ||
+ (!mei_cl_is_connected(cl)));
+ }
Usual pattern to avoid the need to duplicate the branches is to either
replace 0 with MAX_SCHEDULE_TIMEOUT in this function, or make callers
pass that value explicitly in, and so only have the
wait_event_interruptible_timeout branch.
Or if you would like to make the patch even smaller (avoid function
signature churn) maybe you could even add a new flag to the 'mode'
parameter to signify a chosen timeout, in one way or another.
Regards,
Tvrtko
+
mutex_lock(&dev->device_lock);
/* wait_event_interruptible returns -ERESTARTSYS */
if (rets) {
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..311c6a34aeb8 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, 0);
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..799ea95e2bb8 100644
--- a/drivers/misc/mei/mei_dev.h
+++ b/drivers/misc/mei/mei_dev.h
@@ -378,7 +378,7 @@ struct mei_hw_ops {
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);
+ 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);