Re: [PATCH] xen/pci: replace call to is_memory_hole to pci_check_bar

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On Tue, Aug 9, 2022 at 7:26 PM Rahul Singh <Rahul.Singh@xxxxxxx> wrote:

Hi Julien,

Hello Julien, Rahul 

[sorry for possible format issues]

[snip]

 

>
> On 09/08/2022 16:22, Rahul Singh wrote:
>>> On 8 Aug 2022, at 4:30 pm, Oleksandr <olekstysh@xxxxxxxxx> wrote:
>>>> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
>>>> +{
>>>> +    int ret;
>>>> +    struct dt_device_node *dt_node;
>>>> +    struct device *dev = (struct device *)pci_to_dev(pdev);
>>>
>>>
>>> The cast is present here because of the const?
>> Yes you are right, cast is because of the const.
>>>
>>> I would consider passing "const struct pci_dev *pdev" instead of "struct device *dev" to pci_find_host_bridge_node() and dropping conversion (pci<->dev) in both functions.
>
> It looks like this function was added without any callers. The commit message claim there will be some. Can you (or Oleksandr) confirm this is not going to be problem for future patches?

I checked the whole PCI passthrough feature branch this function will be used when
we add iommu support for PCI device. 

 

Can confirm that, it will be called by the iommu code, as I understand there won't be an issue, the more, the exact place where the pci_find_host_bridge_node() will be called will have "pdev" in hand.

 

>
> That said, I agree that the conversion pci -> dev -> pci is pointless. So I would say if there are use case where we only have a 'dev' in hand, then we could ask the caller to do the conversation or we provide an helper if there are too many cases.
>
>> Yes make sense. I will do that in next version.
>
> While you are modifying the prototype for pci_find_host_bridge_node() can you consider to also constify the return (it should not be modified)?

Agree, I will constify the retrun also.

>
> In any case, the change suggested by Oleksandr should preferably be separate to this patch and added before.

Ack.

Regards,
Rahul

--

Regards,

Oleksandr Tyshchenko