All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5 0/6] drm/i915/pxp: Add missing cleanup steps for PXP global-teardown
@ 2023-01-13  1:18 ` Alan Previn
  0 siblings, 0 replies; 37+ messages in thread
From: Alan Previn @ 2023-01-13  1:18 UTC (permalink / raw)
  To: intel-gfx
  Cc: Alan Previn, Vivi, Greg Kroah-Hartman, Rodrigo,
	Alexander Usyskin, dri-devel, Daniele Ceraolo Spurio, Juston Li,
	Tomas Winkler

A customer issue was recently discovered and in the process a
gap in i915's PXP interaction with HW+FW architecure was also
realized. This series adds those missing pieces.

This fix includes changes where i915 calls into the mei
component interface in order to submit requests to the security
firmware during the i915's suspend_prepare flow. This change did
expose a blocking issue in the mei component side that was
discovered while testing in rev1. The issue being the mei-pxp
component driver not being able to runtime-resume while being
within the suspend_prepare callstack.


Thus, we have now included the mei patches (from Alexander) that
fixes that issue by adding a device-link based on the interface
type to ensure mei side runtime resume during the i915's
suspend_prepare call.

That said, as per request from Alexander, we seek Greg's and Tomas'
review for the mei patches (Patch 1, 2 and 3). Patch 2, although is
a change in the i915 code, is the mei component device link change.

The individual patches explain more details. Patch 7 can be ignored
as it won't be merged and is only meant to ensure the CI run's
the PXP subtests with PXP support enabled in KConfig.

Changes from prior revs:
   v1: - Dont need to teardown non-arbitration sessions (Juston).
       - Fix builds when PXP is enabled in config (Alan/CI-build).
       - Fix the broken pm-suspend-resume symmetry when we do this
         pxp-session-teardown during i915s pm_suspend_prepare by
         ensuring the init is done during i915s pm_resume_complete.
   v2: - Rebase on latest drm-tip after PXP subsytem was promoted
         to global.
       - Remove "INTEL_PXP_MAX_HWDRM_SESSIONS" unneeded (Juston Li).
       - Added mei patches that are dependencies for this series
         to successfully pass testing when PXP config is enabled.
   v3: - Added fix for mei patch when CONFIG_PM_SLEEP is off (reported
         by kernel test robot <lkp@intel.com>).
   v4: - Added "DRM_SWITCH_POWER_OFF" check and removed bail-out if
         '!i915' that wont happen in i915_pm_complete (Daniele).
       - move i915_pm_complete to appear in i915_pm_resume.
       - One more fix for mei patch when CONFIG_PM_SLEEP is off
         (reported by kernel test robot <lkp@intel.com>).

Alan Previn (3):
  drm/i915/pxp: Invalidate all PXP fw sessions during teardown
  drm/i915/pxp: Trigger the global teardown for before suspending
  drm/i915/pxp: Pxp hw init should be in resume_complete

Alexander Usyskin (3):
  mei: mei-me: resume device in prepare
  drm/i915/pxp: add device link between i915 and mei_pxp
  mei: clean pending read with vtag on bus

 drivers/gpu/drm/i915/gt/intel_gt_pm.h         |  1 +
 drivers/gpu/drm/i915/i915_driver.c            | 20 ++++++-
 drivers/gpu/drm/i915/pxp/intel_pxp.c          | 60 ++++++++++++++++---
 drivers/gpu/drm/i915/pxp/intel_pxp.h          |  2 +
 .../drm/i915/pxp/intel_pxp_cmd_interface_42.h | 15 +++++
 .../i915/pxp/intel_pxp_cmd_interface_cmn.h    |  3 +
 drivers/gpu/drm/i915/pxp/intel_pxp_pm.c       |  4 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_pm.h       |  6 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_session.c  | 11 +++-
 drivers/gpu/drm/i915/pxp/intel_pxp_session.h  |  5 ++
 drivers/gpu/drm/i915/pxp/intel_pxp_tee.c      | 42 +++++++++++++
 drivers/misc/mei/client.c                     |  4 +-
 drivers/misc/mei/pci-me.c                     | 20 ++++++-
 13 files changed, 172 insertions(+), 21 deletions(-)


base-commit: c0ba6b6312e9139ce4b89da2904b329c13b5e94d
-- 
2.39.0


^ permalink raw reply	[flat|nested] 37+ messages in thread
* [Intel-gfx] [PATCH v5 0/6] drm/i915/pxp: Add missing cleanup steps for PXP global-teardown
@ 2023-01-13  1:18 ` Alan Previn
  0 siblings, 0 replies; 37+ messages in thread
From: Alan Previn @ 2023-01-13  1:18 UTC (permalink / raw)
  To: intel-gfx
  Cc: Alan Previn, Vivi, Greg Kroah-Hartman, Rodrigo,
	Alexander Usyskin, dri-devel, Tomas Winkler

A customer issue was recently discovered and in the process a
gap in i915's PXP interaction with HW+FW architecure was also
realized. This series adds those missing pieces.

This fix includes changes where i915 calls into the mei
component interface in order to submit requests to the security
firmware during the i915's suspend_prepare flow. This change did
expose a blocking issue in the mei component side that was
discovered while testing in rev1. The issue being the mei-pxp
component driver not being able to runtime-resume while being
within the suspend_prepare callstack.


Thus, we have now included the mei patches (from Alexander) that
fixes that issue by adding a device-link based on the interface
type to ensure mei side runtime resume during the i915's
suspend_prepare call.

That said, as per request from Alexander, we seek Greg's and Tomas'
review for the mei patches (Patch 1, 2 and 3). Patch 2, although is
a change in the i915 code, is the mei component device link change.

The individual patches explain more details. Patch 7 can be ignored
as it won't be merged and is only meant to ensure the CI run's
the PXP subtests with PXP support enabled in KConfig.

Changes from prior revs:
   v1: - Dont need to teardown non-arbitration sessions (Juston).
       - Fix builds when PXP is enabled in config (Alan/CI-build).
       - Fix the broken pm-suspend-resume symmetry when we do this
         pxp-session-teardown during i915s pm_suspend_prepare by
         ensuring the init is done during i915s pm_resume_complete.
   v2: - Rebase on latest drm-tip after PXP subsytem was promoted
         to global.
       - Remove "INTEL_PXP_MAX_HWDRM_SESSIONS" unneeded (Juston Li).
       - Added mei patches that are dependencies for this series
         to successfully pass testing when PXP config is enabled.
   v3: - Added fix for mei patch when CONFIG_PM_SLEEP is off (reported
         by kernel test robot <lkp@intel.com>).
   v4: - Added "DRM_SWITCH_POWER_OFF" check and removed bail-out if
         '!i915' that wont happen in i915_pm_complete (Daniele).
       - move i915_pm_complete to appear in i915_pm_resume.
       - One more fix for mei patch when CONFIG_PM_SLEEP is off
         (reported by kernel test robot <lkp@intel.com>).

Alan Previn (3):
  drm/i915/pxp: Invalidate all PXP fw sessions during teardown
  drm/i915/pxp: Trigger the global teardown for before suspending
  drm/i915/pxp: Pxp hw init should be in resume_complete

Alexander Usyskin (3):
  mei: mei-me: resume device in prepare
  drm/i915/pxp: add device link between i915 and mei_pxp
  mei: clean pending read with vtag on bus

 drivers/gpu/drm/i915/gt/intel_gt_pm.h         |  1 +
 drivers/gpu/drm/i915/i915_driver.c            | 20 ++++++-
 drivers/gpu/drm/i915/pxp/intel_pxp.c          | 60 ++++++++++++++++---
 drivers/gpu/drm/i915/pxp/intel_pxp.h          |  2 +
 .../drm/i915/pxp/intel_pxp_cmd_interface_42.h | 15 +++++
 .../i915/pxp/intel_pxp_cmd_interface_cmn.h    |  3 +
 drivers/gpu/drm/i915/pxp/intel_pxp_pm.c       |  4 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_pm.h       |  6 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_session.c  | 11 +++-
 drivers/gpu/drm/i915/pxp/intel_pxp_session.h  |  5 ++
 drivers/gpu/drm/i915/pxp/intel_pxp_tee.c      | 42 +++++++++++++
 drivers/misc/mei/client.c                     |  4 +-
 drivers/misc/mei/pci-me.c                     | 20 ++++++-
 13 files changed, 172 insertions(+), 21 deletions(-)


base-commit: c0ba6b6312e9139ce4b89da2904b329c13b5e94d
-- 
2.39.0


^ permalink raw reply	[flat|nested] 37+ messages in thread
* [PATCH v5 1/6] mei: mei-me: resume device in prepare
  2023-01-13  1:18 ` [Intel-gfx] " Alan Previn
@ 2023-01-13  1:18   ` Alan Previn
  -1 siblings, 0 replies; 37+ messages in thread
From: Alan Previn @ 2023-01-13  1:18 UTC (permalink / raw)
  To: intel-gfx
  Cc: Alan Previn, Vivi, Greg Kroah-Hartman, Rodrigo,
	Alexander Usyskin, dri-devel, Daniele Ceraolo Spurio, Juston Li,
	Tomas Winkler

From: Alexander Usyskin <alexander.usyskin@intel.com>

Asynchronous runtime resume is not possible while the system
is suspending.
The power management subsystem resumes the device only in the
suspend phase, not in the prepare phase.
Force resume device in prepare to allow drivers on mei bus
to communicate in their prepare callbacks.

Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/misc/mei/pci-me.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/mei/pci-me.c b/drivers/misc/mei/pci-me.c
index 704cd0caa172..9f6ff06a94fd 100644
--- a/drivers/misc/mei/pci-me.c
+++ b/drivers/misc/mei/pci-me.c
@@ -340,6 +340,12 @@ static void mei_me_remove(struct pci_dev *pdev)
 }
 
 #ifdef CONFIG_PM_SLEEP
+static int mei_me_pci_prepare(struct device *device)
+{
+	pm_runtime_resume(device);
+	return 0;
+}
+
 static int mei_me_pci_suspend(struct device *device)
 {
 	struct pci_dev *pdev = to_pci_dev(device);
@@ -396,7 +402,17 @@ static int mei_me_pci_resume(struct device *device)
 
 	return 0;
 }
-#endif /* CONFIG_PM_SLEEP */
+
+static void mei_me_pci_complete(struct device *device)
+{
+	pm_runtime_suspend(device);
+}
+#else /* CONFIG_PM_SLEEP */
+
+#define mei_me_pci_prepare NULL
+#define mei_me_pci_complete NULL
+
+#endif /* !CONFIG_PM_SLEEP */
 
 #ifdef CONFIG_PM
 static int mei_me_pm_runtime_idle(struct device *device)
