Thanks Rodrigo - agreed on everything below - will re-rev.
On Tue, 2023-08-15 at 09:51 -0400, Vivi, Rodrigo wrote:
> On Mon, Aug 14, 2023 at 06:12:10PM -0700, Alan Previn wrote:
> > When suspending, add a timeout when calling
> > intel_gt_pm_wait_for_idle else if we have a lost
> > G2H event that holds a wakeref (which would be
> >
[snip]
> > @@ -301,7 +303,10 @@ static void wait_for_suspend(struct intel_gt *gt)
> > intel_gt_retire_requests(gt);
> > }
> >
> > - intel_gt_pm_wait_for_idle(gt);
> > + /* we are suspending, so we shouldn't be waiting forever */
> > + if (intel_gt_pm_wait_timeout_for_idle(gt, timeout_ms) == -ETIME)
>
> you forgot to change the error code here..........................^
>
> but maybe we don't even need this here and a simple
> if (intel_gt_pm_wait_timeout_for_idle(gt, timeout_ms)) should be enough
> since the error from the killable one is unlikely and the only place
> I error I could check on that path would be a catastrophic -ERESTARTSYS.
>
> but up to you.
alan: my bad - I'll fix it - but i agree with not needing to check the failure
type.
and I'll keep the error the same ("bailing from...")
[snip]
> > +static inline int intel_gt_pm_wait_timeout_for_idle(struct intel_gt *gt,
> > int timeout_ms)
> > +{
> > + return intel_wakeref_wait_for_idle(>->wakeref, timeout_ms);
> > }
>
> I was going to ask why you created a single use function here, but then I
> noticed the above one. So it makes sense.
> Then I was going to ask why in here you didn't use the same change of
> timeout = 0 in the existent function like you used below, but then I
> noticed that the above function is called in multiple places and the
> patch with this change is much cleaner and the function is static inline
> so your approach was good here.
alan: yes that was my exact reason - thus no impact across other callers.
[snip]
> Please add a documentation for this function making sure you have the
> following
> mentions:
alan: good catch -will do.
>
> /**
> [snip]
> * @timeout_ms: Timeout in ums, 0 means never timeout.
> *
> * Returns 0 on success, -ETIMEDOUT upon a timeout, or the unlikely
> * error propagation from wait_var_event_killable if timeout_ms is 0.
> */
>
> with the return error fixed above and the documentation in place:
>
> Reviewed-by: Rodrigo Vivi <rodrigo.v...@intel.com>
>
> > -int intel_wakeref_wait_for_idle(struct intel_wakeref *wf)
> > +int intel_wakeref_wait_for_idle(struct intel_wakeref *wf, int timeout_ms)
> > {
> > - int err;
> >
[snip]
