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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Jan