Re: Question about call to pci_assign_unassigned_bus_resources in amdgpu

["Koenig, Christian" <Christian.Koenig@amd.com>]

Am 24.06.19 um 10:47 schrieb Benjamin Herrenschmidt:
> On Mon, 2019-06-24 at 08:42 +0000, Koenig, Christian wrote:
>> Then we resize the VRAM BAR by calling pci_resize_resource(). That in
>> turn tries to resize and shuffle around the parent bridge resources
>> using pci_reassign_bridge_resources().
>>
>> But pci_reassign_bridge_resources() does not assign any device
>> resources, it just tries to make sure the upstream bridges have enough
>> space to fit everything in.
>>
>> Independent if we succeeded or failed with handling the bridge(s) we
>> call pci_assign_unassigned_bus_resources() to re-assign the previously
>> freed up VRAM and doorbell BARs.
>>
>> So yeah, this definitely necessary, or otherwise the driver would crash
>> soon after because the resources are not assigned again.
> Oh, I missed that pci_reassign_bridge_resources() didn't reassign the
> resources under the bridge... ugh... that code is a bloody mess.
>
> We have 4 or 5 "assign bus resources" functions, all subtly different
> for no clear reasons (the historical changelogs from Yinghai may as
> well don't exist, they are basically encrypted), and in most case for
> no good reasons....

Yeah, can't agree more :) It was one of the main challenges implementing 
the resizing support.

If you want to clean this up feel free to CC me and I can take a look as 
well. But honestly I was scared of touching it when I worked on this, 
because of all the little corner cases you have in PCI.

> Question: Do you ever need to assign anything other than that one
> device though ? In my branch, I've added this typically for the case
> where a single device needs to be reassigned:
>
> +void pci_dev_assign_resources(struct pci_dev *dev)
> +{
> +	LIST_HEAD(head);
> +
> +	/* Assign non-fixed resources */
> +	__dev_sort_resources(dev, &head);
> +	__assign_resources_sorted(&head, NULL, NULL);
> +
> +	/* Assign fixed ones if any */
> +	pdev_assign_fixed_resources(dev);
> +}
> +EXPORT_SYMBOL(pci_dev_assign_resources);
>
> Would that work for you ?

That should work perfectly fine.

Regards,
Christian.

>
> If yes, I'll replace pci_assign_unassigned_bus_resources(). I'd like to
> eventually kill it..
>
> Cheers,
> Ben.
>
>