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@xxxxxxxxx>
>
Alan:Thanks will do - will respin rev10
Alan:[snip]