@@ -499,6 +515,8 @@ static inline void mei_me_unset_pm_domain(struct mei_device *dev)
 }
 
 static const struct dev_pm_ops mei_me_pm_ops = {
+	.prepare = mei_me_pci_prepare,
+	.complete = mei_me_pci_complete,
 	SET_SYSTEM_SLEEP_PM_OPS(mei_me_pci_suspend,
 				mei_me_pci_resume)
 	SET_RUNTIME_PM_OPS(
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread
* [Intel-gfx] [PATCH v5 1/6] mei: mei-me: resume device in prepare
@ 2023-01-13  1:18   ` Alan Previn
  0 siblings, 0 replies; 37+ messages in thread
From: Alan Previn @ 2023-01-13  1:18 UTC (permalink / raw)
  To: intel-gfx
  Cc: Alan Previn, Vivi, Greg Kroah-Hartman, Rodrigo,
	Alexander Usyskin, dri-devel, Tomas Winkler

From: Alexander Usyskin <alexander.usyskin@intel.com>

Asynchronous runtime resume is not possible while the system
is suspending.
The power management subsystem resumes the device only in the
suspend phase, not in the prepare phase.
Force resume device in prepare to allow drivers on mei bus
to communicate in their prepare callbacks.

Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/misc/mei/pci-me.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/mei/pci-me.c b/drivers/misc/mei/pci-me.c
index 704cd0caa172..9f6ff06a94fd 100644
--- a/drivers/misc/mei/pci-me.c
+++ b/drivers/misc/mei/pci-me.c
@@ -340,6 +340,12 @@ static void mei_me_remove(struct pci_dev *pdev)
 }
 
 #ifdef CONFIG_PM_SLEEP
+static int mei_me_pci_prepare(struct device *device)
+{
+	pm_runtime_resume(device);
+	return 0;
+}
+
 static int mei_me_pci_suspend(struct device *device)
 {
 	struct pci_dev *pdev = to_pci_dev(device);
@@ -396,7 +402,17 @@ static int mei_me_pci_resume(struct device *device)
 
 	return 0;
 }
-#endif /* CONFIG_PM_SLEEP */
+
+static void mei_me_pci_complete(struct device *device)
+{
+	pm_runtime_suspend(device);
+}
+#else /* CONFIG_PM_SLEEP */
+
+#define mei_me_pci_prepare NULL
+#define mei_me_pci_complete NULL
+
+#endif /* !CONFIG_PM_SLEEP */
 
 #ifdef CONFIG_PM
 static int mei_me_pm_runtime_idle(struct device *device)
@@ -499,6 +515,8 @@ static inline void mei_me_unset_pm_domain(struct mei_device *dev)
 }
 
 static const struct dev_pm_ops mei_me_pm_ops = {
+	.prepare = mei_me_pci_prepare,
+	.complete = mei_me_pci_complete,
 	SET_SYSTEM_SLEEP_PM_OPS(mei_me_pci_suspend,
 				mei_me_pci_resume)
 	SET_RUNTIME_PM_OPS(
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread
* [PATCH v5 2/6] drm/i915/pxp: add device link between i915 and mei_pxp
  2023-01-13  1:18 ` [Intel-gfx] " Alan Previn
@ 2023-01-13  1:18   ` Alan Previn
  -1 siblings, 0 replies; 37+ messages in thread
From: Alan Previn @ 2023-01-13  1:18 UTC (permalink / raw)
  To: intel-gfx
  Cc: Alan Previn, Vivi, Greg Kroah-Hartman, Rodrigo,
	Alexander Usyskin, dri-devel, Daniele Ceraolo Spurio, Juston Li,
	Tomas Winkler

From: Alexander Usyskin <alexander.usyskin@intel.com>

Add device link with i915 as consumer and mei_pxp as supplier
to ensure proper ordering of power flows.

V2: condition on absence of heci_pxp to filter out DG

Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
index d50354bfb993..bef6d7f8ac55 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
@@ -127,6 +127,10 @@ static int i915_pxp_tee_component_bind(struct device *i915_kdev,
 	intel_wakeref_t wakeref;
 	int ret = 0;
 
+	if (!HAS_HECI_PXP(i915) &&
+	    drm_WARN_ON(&i915->drm, !device_link_add(i915_kdev, tee_kdev, DL_FLAG_STATELESS)))
+		return -ENOMEM;
+
 	mutex_lock(&pxp->tee_mutex);
 	pxp->pxp_component = data;
 	pxp->pxp_component->tee_dev = tee_kdev;
@@ -169,6 +173,9 @@ static void i915_pxp_tee_component_unbind(struct device *i915_kdev,
 	mutex_lock(&pxp->tee_mutex);
 	pxp->pxp_component = NULL;
 	mutex_unlock(&pxp->tee_mutex);
+
+	if (!HAS_HECI_PXP(i915))
+		device_link_remove(i915_kdev, tee_kdev);
 }
 
 static const struct component_ops i915_pxp_tee_component_ops = {
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread
* [Intel-gfx] [PATCH v5 2/6] drm/i915/pxp: add device link between i915 and mei_pxp
@ 2023-01-13  1:18   ` Alan Previn
  0 siblings, 0 replies; 37+ messages in thread
From: Alan Previn @ 2023-01-13  1:18 UTC (permalink / raw)
  To: intel-gfx
  Cc: Alan Previn, Vivi, Greg Kroah-Hartman, Rodrigo,
	Alexander Usyskin, dri-devel, Tomas Winkler

From: Alexander Usyskin <alexander.usyskin@intel.com>

Add device link with i915 as consumer and mei_pxp as supplier
to ensure proper ordering of power flows.

V2: condition on absence of heci_pxp to filter out DG

Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
index d50354bfb993..bef6d7f8ac55 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
@@ -127,6 +127,10 @@ static int i915_pxp_tee_component_bind(struct device *i915_kdev,
 	intel_wakeref_t wakeref;
 	int ret = 0;
 
+	if (!HAS_HECI_PXP(i915) &&
+	    drm_WARN_ON(&i915->drm, !device_link_add(i915_kdev, tee_kdev, DL_FLAG_STATELESS)))
+		return -ENOMEM;
+
 	mutex_lock(&pxp->tee_mutex);
 	pxp->pxp_component = data;
 	pxp->pxp_component->tee_dev = tee_kdev;
@@ -169,6 +173,9 @@ static void i915_pxp_tee_component_unbind(struct device *i915_kdev,
 	mutex_lock(&pxp->tee_mutex);
 	pxp->pxp_component = NULL;
 	mutex_unlock(&pxp->tee_mutex);
+
+	if (!HAS_HECI_PXP(i915))
+		device_link_remove(i915_kdev, tee_kdev);
 }
 
 static const struct component_ops i915_pxp_tee_component_ops = {
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread
* [PATCH v5 3/6] mei: clean pending read with vtag on bus
  2023-01-13  1:18 ` [Intel-gfx] " Alan Previn
@ 2023-01-13  1:18   ` Alan Previn
  -1 siblings, 0 replies; 37+ messages in thread
From: Alan Previn @ 2023-01-13  1:18 UTC (permalink / raw)
  To: intel-gfx
  Cc: Alan Previn, Vivi, Greg Kroah-Hartman, Rodrigo,
	Alexander Usyskin, dri-devel, Daniele Ceraolo Spurio, Juston Li,
	Tomas Winkler

From: Alexander Usyskin <alexander.usyskin@intel.com>

Client on bus have only one vtag map slot and should disregard the vtag
value when cleaning pending read flag.
Fixes read flow control message unexpectedly generated when
clent on bus send messages with different vtags.

Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/misc/mei/client.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
index 9ddb854b8155..5c19097266fe 100644
--- a/drivers/misc/mei/client.c
+++ b/drivers/misc/mei/client.c
@@ -1343,7 +1343,9 @@ static void mei_cl_reset_read_by_vtag(const struct mei_cl *cl, u8 vtag)
 	struct mei_cl_vtag *vtag_l;
 
 	list_for_each_entry(vtag_l, &cl->vtag_map, list) {
-		if (vtag_l->vtag == vtag) {
+		/* The client on bus has one fixed vtag map */
+		if ((cl->cldev && mei_cldev_enabled(cl->cldev)) ||
+		    vtag_l->vtag == vtag) {
 			vtag_l->pending_read = false;
 			break;
 		}
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread
* [Intel-gfx] [PATCH v5 3/6] mei: clean pending read with vtag on bus
@ 2023-01-13  1:18   ` Alan Previn
  0 siblings, 0 replies; 37+ messages in thread
From: Alan Previn @ 2023-01-13  1:18 UTC (permalink / raw)
  To: intel-gfx
  Cc: Alan Previn, Vivi, Greg Kroah-Hartman, Rodrigo,
	Alexander Usyskin, dri-devel, Tomas Winkler

From: Alexander Usyskin <alexander.usyskin@intel.com>

Client on bus have only one vtag map slot and should disregard the vtag
value when cleaning pending read flag.
Fixes read flow control message unexpectedly generated when
clent on bus send messages with different vtags.

Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/misc/mei/client.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
index 9ddb854b8155..5c19097266fe 100644
--- a/drivers/misc/mei/client.c
+++ b/drivers/misc/mei/client.c
@@ -1343,7 +1343,9 @@ static void mei_cl_reset_read_by_vtag(const struct mei_cl *cl, u8 vtag)
 	struct mei_cl_vtag *vtag_l;
 
 	list_for_each_entry(vtag_l, &cl->vtag_map, list) {
-		if (vtag_l->vtag == vtag) {
+		/* The client on bus has one fixed vtag map */
+		if ((cl->cldev && mei_cldev_enabled(cl->cldev)) ||
+		    vtag_l->vtag == vtag) {
 			vtag_l->pending_read = false;
 			break;
 		}
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread
* [PATCH v5 4/6] drm/i915/pxp: Invalidate all PXP fw sessions during teardown
  2023-01-13  1:18 ` [Intel-gfx] " Alan Previn
@ 2023-01-13  1:18   ` Alan Previn
  -1 siblings, 0 replies; 37+ messages in thread
From: Alan Previn @ 2023-01-13  1:18 UTC (permalink / raw)
  To: intel-gfx
  Cc: Alan Previn, Vivi, Greg Kroah-Hartman, Rodrigo,
	Alexander Usyskin, dri-devel, Daniele Ceraolo Spurio, Juston Li,
	Tomas Winkler

A gap was recently discovered where if an application did not
invalidate all of the stream keys (intentionally or not), and the
driver did a full PXP global teardown on the GT subsystem, we
find that future session creation would fail on the security
firmware's side of the equation. i915 is the entity that needs
ensure the sessions' state across both iGT and security firmware
are at a known clean point when performing a full global teardown.

Architecturally speaking, i915 should inspect all active sessions
and submit the invalidate-stream-key PXP command to the security
firmware for each of them. However, for the upstream i915 driver
we only support the arbitration session that can be created
so that will be the only session we will cleanup.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Reviewed-by: Juston Li <justonli@chromium.org>
---
 drivers/gpu/drm/i915/pxp/intel_pxp.h          |  1 +
 .../drm/i915/pxp/intel_pxp_cmd_interface_42.h | 15 ++++++++
 .../i915/pxp/intel_pxp_cmd_interface_cmn.h    |  3 ++
 drivers/gpu/drm/i915/pxp/intel_pxp_session.c  |  2 ++
 drivers/gpu/drm/i915/pxp/intel_pxp_tee.c      | 35 +++++++++++++++++++
 5 files changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index 04440fada711..9658d3005222 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -24,6 +24,7 @@ void intel_pxp_init_hw(struct intel_pxp *pxp);
 void intel_pxp_fini_hw(struct intel_pxp *pxp);
 
 void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
+void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id);
 
 int intel_pxp_start(struct intel_pxp *pxp);
 
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
index 739f9072fa5f..26f7d9f01bf3 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
@@ -12,6 +12,9 @@
 /* PXP-Opcode for Init Session */
 #define PXP42_CMDID_INIT_SESSION 0x1e
 
+/* PXP-Opcode for Invalidate Stream Key */
+#define PXP42_CMDID_INVALIDATE_STREAM_KEY 0x00000007
+
 /* PXP-Input-Packet: Init Session (Arb-Session) */
 struct pxp42_create_arb_in {
 	struct pxp_cmd_header header;
@@ -25,4 +28,16 @@ struct pxp42_create_arb_out {
 	struct pxp_cmd_header header;
 } __packed;
 
+/* PXP-Input-Packet: Invalidate Stream Key */
+struct pxp42_inv_stream_key_in {
+	struct pxp_cmd_header header;
+	u32 rsvd[3];
+} __packed;
+
+/* PXP-Output-Packet: Invalidate Stream Key */
+struct pxp42_inv_stream_key_out {
+	struct pxp_cmd_header header;
+	u32 rsvd;
+} __packed;
+
 #endif /* __INTEL_PXP_FW_INTERFACE_42_H__ */
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
index c2f23394f9b8..69e34ec49e78 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
@@ -27,6 +27,9 @@ struct pxp_cmd_header {
 	union {
 		u32 status; /* out */
 		u32 stream_id; /* in */
+#define PXP_CMDHDR_EXTDATA_SESSION_VALID GENMASK(0, 0)
+#define PXP_CMDHDR_EXTDATA_APP_TYPE GENMASK(1, 1)
+#define PXP_CMDHDR_EXTDATA_SESSION_ID GENMASK(17, 2)
 	};
 	/* Length of the message (excluding the header) */
 	u32 buffer_len;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
index ae413580b81a..74ed7e16e481 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
@@ -110,6 +110,8 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
 
 	intel_uncore_write(gt->uncore, PXP_GLOBAL_TERMINATE, 1);
 
+	intel_pxp_tee_end_arb_fw_session(pxp, ARB_SESSION);
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
index bef6d7f8ac55..9e247f38f3bd 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
@@ -311,3 +311,38 @@ int intel_pxp_tee_cmd_create_arb_session(struct intel_pxp *pxp,
 
 	return ret;
 }
+
+void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 session_id)
+{
+	struct drm_i915_private *i915 = pxp->ctrl_gt->i915;
+	struct pxp42_inv_stream_key_in msg_in = {0};
+	struct pxp42_inv_stream_key_out msg_out = {0};
+	int ret, trials = 0;
+
+try_again:
+	memset(&msg_in, 0, sizeof(msg_in));
+	memset(&msg_out, 0, sizeof(msg_out));
+	msg_in.header.api_version = PXP_APIVER(4, 2);
+	msg_in.header.command_id = PXP42_CMDID_INVALIDATE_STREAM_KEY;
+	msg_in.header.buffer_len = sizeof(msg_in) - sizeof(msg_in.header);
+
+	msg_in.header.stream_id = FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_VALID, 1);
+	msg_in.header.stream_id |= FIELD_PREP(PXP_CMDHDR_EXTDATA_APP_TYPE, 0);
+	msg_in.header.stream_id |= FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_ID, session_id);
+
+	ret = intel_pxp_tee_io_message(pxp,
+				       &msg_in, sizeof(msg_in),
+				       &msg_out, sizeof(msg_out),
+				       NULL);
+
+	/* Cleanup coherency between GT and Firmware is critical, so try again if it fails */
+	if ((ret || msg_out.header.status != 0x0) && ++trials < 3)
+		goto try_again;
+
+	if (ret)
+		drm_err(&i915->drm, "Failed to send tee msg for inv-stream-key-%d, ret=[%d]\n",
+			session_id, ret);
+	else if (msg_out.header.status != 0x0)
+		drm_warn(&i915->drm, "PXP firmware failed inv-stream-key-%d with status 0x%08x\n",
+			 session_id, msg_out.header.status);
+}
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread
* [Intel-gfx] [PATCH v5 4/6] drm/i915/pxp: Invalidate all PXP fw sessions during teardown
@ 2023-01-13  1:18   ` Alan Previn
  0 siblings, 0 replies; 37+ messages in thread
From: Alan Previn @ 2023-01-13  1:18 UTC (permalink / raw)
  To: intel-gfx
  Cc: Alan Previn, Vivi, Greg Kroah-Hartman, Rodrigo,
	Alexander Usyskin, dri-devel, Tomas Winkler

A gap was recently discovered where if an application did not
invalidate all of the stream keys (intentionally or not), and the
driver did a full PXP global teardown on the GT subsystem, we
find that future session creation would fail on the security
firmware's side of the equation. i915 is the entity that needs
ensure the sessions' state across both iGT and security firmware
are at a known clean point when performing a full global teardown.

Architecturally speaking, i915 should inspect all active sessions
and submit the invalidate-stream-key PXP command to the security
firmware for each of them. However, for the upstream i915 driver
we only support the arbitration session that can be created
so that will be the only session we will cleanup.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Reviewed-by: Juston Li <justonli@chromium.org>
---
 drivers/gpu/drm/i915/pxp/intel_pxp.h          |  1 +
 .../drm/i915/pxp/intel_pxp_cmd_interface_42.h | 15 ++++++++
 .../i915/pxp/intel_pxp_cmd_interface_cmn.h    |  3 ++
 drivers/gpu/drm/i915/pxp/intel_pxp_session.c  |  2 ++
 drivers/gpu/drm/i915/pxp/intel_pxp_tee.c      | 35 +++++++++++++++++++
 5 files changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index 04440fada711..9658d3005222 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -24,6 +24,7 @@ void intel_pxp_init_hw(struct intel_pxp *pxp);
 void intel_pxp_fini_hw(struct intel_pxp *pxp);
 
 void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
+void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id);
 
 int intel_pxp_start(struct intel_pxp *pxp);
 
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
index 739f9072fa5f..26f7d9f01bf3 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
@@ -12,6 +12,9 @@
 /* PXP-Opcode for Init Session */
 #define PXP42_CMDID_INIT_SESSION 0x1e
 
+/* PXP-Opcode for Invalidate Stream Key */
+#define PXP42_CMDID_INVALIDATE_STREAM_KEY 0x00000007
+
 /* PXP-Input-Packet: Init Session (Arb-Session) */
 struct pxp42_create_arb_in {
 	struct pxp_cmd_header header;
@@ -25,4 +28,16 @@ struct pxp42_create_arb_out {
 	struct pxp_cmd_header header;
 } __packed;
 
+/* PXP-Input-Packet: Invalidate Stream Key */
+struct pxp42_inv_stream_key_in {
+	struct pxp_cmd_header header;
+	u32 rsvd[3];
+} __packed;
+
+/* PXP-Output-Packet: Invalidate Stream Key */
+struct pxp42_inv_stream_key_out {
+	struct pxp_cmd_header header;
+	u32 rsvd;
+} __packed;
+
 #endif /* __INTEL_PXP_FW_INTERFACE_42_H__ */
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
index c2f23394f9b8..69e34ec49e78 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
@@ -27,6 +27,9 @@ struct pxp_cmd_header {
 	union {
 		u32 status; /* out */
 		u32 stream_id; /* in */
+#define PXP_CMDHDR_EXTDATA_SESSION_VALID GENMASK(0, 0)
+#define PXP_CMDHDR_EXTDATA_APP_TYPE GENMASK(1, 1)
+#define PXP_CMDHDR_EXTDATA_SESSION_ID GENMASK(17, 2)
 	};
 	/* Length of the message (excluding the header) */
 	u32 buffer_len;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
index ae413580b81a..74ed7e16e481 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
@@ -110,6 +110,8 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
 
 	intel_uncore_write(gt->uncore, PXP_GLOBAL_TERMINATE, 1);
 
+	intel_pxp_tee_end_arb_fw_session(pxp, ARB_SESSION);
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
index bef6d7f8ac55..9e247f38f3bd 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
@@ -311,3 +311,38 @@ int intel_pxp_tee_cmd_create_arb_session(struct intel_pxp *pxp,
 
 	return ret;
 }
+
+void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 session_id)
+{
+	struct drm_i915_private *i915 = pxp->ctrl_gt->i915;
+	struct pxp42_inv_stream_key_in msg_in = {0};
+	struct pxp42_inv_stream_key_out msg_out = {0};
+	int ret, trials = 0;
+
+try_again:
+	memset(&msg_in, 0, sizeof(msg_in));
+	memset(&msg_out, 0, sizeof(msg_out));
+	msg_in.header.api_version = PXP_APIVER(4, 2);
+	msg_in.header.command_id = PXP42_CMDID_INVALIDATE_STREAM_KEY;
+	msg_in.header.buffer_len = sizeof(msg_in) - sizeof(msg_in.header);
+
+	msg_in.header.stream_id = FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_VALID, 1);
+	msg_in.header.stream_id |= FIELD_PREP(PXP_CMDHDR_EXTDATA_APP_TYPE, 0);
+	msg_in.header.stream_id |= FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_ID, session_id);
+
+	ret = intel_pxp_tee_io_message(pxp,
+				       &msg_in, sizeof(msg_in),
+				       &msg_out, sizeof(msg_out),
+				       NULL);
+
+	/* Cleanup coherency between GT and Firmware is critical, so try again if it fails */
+	if ((ret || msg_out.header.status != 0x0) && ++trials < 3)
+		goto try_again;
+
+	if (ret)
+		drm_err(&i915->drm, "Failed to send tee msg for inv-stream-key-%d, ret=[%d]\n",
+			session_id, ret);
+	else if (msg_out.header.status != 0x0)
+		drm_warn(&i915->drm, "PXP firmware failed inv-stream-key-%d with status 0x%08x\n",
+			 session_id, msg_out.header.status);
+}
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread
* [PATCH v5 5/6] drm/i915/pxp: Trigger the global teardown for before suspending
  2023-01-13  1:18 ` [Intel-gfx] " Alan Previn
@ 2023-01-13  1:18   ` Alan Previn
  -1 siblings, 0 replies; 37+ messages in thread
From: Alan Previn @ 2023-01-13  1:18 UTC (permalink / raw)
  To: intel-gfx
  Cc: Alan Previn, Vivi, Greg Kroah-Hartman, Rodrigo,
	Alexander Usyskin, dri-devel, Daniele Ceraolo Spurio, Juston Li,
	Tomas Winkler

A driver bug was recently discovered where the security firmware was
receiving internal HW signals indicating that session key expirations
had occurred. Architecturally, the firmware was expecting a response
from the GuC to acknowledge the event with the firmware side.
However the OS was in a suspended state and GuC had been reset.

Internal specifications actually required the driver to ensure
that all active sessions be properly cleaned up in such cases where
the system is suspended and the GuC potentially unable to respond.

This patch adds the global teardown code in i915's suspend_prepare
code path.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Reviewed-by: Juston Li <justonli@chromium.org>
---
 drivers/gpu/drm/i915/pxp/intel_pxp.c         | 60 +++++++++++++++++---
 drivers/gpu/drm/i915/pxp/intel_pxp.h         |  1 +
 drivers/gpu/drm/i915/pxp/intel_pxp_pm.c      |  2 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_session.c |  9 ++-
 drivers/gpu/drm/i915/pxp/intel_pxp_session.h |  5 ++
 5 files changed, 64 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index cfc9af8b3d21..f3d9e7989eb7 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -270,6 +270,55 @@ static bool pxp_component_bound(struct intel_pxp *pxp)
 	return bound;
 }
 
+static int __pxp_global_teardown_locked(struct intel_pxp *pxp, bool terminate_for_cleanup)
+{
+	if (terminate_for_cleanup) {
+		if (!pxp->arb_is_valid)
+			return 0;
+		/*
+		 * To ensure synchronous and coherent session teardown completion
+		 * in response to suspend or shutdown triggers, don't use a worker.
+		 */
+		intel_pxp_mark_termination_in_progress(pxp);
+		intel_pxp_terminate(pxp, false);
+	} else {
+		if (pxp->arb_is_valid)
+			return 0;
+		/*
+		 * If we are not in final termination, and the arb-session is currently
+		 * inactive, we are doing a reset and restart due to some runtime event.
+		 * Use the worker that was designed for this.
+		 */
+		pxp_queue_termination(pxp);
+	}
+
+	if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250)))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+void intel_pxp_end(struct intel_pxp *pxp)
+{
+	struct drm_i915_private *i915 = pxp->ctrl_gt->i915;
+	intel_wakeref_t wakeref;
+
+	if (!intel_pxp_is_enabled(pxp))
+		return;
+
+	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
+
+	mutex_lock(&pxp->arb_mutex);
+
+	if (__pxp_global_teardown_locked(pxp, true))
+		drm_dbg(&i915->drm, "PXP end timed out\n");
+
+	mutex_unlock(&pxp->arb_mutex);
+
+	intel_pxp_fini_hw(pxp);
+	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
+}
+
 /*
  * the arb session is restarted from the irq work when we receive the
  * termination completion interrupt
@@ -286,16 +335,9 @@ int intel_pxp_start(struct intel_pxp *pxp)
 
 	mutex_lock(&pxp->arb_mutex);
 
-	if (pxp->arb_is_valid)
-		goto unlock;
-
-	pxp_queue_termination(pxp);
-
-	if (!wait_for_completion_timeout(&pxp->termination,
-					msecs_to_jiffies(250))) {
-		ret = -ETIMEDOUT;
+	ret = __pxp_global_teardown_locked(pxp, false);
+	if (ret)
 		goto unlock;
-	}
 
 	/* make sure the compiler doesn't optimize the double access */
 	barrier();
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index 9658d3005222..3ded0890cd27 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -27,6 +27,7 @@ void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
 void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id);
 
 int intel_pxp_start(struct intel_pxp *pxp);
+void intel_pxp_end(struct intel_pxp *pxp);
 
 int intel_pxp_key_check(struct intel_pxp *pxp,
 			struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
index 892d39cc61c1..e427464aa131 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
@@ -16,7 +16,7 @@ void intel_pxp_suspend_prepare(struct intel_pxp *pxp)
 	if (!intel_pxp_is_enabled(pxp))
 		return;
 
-	pxp->arb_is_valid = false;
+	intel_pxp_end(pxp);
 
 	intel_pxp_invalidate(pxp);
 }
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
index 74ed7e16e481..d8278c4002e3 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
@@ -115,11 +115,14 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
 	return ret;
 }
 
