Re: [PATCH v6 1/5] x86/amd_nb: Add support for northbridges on Aldebaran

[Chatradhi, Naveen Krishna]

Hi Boris,

On 11/9/2021 12:33 AM, Borislav Petkov wrote:
> [CAUTION: External Email]
>
> On Mon, Nov 08, 2021 at 10:23:49PM +0530, Chatradhi, Naveen Krishna wrote:
> > Northbridges on CPUs and GPUs can be described using the elements in the
> > above structure.
> If you're going to describe *northbridges*, then your struct cannot be called
> system_topology...
>
> > I thought organizing the data in a structure would simplify the
> > initialization of cpus and gpus.
> Ehh, did you even read my mail where I tried to explain that sprinkling
>
>          if (gpu)
>                  this
>          else
>                  that
>
> all over amd_cache_northbridges() is not proper design?
>
> ;-\
>
> > I will modify the  patch to enumerate gpu northbridge info only if there are
> >
> > gpu nodes with  pci_device to access the node_map registers.
> Why would you do that? What's the advantage?
>
> How about you answer my questions first so that we agree on the design
> first before you go and do things?

I was trying to handle both cpu and cpu northbridge enumeration in the 
amd_cache_northbridges() itself by reusing the existing structures and APIs.

Should have seen this through more clearly. As, this is working well for 
the following reasons.

a. Allocating the amd_northbridges.nb after identifying both the cpu and 
gpu misc devices, would extend node_to_amd_nb(node) for both cpu and gpu 
nodes.

   It is used extensively in this module. However, the roots_per_misc 
value is different in case of cpus and gpus and that needed to be 
handled seperately.

b. amd_nb_num(void) is used by other modules in the kernel, returning 
the total count of CPU and GPU northbridges would break the existing code.

I understood your point now.

When we create separate functions for caching cpu and gpu devices, is it 
okay to create "struct amd_gpu_nb_info" with the following fields

a. gpu_num;
b. struct amd_northbridge *gpu_nb;
c. gpu_node_start_id;

While, amd_nb_num(), continues to return number of cpu NBs
Add new API amd_gpu_nb_num(), return number of gpu NBs

and modify the node_to_amd_nb(node) to extend the same behavior for gpu 
devices also

Regards,

naveenk

> Hmm.
>
> --
> Regards/Gruss,
>      Boris.
>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7Cad9aea0ddff0446d80b108d9a2ea867d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637719950521959255%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=PthEEyzphEyN3O1FrUcvKyMF%2FEb282qifUHPR6psFhg%3D&reserved=0