mbox series

[v3,0/2] mei: add timeout to send

Message ID 20221116124735.2493847-1-alexander.usyskin@intel.com (mailing list archive)
Headers show
Series mei: add timeout to send | expand

Message

Alexander Usyskin Nov. 16, 2022, 12:47 p.m. UTC
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.

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

V3: - split the state fix into separate patch
    - document define unit
    - expand timeout KDoc

Alexander Usyskin (2):
  mei: add timeout to send
  mei: bus-fixup: change pxp mode only if message was sent

 drivers/misc/mei/bus-fixup.c | 14 +++++++++-----
 drivers/misc/mei/bus.c       | 22 +++++++++++++++++++++-
 drivers/misc/mei/client.c    | 20 ++++++++++++++++----
 drivers/misc/mei/client.h    |  2 +-
 drivers/misc/mei/main.c      |  2 +-
 drivers/misc/mei/mei_dev.h   |  2 ++
 6 files changed, 50 insertions(+), 12 deletions(-)

Comments

Matt Roper Nov. 17, 2022, 12:46 a.m. UTC | #1
On Wed, Nov 16, 2022 at 09:49:20PM +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: mei: add timeout to send (rev3)
> URL   : https://patchwork.freedesktop.org/series/110495/
> State : success
> 
> == Summary ==
> 
> CI Bug Log - changes from CI_DRM_12390 -> Patchwork_110495v3
> ====================================================
> 
> Summary
> -------
> 
>   **SUCCESS**
> 

Since this series is expected to resolve one of the remaining blockers
to DG2 force_probe removal, we've temporarily added this to the
topic/core-for-CI branch that gets included in drm-tip to verify it
fully solves the problem across multiple builds.  We can drop it from
the CI branch when this series actually lands (which I believe will
happen via the mei tree) or if we need to replace it with an updated
version of the series.


Matt

