Re: [PATCH v6 2/8] vpci: Refactor REGISTER_VPCI_INIT

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.06.2025 10:32, Chen, Jiqian wrote:
> On 2025/6/24 16:28, Jan Beulich wrote:
>> On 24.06.2025 10:26, Chen, Jiqian wrote:
>>> On 2025/6/24 16:17, Jan Beulich wrote:
>>>> On 24.06.2025 10:12, Chen, Jiqian wrote:
>>>>> On 2025/6/20 14:34, Jan Beulich wrote:
>>>>>> On 19.06.2025 08:14, Chen, Jiqian wrote:
>>>>>>> On 2025/6/18 22:33, Jan Beulich wrote:
>>>>>>>> On 12.06.2025 11:29, Jiqian Chen wrote:
>>>>>>>>> +} vpci_capability_t;
>>>>>>>>
>>>>>>>> As you have it here, ...
>>>>>>>>
>>>>>>>>> @@ -29,9 +30,22 @@ typedef int vpci_register_init_t(struct pci_dev 
>>>>>>>>> *dev);
>>>>>>>>>   */
>>>>>>>>>  #define VPCI_MAX_VIRT_DEV       (PCI_SLOT(~0) + 1)
>>>>>>>>>  
>>>>>>>>> -#define REGISTER_VPCI_INIT(x, p)                \
>>>>>>>>> -  static vpci_register_init_t *const x##_entry  \
>>>>>>>>> -               __used_section(".data.vpci." p) = (x)
>>>>>>>>> +#define REGISTER_VPCI_CAPABILITY(cap, finit, fclean, ext) \
>>>>>>>>> +    static const vpci_capability_t finit##_t = { \
>>>>>>>>
>>>>>>>> ... _t suffixes generally designate types. I don't think we should 
>>>>>>>> abuse
>>>>>>>> that suffix for an identifier of a variable.
>>>>>>> What do you think I should change to?
>>>>>>
>>>>>> Well, if you take my other advice, this question won't need answering, as
>>>>>> then you only need the ..._entry one.
>>>>>>
>>>>>> Btw, noticing only now - why is it finit that's used to derive the 
>>>>>> identifier?
>>>>>> With that, it could as well be fclean (leaving aside the fact that that's
>>>>>> optional). Imo the name would better be derived from cap, and it would 
>>>>>> better
>>>>>> also reflect the purpose of the variable.
>>>>> I considered this.
>>>>> I think it is easier to use finit, and finit contains the cap type, and 
>>>>> the main purpose of this struct is to initialize the cap.
>>>>
>>>> Yet identifier names should make sense for the object they name.
>>> OK. What's your suggestion about naming the entry?
>>
>> cap##_init or _##cap##_init for example.
> If so, I need to extend the parameter of REGISTER_VPCI_CAPABILITY since 
> current cap is number, not string.
> Maybe:
> REGISTER_VPCI_CAPABILITY (cap, cap_id, finit, fclean, ext)

Well, yes, in the helper macro you may need to take precautions. However, I was
wondering anyway why

REGISTER_VPCI_CAP(PCI_CAP_ID_MSI, init_msi, NULL);

would be necessary, when

REGISTER_VPCI_CAP(MSI, init_msi, NULL);

could do, using e.g.

#define REGISTER_VPCI_CAP(cap, finit, fclean) \
    REGISTER_VPCI_CAPABILITY(PCI_CAP_ID_##cap, finit, fclean, false)
#define REGISTER_VPCI_EXTCAP(cap, finit, fclean) \
    REGISTER_VPCI_CAPABILITY(PCI_EXT_CAP_ID_##cap, finit, fclean, true)

(other variations are possible, of course). Then you could easily derive the
identifier wanted (requiring another parameter to REGISTER_VPCI_CAPABILITY(),
yes).

Jan