Re: [PATCH v3] drm/i915/pxp: don't start pxp without mei_pxp bind

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@xxxxxxxxx>]

On 8/23/2022 2:15 PM, Juston Li wrote:
> On Fri, Aug 19, 2022 at 4:53 AM Andrzej Hajda <andrzej.hajda@xxxxxxxxx> wrote:
> > On 18.08.2022 19:42, Juston Li wrote:
> > > pxp will not start correctly until after mei_pxp bind completes and
> > > intel_pxp_init_hw() is called.
> > > Wait for the bind to complete before proceeding with startup.
> > >
> > > This fixes a race condition during bootup where we observed a small
> > > window for pxp commands to be sent, starting pxp before mei_pxp bind
> > > completed.
> > >
> > > Changes since v2:
> > > - wait for pxp_component to bind instead of returning -EAGAIN (Daniele)
> > >
> > > Changes since v1:
> > > - check pxp_component instead of pxp_component_added (Daniele)
> > > - pxp_component needs tee_mutex (Daniele)
> > > - return -EAGAIN so caller knows to retry (Daniele)
> > >
> > > Signed-off-by: Juston Li <justonli@xxxxxxxxxxxx>
> > In typical usage of component framework driver postpones initialization
> > till component is bound. In such case checking/waiting for component as
> > in this patch is not necessary and the code is more straightforward.
> > I wonder how it behaves on component unbind.

This component is only used for a specific use-case (content 
protection), so we don't want to hold back the whole graphics driver 
initialization for that. Unbind can only happen on suspend or driver 
removal and in both cases we're not accepting userspace submission at 
that point.

> > Anyway:
> > Reviewed-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
> Thanks Andrzej!
>
> Any other comments Daniele?
> Otherwise, need some help from someone to merge this :)

No other comments from me. I've pushed the patch to gt-next.

Thanks,
Daniele

> Thanks
> Juston
>
> > Regards
> > Andrzej
> >
> >
> > > ---
> > >    drivers/gpu/drm/i915/pxp/intel_pxp.c | 15 +++++++++++++++
> > >    1 file changed, 15 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > > index 15311eaed848..17109c513259 100644
> > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > > @@ -176,6 +176,18 @@ static void pxp_queue_termination(struct intel_pxp *pxp)
> > >        spin_unlock_irq(&gt->irq_lock);
> > >    }
> > >
> > > +static bool pxp_component_bound(struct intel_pxp *pxp)
> > > +{
> > > +     bool bound = false;
> > > +
> > > +     mutex_lock(&pxp->tee_mutex);
> > > +     if (pxp->pxp_component)
> > > +             bound = true;
> > > +     mutex_unlock(&pxp->tee_mutex);
> > > +
> > > +     return bound;
> > > +}
> > > +
> > >    /*
> > >     * the arb session is restarted from the irq work when we receive the
> > >     * termination completion interrupt
> > > @@ -187,6 +199,9 @@ int intel_pxp_start(struct intel_pxp *pxp)
> > >        if (!intel_pxp_is_enabled(pxp))
> > >                return -ENODEV;
> > >
> > > +     if (wait_for(pxp_component_bound(pxp), 250))
> > > +             return -ENXIO;
> > > +
> > >        mutex_lock(&pxp->arb_mutex);
> > >
> > >        if (pxp->arb_is_valid)