-static void pxp_terminate(struct intel_pxp *pxp)
+void intel_pxp_terminate(struct intel_pxp *pxp, bool restart_arb)
 {
 	int ret;
 
-	pxp->hw_state_invalidated = true;
+	if (restart_arb)
+		pxp->hw_state_invalidated = true;
+	else
+		pxp->hw_state_invalidated = false;
 
 	/*
 	 * if we fail to submit the termination there is no point in waiting for
@@ -167,7 +170,7 @@ static void pxp_session_work(struct work_struct *work)
 
 	if (events & PXP_TERMINATION_REQUEST) {
 		events &= ~PXP_TERMINATION_COMPLETE;
-		pxp_terminate(pxp);
+		intel_pxp_terminate(pxp, true);
 	}
 
 	if (events & PXP_TERMINATION_COMPLETE)
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
index 903ac52cffa1..4f944b63b5b6 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
@@ -12,9 +12,14 @@ struct intel_pxp;
 
 #ifdef CONFIG_DRM_I915_PXP
 void intel_pxp_session_management_init(struct intel_pxp *pxp);
+void intel_pxp_terminate(struct intel_pxp *pxp, bool restart_arb);
 #else
 static inline void intel_pxp_session_management_init(struct intel_pxp *pxp)
 {
 }
+
+static inline void intel_pxp_terminate(struct intel_pxp *pxp, bool restart_arb)
+{
+}
 #endif
 #endif /* __INTEL_PXP_SESSION_H__ */
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread
* [Intel-gfx] [PATCH v5 5/6] drm/i915/pxp: Trigger the global teardown for before suspending
@ 2023-01-13  1:18   ` Alan Previn
  0 siblings, 0 replies; 37+ messages in thread
From: Alan Previn @ 2023-01-13  1:18 UTC (permalink / raw)
  To: intel-gfx
  Cc: Alan Previn, Vivi, Greg Kroah-Hartman, Rodrigo,
	Alexander Usyskin, dri-devel, Tomas Winkler

A driver bug was recently discovered where the security firmware was
receiving internal HW signals indicating that session key expirations
had occurred. Architecturally, the firmware was expecting a response
from the GuC to acknowledge the event with the firmware side.
However the OS was in a suspended state and GuC had been reset.

Internal specifications actually required the driver to ensure
that all active sessions be properly cleaned up in such cases where
the system is suspended and the GuC potentially unable to respond.

This patch adds the global teardown code in i915's suspend_prepare
code path.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Reviewed-by: Juston Li <justonli@chromium.org>
---
 drivers/gpu/drm/i915/pxp/intel_pxp.c         | 60 +++++++++++++++++---
 drivers/gpu/drm/i915/pxp/intel_pxp.h         |  1 +
 drivers/gpu/drm/i915/pxp/intel_pxp_pm.c      |  2 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_session.c |  9 ++-
 drivers/gpu/drm/i915/pxp/intel_pxp_session.h |  5 ++
 5 files changed, 64 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index cfc9af8b3d21..f3d9e7989eb7 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -270,6 +270,55 @@ static bool pxp_component_bound(struct intel_pxp *pxp)
 	return bound;
 }
 
+static int __pxp_global_teardown_locked(struct intel_pxp *pxp, bool terminate_for_cleanup)
+{
+	if (terminate_for_cleanup) {
+		if (!pxp->arb_is_valid)
+			return 0;
+		/*
+		 * To ensure synchronous and coherent session teardown completion
+		 * in response to suspend or shutdown triggers, don't use a worker.
+		 */
+		intel_pxp_mark_termination_in_progress(pxp);
+		intel_pxp_terminate(pxp, false);
+	} else {
+		if (pxp->arb_is_valid)
+			return 0;
+		/*
+		 * If we are not in final termination, and the arb-session is currently
+		 * inactive, we are doing a reset and restart due to some runtime event.
+		 * Use the worker that was designed for this.
+		 */
+		pxp_queue_termination(pxp);
+	}
+
+	if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250)))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+void intel_pxp_end(struct intel_pxp *pxp)
+{
+	struct drm_i915_private *i915 = pxp->ctrl_gt->i915;
+	intel_wakeref_t wakeref;
+
+	if (!intel_pxp_is_enabled(pxp))
+		return;
+
+	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
+
+	mutex_lock(&pxp->arb_mutex);
+
+	if (__pxp_global_teardown_locked(pxp, true))
+		drm_dbg(&i915->drm, "PXP end timed out\n");
+
+	mutex_unlock(&pxp->arb_mutex);
+
+	intel_pxp_fini_hw(pxp);
+	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
+}
+
 /*
  * the arb session is restarted from the irq work when we receive the
  * termination completion interrupt
@@ -286,16 +335,9 @@ int intel_pxp_start(struct intel_pxp *pxp)
 
 	mutex_lock(&pxp->arb_mutex);
 
-	if (pxp->arb_is_valid)
-		goto unlock;
-
-	pxp_queue_termination(pxp);
-
-	if (!wait_for_completion_timeout(&pxp->termination,
-					msecs_to_jiffies(250))) {
-		ret = -ETIMEDOUT;
+	ret = __pxp_global_teardown_locked(pxp, false);
+	if (ret)
 		goto unlock;
-	}
 
 	/* make sure the compiler doesn't optimize the double access */
 	barrier();
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index 9658d3005222..3ded0890cd27 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -27,6 +27,7 @@ void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
 void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id);
 
 int intel_pxp_start(struct intel_pxp *pxp);
+void intel_pxp_end(struct intel_pxp *pxp);
 
 int intel_pxp_key_check(struct intel_pxp *pxp,
 			struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
index 892d39cc61c1..e427464aa131 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
@@ -16,7 +16,7 @@ void intel_pxp_suspend_prepare(struct intel_pxp *pxp)
 	if (!intel_pxp_is_enabled(pxp))
 		return;
 
-	pxp->arb_is_valid = false;
+	intel_pxp_end(pxp);
 
 	intel_pxp_invalidate(pxp);
 }
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
index 74ed7e16e481..d8278c4002e3 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
@@ -115,11 +115,14 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
 	return ret;
 }
 
-static void pxp_terminate(struct intel_pxp *pxp)
+void intel_pxp_terminate(struct intel_pxp *pxp, bool restart_arb)
 {
 	int ret;
 
-	pxp->hw_state_invalidated = true;
+	if (restart_arb)
+		pxp->hw_state_invalidated = true;
+	else
+		pxp->hw_state_invalidated = false;
 
 	/*
 	 * if we fail to submit the termination there is no point in waiting for
@@ -167,7 +170,7 @@ static void pxp_session_work(struct work_struct *work)
 
 	if (events & PXP_TERMINATION_REQUEST) {
 		events &= ~PXP_TERMINATION_COMPLETE;
-		pxp_terminate(pxp);
+		intel_pxp_terminate(pxp, true);
 	}
 
 	if (events & PXP_TERMINATION_COMPLETE)
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
index 903ac52cffa1..4f944b63b5b6 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
@@ -12,9 +12,14 @@ struct intel_pxp;
 
 #ifdef CONFIG_DRM_I915_PXP
 void intel_pxp_session_management_init(struct intel_pxp *pxp);
+void intel_pxp_terminate(struct intel_pxp *pxp, bool restart_arb);
 #else
 static inline void intel_pxp_session_management_init(struct intel_pxp *pxp)
 {
 }
+
+static inline void intel_pxp_terminate(struct intel_pxp *pxp, bool restart_arb)
+{
+}
 #endif
 #endif /* __INTEL_PXP_SESSION_H__ */
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread
* [PATCH v5 6/6] drm/i915/pxp: Pxp hw init should be in resume_complete
  2023-01-13  1:18 ` [Intel-gfx] " Alan Previn
@ 2023-01-13  1:18   ` Alan Previn
  -1 siblings, 0 replies; 37+ messages in thread
From: Alan Previn @ 2023-01-13  1:18 UTC (permalink / raw)
  To: intel-gfx
  Cc: Alan Previn, Vivi, Greg Kroah-Hartman, Rodrigo,
	Alexander Usyskin, dri-devel, Daniele Ceraolo Spurio, Juston Li,
	Tomas Winkler

During suspend flow, i915 currently achors' on the pm_suspend_prepare
callback as the location where we quiesce the entire GPU and perform
all necessary cleanup in order to go into suspend. PXP is also called
during this time to perform the arbitration session teardown (with
the assurance no additional GEM IOCTLs will come after that could
restart the session).

However, if other devices or drivers fail their suspend_prepare, the
system will not go into suspend and i915 will be expected to resume
operation. In this case, we need to re-initialize the PXP hardware
and this really should be done within the pm_resume_complete callback
which is the correct opposing function in the resume sequence to
match pm_suspend_prepare of the suspend sequence.

Because this callback is the last thing at the end of resuming
we expect little to no impact to the rest of the i915 resume sequence
with this change.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_pm.h   |  1 +
 drivers/gpu/drm/i915/i915_driver.c      | 20 ++++++++++++++++++--
 drivers/gpu/drm/i915/pxp/intel_pxp_pm.c |  2 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_pm.h |  6 +++---
 4 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
index 6c9a46452364..fd1a23621222 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
@@ -77,6 +77,7 @@ void intel_gt_pm_fini(struct intel_gt *gt);
 
 void intel_gt_suspend_prepare(struct intel_gt *gt);
 void intel_gt_suspend_late(struct intel_gt *gt);
+
 int intel_gt_resume(struct intel_gt *gt);
 
 void intel_gt_runtime_suspend(struct intel_gt *gt);
diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index c1e427ba57ae..4c68a3f26e96 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1170,6 +1170,13 @@ static bool suspend_to_idle(struct drm_i915_private *dev_priv)
 	return false;
 }
 
+static void i915_drm_complete(struct drm_device *dev)
+{
+	struct drm_i915_private *i915 = to_i915(dev);
+
+	intel_pxp_resume_complete(i915->pxp);
+}
+
 static int i915_drm_prepare(struct drm_device *dev)
 {
 	struct drm_i915_private *i915 = to_i915(dev);
@@ -1370,8 +1377,6 @@ static int i915_drm_resume(struct drm_device *dev)
 
 	i915_gem_resume(dev_priv);
 
-	intel_pxp_resume(dev_priv->pxp);
-
 	intel_modeset_init_hw(dev_priv);
 	intel_init_clock_gating(dev_priv);
 	intel_hpd_init(dev_priv);
@@ -1563,6 +1568,16 @@ static int i915_pm_resume(struct device *kdev)
 	return i915_drm_resume(&i915->drm);
 }
 
+static void i915_pm_complete(struct device *kdev)
+{
+	struct drm_i915_private *i915 = kdev_to_i915(kdev);
+
+	if (i915->drm.switch_power_state == DRM_SWITCH_POWER_OFF)
+		return;
+
+	i915_drm_complete(&i915->drm);
+}
+
 /* freeze: before creating the hibernation_image */
 static int i915_pm_freeze(struct device *kdev)
 {
@@ -1783,6 +1798,7 @@ const struct dev_pm_ops i915_pm_ops = {
 	.suspend_late = i915_pm_suspend_late,
 	.resume_early = i915_pm_resume_early,
 	.resume = i915_pm_resume,
+	.complete = i915_pm_complete,
 
 	/*
 	 * S4 event handlers
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
index e427464aa131..4f836b317424 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
@@ -34,7 +34,7 @@ void intel_pxp_suspend(struct intel_pxp *pxp)
 	}
 }
 
-void intel_pxp_resume(struct intel_pxp *pxp)
+void intel_pxp_resume_complete(struct intel_pxp *pxp)
 {
 	if (!intel_pxp_is_enabled(pxp))
 		return;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
index 586be769104f..06b46f535b42 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
@@ -11,7 +11,7 @@ struct intel_pxp;
 #ifdef CONFIG_DRM_I915_PXP
 void intel_pxp_suspend_prepare(struct intel_pxp *pxp);
 void intel_pxp_suspend(struct intel_pxp *pxp);
-void intel_pxp_resume(struct intel_pxp *pxp);
+void intel_pxp_resume_complete(struct intel_pxp *pxp);
 void intel_pxp_runtime_suspend(struct intel_pxp *pxp);
 #else
 static inline void intel_pxp_suspend_prepare(struct intel_pxp *pxp)
@@ -22,7 +22,7 @@ static inline void intel_pxp_suspend(struct intel_pxp *pxp)
 {
 }
 
-static inline void intel_pxp_resume(struct intel_pxp *pxp)
+static inline void intel_pxp_resume_complete(struct intel_pxp *pxp)
 {
 }
 
@@ -32,6 +32,6 @@ static inline void intel_pxp_runtime_suspend(struct intel_pxp *pxp)
 #endif
 static inline void intel_pxp_runtime_resume(struct intel_pxp *pxp)
 {
-	intel_pxp_resume(pxp);
+	intel_pxp_resume_complete(pxp);
 }
 #endif /* __INTEL_PXP_PM_H__ */
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread
* [Intel-gfx] [PATCH v5 6/6] drm/i915/pxp: Pxp hw init should be in resume_complete
@ 2023-01-13  1:18   ` Alan Previn
  0 siblings, 0 replies; 37+ messages in thread
From: Alan Previn @ 2023-01-13  1:18 UTC (permalink / raw)
  To: intel-gfx
  Cc: Alan Previn, Vivi, Greg Kroah-Hartman, Rodrigo,
	Alexander Usyskin, dri-devel, Tomas Winkler

During suspend flow, i915 currently achors' on the pm_suspend_prepare
callback as the location where we quiesce the entire GPU and perform
all necessary cleanup in order to go into suspend. PXP is also called
during this time to perform the arbitration session teardown (with
the assurance no additional GEM IOCTLs will come after that could
restart the session).

However, if other devices or drivers fail their suspend_prepare, the
system will not go into suspend and i915 will be expected to resume
operation. In this case, we need to re-initialize the PXP hardware
and this really should be done within the pm_resume_complete callback
which is the correct opposing function in the resume sequence to
match pm_suspend_prepare of the suspend sequence.

Because this callback is the last thing at the end of resuming
we expect little to no impact to the rest of the i915 resume sequence
with this change.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_pm.h   |  1 +
 drivers/gpu/drm/i915/i915_driver.c      | 20 ++++++++++++++++++--
 drivers/gpu/drm/i915/pxp/intel_pxp_pm.c |  2 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_pm.h |  6 +++---
 4 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
index 6c9a46452364..fd1a23621222 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
@@ -77,6 +77,7 @@ void intel_gt_pm_fini(struct intel_gt *gt);
 
 void intel_gt_suspend_prepare(struct intel_gt *gt);
 void intel_gt_suspend_late(struct intel_gt *gt);
+
 int intel_gt_resume(struct intel_gt *gt);
 
 void intel_gt_runtime_suspend(struct intel_gt *gt);
diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index c1e427ba57ae..4c68a3f26e96 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1170,6 +1170,13 @@ static bool suspend_to_idle(struct drm_i915_private *dev_priv)
 	return false;
 }
 
+static void i915_drm_complete(struct drm_device *dev)
+{
+	struct drm_i915_private *i915 = to_i915(dev);
+
+	intel_pxp_resume_complete(i915->pxp);
+}
+
 static int i915_drm_prepare(struct drm_device *dev)
 {
 	struct drm_i915_private *i915 = to_i915(dev);
@@ -1370,8 +1377,6 @@ static int i915_drm_resume(struct drm_device *dev)
 
 	i915_gem_resume(dev_priv);
 
-	intel_pxp_resume(dev_priv->pxp);
-
 	intel_modeset_init_hw(dev_priv);
 	intel_init_clock_gating(dev_priv);
 	intel_hpd_init(dev_priv);
@@ -1563,6 +1568,16 @@ static int i915_pm_resume(struct device *kdev)
 	return i915_drm_resume(&i915->drm);
 }
 
