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

[Gustavo Sousa]

Quoting Jani Nikula (2023-05-10 17:21:00)
>On Wed, 10 May 2023, Gustavo Sousa <gustavo.sousa at intel.com> wrote:
>> Quoting Jani Nikula (2023-05-10 10:22:08)
>>>On Tue, 09 May 2023, Gustavo Sousa <gustavo.sousa at 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 at 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.

Yep. I agree to your points. Let me work on a v2...

Thanks for the feedback!

--
Gustavo Sousa