Re: [PATCH 1/2] x86/amd_node: Fix AMD root device caching

[Yazen Ghannam <yazen.ghannam@amd.com>]

On Wed, Oct 22, 2025 at 01:13:42PM +0200, Borislav Petkov wrote:
> On Tue, Sep 30, 2025 at 04:45:45PM +0000, Yazen Ghannam wrote:
> > This behavior is benign on AMD reference design boards, since the bus
> > numbers are aligned. This results in a bitwise-OR value matching one of
> > the buses. For example, 0x00 | 0x40 | 0xA0 | 0xE0 = 0xE0.
> > 
> > This behavior breaks on boards where the bus numbers are not exactly
> > aligned. For example, 0x00 | 0x07 | 0xE0 | 0x15 = 0x1F.
> 
> <---
> 
> Please add here something along the lines of:
> 
> "And even if one could say, they both have bus 0x0 containing the root
> devices, this is not true on the other AMD nodes besides 0."
> 

Okay, will do.

> >  static int amd_cache_roots(void)
> >  {
> > -	u16 node, num_nodes = amd_num_nodes();
> > +	u16 count = 0, num_roots = 0, roots_per_node, node = 0, num_nodes = amd_num_nodes();
> > +	struct pci_dev *root = NULL;
> >  
> >  	amd_roots = kcalloc(num_nodes, sizeof(*amd_roots), GFP_KERNEL);
> >  	if (!amd_roots)
> >  		return -ENOMEM;
> >  
> > -	for (node = 0; node < num_nodes; node++)
> > -		amd_roots[node] = amd_node_get_root(node);
> > +	while ((root = get_next_root(root))) {
> > +		pci_dbg(root, "is an AMD root device\n");
> > +		num_roots++;
> > +	}
> > +
> > +	pr_debug("Found %d AMD root devices\n", num_roots);
> > +
> > +	roots_per_node = num_roots / num_nodes;
> 
> What happens if num_roots = 0? IOW, you need to handle that here.

Yes, will do.

> 
> > +
> > +	while ((root = get_next_root(root)) && node < num_nodes) {
> > +		/* Use one root for each node and skip the rest. */
> > +		if (count++ % roots_per_node)
> > +			continue;
> > +
> > +		pci_dbg(root, "is root for AMD node %u\n", node);
> > +		amd_roots[node++] = root;
> > +	}
> 
> If I squint my eyes hard enough, I can see you getting rid of the *three*
> while loops here. So please try again.
> 

I don't follow. Do you mean to combine the other loops into this one? Or
that this loop should be expanded into three loops explicitly doing one
thing each?

Thanks,
Yazen