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

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2025/6/24 16:44, Jan Beulich wrote:
> 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).
Seems better.
Thank you very much!

> 
> Jan

-- 
Best regards,
Jiqian Chen.