Re: [Intel-gfx] [PATCH v7 2/6] drm/i915/pxp: add device link between i915 and mei_pxp

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Jan 25, 2023 at 12:26:33AM -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   | 11 +++++++++++
>  drivers/gpu/drm/i915/pxp/intel_pxp_types.h |  3 +++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> index 73aa8015f828..b5bc0b87a1d0 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> @@ -127,6 +127,12 @@ static int i915_pxp_tee_component_bind(struct device *i915_kdev,
>  	intel_wakeref_t wakeref;
>  	int ret = 0;
>  
> +	if (!HAS_HECI_PXP(i915)) {
> +		pxp->dev_link = device_link_add(i915_kdev, tee_kdev, DL_FLAG_STATELESS);
> +		if (drm_WARN_ON(&i915->drm, !pxp->dev_link))
> +			return -ENODEV;
> +	}
> +
>  	mutex_lock(&pxp->tee_mutex);
>  	pxp->pxp_component = data;
>  	pxp->pxp_component->tee_dev = tee_kdev;
> @@ -169,6 +175,11 @@ 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 (pxp->dev_link) {
> +		device_link_del(pxp->dev_link);
> +		pxp->dev_link = NULL;

since no one else is doing the null assignment I thought this was taken care
by the core function, but it doesn't do that indeed.
Maybe in the most used case, folks are sure that this won't be hit without
having running the device_link_add previously, what may or may not be our case.

Anyway better safe than sorry, so let's keep it.

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

> +	}
>  }
>  
>  static const struct component_ops i915_pxp_tee_component_ops = {
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> index 7dc5f08d1583..007de49e1ea4 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> @@ -32,6 +32,9 @@ struct intel_pxp {
>  	 * which are protected by &tee_mutex.
>  	 */
>  	struct i915_pxp_component *pxp_component;
> +
> +	/* @dev_link: Enforce module relationship for power management ordering. */
> +	struct device_link *dev_link;
>  	/**
>  	 * @pxp_component_added: track if the pxp component has been added.
>  	 * Set and cleared in tee init and fini functions respectively.
> -- 
> 2.39.0
>