Re: [PATCH v4 01/11] drm/xe: Simplify module initialization code

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Jul 28, 2025 at 09:47:39PM +0200, Michal Wajdeczko wrote:
> 
> 
> On 7/28/2025 9:35 PM, Rodrigo Vivi wrote:
> > On Sun, Jul 27, 2025 at 07:19:58PM +0200, Michal Wajdeczko wrote:
> >> There is no need to have extra checks and WARN() in the helpers
> >> as instead of an index of the entry with function pointers, we
> >> can pass pointer to the entry which we prepare directly in the
> >> main loop, that is guaranteed to be valid.
> >>
> >>   add/remove: 0/0 grow/shrink: 0/4 up/down: 0/-180 (-180)
> >>   Function                                     old     new   delta
> >>   xe_exit                                      109      79     -30
> >>   cleanup_module                               109      79     -30
> >>   xe_init                                      248     188     -60
> >>   init_module                                  248     188     -60
> >>   Total: Before=2774145, After=2773965, chg -0.01%
> >>
> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> >> ---
> >>  drivers/gpu/drm/xe/xe_module.c | 27 ++++++++++-----------------
> >>  1 file changed, 10 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_module.c
> >> index d9391bd08194..593bc9e5851a 100644
> >> --- a/drivers/gpu/drm/xe/xe_module.c
> >> +++ b/drivers/gpu/drm/xe/xe_module.c
> >> @@ -135,24 +135,17 @@ static const struct init_funcs init_funcs[] = {
> >>  	},
> >>  };
> >>  
> >> -static int __init xe_call_init_func(unsigned int i)
> >> +static int __init xe_call_init_func(const struct init_funcs *func)
> >>  {
> >> -	if (WARN_ON(i >= ARRAY_SIZE(init_funcs)))
> >> -		return 0;
> >> -	if (!init_funcs[i].init)
> >> -		return 0;
> >> -
> >> -	return init_funcs[i].init();
> >> +	if (func->init)
> >> +		return func->init();
> >> +	return 0;
> >>  }
> >>  
> >> -static void xe_call_exit_func(unsigned int i)
> >> +static void xe_call_exit_func(const struct init_funcs *func)
> >>  {
> >> -	if (WARN_ON(i >= ARRAY_SIZE(init_funcs)))
> >> -		return;
> >> -	if (!init_funcs[i].exit)
> >> -		return;
> >> -
> >> -	init_funcs[i].exit();
> >> +	if (func->exit)
> >> +		func->exit();
> >>  }
> >>  
> >>  static int __init xe_init(void)
> >> @@ -160,10 +153,10 @@ static int __init xe_init(void)
> >>  	int err, i;
> >>  
> >>  	for (i = 0; i < ARRAY_SIZE(init_funcs); i++) {
> >> -		err = xe_call_init_func(i);
> >> +		err = xe_call_init_func(&init_funcs[i]);
> > 
> > perhaps we can go further and avoid this extra function calling
> > directly here:
> > 
> > err = init_funcs[i].init();
> 
> it depends if we want to preserve support for unset .init
> (I assumed it was added on purpose)
> 
> > 
> >>  		if (err) {
> >>  			while (i--)
> >> -				xe_call_exit_func(i);
> >> +				xe_call_exit_func(&init_funcs[i]);
> > 
> > and
> > init_funcs[i].exit();
> > here ?
> 
> .exit is optional, so this will have to be:
> 
> if (init_funcs[i].exit)
> 	init_funcs[i].exit();
> 
> but likely compiler will do  the same with current code
> 
> > 
> >>  			return err;
> >>  		}
> >>  	}
> >> @@ -176,7 +169,7 @@ static void __exit xe_exit(void)
> >>  	int i;
> >>  
> >>  	for (i = ARRAY_SIZE(init_funcs) - 1; i >= 0; i--)
> >> -		xe_call_exit_func(i);
> >> +		xe_call_exit_func(&init_funcs[i]);
> > 
> > and here
> > init_funcs[i].exit();
> 
> since .exit is optional, better to keep one helper than
> duplicate code here 

agreed... keep the helpers than and the rv-b

> 
> > 
> > But either way is fine I guess...  up to you:
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> thanks!
> > 
> >>  }
> >>  
> >>  module_init(xe_init);
> >> -- 
> >> 2.47.1
> >>
>