+static void i915_pm_complete(struct device *kdev)
+{
+	struct drm_i915_private *i915 = kdev_to_i915(kdev);
+
+	if (i915->drm.switch_power_state == DRM_SWITCH_POWER_OFF)
+		return;
+
+	i915_drm_complete(&i915->drm);
+}
+
 /* freeze: before creating the hibernation_image */
 static int i915_pm_freeze(struct device *kdev)
 {
@@ -1783,6 +1798,7 @@ const struct dev_pm_ops i915_pm_ops = {
 	.suspend_late = i915_pm_suspend_late,
 	.resume_early = i915_pm_resume_early,
 	.resume = i915_pm_resume,
+	.complete = i915_pm_complete,
 
 	/*
 	 * S4 event handlers
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
index e427464aa131..4f836b317424 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
@@ -34,7 +34,7 @@ void intel_pxp_suspend(struct intel_pxp *pxp)
 	}
 }
 
-void intel_pxp_resume(struct intel_pxp *pxp)
+void intel_pxp_resume_complete(struct intel_pxp *pxp)
 {
 	if (!intel_pxp_is_enabled(pxp))
 		return;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
index 586be769104f..06b46f535b42 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
@@ -11,7 +11,7 @@ struct intel_pxp;
 #ifdef CONFIG_DRM_I915_PXP
 void intel_pxp_suspend_prepare(struct intel_pxp *pxp);
 void intel_pxp_suspend(struct intel_pxp *pxp);
-void intel_pxp_resume(struct intel_pxp *pxp);
+void intel_pxp_resume_complete(struct intel_pxp *pxp);
 void intel_pxp_runtime_suspend(struct intel_pxp *pxp);
 #else
 static inline void intel_pxp_suspend_prepare(struct intel_pxp *pxp)
@@ -22,7 +22,7 @@ static inline void intel_pxp_suspend(struct intel_pxp *pxp)
 {
 }
 
-static inline void intel_pxp_resume(struct intel_pxp *pxp)
+static inline void intel_pxp_resume_complete(struct intel_pxp *pxp)
 {
 }
 
@@ -32,6 +32,6 @@ static inline void intel_pxp_runtime_suspend(struct intel_pxp *pxp)
 #endif
 static inline void intel_pxp_runtime_resume(struct intel_pxp *pxp)
 {
-	intel_pxp_resume(pxp);
+	intel_pxp_resume_complete(pxp);
 }
 #endif /* __INTEL_PXP_PM_H__ */
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 37+ messages in thread
* [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/pxp: Add missing cleanup steps for PXP global-teardown
  2023-01-13  1:18 ` [Intel-gfx] " Alan Previn
                   ` (6 preceding siblings ...)
  (?)
@ 2023-01-13  2:10 ` Patchwork
  -1 siblings, 0 replies; 37+ messages in thread
From: Patchwork @ 2023-01-13  2:10 UTC (permalink / raw)
  To: Alan Previn; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/pxp: Add missing cleanup steps for PXP global-teardown
URL   : https://patchwork.freedesktop.org/series/112763/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.
+./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:148:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:148:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:148:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:148:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:148:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:148:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:148:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:150:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:150:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:150:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:150:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:150:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:150:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:150:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:154:26: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:154:26: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:154:26: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:154:26: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:154:26: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:154:26: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:154:26: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:156:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:156:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:156:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:156:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:156:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:156:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:156:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:156:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:156:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:156:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:156:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:156:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:156:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:156:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:174:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:174:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:174:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:174:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:174:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:174:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:174:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:176:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:176:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:176:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:176:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:176:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:176:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:176:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:180:35: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:180:35: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:180:35: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:180:35: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:180:35: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:180:35: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:180:35: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:182:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:182:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:182:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:182:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:182:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:182:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:182:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:182:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:182:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:182:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:182:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:182:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:182:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:182:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:186:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:186:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:186:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:186:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:186:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:186:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:186:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:188:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:188:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:188:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:188:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:188:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:188:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:188:9: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:192:35: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:192:35: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:192:35: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:192:35: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:192:35: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:192:35: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:192:35: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:195:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:195:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:195:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:195:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:195:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:195:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:195:16: warning: unreplaced symbol 'oldbit'
+./arch/x86/include/asm/bitops.h:195:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:195:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:195:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:195:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:195:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:195:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:195:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:237:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:237:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:237:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:237:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:237:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:237:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:237:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:239:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:239:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:239:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:239:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:239:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:239:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:239:9: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:66:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:66:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:66:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:66:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:66:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:66:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:66:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:92:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:92:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:92:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:92:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:92:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:92:1: warning: unreplaced symbol 'return'
+./arch/x86/include/asm/bitops.h:92:1: warning: unreplaced symbol 'return'
+./drivers/gpu/drm/i915/intel_uncore.h:351:1: warning: trying to copy expression type 31
+drivers/gpu/drm/i915/pxp/intel_pxp_huc.c:43:35:    expected restricted __le64 [assigned] [usertype] huc_base_address
+drivers/gpu/drm/i915/pxp/intel_pxp_huc.c:43:35:    got unsigned long long [assigned] [usertype] huc_phys_addr
+drivers/gpu/drm/i915/pxp/intel_pxp_huc.c:43:35: warning: incorrect type in assignment (different base types)
+./include/asm-generic/bitops/generic-non-atomic.h:100:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:100:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:100:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:100:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:100:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:100:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:100:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:100:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:100:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:100:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:100:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:100:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:100:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:100:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:100:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:100:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:100:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:100:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:100:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:100:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:100:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:105:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:105:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:105:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:105:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:105:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:105:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:105:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:107:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:107:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:107:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:107:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:107:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:107:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:107:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:108:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:108:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:108:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:108:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:108:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:108:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:108:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:109:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:109:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:109:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:109:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:109:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:109:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:109:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:111:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:111:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:111:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:111:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:111:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:111:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:111:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:111:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:111:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:111:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:111:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:111:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:111:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:111:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:111:20: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:111:20: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:111:20: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:111:20: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:111:20: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:111:20: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:111:20: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:112:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:112:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:112:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:112:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:112:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:112:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:112:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:112:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:112:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:112:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:112:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:112:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:112:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:112:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:112:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:112:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:112:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:112:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:112:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:112:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:112:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:121:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:121:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:121:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:121:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:121:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:121:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:121:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:128:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:128:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:128:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:128:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:128:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:128:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:128:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:166:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:166:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:166:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:166:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:166:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:166:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:166:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:168:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:168:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:168:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:168:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:168:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:168:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:168:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:169:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:169:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:169:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:169:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:169:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:169:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:169:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:170:9: warning: unreplaced symbol 'val'
+./include/asm-generic/bitops/generic-non-atomic.h:170:9: warning: unreplaced symbol 'val'
+./include/asm-generic/bitops/generic-non-atomic.h:170:9: warning: unreplaced symbol 'val'
+./include/asm-generic/bitops/generic-non-atomic.h:170:9: warning: unreplaced symbol 'val'
+./include/asm-generic/bitops/generic-non-atomic.h:170:9: warning: unreplaced symbol 'val'
+./include/asm-generic/bitops/generic-non-atomic.h:170:9: warning: unreplaced symbol 'val'
+./include/asm-generic/bitops/generic-non-atomic.h:170:9: warning: unreplaced symbol 'val'
+./include/asm-generic/bitops/generic-non-atomic.h:172:19: warning: unreplaced symbol 'val'
+./include/asm-generic/bitops/generic-non-atomic.h:172:19: warning: unreplaced symbol 'val'
+./include/asm-generic/bitops/generic-non-atomic.h:172:19: warning: unreplaced symbol 'val'
+./include/asm-generic/bitops/generic-non-atomic.h:172:19: warning: unreplaced symbol 'val'
+./include/asm-generic/bitops/generic-non-atomic.h:172:19: warning: unreplaced symbol 'val'
+./include/asm-generic/bitops/generic-non-atomic.h:172:19: warning: unreplaced symbol 'val'
+./include/asm-generic/bitops/generic-non-atomic.h:172:19: warning: unreplaced symbol 'val'
+./include/asm-generic/bitops/generic-non-atomic.h:172:25: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:172:25: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:172:25: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:172:25: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:172:25: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:172:25: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:172:25: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:172:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:172:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:172:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:172:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:172:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:172:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:172:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:28:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:28:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:28:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:28:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:28:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:28:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:28:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:30:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:30:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:30:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:30:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:30:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:30:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:30:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:31:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:31:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:31:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:31:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:31:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:31:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:31:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:33:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:33:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:33:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:33:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:33:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:33:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:33:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:33:16: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:33:16: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:33:16: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:33:16: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:33:16: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:33:16: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:33:16: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:37:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:37:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:37:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:37:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:37:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:37:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:37:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:39:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:39:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:39:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:39:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:39:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:39:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:39:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:40:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:40:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:40:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:40:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:40:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:40:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:40:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:42:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:42:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:42:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:42:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:42:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:42:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:42:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:42:16: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:42:16: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:42:16: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:42:16: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:42:16: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:42:16: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:42:16: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:55:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:55:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:55:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:55:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:55:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:55:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:55:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:57:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:57:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:57:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:57:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:57:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:57:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:57:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:58:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:58:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:58:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:58:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:58:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:58:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:58:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:60:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:60:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:60:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:60:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:60:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:60:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:60:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:60:15: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:60:15: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:60:15: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:60:15: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:60:15: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:60:15: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:60:15: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:73:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:73:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:73:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:73:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:73:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:73:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:73:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:75:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:75:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:75:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:75:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:75:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:75:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:75:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:76:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:76:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:76:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:76:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:76:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:76:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:76:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:77:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:77:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:77:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:77:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:77:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:77:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:77:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:79:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:79:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:79:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:79:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:79:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:79:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:79:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:79:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:79:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:79:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:79:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:79:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:79:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:79:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:79:20: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:79:20: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:79:20: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:79:20: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:79:20: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:79:20: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:79:20: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:80:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:80:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:80:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:80:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:80:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:80:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:80:17: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:80:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:80:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:80:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:80:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:80:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:80:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:80:23: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:80:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:80:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:80:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:80:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:80:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:80:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:80:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:93:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:93:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:93:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:93:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:93:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:93:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:93:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/generic-non-atomic.h:95:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:95:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:95:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:95:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:95:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:95:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:95:9: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:96:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:96:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:96:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:96:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:96:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:96:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:96:9: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:97:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:97:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:97:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:97:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:97:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:97:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:97:9: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:99:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:99:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:99:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:99:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:99:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:99:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:99:10: warning: unreplaced symbol 'p'
+./include/asm-generic/bitops/generic-non-atomic.h:99:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:99:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:99:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:99:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:99:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:99:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:99:14: warning: unreplaced symbol 'old'
+./include/asm-generic/bitops/generic-non-atomic.h:99:21: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:99:21: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:99:21: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:99:21: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:99:21: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:99:21: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/generic-non-atomic.h:99:21: warning: unreplaced symbol 'mask'
+./include/asm-generic/bitops/instrumented-non-atomic.h:100:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:100:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:100:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:100:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:100:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:100:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:100:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:112:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:112:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:112:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:112:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:112:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:112:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:112:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:115:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:115:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:115:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:115:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:115:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:115:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:115:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:127:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:127:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:127:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:127:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:127:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:127:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:127:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:130:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:130:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:130:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:130:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:130:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:130:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:130:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:139:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:139:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:139:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:139:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:139:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:139:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:139:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:142:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:142:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:142:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:142:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:142:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:142:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:142:9: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:26:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:26:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:26:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:26:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:26:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:26:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:26:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:42:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:42:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:42:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:42:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:42:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:42:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:42:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:58:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:58:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:58:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:58:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:58:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:58:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:58:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:97:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:97:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:97:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:97:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:97:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:97:1: warning: unreplaced symbol 'return'
+./include/asm-generic/bitops/instrumented-non-atomic.h:97:1: warning: unreplaced symbol 'return'



^ permalink raw reply	[flat|nested] 37+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/pxp: Add missing cleanup steps for PXP global-teardown
  2023-01-13  1:18 ` [Intel-gfx] " Alan Previn
                   ` (7 preceding siblings ...)
  (?)
@ 2023-01-13  2:38 ` Patchwork
  -1 siblings, 0 replies; 37+ messages in thread
From: Patchwork @ 2023-01-13  2:38 UTC (permalink / raw)
  To: Alan Previn; +Cc: intel-gfx

[-- Attachment #1: Type: text/plain, Size: 7456 bytes --]

== Series Details ==

Series: drm/i915/pxp: Add missing cleanup steps for PXP global-teardown
URL   : https://patchwork.freedesktop.org/series/112763/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12578 -> Patchwork_112763v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_112763v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_112763v1, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112763v1/index.html

Participating hosts (42 -> 43)
------------------------------

  Additional (2): fi-kbl-soraka fi-bsw-kefka 
  Missing    (1): fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_112763v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@guc:
    - fi-kbl-soraka:      NOTRUN -> [INCOMPLETE][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112763v1/fi-kbl-soraka/igt@i915_selftest@live@guc.html

  
Known issues
------------

  Here are the changes found in Patchwork_112763v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_gttfill@basic:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][2] ([fdo#109271]) +15 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112763v1/fi-kbl-soraka/igt@gem_exec_gttfill@basic.html

  * igt@gem_exec_suspend@basic-s3@smem:
    - fi-rkl-11600:       NOTRUN -> [INCOMPLETE][3] ([i915#7793])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112763v1/fi-rkl-11600/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@gem_huc_copy@huc-copy:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][4] ([fdo#109271] / [i915#2190])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112763v1/fi-kbl-soraka/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@basic:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][5] ([fdo#109271] / [i915#4613]) +3 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112763v1/fi-kbl-soraka/igt@gem_lmem_swapping@basic.html

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-apl-guc:         [PASS][6] -> [DMESG-FAIL][7] ([i915#5334])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12578/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112763v1/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@gt_pm:
    - fi-kbl-soraka:      NOTRUN -> [DMESG-FAIL][8] ([i915#1886])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112763v1/fi-kbl-soraka/igt@i915_selftest@live@gt_pm.html

  * igt@i915_selftest@live@hangcheck:
    - fi-rkl-guc:         [PASS][9] -> [INCOMPLETE][10] ([i915#4983])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12578/fi-rkl-guc/igt@i915_selftest@live@hangcheck.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112763v1/fi-rkl-guc/igt@i915_selftest@live@hangcheck.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions:
    - fi-bsw-kefka:       NOTRUN -> [FAIL][11] ([i915#6298])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112763v1/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html

  * igt@prime_vgem@basic-fence-flip:
    - fi-bsw-kefka:       NOTRUN -> [SKIP][12] ([fdo#109271]) +26 similar issues
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112763v1/fi-bsw-kefka/igt@prime_vgem@basic-fence-flip.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_pm:
    - {bat-rpls-2}:       [DMESG-FAIL][13] ([i915#4258]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12578/bat-rpls-2/igt@i915_selftest@live@gt_pm.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112763v1/bat-rpls-2/igt@i915_selftest@live@gt_pm.html

  * igt@i915_selftest@live@requests:
    - {bat-rpls-2}:       [INCOMPLETE][15] ([i915#4983] / [i915#6257]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12578/bat-rpls-2/igt@i915_selftest@live@requests.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112763v1/bat-rpls-2/igt@i915_selftest@live@requests.html

  * igt@i915_selftest@live@slpc:
    - {bat-adlp-9}:       [DMESG-FAIL][17] ([i915#6367]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12578/bat-adlp-9/igt@i915_selftest@live@slpc.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112763v1/bat-adlp-9/igt@i915_selftest@live@slpc.html

  
#### Warnings ####

  * igt@i915_suspend@basic-s3-without-i915:
    - fi-rkl-11600:       [INCOMPLETE][19] ([i915#4817]) -> [FAIL][20] ([fdo#103375])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12578/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112763v1/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#1886]: https://gitlab.freedesktop.org/drm/intel/issues/1886
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#4258]: https://gitlab.freedesktop.org/drm/intel/issues/4258
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4817]: https://gitlab.freedesktop.org/drm/intel/issues/4817
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#6257]: https://gitlab.freedesktop.org/drm/intel/issues/6257
  [i915#6298]: https://gitlab.freedesktop.org/drm/intel/issues/6298
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6997]: https://gitlab.freedesktop.org/drm/intel/issues/6997
  [i915#7609]: https://gitlab.freedesktop.org/drm/intel/issues/7609
  [i915#7793]: https://gitlab.freedesktop.org/drm/intel/issues/7793
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828


Build changes
-------------

  * Linux: CI_DRM_12578 -> Patchwork_112763v1

  CI-20190529: 20190529
  CI_DRM_12578: c0ba6b6312e9139ce4b89da2904b329c13b5e94d @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7119: 1e6d24e6dfa42b22f950f7d5e436b8f9acf8747f @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_112763v1: c0ba6b6312e9139ce4b89da2904b329c13b5e94d @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

cb28e3e23fef drm/i915/pxp: Pxp hw init should be in resume_complete
f6e17c7c182b drm/i915/pxp: Trigger the global teardown for before suspending
2f57ac6ae7b7 drm/i915/pxp: Invalidate all PXP fw sessions during teardown
76daf84017ff mei: clean pending read with vtag on bus
3385d9e90928 drm/i915/pxp: add device link between i915 and mei_pxp
d140d6c0404e mei: mei-me: resume device in prepare

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112763v1/index.html

[-- Attachment #2: Type: text/html, Size: 8451 bytes --]

^ permalink raw reply	[flat|nested] 37+ messages in thread
* RE: [PATCH v5 1/6] mei: mei-me: resume device in prepare
  2023-01-13  1:18   ` [Intel-gfx] " Alan Previn
@ 2023-01-18 11:42     ` Winkler, Tomas
  -1 siblings, 0 replies; 37+ messages in thread
From: Winkler, Tomas @ 2023-01-18 11:42 UTC (permalink / raw)
  To: Teres Alexis, Alan Previn, intel-gfx
  Cc: Greg Kroah-Hartman, Usyskin, Alexander, dri-devel,
	Ceraolo Spurio, Daniele, Vivi,  Rodrigo, Juston Li

> 
> From: Alexander Usyskin <alexander.usyskin@intel.com>
> 
> Asynchronous runtime resume is not possible while the system is
> suspending.
> The power management subsystem resumes the device only in the suspend
> phase, not in the prepare phase.
> Force resume device in prepare to allow drivers on mei bus to communicate
> in their prepare callbacks.
> 
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Reviewed-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  drivers/misc/mei/pci-me.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/mei/pci-me.c b/drivers/misc/mei/pci-me.c index
> 704cd0caa172..9f6ff06a94fd 100644
> --- a/drivers/misc/mei/pci-me.c
> +++ b/drivers/misc/mei/pci-me.c
> @@ -340,6 +340,12 @@ static void mei_me_remove(struct pci_dev *pdev)  }
> 
>  #ifdef CONFIG_PM_SLEEP
> +static int mei_me_pci_prepare(struct device *device) {
> +	pm_runtime_resume(device);
> +	return 0;
> +}
> +
>  static int mei_me_pci_suspend(struct device *device)  {
>  	struct pci_dev *pdev = to_pci_dev(device); @@ -396,7 +402,17 @@
> static int mei_me_pci_resume(struct device *device)
> 
>  	return 0;
>  }
> -#endif /* CONFIG_PM_SLEEP */
> +
> +static void mei_me_pci_complete(struct device *device) {
> +	pm_runtime_suspend(device);
> +}
> +#else /* CONFIG_PM_SLEEP */
> +
> +#define mei_me_pci_prepare NULL
> +#define mei_me_pci_complete NULL
> +
> +#endif /* !CONFIG_PM_SLEEP */
> 
>  #ifdef CONFIG_PM
>  static int mei_me_pm_runtime_idle(struct device *device) @@ -499,6
> +515,8 @@ static inline void mei_me_unset_pm_domain(struct mei_device
> *dev)  }
> 
>  static const struct dev_pm_ops mei_me_pm_ops = {
> +	.prepare = mei_me_pci_prepare,
> +	.complete = mei_me_pci_complete,
>  	SET_SYSTEM_SLEEP_PM_OPS(mei_me_pci_suspend,
>  				mei_me_pci_resume)
>  	SET_RUNTIME_PM_OPS(
> --
> 2.39.0


^ permalink raw reply	[flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5 1/6] mei: mei-me: resume device in prepare
@ 2023-01-18 11:42     ` Winkler, Tomas
  0 siblings, 0 replies; 37+ messages in thread
From: Winkler, Tomas @ 2023-01-18 11:42 UTC (permalink / raw)
  To: Teres Alexis, Alan Previn, intel-gfx
  Cc: Greg Kroah-Hartman, Usyskin, Alexander, dri-devel, Vivi,  Rodrigo

> 
> From: Alexander Usyskin <alexander.usyskin@intel.com>
> 
> Asynchronous runtime resume is not possible while the system is
> suspending.
> The power management subsystem resumes the device only in the suspend
> phase, not in the prepare phase.
> Force resume device in prepare to allow drivers on mei bus to communicate
> in their prepare callbacks.
> 
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Reviewed-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  drivers/misc/mei/pci-me.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/mei/pci-me.c b/drivers/misc/mei/pci-me.c index
> 704cd0caa172..9f6ff06a94fd 100644
> --- a/drivers/misc/mei/pci-me.c
> +++ b/drivers/misc/mei/pci-me.c
> @@ -340,6 +340,12 @@ static void mei_me_remove(struct pci_dev *pdev)  }
> 
>  #ifdef CONFIG_PM_SLEEP
> +static int mei_me_pci_prepare(struct device *device) {
> +	pm_runtime_resume(device);
> +	return 0;
> +}
> +
>  static int mei_me_pci_suspend(struct device *device)  {
>  	struct pci_dev *pdev = to_pci_dev(device); @@ -396,7 +402,17 @@
> static int mei_me_pci_resume(struct device *device)
> 
>  	return 0;
>  }
> -#endif /* CONFIG_PM_SLEEP */
> +
> +static void mei_me_pci_complete(struct device *device) {
> +	pm_runtime_suspend(device);
> +}
> +#else /* CONFIG_PM_SLEEP */
> +
> +#define mei_me_pci_prepare NULL
> +#define mei_me_pci_complete NULL
> +
> +#endif /* !CONFIG_PM_SLEEP */
> 
>  #ifdef CONFIG_PM
>  static int mei_me_pm_runtime_idle(struct device *device) @@ -499,6
> +515,8 @@ static inline void mei_me_unset_pm_domain(struct mei_device
> *dev)  }
> 
>  static const struct dev_pm_ops mei_me_pm_ops = {
> +	.prepare = mei_me_pci_prepare,
> +	.complete = mei_me_pci_complete,
>  	SET_SYSTEM_SLEEP_PM_OPS(mei_me_pci_suspend,
>  				mei_me_pci_resume)
>  	SET_RUNTIME_PM_OPS(
> --
> 2.39.0


^ permalink raw reply	[flat|nested] 37+ messages in thread
* RE: [PATCH v5 3/6] mei: clean pending read with vtag on bus
  2023-01-13  1:18   ` [Intel-gfx] " Alan Previn
@ 2023-01-18 11:43     ` Winkler, Tomas
  -1 siblings, 0 replies; 37+ messages in thread
From: Winkler, Tomas @ 2023-01-18 11:43 UTC (permalink / raw)
  To: Teres Alexis, Alan Previn, intel-gfx
  Cc: Greg Kroah-Hartman, Usyskin, Alexander, dri-devel,
	Ceraolo Spurio, Daniele, Vivi,  Rodrigo, Juston Li


> 
> From: Alexander Usyskin <alexander.usyskin@intel.com>
> 
> Client on bus have only one vtag map slot and should disregard the vtag
> value when cleaning pending read flag.
> Fixes read flow control message unexpectedly generated when clent on bus
> send messages with different vtags.
> 
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Reviewed-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  drivers/misc/mei/client.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c index
> 9ddb854b8155..5c19097266fe 100644
> --- a/drivers/misc/mei/client.c
> +++ b/drivers/misc/mei/client.c
> @@ -1343,7 +1343,9 @@ static void mei_cl_reset_read_by_vtag(const struct
> mei_cl *cl, u8 vtag)
>  	struct mei_cl_vtag *vtag_l;
> 
>  	list_for_each_entry(vtag_l, &cl->vtag_map, list) {
> -		if (vtag_l->vtag == vtag) {
> +		/* The client on bus has one fixed vtag map */
> +		if ((cl->cldev && mei_cldev_enabled(cl->cldev)) ||
> +		    vtag_l->vtag == vtag) {
>  			vtag_l->pending_read = false;
>  			break;
>  		}
> --
> 2.39.0


^ permalink raw reply	[flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5 3/6] mei: clean pending read with vtag on bus
@ 2023-01-18 11:43     ` Winkler, Tomas
  0 siblings, 0 replies; 37+ messages in thread
From: Winkler, Tomas @ 2023-01-18 11:43 UTC (permalink / raw)
  To: Teres Alexis, Alan Previn, intel-gfx
  Cc: Greg Kroah-Hartman, Usyskin, Alexander, dri-devel, Vivi,  Rodrigo


> 
> From: Alexander Usyskin <alexander.usyskin@intel.com>
> 
> Client on bus have only one vtag map slot and should disregard the vtag
> value when cleaning pending read flag.
> Fixes read flow control message unexpectedly generated when clent on bus
> send messages with different vtags.
> 
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Reviewed-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  drivers/misc/mei/client.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c index
> 9ddb854b8155..5c19097266fe 100644
> --- a/drivers/misc/mei/client.c
> +++ b/drivers/misc/mei/client.c
> @@ -1343,7 +1343,9 @@ static void mei_cl_reset_read_by_vtag(const struct
> mei_cl *cl, u8 vtag)
>  	struct mei_cl_vtag *vtag_l;
> 
>  	list_for_each_entry(vtag_l, &cl->vtag_map, list) {
> -		if (vtag_l->vtag == vtag) {
> +		/* The client on bus has one fixed vtag map */
> +		if ((cl->cldev && mei_cldev_enabled(cl->cldev)) ||
> +		    vtag_l->vtag == vtag) {
>  			vtag_l->pending_read = false;
>  			break;
>  		}
> --
> 2.39.0


^ permalink raw reply	[flat|nested] 37+ messages in thread
* Re: [PATCH v5 6/6] drm/i915/pxp: Pxp hw init should be in resume_complete
  2023-01-13  1:18   ` [Intel-gfx] " Alan Previn
@ 2023-01-19 19:10     ` Ceraolo Spurio, Daniele
  -1 siblings, 0 replies; 37+ messages in thread
From: Ceraolo Spurio, Daniele @ 2023-01-19 19:10 UTC (permalink / raw)
  To: Alan Previn, intel-gfx
  Cc: Greg Kroah-Hartman, Alexander Usyskin, dri-devel, Rodrigo,
	Tomas Winkler, Juston Li



On 1/12/2023 5:18 PM, Alan Previn wrote:
> During suspend flow, i915 currently achors' on the pm_suspend_prepare
> callback as the location where we quiesce the entire GPU and perform
> all necessary cleanup in order to go into suspend. PXP is also called
> during this time to perform the arbitration session teardown (with
> the assurance no additional GEM IOCTLs will come after that could
> restart the session).
>
> However, if other devices or drivers fail their suspend_prepare, the
> system will not go into suspend and i915 will be expected to resume
> operation. In this case, we need to re-initialize the PXP hardware
> and this really should be done within the pm_resume_complete callback
> which is the correct opposing function in the resume sequence to
> match pm_suspend_prepare of the suspend sequence.
>
> Because this callback is the last thing at the end of resuming
> we expect little to no impact to the rest of the i915 resume sequence
> with this change.
>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_gt_pm.h   |  1 +
>   drivers/gpu/drm/i915/i915_driver.c      | 20 ++++++++++++++++++--
>   drivers/gpu/drm/i915/pxp/intel_pxp_pm.c |  2 +-
>   drivers/gpu/drm/i915/pxp/intel_pxp_pm.h |  6 +++---
>   4 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> index 6c9a46452364..fd1a23621222 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> @@ -77,6 +77,7 @@ void intel_gt_pm_fini(struct intel_gt *gt);
>   
>   void intel_gt_suspend_prepare(struct intel_gt *gt);
>   void intel_gt_suspend_late(struct intel_gt *gt);
> +

Stray newline. With this removed:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

>   int intel_gt_resume(struct intel_gt *gt);
>   
>   void intel_gt_runtime_suspend(struct intel_gt *gt);
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index c1e427ba57ae..4c68a3f26e96 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1170,6 +1170,13 @@ static bool suspend_to_idle(struct drm_i915_private *dev_priv)
>   	return false;
>   }
>   
> +static void i915_drm_complete(struct drm_device *dev)
> +{
> +	struct drm_i915_private *i915 = to_i915(dev);
> +
> +	intel_pxp_resume_complete(i915->pxp);
> +}
> +
>   static int i915_drm_prepare(struct drm_device *dev)
>   {
>   	struct drm_i915_private *i915 = to_i915(dev);
> @@ -1370,8 +1377,6 @@ static int i915_drm_resume(struct drm_device *dev)
>   
>   	i915_gem_resume(dev_priv);
>   
> -	intel_pxp_resume(dev_priv->pxp);
> -
>   	intel_modeset_init_hw(dev_priv);
>   	intel_init_clock_gating(dev_priv);
>   	intel_hpd_init(dev_priv);
> @@ -1563,6 +1568,16 @@ static int i915_pm_resume(struct device *kdev)
>   	return i915_drm_resume(&i915->drm);
>   }
>   
> +static void i915_pm_complete(struct device *kdev)
> +{
> +	struct drm_i915_private *i915 = kdev_to_i915(kdev);
> +
> +	if (i915->drm.switch_power_state == DRM_SWITCH_POWER_OFF)
> +		return;
> +
> +	i915_drm_complete(&i915->drm);
> +}
> +
>   /* freeze: before creating the hibernation_image */
>   static int i915_pm_freeze(struct device *kdev)
>   {
> @@ -1783,6 +1798,7 @@ const struct dev_pm_ops i915_pm_ops = {
>   	.suspend_late = i915_pm_suspend_late,
>   	.resume_early = i915_pm_resume_early,
>   	.resume = i915_pm_resume,
> +	.complete = i915_pm_complete,
>   
>   	/*
>   	 * S4 event handlers
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> index e427464aa131..4f836b317424 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> @@ -34,7 +34,7 @@ void intel_pxp_suspend(struct intel_pxp *pxp)
>   	}
>   }
>   
> -void intel_pxp_resume(struct intel_pxp *pxp)
> +void intel_pxp_resume_complete(struct intel_pxp *pxp)
>   {
>   	if (!intel_pxp_is_enabled(pxp))
>   		return;
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
> index 586be769104f..06b46f535b42 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
> @@ -11,7 +11,7 @@ struct intel_pxp;
>   #ifdef CONFIG_DRM_I915_PXP
>   void intel_pxp_suspend_prepare(struct intel_pxp *pxp);
>   void intel_pxp_suspend(struct intel_pxp *pxp);
> -void intel_pxp_resume(struct intel_pxp *pxp);
> +void intel_pxp_resume_complete(struct intel_pxp *pxp);
>   void intel_pxp_runtime_suspend(struct intel_pxp *pxp);
>   #else
>   static inline void intel_pxp_suspend_prepare(struct intel_pxp *pxp)
> @@ -22,7 +22,7 @@ static inline void intel_pxp_suspend(struct intel_pxp *pxp)
>   {
>   }
>   
> -static inline void intel_pxp_resume(struct intel_pxp *pxp)
> +static inline void intel_pxp_resume_complete(struct intel_pxp *pxp)
>   {
>   }
>   
> @@ -32,6 +32,6 @@ static inline void intel_pxp_runtime_suspend(struct intel_pxp *pxp)
>   #endif
>   static inline void intel_pxp_runtime_resume(struct intel_pxp *pxp)
>   {
> -	intel_pxp_resume(pxp);
> +	intel_pxp_resume_complete(pxp);
>   }
>   #endif /* __INTEL_PXP_PM_H__ */


^ permalink raw reply	[flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5 6/6] drm/i915/pxp: Pxp hw init should be in resume_complete
@ 2023-01-19 19:10     ` Ceraolo Spurio, Daniele
  0 siblings, 0 replies; 37+ messages in thread
From: Ceraolo Spurio, Daniele @ 2023-01-19 19:10 UTC (permalink / raw)
  To: Alan Previn, intel-gfx
  Cc: Greg Kroah-Hartman, Alexander Usyskin, dri-devel, Rodrigo, Tomas Winkler



On 1/12/2023 5:18 PM, Alan Previn wrote:
> During suspend flow, i915 currently achors' on the pm_suspend_prepare
> callback as the location where we quiesce the entire GPU and perform
> all necessary cleanup in order to go into suspend. PXP is also called
> during this time to perform the arbitration session teardown (with
> the assurance no additional GEM IOCTLs will come after that could
> restart the session).
>
> However, if other devices or drivers fail their suspend_prepare, the
> system will not go into suspend and i915 will be expected to resume
> operation. In this case, we need to re-initialize the PXP hardware
> and this really should be done within the pm_resume_complete callback
> which is the correct opposing function in the resume sequence to
> match pm_suspend_prepare of the suspend sequence.
>
> Because this callback is the last thing at the end of resuming
> we expect little to no impact to the rest of the i915 resume sequence
> with this change.
>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_gt_pm.h   |  1 +
>   drivers/gpu/drm/i915/i915_driver.c      | 20 ++++++++++++++++++--
>   drivers/gpu/drm/i915/pxp/intel_pxp_pm.c |  2 +-
>   drivers/gpu/drm/i915/pxp/intel_pxp_pm.h |  6 +++---
>   4 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> index 6c9a46452364..fd1a23621222 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> @@ -77,6 +77,7 @@ void intel_gt_pm_fini(struct intel_gt *gt);
>   
>   void intel_gt_suspend_prepare(struct intel_gt *gt);
>   void intel_gt_suspend_late(struct intel_gt *gt);
> +

Stray newline. With this removed:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

>   int intel_gt_resume(struct intel_gt *gt);
>   
>   void intel_gt_runtime_suspend(struct intel_gt *gt);
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index c1e427ba57ae..4c68a3f26e96 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1170,6 +1170,13 @@ static bool suspend_to_idle(struct drm_i915_private *dev_priv)
>   	return false;
>   }
>   
> +static void i915_drm_complete(struct drm_device *dev)
> +{
> +	struct drm_i915_private *i915 = to_i915(dev);
> +
> +	intel_pxp_resume_complete(i915->pxp);
> +}
> +
>   static int i915_drm_prepare(struct drm_device *dev)
>   {
>   	struct drm_i915_private *i915 = to_i915(dev);
> @@ -1370,8 +1377,6 @@ static int i915_drm_resume(struct drm_device *dev)
>   
>   	i915_gem_resume(dev_priv);
>   
> -	intel_pxp_resume(dev_priv->pxp);
> -
>   	intel_modeset_init_hw(dev_priv);
>   	intel_init_clock_gating(dev_priv);
>   	intel_hpd_init(dev_priv);
> @@ -1563,6 +1568,16 @@ static int i915_pm_resume(struct device *kdev)
>   	return i915_drm_resume(&i915->drm);
>   }
>   
> +static void i915_pm_complete(struct device *kdev)
> +{
> +	struct drm_i915_private *i915 = kdev_to_i915(kdev);
> +
> +	if (i915->drm.switch_power_state == DRM_SWITCH_POWER_OFF)
> +		return;
> +
> +	i915_drm_complete(&i915->drm);
> +}
> +
>   /* freeze: before creating the hibernation_image */
>   static int i915_pm_freeze(struct device *kdev)
>   {
> @@ -1783,6 +1798,7 @@ const struct dev_pm_ops i915_pm_ops = {
>   	.suspend_late = i915_pm_suspend_late,
>   	.resume_early = i915_pm_resume_early,
>   	.resume = i915_pm_resume,
> +	.complete = i915_pm_complete,
>   
>   	/*
>   	 * S4 event handlers
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> index e427464aa131..4f836b317424 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> @@ -34,7 +34,7 @@ void intel_pxp_suspend(struct intel_pxp *pxp)
>   	}
>   }
>   
> -void intel_pxp_resume(struct intel_pxp *pxp)
> +void intel_pxp_resume_complete(struct intel_pxp *pxp)
>   {
>   	if (!intel_pxp_is_enabled(pxp))
>   		return;
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
> index 586be769104f..06b46f535b42 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
> @@ -11,7 +11,7 @@ struct intel_pxp;
>   #ifdef CONFIG_DRM_I915_PXP
>   void intel_pxp_suspend_prepare(struct intel_pxp *pxp);
>   void intel_pxp_suspend(struct intel_pxp *pxp);
> -void intel_pxp_resume(struct intel_pxp *pxp);
> +void intel_pxp_resume_complete(struct intel_pxp *pxp);
>   void intel_pxp_runtime_suspend(struct intel_pxp *pxp);
>   #else
>   static inline void intel_pxp_suspend_prepare(struct intel_pxp *pxp)
> @@ -22,7 +22,7 @@ static inline void intel_pxp_suspend(struct intel_pxp *pxp)
>   {
>   }
>   
> -static inline void intel_pxp_resume(struct intel_pxp *pxp)
> +static inline void intel_pxp_resume_complete(struct intel_pxp *pxp)
>   {
>   }
>   
> @@ -32,6 +32,6 @@ static inline void intel_pxp_runtime_suspend(struct intel_pxp *pxp)
>   #endif
>   static inline void intel_pxp_runtime_resume(struct intel_pxp *pxp)
>   {
> -	intel_pxp_resume(pxp);
> +	intel_pxp_resume_complete(pxp);
>   }
>   #endif /* __INTEL_PXP_PM_H__ */


^ permalink raw reply	[flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5 2/6] drm/i915/pxp: add device link between i915 and mei_pxp
  2023-01-13  1:18   ` [Intel-gfx] " Alan Previn
  (?)
@ 2023-01-19 19:25   ` Rodrigo Vivi
  2023-01-19 23:01     ` Teres Alexis, Alan Previn
  2023-01-22  6:57       ` Usyskin, Alexander
  -1 siblings, 2 replies; 37+ messages in thread
From: Rodrigo Vivi @ 2023-01-19 19:25 UTC (permalink / raw)
  To: Alan Previn
  Cc: Greg Kroah-Hartman, intel-gfx, Alexander Usyskin, dri-devel,
	Tomas Winkler, Vivi

On Thu, Jan 12, 2023 at 05:18:46PM -0800, Alan Previn wrote:
> From: Alexander Usyskin <alexander.usyskin@intel.com>
> 
> Add device link with i915 as consumer and mei_pxp as supplier
> to ensure proper ordering of power flows.
> 
> V2: condition on absence of heci_pxp to filter out DG
> 
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>  drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> index d50354bfb993..bef6d7f8ac55 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> @@ -127,6 +127,10 @@ static int i915_pxp_tee_component_bind(struct device *i915_kdev,
>  	intel_wakeref_t wakeref;
>  	int ret = 0;
>  
> +	if (!HAS_HECI_PXP(i915) &&
> +	    drm_WARN_ON(&i915->drm, !device_link_add(i915_kdev, tee_kdev,

I don't like the action here hidden behind the drm_WARN_ON.
Please notice that almost every use of this and other helpers like
this expect the param as a failure. Not an actual action. So,
most of lazy readers like me might ignore that the main function
is actually a param inside  this warn condition.

We should probably stash the link as well...

pxp->dev_link = device_link_add(i915_kdev, tee_kdev,...);

so in the end below, instead of checking the HAS_HECI_PXP again
and use the remove version you check the dev_link and use the del
function.

something like:

if (pxp->dev_link)
   device_link_del(pxp->dev_link);

Also, do you really need the WARN to see the stack when this happens
or you already know the callers?
Why not a simple drm_error msg?

if (!HAS_HECI_PXP(i915) {
	pxp->dev_link = device_link_add(i915_kdev, tee_kdev,...);
	if (!pxp->dev_link) {
	   drm_error();
	   return -ESOMETHING;

>  DL_FLAG_STATELESS)))

do we need the RPM in sync as well?
I mean:

DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME)))

?

> +		return -ENOMEM;

why ENOMEM?

> +
>  	mutex_lock(&pxp->tee_mutex);
>  	pxp->pxp_component = data;
>  	pxp->pxp_component->tee_dev = tee_kdev;
> @@ -169,6 +173,9 @@ static void i915_pxp_tee_component_unbind(struct device *i915_kdev,
>  	mutex_lock(&pxp->tee_mutex);
>  	pxp->pxp_component = NULL;
>  	mutex_unlock(&pxp->tee_mutex);
> +
> +	if (!HAS_HECI_PXP(i915))
> +		device_link_remove(i915_kdev, tee_kdev);
>  }
>  
>  static const struct component_ops i915_pxp_tee_component_ops = {
> -- 
> 2.39.0
> 

^ permalink raw reply	[flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5 4/6] drm/i915/pxp: Invalidate all PXP fw sessions during teardown
  2023-01-13  1:18   ` [Intel-gfx] " Alan Previn
  (?)
@ 2023-01-19 19:28   ` Rodrigo Vivi
  2023-01-19 22:00     ` Teres Alexis, Alan Previn
  -1 siblings, 1 reply; 37+ messages in thread
From: Rodrigo Vivi @ 2023-01-19 19:28 UTC (permalink / raw)
  To: Alan Previn
  Cc: Greg Kroah-Hartman, intel-gfx, Alexander Usyskin, dri-devel,
	Tomas Winkler, Vivi

On Thu, Jan 12, 2023 at 05:18:48PM -0800, Alan Previn wrote:
> A gap was recently discovered where if an application did not
> invalidate all of the stream keys (intentionally or not), and the
> driver did a full PXP global teardown on the GT subsystem, we
> find that future session creation would fail on the security
> firmware's side of the equation. i915 is the entity that needs
> ensure the sessions' state across both iGT and security firmware
> are at a known clean point when performing a full global teardown.
> 
> Architecturally speaking, i915 should inspect all active sessions
> and submit the invalidate-stream-key PXP command to the security
> firmware for each of them. However, for the upstream i915 driver
> we only support the arbitration session that can be created
> so that will be the only session we will cleanup.
> 
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> Reviewed-by: Juston Li <justonli@chromium.org>

Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/pxp/intel_pxp.h          |  1 +
>  .../drm/i915/pxp/intel_pxp_cmd_interface_42.h | 15 ++++++++
>  .../i915/pxp/intel_pxp_cmd_interface_cmn.h    |  3 ++
>  drivers/gpu/drm/i915/pxp/intel_pxp_session.c  |  2 ++
>  drivers/gpu/drm/i915/pxp/intel_pxp_tee.c      | 35 +++++++++++++++++++
>  5 files changed, 56 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> index 04440fada711..9658d3005222 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -24,6 +24,7 @@ void intel_pxp_init_hw(struct intel_pxp *pxp);
>  void intel_pxp_fini_hw(struct intel_pxp *pxp);
>  
>  void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
> +void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id);
>  
>  int intel_pxp_start(struct intel_pxp *pxp);
>  
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
> index 739f9072fa5f..26f7d9f01bf3 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
> @@ -12,6 +12,9 @@
>  /* PXP-Opcode for Init Session */
>  #define PXP42_CMDID_INIT_SESSION 0x1e
>  
> +/* PXP-Opcode for Invalidate Stream Key */
> +#define PXP42_CMDID_INVALIDATE_STREAM_KEY 0x00000007
> +
>  /* PXP-Input-Packet: Init Session (Arb-Session) */
>  struct pxp42_create_arb_in {
>  	struct pxp_cmd_header header;
> @@ -25,4 +28,16 @@ struct pxp42_create_arb_out {
>  	struct pxp_cmd_header header;
>  } __packed;
>  
> +/* PXP-Input-Packet: Invalidate Stream Key */
> +struct pxp42_inv_stream_key_in {
> +	struct pxp_cmd_header header;
> +	u32 rsvd[3];
> +} __packed;
> +
> +/* PXP-Output-Packet: Invalidate Stream Key */
> +struct pxp42_inv_stream_key_out {
> +	struct pxp_cmd_header header;
> +	u32 rsvd;
> +} __packed;
> +
>  #endif /* __INTEL_PXP_FW_INTERFACE_42_H__ */
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
> index c2f23394f9b8..69e34ec49e78 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
> @@ -27,6 +27,9 @@ struct pxp_cmd_header {
>  	union {
>  		u32 status; /* out */
>  		u32 stream_id; /* in */
> +#define PXP_CMDHDR_EXTDATA_SESSION_VALID GENMASK(0, 0)
> +#define PXP_CMDHDR_EXTDATA_APP_TYPE GENMASK(1, 1)
> +#define PXP_CMDHDR_EXTDATA_SESSION_ID GENMASK(17, 2)
>  	};
>  	/* Length of the message (excluding the header) */
>  	u32 buffer_len;
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> index ae413580b81a..74ed7e16e481 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> @@ -110,6 +110,8 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
>  
>  	intel_uncore_write(gt->uncore, PXP_GLOBAL_TERMINATE, 1);
>  
> +	intel_pxp_tee_end_arb_fw_session(pxp, ARB_SESSION);
> +
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> index bef6d7f8ac55..9e247f38f3bd 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> @@ -311,3 +311,38 @@ int intel_pxp_tee_cmd_create_arb_session(struct intel_pxp *pxp,
>  
>  	return ret;
>  }
> +
> +void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 session_id)
> +{
> +	struct drm_i915_private *i915 = pxp->ctrl_gt->i915;
> +	struct pxp42_inv_stream_key_in msg_in = {0};
> +	struct pxp42_inv_stream_key_out msg_out = {0};
> +	int ret, trials = 0;
> +
> +try_again:
> +	memset(&msg_in, 0, sizeof(msg_in));
> +	memset(&msg_out, 0, sizeof(msg_out));
> +	msg_in.header.api_version = PXP_APIVER(4, 2);
> +	msg_in.header.command_id = PXP42_CMDID_INVALIDATE_STREAM_KEY;
> +	msg_in.header.buffer_len = sizeof(msg_in) - sizeof(msg_in.header);
> +
> +	msg_in.header.stream_id = FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_VALID, 1);
> +	msg_in.header.stream_id |= FIELD_PREP(PXP_CMDHDR_EXTDATA_APP_TYPE, 0);
> +	msg_in.header.stream_id |= FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_ID, session_id);
> +
> +	ret = intel_pxp_tee_io_message(pxp,
> +				       &msg_in, sizeof(msg_in),
> +				       &msg_out, sizeof(msg_out),
> +				       NULL);
> +
> +	/* Cleanup coherency between GT and Firmware is critical, so try again if it fails */
> +	if ((ret || msg_out.header.status != 0x0) && ++trials < 3)
> +		goto try_again;
> +
> +	if (ret)
> +		drm_err(&i915->drm, "Failed to send tee msg for inv-stream-key-%d, ret=[%d]\n",
> +			session_id, ret);
> +	else if (msg_out.header.status != 0x0)
> +		drm_warn(&i915->drm, "PXP firmware failed inv-stream-key-%d with status 0x%08x\n",
> +			 session_id, msg_out.header.status);
> +}
> -- 
> 2.39.0
> 

^ permalink raw reply	[flat|nested] 37+ messages in thread
* Re: [PATCH v5 5/6] drm/i915/pxp: Trigger the global teardown for before suspending
  2023-01-13  1:18   ` [Intel-gfx] " Alan Previn
@ 2023-01-19 19:35     ` Rodrigo Vivi
  -1 siblings, 0 replies; 37+ messages in thread
From: Rodrigo Vivi @ 2023-01-19 19:35 UTC (permalink / raw)
  To: Alan Previn
  Cc: Greg Kroah-Hartman, intel-gfx, Alexander Usyskin, dri-devel,
	Daniele Ceraolo Spurio, Juston Li, Vivi, Tomas Winkler

On Thu, Jan 12, 2023 at 05:18:49PM -0800, Alan Previn wrote:
> A driver bug was recently discovered where the security firmware was
> receiving internal HW signals indicating that session key expirations
> had occurred. Architecturally, the firmware was expecting a response
> from the GuC to acknowledge the event with the firmware side.
> However the OS was in a suspended state and GuC had been reset.
> 
> Internal specifications actually required the driver to ensure
> that all active sessions be properly cleaned up in such cases where
> the system is suspended and the GuC potentially unable to respond.
> 
> This patch adds the global teardown code in i915's suspend_prepare
> code path.
> 
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> Reviewed-by: Juston Li <justonli@chromium.org>
> ---
>  drivers/gpu/drm/i915/pxp/intel_pxp.c         | 60 +++++++++++++++++---
>  drivers/gpu/drm/i915/pxp/intel_pxp.h         |  1 +
>  drivers/gpu/drm/i915/pxp/intel_pxp_pm.c      |  2 +-
>  drivers/gpu/drm/i915/pxp/intel_pxp_session.c |  9 ++-
>  drivers/gpu/drm/i915/pxp/intel_pxp_session.h |  5 ++
>  5 files changed, 64 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index cfc9af8b3d21..f3d9e7989eb7 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -270,6 +270,55 @@ static bool pxp_component_bound(struct intel_pxp *pxp)
>  	return bound;
>  }
>  
> +static int __pxp_global_teardown_locked(struct intel_pxp *pxp, bool terminate_for_cleanup)
> +{
> +	if (terminate_for_cleanup) {
> +		if (!pxp->arb_is_valid)
> +			return 0;
> +		/*
> +		 * To ensure synchronous and coherent session teardown completion
> +		 * in response to suspend or shutdown triggers, don't use a worker.
> +		 */
> +		intel_pxp_mark_termination_in_progress(pxp);
> +		intel_pxp_terminate(pxp, false);
> +	} else {
> +		if (pxp->arb_is_valid)
> +			return 0;
> +		/*
> +		 * If we are not in final termination, and the arb-session is currently
> +		 * inactive, we are doing a reset and restart due to some runtime event.
> +		 * Use the worker that was designed for this.
> +		 */
> +		pxp_queue_termination(pxp);
> +	}

I really don't see why you need 1 function for totally 2 different cases.
Why not 2 functions then?

> +
> +	if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250)))
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +void intel_pxp_end(struct intel_pxp *pxp)
> +{
> +	struct drm_i915_private *i915 = pxp->ctrl_gt->i915;
> +	intel_wakeref_t wakeref;
> +
> +	if (!intel_pxp_is_enabled(pxp))
> +		return;
> +
> +	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> +
> +	mutex_lock(&pxp->arb_mutex);
> +
> +	if (__pxp_global_teardown_locked(pxp, true))
> +		drm_dbg(&i915->drm, "PXP end timed out\n");
> +
> +	mutex_unlock(&pxp->arb_mutex);
> +
> +	intel_pxp_fini_hw(pxp);
> +	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> +}
> +
>  /*
>   * the arb session is restarted from the irq work when we receive the
>   * termination completion interrupt
> @@ -286,16 +335,9 @@ int intel_pxp_start(struct intel_pxp *pxp)
>  
>  	mutex_lock(&pxp->arb_mutex);
>  
> -	if (pxp->arb_is_valid)
> -		goto unlock;
> -
> -	pxp_queue_termination(pxp);
> -
> -	if (!wait_for_completion_timeout(&pxp->termination,
> -					msecs_to_jiffies(250))) {
> -		ret = -ETIMEDOUT;
> +	ret = __pxp_global_teardown_locked(pxp, false);
> +	if (ret)
>  		goto unlock;
> -	}
>  
>  	/* make sure the compiler doesn't optimize the double access */
>  	barrier();
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> index 9658d3005222..3ded0890cd27 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -27,6 +27,7 @@ void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
>  void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id);
>  
>  int intel_pxp_start(struct intel_pxp *pxp);
> +void intel_pxp_end(struct intel_pxp *pxp);
>  
>  int intel_pxp_key_check(struct intel_pxp *pxp,
>  			struct drm_i915_gem_object *obj,
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> index 892d39cc61c1..e427464aa131 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> @@ -16,7 +16,7 @@ void intel_pxp_suspend_prepare(struct intel_pxp *pxp)
>  	if (!intel_pxp_is_enabled(pxp))
>  		return;
>  
> -	pxp->arb_is_valid = false;
> +	intel_pxp_end(pxp);
>  
>  	intel_pxp_invalidate(pxp);
>  }
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> index 74ed7e16e481..d8278c4002e3 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> @@ -115,11 +115,14 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
>  	return ret;
>  }
>  
> -static void pxp_terminate(struct intel_pxp *pxp)
> +void intel_pxp_terminate(struct intel_pxp *pxp, bool restart_arb)
>  {
>  	int ret;
>  
> -	pxp->hw_state_invalidated = true;
> +	if (restart_arb)
> +		pxp->hw_state_invalidated = true;
> +	else
> +		pxp->hw_state_invalidated = false;

o.O

pxp->hw_state_invalidate = restart_arb;

?

or even a better name for the restart_arb to already indicate that is
the hw_state_invalidate ?

>  
>  	/*
>  	 * if we fail to submit the termination there is no point in waiting for
> @@ -167,7 +170,7 @@ static void pxp_session_work(struct work_struct *work)
>  
>  	if (events & PXP_TERMINATION_REQUEST) {
>  		events &= ~PXP_TERMINATION_COMPLETE;
> -		pxp_terminate(pxp);
> +		intel_pxp_terminate(pxp, true);
>  	}
>  
>  	if (events & PXP_TERMINATION_COMPLETE)
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
> index 903ac52cffa1..4f944b63b5b6 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
> @@ -12,9 +12,14 @@ struct intel_pxp;
>  
>  #ifdef CONFIG_DRM_I915_PXP
>  void intel_pxp_session_management_init(struct intel_pxp *pxp);
> +void intel_pxp_terminate(struct intel_pxp *pxp, bool restart_arb);
>  #else
>  static inline void intel_pxp_session_management_init(struct intel_pxp *pxp)
>  {
>  }
> +
> +static inline void intel_pxp_terminate(struct intel_pxp *pxp, bool restart_arb)
> +{
> +}
>  #endif
>  #endif /* __INTEL_PXP_SESSION_H__ */
> -- 
> 2.39.0
> 

^ permalink raw reply	[flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5 5/6] drm/i915/pxp: Trigger the global teardown for before suspending
@ 2023-01-19 19:35     ` Rodrigo Vivi
  0 siblings, 0 replies; 37+ messages in thread
From: Rodrigo Vivi @ 2023-01-19 19:35 UTC (permalink / raw)
  To: Alan Previn
  Cc: Greg Kroah-Hartman, intel-gfx, Alexander Usyskin, dri-devel,
	Vivi, Tomas Winkler

On Thu, Jan 12, 2023 at 05:18:49PM -0800, Alan Previn wrote:
> A driver bug was recently discovered where the security firmware was
> receiving internal HW signals indicating that session key expirations
> had occurred. Architecturally, the firmware was expecting a response
> from the GuC to acknowledge the event with the firmware side.
> However the OS was in a suspended state and GuC had been reset.
> 
> Internal specifications actually required the driver to ensure
> that all active sessions be properly cleaned up in such cases where
> the system is suspended and the GuC potentially unable to respond.
> 
> This patch adds the global teardown code in i915's suspend_prepare
> code path.
> 
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> Reviewed-by: Juston Li <justonli@chromium.org>
> ---
>  drivers/gpu/drm/i915/pxp/intel_pxp.c         | 60 +++++++++++++++++---
>  drivers/gpu/drm/i915/pxp/intel_pxp.h         |  1 +
>  drivers/gpu/drm/i915/pxp/intel_pxp_pm.c      |  2 +-
>  drivers/gpu/drm/i915/pxp/intel_pxp_session.c |  9 ++-
>  drivers/gpu/drm/i915/pxp/intel_pxp_session.h |  5 ++
>  5 files changed, 64 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index cfc9af8b3d21..f3d9e7989eb7 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -270,6 +270,55 @@ static bool pxp_component_bound(struct intel_pxp *pxp)
>  	return bound;
>  }
>  
> +static int __pxp_global_teardown_locked(struct intel_pxp *pxp, bool terminate_for_cleanup)
> +{
> +	if (terminate_for_cleanup) {
> +		if (!pxp->arb_is_valid)
> +			return 0;
> +		/*
> +		 * To ensure synchronous and coherent session teardown completion
> +		 * in response to suspend or shutdown triggers, don't use a worker.
> +		 */
> +		intel_pxp_mark_termination_in_progress(pxp);
> +		intel_pxp_terminate(pxp, false);
> +	} else {
> +		if (pxp->arb_is_valid)
> +			return 0;
> +		/*
> +		 * If we are not in final termination, and the arb-session is currently
> +		 * inactive, we are doing a reset and restart due to some runtime event.
> +		 * Use the worker that was designed for this.
> +		 */
> +		pxp_queue_termination(pxp);
> +	}

I really don't see why you need 1 function for totally 2 different cases.
Why not 2 functions then?

> +
> +	if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250)))
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +void intel_pxp_end(struct intel_pxp *pxp)
> +{
> +	struct drm_i915_private *i915 = pxp->ctrl_gt->i915;
> +	intel_wakeref_t wakeref;
> +
> +	if (!intel_pxp_is_enabled(pxp))
> +		return;
> +
> +	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> +
> +	mutex_lock(&pxp->arb_mutex);
> +
> +	if (__pxp_global_teardown_locked(pxp, true))
> +		drm_dbg(&i915->drm, "PXP end timed out\n");
> +
> +	mutex_unlock(&pxp->arb_mutex);
> +
> +	intel_pxp_fini_hw(pxp);
> +	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> +}
> +
>  /*
>   * the arb session is restarted from the irq work when we receive the
>   * termination completion interrupt
> @@ -286,16 +335,9 @@ int intel_pxp_start(struct intel_pxp *pxp)
>  
>  	mutex_lock(&pxp->arb_mutex);
>  
> -	if (pxp->arb_is_valid)
> -		goto unlock;
> -
> -	pxp_queue_termination(pxp);
> -
> -	if (!wait_for_completion_timeout(&pxp->termination,
> -					msecs_to_jiffies(250))) {
> -		ret = -ETIMEDOUT;
> +	ret = __pxp_global_teardown_locked(pxp, false);
> +	if (ret)
>  		goto unlock;
> -	}
>  
>  	/* make sure the compiler doesn't optimize the double access */
>  	barrier();
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> index 9658d3005222..3ded0890cd27 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -27,6 +27,7 @@ void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
>  void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id);
>  
>  int intel_pxp_start(struct intel_pxp *pxp);
> +void intel_pxp_end(struct intel_pxp *pxp);
>  
>  int intel_pxp_key_check(struct intel_pxp *pxp,
>  			struct drm_i915_gem_object *obj,
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> index 892d39cc61c1..e427464aa131 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> @@ -16,7 +16,7 @@ void intel_pxp_suspend_prepare(struct intel_pxp *pxp)
>  	if (!intel_pxp_is_enabled(pxp))
>  		return;
>  
> -	pxp->arb_is_valid = false;
> +	intel_pxp_end(pxp);
>  
>  	intel_pxp_invalidate(pxp);
>  }
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> index 74ed7e16e481..d8278c4002e3 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> @@ -115,11 +115,14 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
>  	return ret;
>  }
>  
> -static void pxp_terminate(struct intel_pxp *pxp)
> +void intel_pxp_terminate(struct intel_pxp *pxp, bool restart_arb)
>  {
>  	int ret;
>  
> -	pxp->hw_state_invalidated = true;
> +	if (restart_arb)
> +		pxp->hw_state_invalidated = true;
> +	else
> +		pxp->hw_state_invalidated = false;

o.O

pxp->hw_state_invalidate = restart_arb;

?

or even a better name for the restart_arb to already indicate that is
the hw_state_invalidate ?

>  
>  	/*
>  	 * if we fail to submit the termination there is no point in waiting for
> @@ -167,7 +170,7 @@ static void pxp_session_work(struct work_struct *work)
>  
>  	if (events & PXP_TERMINATION_REQUEST) {
>  		events &= ~PXP_TERMINATION_COMPLETE;
> -		pxp_terminate(pxp);
> +		intel_pxp_terminate(pxp, true);
>  	}
>  
>  	if (events & PXP_TERMINATION_COMPLETE)
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
> index 903ac52cffa1..4f944b63b5b6 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.h
> @@ -12,9 +12,14 @@ struct intel_pxp;
>  
>  #ifdef CONFIG_DRM_I915_PXP
>  void intel_pxp_session_management_init(struct intel_pxp *pxp);
> +void intel_pxp_terminate(struct intel_pxp *pxp, bool restart_arb);
>  #else
>  static inline void intel_pxp_session_management_init(struct intel_pxp *pxp)
>  {
>  }
> +
> +static inline void intel_pxp_terminate(struct intel_pxp *pxp, bool restart_arb)
> +{
> +}
>  #endif
>  #endif /* __INTEL_PXP_SESSION_H__ */
> -- 
> 2.39.0
> 

^ permalink raw reply	[flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5 6/6] drm/i915/pxp: Pxp hw init should be in resume_complete
  2023-01-19 19:10     ` [Intel-gfx] " Ceraolo Spurio, Daniele
  (?)
@ 2023-01-19 19:40     ` Rodrigo Vivi
  -1 siblings, 0 replies; 37+ messages in thread
From: Rodrigo Vivi @ 2023-01-19 19:40 UTC (permalink / raw)
  To: Ceraolo Spurio, Daniele
  Cc: Alan Previn, Greg Kroah-Hartman, intel-gfx, Alexander Usyskin,
	dri-devel, Tomas Winkler

On Thu, Jan 19, 2023 at 11:10:21AM -0800, Ceraolo Spurio, Daniele wrote:
> 
> 
> On 1/12/2023 5:18 PM, Alan Previn wrote:
> > During suspend flow, i915 currently achors' on the pm_suspend_prepare
> > callback as the location where we quiesce the entire GPU and perform
> > all necessary cleanup in order to go into suspend. PXP is also called
> > during this time to perform the arbitration session teardown (with
> > the assurance no additional GEM IOCTLs will come after that could
> > restart the session).
> > 
> > However, if other devices or drivers fail their suspend_prepare, the
> > system will not go into suspend and i915 will be expected to resume
> > operation. In this case, we need to re-initialize the PXP hardware
> > and this really should be done within the pm_resume_complete callback
> > which is the correct opposing function in the resume sequence to
> > match pm_suspend_prepare of the suspend sequence.
> > 
> > Because this callback is the last thing at the end of resuming
> > we expect little to no impact to the rest of the i915 resume sequence
> > with this change.
> > 
> > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_gt_pm.h   |  1 +
> >   drivers/gpu/drm/i915/i915_driver.c      | 20 ++++++++++++++++++--
> >   drivers/gpu/drm/i915/pxp/intel_pxp_pm.c |  2 +-
> >   drivers/gpu/drm/i915/pxp/intel_pxp_pm.h |  6 +++---
> >   4 files changed, 23 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> > index 6c9a46452364..fd1a23621222 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> > @@ -77,6 +77,7 @@ void intel_gt_pm_fini(struct intel_gt *gt);
> >   void intel_gt_suspend_prepare(struct intel_gt *gt);
> >   void intel_gt_suspend_late(struct intel_gt *gt);
> > +
> 
> Stray newline. With this removed:
> 
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> Daniele
> 
> >   int intel_gt_resume(struct intel_gt *gt);
> >   void intel_gt_runtime_suspend(struct intel_gt *gt);
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > index c1e427ba57ae..4c68a3f26e96 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -1170,6 +1170,13 @@ static bool suspend_to_idle(struct drm_i915_private *dev_priv)
> >   	return false;
> >   }
> > +static void i915_drm_complete(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *i915 = to_i915(dev);
> > +
> > +	intel_pxp_resume_complete(i915->pxp);
> > +}
> > +
> >   static int i915_drm_prepare(struct drm_device *dev)
> >   {
> >   	struct drm_i915_private *i915 = to_i915(dev);
> > @@ -1370,8 +1377,6 @@ static int i915_drm_resume(struct drm_device *dev)
> >   	i915_gem_resume(dev_priv);
> > -	intel_pxp_resume(dev_priv->pxp);
> > -
> >   	intel_modeset_init_hw(dev_priv);
> >   	intel_init_clock_gating(dev_priv);
> >   	intel_hpd_init(dev_priv);
> > @@ -1563,6 +1568,16 @@ static int i915_pm_resume(struct device *kdev)
> >   	return i915_drm_resume(&i915->drm);
> >   }
> > +static void i915_pm_complete(struct device *kdev)
> > +{
> > +	struct drm_i915_private *i915 = kdev_to_i915(kdev);
> > +
> > +	if (i915->drm.switch_power_state == DRM_SWITCH_POWER_OFF)
> > +		return;
> > +
> > +	i915_drm_complete(&i915->drm);
> > +}
> > +
> >   /* freeze: before creating the hibernation_image */
> >   static int i915_pm_freeze(struct device *kdev)
> >   {
> > @@ -1783,6 +1798,7 @@ const struct dev_pm_ops i915_pm_ops = {
> >   	.suspend_late = i915_pm_suspend_late,
> >   	.resume_early = i915_pm_resume_early,
> >   	.resume = i915_pm_resume,
> > +	.complete = i915_pm_complete,
> >   	/*
> >   	 * S4 event handlers
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> > index e427464aa131..4f836b317424 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> > @@ -34,7 +34,7 @@ void intel_pxp_suspend(struct intel_pxp *pxp)
> >   	}
> >   }
> > -void intel_pxp_resume(struct intel_pxp *pxp)
> > +void intel_pxp_resume_complete(struct intel_pxp *pxp)
> >   {
> >   	if (!intel_pxp_is_enabled(pxp))
> >   		return;
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
> > index 586be769104f..06b46f535b42 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
> > @@ -11,7 +11,7 @@ struct intel_pxp;
> >   #ifdef CONFIG_DRM_I915_PXP
> >   void intel_pxp_suspend_prepare(struct intel_pxp *pxp);
> >   void intel_pxp_suspend(struct intel_pxp *pxp);
> > -void intel_pxp_resume(struct intel_pxp *pxp);
> > +void intel_pxp_resume_complete(struct intel_pxp *pxp);
> >   void intel_pxp_runtime_suspend(struct intel_pxp *pxp);
> >   #else
> >   static inline void intel_pxp_suspend_prepare(struct intel_pxp *pxp)
> > @@ -22,7 +22,7 @@ static inline void intel_pxp_suspend(struct intel_pxp *pxp)
> >   {
> >   }
> > -static inline void intel_pxp_resume(struct intel_pxp *pxp)
> > +static inline void intel_pxp_resume_complete(struct intel_pxp *pxp)
> >   {
> >   }
> > @@ -32,6 +32,6 @@ static inline void intel_pxp_runtime_suspend(struct intel_pxp *pxp)
> >   #endif
> >   static inline void intel_pxp_runtime_resume(struct intel_pxp *pxp)
> >   {
> > -	intel_pxp_resume(pxp);
> > +	intel_pxp_resume_complete(pxp);
> >   }
> >   #endif /* __INTEL_PXP_PM_H__ */
> 

^ permalink raw reply	[flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5 4/6] drm/i915/pxp: Invalidate all PXP fw sessions during teardown
  2023-01-19 19:28   ` Rodrigo Vivi
@ 2023-01-19 22:00     ` Teres Alexis, Alan Previn
  0 siblings, 0 replies; 37+ messages in thread
From: Teres Alexis, Alan Previn @ 2023-01-19 22:00 UTC (permalink / raw)
  To: Vivi, Rodrigo
  Cc: gregkh, intel-gfx, Usyskin, Alexander, dri-devel, Vivi, Winkler, Tomas

Thanks Rodrigo for the ack.

On Thu, 2023-01-19 at 14:28 -0500, Vivi, Rodrigo wrote:
> On Thu, Jan 12, 2023 at 05:18:48PM -0800, Alan Previn wrote:
> > A gap was recently discovered where if an application did not
> > invalidate all of the stream keys (intentionally or not), and the
> > driver did a full PXP global teardown on the GT subsystem, we
> > find that future session creation would fail on the security
> > firmware's side of the equation. i915 is the entity that needs
> > ensure the sessions' state across both iGT and security firmware
> > are at a known clean point when performing a full global teardown.
> > 
> > Architecturally speaking, i915 should inspect all active sessions
> > and submit the invalidate-stream-key PXP command to the security
> > firmware for each of them. However, for the upstream i915 driver
> > we only support the arbitration session that can be created
> > so that will be the only session we will cleanup.
> > 
> > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > Reviewed-by: Juston Li <justonli@chromium.org>
> 
> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/pxp/intel_pxp.h          |  1 +
> >  .../drm/i915/pxp/intel_pxp_cmd_interface_42.h | 15 ++++++++
> >  .../i915/pxp/intel_pxp_cmd_interface_cmn.h    |  3 ++
> >  drivers/gpu/drm/i915/pxp/intel_pxp_session.c  |  2 ++
> >  drivers/gpu/drm/i915/pxp/intel_pxp_tee.c      | 35 +++++++++++++++++++
> >  5 files changed, 56 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > index 04440fada711..9658d3005222 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > @@ -24,6 +24,7 @@ void intel_pxp_init_hw(struct intel_pxp *pxp);
> >  void intel_pxp_fini_hw(struct intel_pxp *pxp);
> >  
> >  void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
> > +void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id);
> >  
> >  int intel_pxp_start(struct intel_pxp *pxp);
> >  
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
> > index 739f9072fa5f..26f7d9f01bf3 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_42.h
> > @@ -12,6 +12,9 @@
> >  /* PXP-Opcode for Init Session */
> >  #define PXP42_CMDID_INIT_SESSION 0x1e
> >  
> > +/* PXP-Opcode for Invalidate Stream Key */
> > +#define PXP42_CMDID_INVALIDATE_STREAM_KEY 0x00000007
> > +
> >  /* PXP-Input-Packet: Init Session (Arb-Session) */
> >  struct pxp42_create_arb_in {
> >         struct pxp_cmd_header header;
> > @@ -25,4 +28,16 @@ struct pxp42_create_arb_out {
> >         struct pxp_cmd_header header;
> >  } __packed;
> >  
> > +/* PXP-Input-Packet: Invalidate Stream Key */
> > +struct pxp42_inv_stream_key_in {
> > +       struct pxp_cmd_header header;
> > +       u32 rsvd[3];
> > +} __packed;
> > +
> > +/* PXP-Output-Packet: Invalidate Stream Key */
> > +struct pxp42_inv_stream_key_out {
> > +       struct pxp_cmd_header header;
> > +       u32 rsvd;
> > +} __packed;
> > +
> >  #endif /* __INTEL_PXP_FW_INTERFACE_42_H__ */
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
> > index c2f23394f9b8..69e34ec49e78 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_cmn.h
> > @@ -27,6 +27,9 @@ struct pxp_cmd_header {
> >         union {
> >                 u32 status; /* out */
> >                 u32 stream_id; /* in */
> > +#define PXP_CMDHDR_EXTDATA_SESSION_VALID GENMASK(0, 0)
> > +#define PXP_CMDHDR_EXTDATA_APP_TYPE GENMASK(1, 1)
> > +#define PXP_CMDHDR_EXTDATA_SESSION_ID GENMASK(17, 2)
> >         };
> >         /* Length of the message (excluding the header) */
> >         u32 buffer_len;
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> > index ae413580b81a..74ed7e16e481 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> > @@ -110,6 +110,8 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
> >  
> >         intel_uncore_write(gt->uncore, PXP_GLOBAL_TERMINATE, 1);
> >  
> > +       intel_pxp_tee_end_arb_fw_session(pxp, ARB_SESSION);
> > +
> >         return ret;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > index bef6d7f8ac55..9e247f38f3bd 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > @@ -311,3 +311,38 @@ int intel_pxp_tee_cmd_create_arb_session(struct intel_pxp *pxp,
> >  
> >         return ret;
> >  }
> > +
> > +void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 session_id)
> > +{
> > +       struct drm_i915_private *i915 = pxp->ctrl_gt->i915;
> > +       struct pxp42_inv_stream_key_in msg_in = {0};
> > +       struct pxp42_inv_stream_key_out msg_out = {0};
> > +       int ret, trials = 0;
> > +
> > +try_again:
> > +       memset(&msg_in, 0, sizeof(msg_in));
> > +       memset(&msg_out, 0, sizeof(msg_out));
> > +       msg_in.header.api_version = PXP_APIVER(4, 2);
> > +       msg_in.header.command_id = PXP42_CMDID_INVALIDATE_STREAM_KEY;
> > +       msg_in.header.buffer_len = sizeof(msg_in) - sizeof(msg_in.header);
> > +
> > +       msg_in.header.stream_id = FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_VALID, 1);
> > +       msg_in.header.stream_id |= FIELD_PREP(PXP_CMDHDR_EXTDATA_APP_TYPE, 0);
> > +       msg_in.header.stream_id |= FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_ID, session_id);
> > +
> > +       ret = intel_pxp_tee_io_message(pxp,
> > +                                      &msg_in, sizeof(msg_in),
> > +                                      &msg_out, sizeof(msg_out),
> > +                                      NULL);
> > +
> > +       /* Cleanup coherency between GT and Firmware is critical, so try again if it fails */
> > +       if ((ret || msg_out.header.status != 0x0) && ++trials < 3)
> > +               goto try_again;
> > +
> > +       if (ret)
> > +               drm_err(&i915->drm, "Failed to send tee msg for inv-stream-key-%d, ret=[%d]\n",
> > +                       session_id, ret);
> > +       else if (msg_out.header.status != 0x0)
> > +               drm_warn(&i915->drm, "PXP firmware failed inv-stream-key-%d with status 0x%08x\n",
> > +                        session_id, msg_out.header.status);
> > +}
> > -- 
> > 2.39.0
> > 


^ permalink raw reply	[flat|nested] 37+ messages in thread
* Re: [PATCH v5 5/6] drm/i915/pxp: Trigger the global teardown for before suspending
  2023-01-19 19:35     ` [Intel-gfx] " Rodrigo Vivi
@ 2023-01-19 22:31       ` Teres Alexis, Alan Previn
  -1 siblings, 0 replies; 37+ messages in thread
From: Teres Alexis, Alan Previn @ 2023-01-19 22:31 UTC (permalink / raw)
  To: Vivi, Rodrigo
  Cc: justonli, intel-gfx, Usyskin, Alexander, dri-devel,
	Ceraolo Spurio, Daniele, gregkh, Vivi, Winkler, Tomas

Thanks for reviewing - responses below.

On Thu, 2023-01-19 at 14:35 -0500, Vivi, Rodrigo wrote:
> On Thu, Jan 12, 2023 at 05:18:49PM -0800, Alan Previn wrote:
> > A driver bug was recently discovered where the security firmware was
> > receiving internal HW signals indicating that session key expirations
> > had occurred. Architecturally, the firmware was expecting a response
> > from the GuC to acknowledge the event with the firmware side.
> > However the OS was in a suspended state and GuC had been reset.
> > 
> > Internal specifications actually required the driver to ensure
> > that all active sessions be properly cleaned up in such cases where
> > the system is suspended and the GuC potentially unable to respond.
> > 
> > This patch adds the global teardown code in i915's suspend_prepare
> > code path.
> > 
> > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > Reviewed-by: Juston Li <justonli@chromium.org>
> > 
Alan: [snip]
> >  
> > +static int __pxp_global_teardown_locked(struct intel_pxp *pxp, bool terminate_for_cleanup)
> > +{
> > +       if (terminate_for_cleanup) {
> > +               if (!pxp->arb_is_valid)
> > +                       return 0;
> > +               /*
> > +                * To ensure synchronous and coherent session teardown completion
> > +                * in response to suspend or shutdown triggers, don't use a worker.
> > +                */
> > +               intel_pxp_mark_termination_in_progress(pxp);
> > +               intel_pxp_terminate(pxp, false);
> > +       } else {
> > +               if (pxp->arb_is_valid)
> > +                       return 0;
> > +               /*
> > +                * If we are not in final termination, and the arb-session is currently
> > +                * inactive, we are doing a reset and restart due to some runtime event.
> > +                * Use the worker that was designed for this.
> > +                */
> > +               pxp_queue_termination(pxp);
> > +       }
> 
> I really don't see why you need 1 function for totally 2 different cases.
> Why not 2 functions then?
> 

Alan: I don't see why not ;) My goal with above method was was to concentrate the teardown steps in a single function so if future changes are required, we can keep it in this single function entry point. For now i will assume that was a nack so i shall split it on next rev.

> > +
> > +       if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250)))
> > +               return -ETIMEDOUT;
> > +
> > +       return 0;
> > +}
> > +
> > 
Alan: [snip]

> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > index 9658d3005222..3ded0890cd27 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > @@ -27,6 +27,7 @@ void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
> >  void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id);
> >  
> >  int intel_pxp_start(struct intel_pxp *pxp);
> > +void intel_pxp_end(struct intel_pxp *pxp);
> >  
> >  int intel_pxp_key_check(struct intel_pxp *pxp,
> >                         struct drm_i915_gem_object *obj,
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> > index 892d39cc61c1..e427464aa131 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> > @@ -16,7 +16,7 @@ void intel_pxp_suspend_prepare(struct intel_pxp *pxp)
> >         if (!intel_pxp_is_enabled(pxp))
> >                 return;
> >  
> > -       pxp->arb_is_valid = false;
> > +       intel_pxp_end(pxp);
> >  
> >         intel_pxp_invalidate(pxp);
> >  }
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> > index 74ed7e16e481..d8278c4002e3 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> > @@ -115,11 +115,14 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
> >         return ret;
> >  }
> >  
> > -static void pxp_terminate(struct intel_pxp *pxp)
> > +void intel_pxp_terminate(struct intel_pxp *pxp, bool restart_arb)
> >  {
> >         int ret;
> >  
> > -       pxp->hw_state_invalidated = true;
> > +       if (restart_arb)
> > +               pxp->hw_state_invalidated = true;
> > +       else
> > +               pxp->hw_state_invalidated = false;
> 
> o.O
> 
> pxp->hw_state_invalidate = restart_arb;
Alan: duhhhh... (my bad)
> 
> ?
> 
> or even a better name for the restart_arb to already indicate that is
> the hw_state_invalidate ?
> 
Alan: hmmm... you something mean like:

    hw_state_invalidated = post_invalidation_needs_restart;


Alan: actually i wish we couold redo "hw_state_invalidate" which is currently defined
as a boolean that only means one thing -> teardown and restart. It would be more scalable
if we can replace it with a bitmask of "current + (infered)pending state" with a documented
state-machine with a fixed set of state-transition paths. 

INACTIVE----> STARTING----> ACTIVE ----> TEARDOWN_RESTART--->|
      ^           ^               |                          |
      |           |               |                          V
      |           |<--------------)----------<---------------|
      |                           |
      |                           |-----> TEARDOWN_END---->--|
      |                                                      V
      |<-----------------<----------------<------------------|


However, I didn't do this initially because it would mean a wider set of changes that might
take more time to test and review (downstream customers impacts) but for only 5 states but
where only 2 of em are impacted by this change. For now i shall go with the simpler name change
as you hint above - unless you request this instead.

Alan: [snip]

^ permalink raw reply	[flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5 5/6] drm/i915/pxp: Trigger the global teardown for before suspending
@ 2023-01-19 22:31       ` Teres Alexis, Alan Previn
  0 siblings, 0 replies; 37+ messages in thread
From: Teres Alexis, Alan Previn @ 2023-01-19 22:31 UTC (permalink / raw)
  To: Vivi, Rodrigo
  Cc: intel-gfx, Usyskin, Alexander, dri-devel, gregkh, Vivi, Winkler, Tomas

Thanks for reviewing - responses below.

On Thu, 2023-01-19 at 14:35 -0500, Vivi, Rodrigo wrote:
> On Thu, Jan 12, 2023 at 05:18:49PM -0800, Alan Previn wrote:
> > A driver bug was recently discovered where the security firmware was
> > receiving internal HW signals indicating that session key expirations
> > had occurred. Architecturally, the firmware was expecting a response
> > from the GuC to acknowledge the event with the firmware side.
> > However the OS was in a suspended state and GuC had been reset.
> > 
> > Internal specifications actually required the driver to ensure
> > that all active sessions be properly cleaned up in such cases where
> > the system is suspended and the GuC potentially unable to respond.
> > 
> > This patch adds the global teardown code in i915's suspend_prepare
> > code path.
> > 
> > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > Reviewed-by: Juston Li <justonli@chromium.org>
> > 
Alan: [snip]
> >  
> > +static int __pxp_global_teardown_locked(struct intel_pxp *pxp, bool terminate_for_cleanup)
> > +{
> > +       if (terminate_for_cleanup) {
> > +               if (!pxp->arb_is_valid)
> > +                       return 0;
> > +               /*
> > +                * To ensure synchronous and coherent session teardown completion
> > +                * in response to suspend or shutdown triggers, don't use a worker.
> > +                */
> > +               intel_pxp_mark_termination_in_progress(pxp);
> > +               intel_pxp_terminate(pxp, false);
> > +       } else {
> > +               if (pxp->arb_is_valid)
> > +                       return 0;
> > +               /*
> > +                * If we are not in final termination, and the arb-session is currently
> > +                * inactive, we are doing a reset and restart due to some runtime event.
> > +                * Use the worker that was designed for this.
> > +                */
> > +               pxp_queue_termination(pxp);
> > +       }
> 
> I really don't see why you need 1 function for totally 2 different cases.
> Why not 2 functions then?
> 

Alan: I don't see why not ;) My goal with above method was was to concentrate the teardown steps in a single function so if future changes are required, we can keep it in this single function entry point. For now i will assume that was a nack so i shall split it on next rev.

> > +
> > +       if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250)))
> > +               return -ETIMEDOUT;
> > +
> > +       return 0;
> > +}
> > +
> > 
Alan: [snip]

> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > index 9658d3005222..3ded0890cd27 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > @@ -27,6 +27,7 @@ void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
> >  void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id);
> >  
> >  int intel_pxp_start(struct intel_pxp *pxp);
> > +void intel_pxp_end(struct intel_pxp *pxp);
> >  
> >  int intel_pxp_key_check(struct intel_pxp *pxp,
> >                         struct drm_i915_gem_object *obj,
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> > index 892d39cc61c1..e427464aa131 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> > @@ -16,7 +16,7 @@ void intel_pxp_suspend_prepare(struct intel_pxp *pxp)
> >         if (!intel_pxp_is_enabled(pxp))
> >                 return;
> >  
> > -       pxp->arb_is_valid = false;
> > +       intel_pxp_end(pxp);
> >  
> >         intel_pxp_invalidate(pxp);
> >  }
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> > index 74ed7e16e481..d8278c4002e3 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> > @@ -115,11 +115,14 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
> >         return ret;
> >  }
> >  
> > -static void pxp_terminate(struct intel_pxp *pxp)
> > +void intel_pxp_terminate(struct intel_pxp *pxp, bool restart_arb)
> >  {
> >         int ret;
> >  
> > -       pxp->hw_state_invalidated = true;
> > +       if (restart_arb)
> > +               pxp->hw_state_invalidated = true;
> > +       else
> > +               pxp->hw_state_invalidated = false;
> 
> o.O
> 
> pxp->hw_state_invalidate = restart_arb;
Alan: duhhhh... (my bad)
> 
> ?
> 
> or even a better name for the restart_arb to already indicate that is
> the hw_state_invalidate ?
> 
Alan: hmmm... you something mean like:

    hw_state_invalidated = post_invalidation_needs_restart;


Alan: actually i wish we couold redo "hw_state_invalidate" which is currently defined
as a boolean that only means one thing -> teardown and restart. It would be more scalable
if we can replace it with a bitmask of "current + (infered)pending state" with a documented
state-machine with a fixed set of state-transition paths. 

INACTIVE----> STARTING----> ACTIVE ----> TEARDOWN_RESTART--->|
      ^           ^               |                          |
      |           |               |                          V
      |           |<--------------)----------<---------------|
      |                           |
      |                           |-----> TEARDOWN_END---->--|
      |                                                      V
      |<-----------------<----------------<------------------|


However, I didn't do this initially because it would mean a wider set of changes that might
take more time to test and review (downstream customers impacts) but for only 5 states but
where only 2 of em are impacted by this change. For now i shall go with the simpler name change
as you hint above - unless you request this instead.

Alan: [snip]

^ permalink raw reply	[flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5 2/6] drm/i915/pxp: add device link between i915 and mei_pxp
  2023-01-19 19:25   ` Rodrigo Vivi
@ 2023-01-19 23:01     ` Teres Alexis, Alan Previn
  2023-01-22  6:57       ` Usyskin, Alexander
  1 sibling, 0 replies; 37+ messages in thread
From: Teres Alexis, Alan Previn @ 2023-01-19 23:01 UTC (permalink / raw)
  To: Vivi, Rodrigo
  Cc: gregkh, intel-gfx, Usyskin, Alexander, dri-devel, Vivi, Winkler, Tomas

Thanks for reviewing the patch. I will fix the code according to your recommendation.
I assume we could probably go with -ENOLINK as the error (instead of -ENOMEM).
However, i'll wait for Alexander to respond on whether he needs drm_WARN_ON and your question on RPM.

...alan

On Thu, 2023-01-19 at 14:25 -0500, Vivi, Rodrigo wrote:
> On Thu, Jan 12, 2023 at 05:18:46PM -0800, Alan Previn wrote:
> > From: Alexander Usyskin <alexander.usyskin@intel.com>
> > 
> > Add device link with i915 as consumer and mei_pxp as supplier
> > to ensure proper ordering of power flows.
> > 
> > V2: condition on absence of heci_pxp to filter out DG
> > 
> > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > ---
> >  drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
Alan: [snip]

^ permalink raw reply	[flat|nested] 37+ messages in thread
* RE: [Intel-gfx] [PATCH v5 2/6] drm/i915/pxp: add device link between i915 and mei_pxp
  2023-01-19 19:25   ` Rodrigo Vivi
@ 2023-01-22  6:57       ` Usyskin, Alexander
  2023-01-22  6:57       ` Usyskin, Alexander
  1 sibling, 0 replies; 37+ messages in thread
From: Usyskin, Alexander @ 2023-01-22  6:57 UTC (permalink / raw)
  To: Vivi, Rodrigo, Teres Alexis, Alan Previn
  Cc: Greg Kroah-Hartman, intel-gfx, Winkler, Tomas, Vivi, dri-devel

> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > index d50354bfb993..bef6d7f8ac55 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > @@ -127,6 +127,10 @@ static int i915_pxp_tee_component_bind(struct
> device *i915_kdev,
> >  	intel_wakeref_t wakeref;
> >  	int ret = 0;
> >
> > +	if (!HAS_HECI_PXP(i915) &&
> > +	    drm_WARN_ON(&i915->drm, !device_link_add(i915_kdev,
> tee_kdev,
> 
> I don't like the action here hidden behind the drm_WARN_ON.
> Please notice that almost every use of this and other helpers like
> this expect the param as a failure. Not an actual action. So,
> most of lazy readers like me might ignore that the main function
> is actually a param inside  this warn condition.
> 
Honestly, copy-pasted from drivers/gpu/drm/i915/display/intel_audio.c +1266
I don't have deep knowledge of drm macros, so thought this should be ok.
Can make it any other form that acceptable in drm tree...

> We should probably stash the link as well...
> 
> pxp->dev_link = device_link_add(i915_kdev, tee_kdev,...);
> 
> so in the end below, instead of checking the HAS_HECI_PXP again
> and use the remove version you check the dev_link and use the del
> function.
> 
> something like:
> 
> if (pxp->dev_link)
>    device_link_del(pxp->dev_link);
> 
Not sure that this simplification warrants additional clutter in struct.

> Also, do you really need the WARN to see the stack when this happens
> or you already know the callers?
> Why not a simple drm_error msg?
> 
> if (!HAS_HECI_PXP(i915) {
> 	pxp->dev_link = device_link_add(i915_kdev, tee_kdev,...);
> 	if (!pxp->dev_link) {
> 	   drm_error();
> 	   return -ESOMETHING;
> 
> >  DL_FLAG_STATELESS)))
> 
> do we need the RPM in sync as well?
> I mean:
> 
> DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME)))
> 
> ?

No, the mei device should not be active all the time when i915 is active, only when pxp requires it.

> 
> > +		return -ENOMEM;
> 
> why ENOMEM?
Copy-paste from i915 audio.

> 
> > +
> >  	mutex_lock(&pxp->tee_mutex);
> >  	pxp->pxp_component = data;
> >  	pxp->pxp_component->tee_dev = tee_kdev;
> > @@ -169,6 +173,9 @@ static void i915_pxp_tee_component_unbind(struct
> device *i915_kdev,
> >  	mutex_lock(&pxp->tee_mutex);
> >  	pxp->pxp_component = NULL;
> >  	mutex_unlock(&pxp->tee_mutex);
> > +
> > +	if (!HAS_HECI_PXP(i915))
> > +		device_link_remove(i915_kdev, tee_kdev);
> >  }
> >
> >  static const struct component_ops i915_pxp_tee_component_ops = {
> > --
> > 2.39.0
> >

^ permalink raw reply	[flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5 2/6] drm/i915/pxp: add device link between i915 and mei_pxp
@ 2023-01-22  6:57       ` Usyskin, Alexander
  0 siblings, 0 replies; 37+ messages in thread
From: Usyskin, Alexander @ 2023-01-22  6:57 UTC (permalink / raw)
  To: Vivi, Rodrigo, Teres Alexis, Alan Previn
  Cc: Greg Kroah-Hartman, intel-gfx, Winkler, Tomas, Vivi, dri-devel

> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > index d50354bfb993..bef6d7f8ac55 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > @@ -127,6 +127,10 @@ static int i915_pxp_tee_component_bind(struct
> device *i915_kdev,
> >  	intel_wakeref_t wakeref;
> >  	int ret = 0;
> >
> > +	if (!HAS_HECI_PXP(i915) &&
> > +	    drm_WARN_ON(&i915->drm, !device_link_add(i915_kdev,
> tee_kdev,
> 
> I don't like the action here hidden behind the drm_WARN_ON.
> Please notice that almost every use of this and other helpers like
> this expect the param as a failure. Not an actual action. So,
> most of lazy readers like me might ignore that the main function
> is actually a param inside  this warn condition.
> 
Honestly, copy-pasted from drivers/gpu/drm/i915/display/intel_audio.c +1266
I don't have deep knowledge of drm macros, so thought this should be ok.
Can make it any other form that acceptable in drm tree...

> We should probably stash the link as well...
> 
> pxp->dev_link = device_link_add(i915_kdev, tee_kdev,...);
> 
> so in the end below, instead of checking the HAS_HECI_PXP again
> and use the remove version you check the dev_link and use the del
> function.
> 
> something like:
> 
> if (pxp->dev_link)
>    device_link_del(pxp->dev_link);
> 
Not sure that this simplification warrants additional clutter in struct.

> Also, do you really need the WARN to see the stack when this happens
> or you already know the callers?
> Why not a simple drm_error msg?
> 
> if (!HAS_HECI_PXP(i915) {
> 	pxp->dev_link = device_link_add(i915_kdev, tee_kdev,...);
> 	if (!pxp->dev_link) {
> 	   drm_error();
> 	   return -ESOMETHING;
> 
> >  DL_FLAG_STATELESS)))
> 
> do we need the RPM in sync as well?
> I mean:
> 
> DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME)))
> 
> ?

No, the mei device should not be active all the time when i915 is active, only when pxp requires it.

> 
> > +		return -ENOMEM;
> 
> why ENOMEM?
Copy-paste from i915 audio.

> 
> > +
> >  	mutex_lock(&pxp->tee_mutex);
> >  	pxp->pxp_component = data;
> >  	pxp->pxp_component->tee_dev = tee_kdev;
> > @@ -169,6 +173,9 @@ static void i915_pxp_tee_component_unbind(struct
> device *i915_kdev,
> >  	mutex_lock(&pxp->tee_mutex);
> >  	pxp->pxp_component = NULL;
> >  	mutex_unlock(&pxp->tee_mutex);
> > +
> > +	if (!HAS_HECI_PXP(i915))
> > +		device_link_remove(i915_kdev, tee_kdev);
> >  }
> >
> >  static const struct component_ops i915_pxp_tee_component_ops = {
> > --
> > 2.39.0
> >

^ permalink raw reply	[flat|nested] 37+ messages in thread
* RE: [Intel-gfx] [PATCH v5 2/6] drm/i915/pxp: add device link between i915 and mei_pxp
  2023-01-22  6:57       ` Usyskin, Alexander
@ 2023-01-23 12:43         ` Jani Nikula
  -1 siblings, 0 replies; 37+ messages in thread
From: Jani Nikula @ 2023-01-23 12:43 UTC (permalink / raw)
  To: Usyskin, Alexander, Vivi, Rodrigo, Teres Alexis, Alan Previn
  Cc: dri-devel, Greg Kroah-Hartman, intel-gfx, Winkler, Tomas, Vivi

On Sun, 22 Jan 2023, "Usyskin, Alexander" <alexander.usyskin@intel.com> wrote:
>> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
>> b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
>> > index d50354bfb993..bef6d7f8ac55 100644
>> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
>> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
>> > @@ -127,6 +127,10 @@ static int i915_pxp_tee_component_bind(struct
>> device *i915_kdev,
>> >  	intel_wakeref_t wakeref;
>> >  	int ret = 0;
>> >
>> > +	if (!HAS_HECI_PXP(i915) &&
>> > +	    drm_WARN_ON(&i915->drm, !device_link_add(i915_kdev,
>> tee_kdev,
>> 
>> I don't like the action here hidden behind the drm_WARN_ON.
>> Please notice that almost every use of this and other helpers like
>> this expect the param as a failure. Not an actual action. So,
>> most of lazy readers like me might ignore that the main function
>> is actually a param inside  this warn condition.
>> 
> Honestly, copy-pasted from drivers/gpu/drm/i915/display/intel_audio.c +1266
> I don't have deep knowledge of drm macros, so thought this should be ok.
> Can make it any other form that acceptable in drm tree...

Unfortunately, some pattern being present in the driver does not mean
it's a good example to be emulated. If we copy a bad pattern, it seems
more acceptable, and even more people will copy it.


BR,
Jani.

>
>> We should probably stash the link as well...
>> 
>> pxp->dev_link = device_link_add(i915_kdev, tee_kdev,...);
>> 
>> so in the end below, instead of checking the HAS_HECI_PXP again
>> and use the remove version you check the dev_link and use the del
>> function.
>> 
>> something like:
>> 
>> if (pxp->dev_link)
>>    device_link_del(pxp->dev_link);
>> 
> Not sure that this simplification warrants additional clutter in struct.
>
>> Also, do you really need the WARN to see the stack when this happens
>> or you already know the callers?
>> Why not a simple drm_error msg?
>> 
>> if (!HAS_HECI_PXP(i915) {
>> 	pxp->dev_link = device_link_add(i915_kdev, tee_kdev,...);
>> 	if (!pxp->dev_link) {
>> 	   drm_error();
>> 	   return -ESOMETHING;
>> 
>> >  DL_FLAG_STATELESS)))
>> 
>> do we need the RPM in sync as well?
>> I mean:
>> 
>> DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME)))
>> 
>> ?
>
> No, the mei device should not be active all the time when i915 is active, only when pxp requires it.
>
>> 
>> > +		return -ENOMEM;
>> 
>> why ENOMEM?
> Copy-paste from i915 audio.
>
>> 
>> > +
>> >  	mutex_lock(&pxp->tee_mutex);
>> >  	pxp->pxp_component = data;
>> >  	pxp->pxp_component->tee_dev = tee_kdev;
>> > @@ -169,6 +173,9 @@ static void i915_pxp_tee_component_unbind(struct
>> device *i915_kdev,
>> >  	mutex_lock(&pxp->tee_mutex);
>> >  	pxp->pxp_component = NULL;
>> >  	mutex_unlock(&pxp->tee_mutex);
>> > +
>> > +	if (!HAS_HECI_PXP(i915))
>> > +		device_link_remove(i915_kdev, tee_kdev);
>> >  }
>> >
>> >  static const struct component_ops i915_pxp_tee_component_ops = {
>> > --
>> > 2.39.0
>> >

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5 2/6] drm/i915/pxp: add device link between i915 and mei_pxp
@ 2023-01-23 12:43         ` Jani Nikula
  0 siblings, 0 replies; 37+ messages in thread
From: Jani Nikula @ 2023-01-23 12:43 UTC (permalink / raw)
  To: Usyskin, Alexander, Vivi, Rodrigo, Teres Alexis, Alan Previn
  Cc: dri-devel, Greg Kroah-Hartman, intel-gfx, Winkler, Tomas, Vivi

On Sun, 22 Jan 2023, "Usyskin, Alexander" <alexander.usyskin@intel.com> wrote:
>> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
>> b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
>> > index d50354bfb993..bef6d7f8ac55 100644
>> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
>> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
>> > @@ -127,6 +127,10 @@ static int i915_pxp_tee_component_bind(struct
>> device *i915_kdev,
>> >  	intel_wakeref_t wakeref;
>> >  	int ret = 0;
>> >
>> > +	if (!HAS_HECI_PXP(i915) &&
>> > +	    drm_WARN_ON(&i915->drm, !device_link_add(i915_kdev,
>> tee_kdev,
>> 
>> I don't like the action here hidden behind the drm_WARN_ON.
>> Please notice that almost every use of this and other helpers like
>> this expect the param as a failure. Not an actual action. So,
>> most of lazy readers like me might ignore that the main function
>> is actually a param inside  this warn condition.
>> 
> Honestly, copy-pasted from drivers/gpu/drm/i915/display/intel_audio.c +1266
> I don't have deep knowledge of drm macros, so thought this should be ok.
> Can make it any other form that acceptable in drm tree...

Unfortunately, some pattern being present in the driver does not mean
it's a good example to be emulated. If we copy a bad pattern, it seems
more acceptable, and even more people will copy it.


BR,
Jani.

>
>> We should probably stash the link as well...
>> 
>> pxp->dev_link = device_link_add(i915_kdev, tee_kdev,...);
>> 
>> so in the end below, instead of checking the HAS_HECI_PXP again
>> and use the remove version you check the dev_link and use the del
>> function.
>> 
>> something like:
>> 
>> if (pxp->dev_link)
>>    device_link_del(pxp->dev_link);
>> 
> Not sure that this simplification warrants additional clutter in struct.
>
>> Also, do you really need the WARN to see the stack when this happens
>> or you already know the callers?
>> Why not a simple drm_error msg?
>> 
>> if (!HAS_HECI_PXP(i915) {
>> 	pxp->dev_link = device_link_add(i915_kdev, tee_kdev,...);
>> 	if (!pxp->dev_link) {
>> 	   drm_error();
>> 	   return -ESOMETHING;
>> 
>> >  DL_FLAG_STATELESS)))
>> 
>> do we need the RPM in sync as well?
>> I mean:
>> 
>> DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME)))
>> 
>> ?
>
> No, the mei device should not be active all the time when i915 is active, only when pxp requires it.
>
>> 
>> > +		return -ENOMEM;
>> 
>> why ENOMEM?
> Copy-paste from i915 audio.
>
>> 
>> > +
>> >  	mutex_lock(&pxp->tee_mutex);
>> >  	pxp->pxp_component = data;
>> >  	pxp->pxp_component->tee_dev = tee_kdev;
>> > @@ -169,6 +173,9 @@ static void i915_pxp_tee_component_unbind(struct
>> device *i915_kdev,
>> >  	mutex_lock(&pxp->tee_mutex);
>> >  	pxp->pxp_component = NULL;
>> >  	mutex_unlock(&pxp->tee_mutex);
>> > +
>> > +	if (!HAS_HECI_PXP(i915))
>> > +		device_link_remove(i915_kdev, tee_kdev);
>> >  }
>> >
>> >  static const struct component_ops i915_pxp_tee_component_ops = {
>> > --
>> > 2.39.0
>> >

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5 2/6] drm/i915/pxp: add device link between i915 and mei_pxp
  2023-01-22  6:57       ` Usyskin, Alexander
  (?)
  (?)
@ 2023-01-23 14:31       ` Rodrigo Vivi
  2023-01-24  1:53         ` Teres Alexis, Alan Previn
  -1 siblings, 1 reply; 37+ messages in thread
From: Rodrigo Vivi @ 2023-01-23 14:31 UTC (permalink / raw)
  To: Usyskin, Alexander
  Cc: Teres Alexis, Alan Previn, Greg Kroah-Hartman, intel-gfx,
	dri-devel, Vivi, Winkler, Tomas

On Sun, Jan 22, 2023 at 06:57:24AM +0000, Usyskin, Alexander wrote:
> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > > index d50354bfb993..bef6d7f8ac55 100644
> > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > > @@ -127,6 +127,10 @@ static int i915_pxp_tee_component_bind(struct
> > device *i915_kdev,
> > >  	intel_wakeref_t wakeref;
> > >  	int ret = 0;
> > >
> > > +	if (!HAS_HECI_PXP(i915) &&
> > > +	    drm_WARN_ON(&i915->drm, !device_link_add(i915_kdev,
> > tee_kdev,
> > 
> > I don't like the action here hidden behind the drm_WARN_ON.
> > Please notice that almost every use of this and other helpers like
> > this expect the param as a failure. Not an actual action. So,
> > most of lazy readers like me might ignore that the main function
> > is actually a param inside  this warn condition.
> > 
> Honestly, copy-pasted from drivers/gpu/drm/i915/display/intel_audio.c +1266
> I don't have deep knowledge of drm macros, so thought this should be ok.
> Can make it any other form that acceptable in drm tree...

something like I suggested in my previous reply please.

> 
> > We should probably stash the link as well...
> > 
> > pxp->dev_link = device_link_add(i915_kdev, tee_kdev,...);
> > 
> > so in the end below, instead of checking the HAS_HECI_PXP again
> > and use the remove version you check the dev_link and use the del
> > function.
> > 
> > something like:
> > 
> > if (pxp->dev_link)
> >    device_link_del(pxp->dev_link);
> > 
> Not sure that this simplification warrants additional clutter in struct.
> 
> > Also, do you really need the WARN to see the stack when this happens
> > or you already know the callers?
> > Why not a simple drm_error msg?
> > 
> > if (!HAS_HECI_PXP(i915) {
> > 	pxp->dev_link = device_link_add(i915_kdev, tee_kdev,...);
> > 	if (!pxp->dev_link) {
> > 	   drm_error();
> > 	   return -ESOMETHING;
> > 
> > >  DL_FLAG_STATELESS)))
> > 
> > do we need the RPM in sync as well?
> > I mean:
> > 
> > DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME)))
> > 
> > ?
> 
> No, the mei device should not be active all the time when i915 is active, only when pxp requires it.

okay then

> 
> > 
> > > +		return -ENOMEM;
> > 
> > why ENOMEM?
> Copy-paste from i915 audio.

It doesn't make sense to me.
Looking to other derivers -ENODEV or -EINVAL seems to be
the most used choices...

looking to the error codes probably ECHILD would make sense
but no one is using it... so let's stay with ENODEV?!

> 
> > 
> > > +
> > >  	mutex_lock(&pxp->tee_mutex);
> > >  	pxp->pxp_component = data;
> > >  	pxp->pxp_component->tee_dev = tee_kdev;
> > > @@ -169,6 +173,9 @@ static void i915_pxp_tee_component_unbind(struct
> > device *i915_kdev,
> > >  	mutex_lock(&pxp->tee_mutex);
> > >  	pxp->pxp_component = NULL;
> > >  	mutex_unlock(&pxp->tee_mutex);
> > > +
> > > +	if (!HAS_HECI_PXP(i915))
> > > +		device_link_remove(i915_kdev, tee_kdev);
> > >  }
> > >
> > >  static const struct component_ops i915_pxp_tee_component_ops = {
> > > --
> > > 2.39.0
> > >

^ permalink raw reply	[flat|nested] 37+ messages in thread
* Re: [Intel-gfx] [PATCH v5 2/6] drm/i915/pxp: add device link between i915 and mei_pxp
  2023-01-23 14:31       ` Rodrigo Vivi
@ 2023-01-24  1:53         ` Teres Alexis, Alan Previn
  0 siblings, 0 replies; 37+ messages in thread
From: Teres Alexis, Alan Previn @ 2023-01-24  1:53 UTC (permalink / raw)
  To: Vivi, Rodrigo, Usyskin, Alexander
  Cc: gregkh, Vivi, intel-gfx, Winkler, Tomas, dri-devel


On Mon, 2023-01-23 at 09:31 -0500, Vivi, Rodrigo wrote:
> On Sun, Jan 22, 2023 at 06:57:24AM +0000, Usyskin, Alexander wrote:
> > > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > > b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > > > index d50354bfb993..bef6d7f8ac55 100644
> > > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > > > @@ -127,6 +127,10 @@ static int i915_pxp_tee_component_bind(struct
> > > 
alan:snip.

Thanks Jani, Rodrigo and Alex. I'll re-rev with all of Rodrigo's recommendation:
- will break down the initial code so we dont hide device-link behind drm_WARN_ON 
- since i didnt hear any hard objection - I will stick with Rodrigo's recommendation to stash the device-link.
- dont need DL_FLAG_PM_RUNTIME
- use -ENODEV

probably like this:

	if (!HAS_HECI_PXP(i915)) {
		pxp->component_dev_link = device_link_add(i915_kdev, tee_kdev, DL_FLAG_STATELESS);
		if (drm_WARN_ON(&i915->drm, !pxp->component_dev_link))
			return -ENODEV;
	}


alan:snip.

^ permalink raw reply	[flat|nested] 37+ messages in thread
end of thread, other threads:[~2023-01-24  1:53 UTC | newest]

Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-13  1:18 [PATCH v5 0/6] drm/i915/pxp: Add missing cleanup steps for PXP global-teardown Alan Previn
2023-01-13  1:18 ` [Intel-gfx] " Alan Previn
2023-01-13  1:18 ` [PATCH v5 1/6] mei: mei-me: resume device in prepare Alan Previn
2023-01-13  1:18   ` [Intel-gfx] " Alan Previn
2023-01-18 11:42   ` Winkler, Tomas
2023-01-18 11:42     ` [Intel-gfx] " Winkler, Tomas
2023-01-13  1:18 ` [PATCH v5 2/6] drm/i915/pxp: add device link between i915 and mei_pxp Alan Previn
2023-01-13  1:18   ` [Intel-gfx] " Alan Previn
2023-01-19 19:25   ` Rodrigo Vivi
2023-01-19 23:01     ` Teres Alexis, Alan Previn
2023-01-22  6:57     ` Usyskin, Alexander
2023-01-22  6:57       ` Usyskin, Alexander
2023-01-23 12:43       ` Jani Nikula
2023-01-23 12:43         ` Jani Nikula
2023-01-23 14:31       ` Rodrigo Vivi
2023-01-24  1:53         ` Teres Alexis, Alan Previn
2023-01-13  1:18 ` [PATCH v5 3/6] mei: clean pending read with vtag on bus Alan Previn
2023-01-13  1:18   ` [Intel-gfx] " Alan Previn
2023-01-18 11:43   ` Winkler, Tomas
2023-01-18 11:43     ` [Intel-gfx] " Winkler, Tomas
2023-01-13  1:18 ` [PATCH v5 4/6] drm/i915/pxp: Invalidate all PXP fw sessions during teardown Alan Previn
2023-01-13  1:18   ` [Intel-gfx] " Alan Previn
2023-01-19 19:28   ` Rodrigo Vivi
2023-01-19 22:00     ` Teres Alexis, Alan Previn
2023-01-13  1:18 ` [PATCH v5 5/6] drm/i915/pxp: Trigger the global teardown for before suspending Alan Previn
2023-01-13  1:18   ` [Intel-gfx] " Alan Previn
2023-01-19 19:35   ` Rodrigo Vivi
2023-01-19 19:35     ` [Intel-gfx] " Rodrigo Vivi
2023-01-19 22:31     ` Teres Alexis, Alan Previn
2023-01-19 22:31       ` [Intel-gfx] " Teres Alexis, Alan Previn
2023-01-13  1:18 ` [PATCH v5 6/6] drm/i915/pxp: Pxp hw init should be in resume_complete Alan Previn
2023-01-13  1:18   ` [Intel-gfx] " Alan Previn
2023-01-19 19:10   ` Ceraolo Spurio, Daniele
2023-01-19 19:10     ` [Intel-gfx] " Ceraolo Spurio, Daniele
2023-01-19 19:40     ` Rodrigo Vivi
2023-01-13  2:10 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/pxp: Add missing cleanup steps for PXP global-teardown Patchwork
2023-01-13  2:38 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.