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

[Jan Beulich <jbeulich@xxxxxxxx>]

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:
>>>>> --- a/xen/include/xen/vpci.h
>>>>> +++ b/xen/include/xen/vpci.h
>>>>> @@ -13,11 +13,12 @@ typedef uint32_t vpci_read_t(const struct pci_dev 
>>>>> *pdev, unsigned int reg,
>>>>>  typedef void vpci_write_t(const struct pci_dev *pdev, unsigned int reg,
>>>>>                            uint32_t val, void *data);
>>>>>  
>>>>> -typedef int vpci_register_init_t(struct pci_dev *dev);
>>>>> -
>>>>> -#define VPCI_PRIORITY_HIGH      "1"
>>>>> -#define VPCI_PRIORITY_MIDDLE    "5"
>>>>> -#define VPCI_PRIORITY_LOW       "9"
>>>>> +typedef struct {
>>>>> +    unsigned int id;
>>>>> +    bool is_ext;
>>>>> +    int (*init)(struct pci_dev *pdev);
>>>>> +    int (*cleanup)(struct pci_dev *pdev);
>>>>
>>>> Is const really not possible to add to at least one of these two?
>>> Will change to be :
>>>
>>> typedef struct {
>>>     unsigned int id;
>>>     bool is_ext;
>>>     int (* const init)(struct pci_dev *pdev);
>>>     int (* const cleanup)(struct pci_dev *pdev);
>>> } vpci_capability_t;
>>
>> Ehm, no. The question was for the two function (pointer) parameters. "const"
>> on struct fields themselves can be useful, too, but for an entirely different
>> purpose.
> OK, will add const both for the struct field and the function parameters.

If you add (or rather keep) const for the struct field, the next question is
going to be why the other fields don't have const. Imo it's only the function
parameters which want it.

>>>>> +} 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.

Jan