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

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

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:
>>>>>> --- 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.
OK, got it, just the function parameters.

> 
>>>>>> +} 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?
What I can think of, it seems to need more work to derived from cap and will be 
more complex.

> 
> Jan

-- 
Best regards,
Jiqian Chen.