Re: [Intel-gfx] [PATCH v9 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

On Mon, 2022-12-05 at 21:06 -0800, Ceraolo Spurio, Daniele wrote:
> 
> On 12/5/2022 4:03 PM, Alan Previn wrote:

Alan:[snip]

> > @@ -39,18 +45,26 @@
> >    * performed via the mei_pxp component module.
> >    */
> >   
> > -struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp)
> > +bool intel_pxp_is_supported(const struct intel_pxp *pxp)
> >   {
> > -	return container_of(pxp, struct intel_gt, pxp);
> > +	if (!IS_ENABLED(CONFIG_DRM_I915_PXP))
> > +		return false;
> > +	else if (!pxp)
> > +		return false;
> 
> nit: this could be squashed in a single line:
> 
>      if (!IS_ENABLED(CONFIG_DRM_I915_PXP) || !pxp)
> 
> not a blocker
> 
> > +	return (INTEL_INFO(pxp->ctrl_gt->i915)->has_pxp && VDBOX_MASK(pxp->ctrl_gt));
> >   }
> >   
> >   bool intel_pxp_is_enabled(const struct intel_pxp *pxp)
> >   {
> > +	if (!pxp)
> > +		return false;
> >   	return pxp->ce;
> 
> nit: this can be squashed as:
> 
> return pxp && pxp->ce;
> 
> same for the is_active below. not a blocker
> 
> >   }
> >   
> >   bool intel_pxp_is_active(const struct intel_pxp *pxp)
> >   {
> > +	if (!pxp)
> > +		return false;
> >   	return pxp->arb_is_valid;
> >   }
> > 
> > 

Alan: okay - will handle some of these nits since i need to re-rev anyway
Alan:[snip]

> 
> 
> This comment doesn't really explain why we exclude the case with 
> media_gt. I would have preferred a line to explain that the module only 
> works for pre-media_gt platforms. not a blocker.
> 
Alan: Okay will add a comment.
Alan:[snip]

> > @@ -18,7 +18,7 @@
> >   
> >   int intel_pxp_huc_load_and_auth(struct intel_pxp *pxp)
> >   {
> > -	struct intel_gt *gt = pxp_to_gt(pxp);
> > +	struct intel_gt *gt = pxp->ctrl_gt;
> 
> This is called from outside the PXP code, so we need to check the pxp 
> pointer before de-referencing it (see also pxp->pxp_component further 
> down this function). It does look like the stack it's kind of circular 
> so it should be safe (pxp bind -> huc load -> this function), but IMO 
> better stick to the rule that all functions called from outside need a 
> check.
> 
> With this fixed:
> 
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
Alan:Thanks will do - will respin rev10
Alan:[snip]