| 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
| ||
|
Date: Tue, 15 Nov 2022 14:27:02 +0000
From: "Usyskin, Alexander" <alexander.usyskin@...el.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC: Jani Nikula <jani.nikula@...ux.intel.com>,
Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
"Vivi, Rodrigo" <rodrigo.vivi@...el.com>,
David Airlie <airlied@...hat.com>,
Daniel Vetter <daniel@...ll.ch>,
"Tvrtko Ursulin" <tvrtko.ursulin@...ux.intel.com>,
"Winkler, Tomas" <tomas.winkler@...el.com>,
"Lubart, Vitaly" <vitaly.lubart@...el.com>,
"intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2] mei: add timeout to send
> > 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@...el.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
--
Thanks,
Sasha
Powered by blists - more mailing lists