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: Mon, 8 Nov 2021 22:23:49 +0530	[thread overview]
Message-ID: <bcf5e86c-d3f1-0dab-2bed-505b1eb95f17@amd.com> (raw)
In-Reply-To: <YYknXBpOUQtV1aZ8@zn.tnic>

Hi Boris,

On 11/8/2021 7:04 PM, Borislav Petkov wrote:
> [CAUTION: External Email]
>
> On Thu, Nov 04, 2021 at 06:48:29PM +0530, Chatradhi, Naveen Krishna wrote:
>> I know, this is confusion. we will try to give a meaning for definition
>> here.
> Well, not that - you will need to keep adding PCI device IDs for the
> future GPUs supporting this stuff.
Creating a pci_device_id list for GPUs will be needed.
>
>> How about, defining a new struct
>>
>> +struct system_topology {
>> +       const struct pci_device_id *misc_ids;
>> +       const struct pci_device_id *link_ids;
>> +       const struct pci_device_id *root_ids;
>> +       u16 roots_per_misc;
>> +       u16 misc_count;
>> +       u16 root_count;
>> +};
> Well, how does having a single struct help make things easier?

Northbridges on CPUs and GPUs can be described using the elements in the 
above structure.

>
> IOW, if you use accessors to get the information you need, it doesn't
> really matter what the underlying organization of the data is. And if
> it helps to keep 'em separate because stuff is simple altogether, then,
> they should be separate.

I thought organizing the data in a structure would simplify the 
initialization of cpus and gpus.

>
> So, before you ask "How about", think of answering the question "Why
> should it be done this way? What are the advantages?"

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.

>
>> This way, creating appropriate number MCs under EDAC and existing exported
>> APIs can remain the same.
> Why does that matter?
>
> Also, have you verified in what order the init_amd_nbs() fs initcall and
> amd64_edac_init() get executed?
>
> I'm going to venture a pretty sure guess that the initcall runs first
> and that amd_cache_northbridges() call in amd64_edac_init() is probably
> not even needed anymore...
Yes, fs_initcall come before module_init (which inturn is device_init 
level 6). We can replace the  amd_cache_northbridges() call in  
amd64_edac_init() with if(amd_nb_num() < 0).
> Thx.
>
> --
> 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%7C45b6d75b206849ab022808d9a2bc7d4f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637719752712242190%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=WW26EMOnu%2FBazap9njmnQIvY9hVSZxVpNol4uptGcec%3D&amp;reserved=0