>   No regressions found.
> 
>   External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110495v3/index.html
> 
> Participating hosts (41 -> 38)
> ------------------------------
> 
>   Additional (1): bat-atsm-1 
>   Missing    (4): bat-kbl-2 bat-jsl-3 bat-dg1-6 bat-dg1-5 
> 
> Known issues
> ------------
> 
>   Here are the changes found in Patchwork_110495v3 that come from known issues:
> 
> ### IGT changes ###
> 
> #### Issues hit ####
> 
>   * igt@gem_lmem_swapping@parallel-random-engines:
>     - bat-adlp-4:         NOTRUN -> [SKIP][1] ([i915#4613]) +3 similar issues
>    [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110495v3/bat-adlp-4/igt@gem_lmem_swapping@parallel-random-engines.html
> 
>   * igt@gem_render_tiled_blits@basic:
>     - fi-apl-guc:         [PASS][2] -> [INCOMPLETE][3] ([i915#7056])
>    [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/fi-apl-guc/igt@gem_render_tiled_blits@basic.html
>    [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110495v3/fi-apl-guc/igt@gem_render_tiled_blits@basic.html
> 
>   * igt@i915_pm_rps@basic-api:
>     - bat-adlp-4:         NOTRUN -> [SKIP][4] ([i915#6621])
>    [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110495v3/bat-adlp-4/igt@i915_pm_rps@basic-api.html
> 
>   * igt@kms_chamelium@common-hpd-after-suspend:
>     - fi-hsw-4770:        NOTRUN -> [SKIP][5] ([fdo#109271] / [fdo#111827])
>    [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110495v3/fi-hsw-4770/igt@kms_chamelium@common-hpd-after-suspend.html
>     - bat-adlp-4:         NOTRUN -> [SKIP][6] ([fdo#111827])
>    [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110495v3/bat-adlp-4/igt@kms_chamelium@common-hpd-after-suspend.html
> 
>   * igt@kms_pipe_crc_basic@suspend-read-crc:
>     - bat-adlp-4:         NOTRUN -> [SKIP][7] ([i915#3546])
>    [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110495v3/bat-adlp-4/igt@kms_pipe_crc_basic@suspend-read-crc.html
> 
>   * igt@prime_vgem@basic-userptr:
>     - bat-adlp-4:         NOTRUN -> [SKIP][8] ([fdo#109295] / [i915#3301] / [i915#3708])
>    [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110495v3/bat-adlp-4/igt@prime_vgem@basic-userptr.html
> 
>   * igt@prime_vgem@basic-write:
>     - bat-adlp-4:         NOTRUN -> [SKIP][9] ([fdo#109295] / [i915#3291] / [i915#3708]) +2 similar issues
>    [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110495v3/bat-adlp-4/igt@prime_vgem@basic-write.html
> 
>   
> #### Possible fixes ####
> 
>   * igt@i915_module_load@reload:
>     - {bat-rpls-2}:       [INCOMPLETE][10] ([i915#6434]) -> [PASS][11]
>    [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/bat-rpls-2/igt@i915_module_load@reload.html
>    [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110495v3/bat-rpls-2/igt@i915_module_load@reload.html
> 
>   * igt@i915_pm_rpm@basic-pci-d3-state:
>     - bat-adlp-4:         [DMESG-WARN][12] ([i915#7077]) -> [PASS][13]
>    [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/bat-adlp-4/igt@i915_pm_rpm@basic-pci-d3-state.html
>    [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110495v3/bat-adlp-4/igt@i915_pm_rpm@basic-pci-d3-state.html
> 
>   * igt@i915_selftest@live@hangcheck:
>     - fi-hsw-4770:        [INCOMPLETE][14] ([i915#4785]) -> [PASS][15]
>    [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html
>    [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110495v3/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html
> 
>   * igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-2:
>     - {bat-dg2-11}:       [FAIL][16] -> [PASS][17]
>    [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12390/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-2.html
>    [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110495v3/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-2.html
> 
>   
>   {name}: This element is suppressed. This means it is ignored when computing
>           the status of the difference (SUCCESS, WARNING, or FAILURE).
> 
>   [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
>   [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
>   [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
>   [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
>   [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
>   [i915#1836]: https://gitlab.freedesktop.org/drm/intel/issues/1836
>   [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
>   [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
>   [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
>   [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
>   [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
>   [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
>   [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
>   [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
>   [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
>   [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
>   [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
>   [i915#4258]: https://gitlab.freedesktop.org/drm/intel/issues/4258
>   [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
>   [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
>   [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
>   [i915#4785]: https://gitlab.freedesktop.org/drm/intel/issues/4785
>   [i915#6077]: https://gitlab.freedesktop.org/drm/intel/issues/6077
>   [i915#6078]: https://gitlab.freedesktop.org/drm/intel/issues/6078
>   [i915#6093]: https://gitlab.freedesktop.org/drm/intel/issues/6093
>   [i915#6094]: https://gitlab.freedesktop.org/drm/intel/issues/6094
>   [i915#6106]: https://gitlab.freedesktop.org/drm/intel/issues/6106
>   [i915#6166]: https://gitlab.freedesktop.org/drm/intel/issues/6166
>   [i915#6434]: https://gitlab.freedesktop.org/drm/intel/issues/6434
>   [i915#6559]: https://gitlab.freedesktop.org/drm/intel/issues/6559
>   [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
>   [i915#6997]: https://gitlab.freedesktop.org/drm/intel/issues/6997
>   [i915#7056]: https://gitlab.freedesktop.org/drm/intel/issues/7056
>   [i915#7077]: https://gitlab.freedesktop.org/drm/intel/issues/7077
>   [i915#7355]: https://gitlab.freedesktop.org/drm/intel/issues/7355
>   [i915#7357]: https://gitlab.freedesktop.org/drm/intel/issues/7357
>   [i915#7358]: https://gitlab.freedesktop.org/drm/intel/issues/7358
>   [i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456
>   [i915#7535]: https://gitlab.freedesktop.org/drm/intel/issues/7535
> 
> 
> Build changes
> -------------
> 
>   * Linux: CI_DRM_12390 -> Patchwork_110495v3
> 
>   CI-20190529: 20190529
>   CI_DRM_12390: b7288a4715c68710aadbd63112b699356e8a2b65 @ git://anongit.freedesktop.org/gfx-ci/linux
>   IGT_7062: 6539ea5fe17fce683133c45f07fac316593ee1f7 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
>   Patchwork_110495v3: b7288a4715c68710aadbd63112b699356e8a2b65 @ git://anongit.freedesktop.org/gfx-ci/linux
> 
> 
> ### Linux commits
> 
> f870a1aab343 mei: bus-fixup: change pxp mode only if message was sent
> 5bb5237469d7 mei: add timeout to send
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110495v3/index.html
Rodrigo Vivi Nov. 30, 2022, 2:20 p.m. UTC | #2
On Wed, Nov 16, 2022 at 02:47:33PM +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.
> 
> 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.
> 
> 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
> 
> V3: - split the state fix into separate patch
>     - document define unit
>     - expand timeout KDoc

These 2 patches looks good to me now.

Greg, whenever you review it, please let me know if it is
okay to me to push these through the drm-fixes, or if you
prefer these to the mei branches.

Btw, Alexander, would we have a good "Fixes:" tag for these
patches?

Thanks,
Rodrigo.

> 
> Alexander Usyskin (2):
>   mei: add timeout to send
>   mei: bus-fixup: change pxp mode only if message was sent
> 
>  drivers/misc/mei/bus-fixup.c | 14 +++++++++-----
>  drivers/misc/mei/bus.c       | 22 +++++++++++++++++++++-
>  drivers/misc/mei/client.c    | 20 ++++++++++++++++----
>  drivers/misc/mei/client.h    |  2 +-
>  drivers/misc/mei/main.c      |  2 +-
>  drivers/misc/mei/mei_dev.h   |  2 ++
>  6 files changed, 50 insertions(+), 12 deletions(-)
> 
> -- 
> 2.34.1
>
Greg Kroah-Hartman Nov. 30, 2022, 5:43 p.m. UTC | #3
On Wed, Nov 30, 2022 at 09:20:28AM -0500, Rodrigo Vivi wrote:
> On Wed, Nov 16, 2022 at 02:47:33PM +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.
> > 
> > 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.
> > 
> > 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
> > 
> > V3: - split the state fix into separate patch
> >     - document define unit
> >     - expand timeout KDoc
> 
> These 2 patches looks good to me now.
> 
> Greg, whenever you review it, please let me know if it is
> okay to me to push these through the drm-fixes, or if you
> prefer these to the mei branches.

These have been in my tree for over a week now, sorry.  No one told me
not to take them...

{sigh}

greg k-h
Rodrigo Vivi Nov. 30, 2022, 5:48 p.m. UTC | #4
On Wed, 2022-11-30 at 18:43 +0100, Greg Kroah-Hartman wrote:
> On Wed, Nov 30, 2022 at 09:20:28AM -0500, Rodrigo Vivi wrote:
> > On Wed, Nov 16, 2022 at 02:47:33PM +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.
> > > 
> > > 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.
> > > 
> > > 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
> > > 
> > > V3: - split the state fix into separate patch
> > >     - document define unit
> > >     - expand timeout KDoc
> > 
> > These 2 patches looks good to me now.
> > 
> > Greg, whenever you review it, please let me know if it is
> > okay to me to push these through the drm-fixes, or if you
> > prefer these to the mei branches.
> 
> These have been in my tree for over a week now, sorry.  No one told
> me
> not to take them...
> 
> {sigh}

no worries at all. The important thing is that the users will get the
fix.
We can keep them in our topic/core-for-CI while our branches are out-
of-sync.

Thanks a lot,
Rodrigo.

> 
> greg k-h