| Message ID | 20220818174205.2412730-1-justonli@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/i915/pxp: don't start pxp without mei_pxp bind | expand |
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@chromium.org> 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. Anyway: Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> 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(>->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)
On Fri, Aug 19, 2022 at 4:53 AM Andrzej Hajda <andrzej.hajda@intel.com> 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@chromium.org> > > 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. > > Anyway: > Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> Thanks Andrzej! Any other comments Daniele? Otherwise, need some help from someone to merge this :) 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(>->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) >
On 8/23/2022 2:15 PM, Juston Li wrote: > On Fri, Aug 19, 2022 at 4:53 AM Andrzej Hajda <andrzej.hajda@intel.com> 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@chromium.org> >> 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@intel.com> > 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(>->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)
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(>->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)
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@chromium.org> --- drivers/gpu/drm/i915/pxp/intel_pxp.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)