[Intel-gfx] [PATCH 14/38] drm/i915: Introduce the i915_user_extension_method

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-03-04 08:54:10)
> 
> On 01/03/2019 18:57, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-03-01 15:39:13)
> >>
> >> On 01/03/2019 14:03, Chris Wilson wrote:
> >>> +int i915_user_extensions(struct i915_user_extension __user *ext,
> >>> +                      const i915_user_extension_fn *tbl,
> >>> +                      unsigned long count,
> >>> +                      void *data)
> >>> +{
> >>> +     unsigned int stackdepth = 512;
> >>> +
> >>> +     while (ext) {
> >>> +             int err;
> >>> +             u64 x;
> >>> +
> >>> +             if (!stackdepth--) /* recursion vs useful flexibility */
> >>> +                     return -EINVAL;
> >>
> >> I don't get this. What stack? Did you mean "static unsigned int
> >> stackdepth" in case someone puts i915_user_extension into the extension
> >> table? Or just a limit on number of chained extensions? But you are not
> >> processing the recursively here.
> > 
> > It's iterative recursion :)
> > 
> > I still think of this loop in terms of its simple tail recursion.
> > 
> > And if we need to individual levels for unwind, that is a manual stack.
> Okay..
> 
> One thought I had - if unwind is too unwieldy, could we mandate each 
> user extension user has an additional output field in the embedding 
> struct which would hold the last processed extension id?

id? I guess we could do depth, though I guess pointer is more practical.
I don't think you mean ext.name as that we expect to repeat a few times.
 
> This way the state of the object would be less undefined after failure. 
> Userspace would be able to tell how far in the extension chain i915 
> managed to get to.

But I'm not convinced how practical that is. The answer is pretty much
always -EINVAL (anything else is much less ambiguous) and even then you
have no idea which field tripped EINVAL or why?
-Chris