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

From: "Chatradhi, Naveen Krishna" <nchatrad@amd.com>
To: Borislav Petkov <bp@alien8.de>
Cc: linux-edac@vger.kernel.org, x86@kernel.org,
	linux-kernel@vger.kernel.org, mingo@redhat.com,
	mchehab@kernel.org, yazen.ghannam@amd.com,
	Muralidhara M K <muralimk@amd.com>
Subject: Re: [PATCH v6 1/5] x86/amd_nb: Add support for northbridges on Aldebaran
Date: Tue, 9 Nov 2021 17:00:11 +0530	[thread overview]
Message-ID: <9de7f807-32a6-b009-d8b7-28771c80bfaf@amd.com> (raw)
In-Reply-To: <YYl0l+XV/QRZieGY@zn.tnic>

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&amp;data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7Cad9aea0ddff0446d80b108d9a2ea867d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637719950521959255%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=PthEEyzphEyN3O1FrUcvKyMF%2FEb282qifUHPR6psFhg%3D&amp;reserved=0