Re: [PATCH v2 05/11] xen/arm: Mark device as PCI while creating one

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.09.2021 12:04, Oleksandr Andrushchenko wrote:
> 
> On 27.09.21 13:00, Jan Beulich wrote:
>> On 27.09.2021 11:35, Oleksandr Andrushchenko wrote:
>>> On 27.09.21 12:19, Jan Beulich wrote:
>>>> On 27.09.2021 10:45, Oleksandr Andrushchenko wrote:
>>>>> On 27.09.21 10:45, Jan Beulich wrote:
>>>>>> On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
>>>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>>>> @@ -328,6 +328,9 @@ static struct pci_dev *alloc_pdev(struct pci_seg 
>>>>>>> *pseg, u8 bus, u8 devfn)
>>>>>>>         *((u8*) &pdev->bus) = bus;
>>>>>>>         *((u8*) &pdev->devfn) = devfn;
>>>>>>>         pdev->domain = NULL;
>>>>>>> +#ifdef CONFIG_ARM
>>>>>>> +    pci_to_dev(pdev)->type = DEV_PCI;
>>>>>>> +#endif
>>>>>> I have to admit that I'm not happy about new CONFIG_<arch> conditionals
>>>>>> here. I'd prefer to see this done by a new arch helper, unless there are
>>>>>> obstacles I'm overlooking.
>>>>> Do you mean something like arch_pci_alloc_pdev(dev)?
>>>> I'd recommend against "alloc" in its name; "new" instead maybe?
>>> I am fine with arch_pci_new_pdev, but arch prefix points to the fact that
>>> this is just an architecture specific part of the pdev allocation rather 
>>> than
>>> actual pdev allocation itself, so with this respect arch_pci_alloc_pdev 
>>> seems
>>> more natural to me.
>> The bulk of the function is about populating the just allocated struct.
>> There's no arch-specific part of the allocation (so far, leaving aside
>> MSI-X), you only want and arch-specific part of the initialization. I
>> would agree with "alloc" in the name if further allocation was to
>> happen there.
> Hm, then arch_pci_init_pdev sounds more reasonable

Fine with me.

Jan