Re: [Intel-xe] [PATCH] drm/xe: Call exit functions when xe_register_pci_driver() fails

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 10 May 2023, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> Quoting Jani Nikula (2023-05-10 10:22:08)
>>On Tue, 09 May 2023, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>>> Make sure all necessary exit functions are also called when
>>> xe_register_pci_driver() fails.
>>>
>>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>>> ---
>>>  drivers/gpu/drm/xe/xe_module.c | 16 ++++++++++------
>>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_module.c
>>> index 6860586ce7f8..9698875852d3 100644
>>> --- a/drivers/gpu/drm/xe/xe_module.c
>>> +++ b/drivers/gpu/drm/xe/xe_module.c
>>> @@ -53,14 +53,18 @@ static int __init xe_init(void)
>>>  
>>>          for (i = 0; i < ARRAY_SIZE(init_funcs); i++) {
>>>                  err = init_funcs[i].init();
>>> -                if (err) {
>>> -                        while (i--)
>>> -                                init_funcs[i].exit();
>>> -                        return err;
>>> -                }
>>> +                if (err)
>>> +                        break;
>>>          }
>>>  
>>> -        return xe_register_pci_driver();
>>> +        if (!err)
>>> +                err = xe_register_pci_driver();
>>
>>Why aren't xe_register_pci_driver() and xe_unregister_pci_driver()
>>init/exit functions in the array? It would avoid the special casing
>>here, and allow some init functions to be called after register.
>
> Hm... I would imagine that the intention would to make it explicit that
> xe_register_pci_driver() must be the latest one called and, if that was the
> case (which is probably false based on your comment), maybe adding a comment on
> the array entry would also suffice.

In i915, i915_pci_register_driver() is *not* the last one!

> So should we just add them to the array?
>
> One issue would to be related to the function names, which would be
> incompatible with using MAKE_INIT_EXIT_FUNCS(). Since those functions are only
> used in this file, I guess it would be okay to rename them to
> xe_pci_module_init() and xe_pci_module_exit() for consistency sake. Thoughts?

Personally, I don't like MAKE_INIT_EXIT_FUNCS(). It does force a certain
naming pattern, but it also throws off code cross-referencing tools. I
can't just go on top of MAKE_INIT_EXIT_FUNCS(hw_fence) and have my
editor find the definition, and neither can I find all the call sites of
xe_hw_fence_module_init(). git grep doesn't fare any better. For me,
that's a productivity hit that's perpetually worse than having
init_funcs[] fully spelled out.

BR,
Jani.




>
> --
> Gustavo Sousa
>
>>
>>BR,
>>Jani.
>>
>>
>>> +
>>> +        if (err)
>>> +                while (i--)
>>> +                        init_funcs[i].exit();
>>> +
>>> +        return err;
>>>  }
>>>  
>>>  static void __exit xe_exit(void)
>>
>>-- 
>>Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center