| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <27f9ac82-dba9-0122-707b-c21d4c606580@linux.intel.com>
Date: Fri, 4 Nov 2022 08:25:22 +0000
From: Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>
To: Alexander Usyskin <alexander.usyskin@...el.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jani Nikula <jani.nikula@...ux.intel.com>,
Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@...el.com>,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>
Cc: Tomas Winkler <tomas.winkler@...el.com>,
Vitaly Lubart <vitaly.lubart@...el.com>,
intel-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mei: add timeout to send
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@...el.com>
> ---
> 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);
Powered by blists - more mailing lists