Messages in this thread | | | Date | Mon, 1 Nov 2021 18:28:18 +0100 | From | Borislav Petkov <> | Subject | Re: [PATCH v6 1/5] x86/amd_nb: Add support for northbridges on Aldebaran |
| |
On Thu, Oct 28, 2021 at 06:31:02PM +0530, Naveen Krishna Chatradhi wrote: > +/* GPU Data Fabric ID Device 24 Function 1 */ > +#define PCI_DEVICE_ID_AMD_ALDEBARAN_DF_F1 0x14d1 > + > +/* DF18xF1 registers on Aldebaran GPU */ > +#define REG_LOCAL_NODE_TYPE_MAP 0x144 > +#define REG_RMT_NODE_TYPE_MAP 0x148
Move those defines up, along with the rest of them. While at it, you can align them all vertically.
> + > +/* > + * Newer AMD CPUs and GPUs whose data fabrics can be connected via custom xGMI
"Newer" is a commit message type of adjective and doesn't belong in permanent comments because when years pass, they won't be "newer" anymore. IOW, you can simply drop it here.
> + * links, comes with registers to gather local and remote node type map info.
"come"
> + * > + * "Local Node Type" refers to nodes with the same type as that from which the > + * register is read, and "Remote Node Type" refers to nodes with a different type.
This sure sounds weird.
With my simplistic thinking I'd assume "local" is the CPU and "remote" is the GPU...
> + * This function, reads the registers from GPU DF function 1. > + * Hence, local nodes are GPU and remote nodes are CPUs. > + */ > +static int amd_get_node_map(void) > +{ > + struct amd_node_map *nodemap; > + struct pci_dev *pdev; > + u32 tmp; > + > + pdev = pci_get_device(PCI_VENDOR_ID_AMD, > + PCI_DEVICE_ID_AMD_ALDEBARAN_DF_F1, NULL); > + if (!pdev) { > + pr_debug("DF Func1 PCI device not found on this node.\n"); > + return -ENODEV; > + } > + > + nodemap = kmalloc(sizeof(*nodemap), GFP_KERNEL);
You allocate a whopping 4 bytes? Just do
struct amd_node_map nodemap;
in the nb info descriptor.
I still need to see whether those node maps and functions you're adding and exporting even make sense but that will happen later.
> + if (!nodemap) > + return -ENOMEM; > + > + pci_read_config_dword(pdev, REG_LOCAL_NODE_TYPE_MAP, &tmp);
Check retval.
> + nodemap->gpu_node_start_id = tmp & 0xFFF; > + > + pci_read_config_dword(pdev, REG_RMT_NODE_TYPE_MAP, &tmp);
Ditto.
> + nodemap->cpu_node_count = tmp >> 16 & 0xFFF; > + > + amd_northbridges.nodemap = nodemap; > + return 0; > +} > + > static struct pci_dev *next_northbridge(struct pci_dev *dev, > const struct pci_device_id *ids) > { > @@ -230,6 +297,27 @@ int amd_df_indirect_read(u16 node, u8 func, u16 reg, u8 instance_id, u32 *lo) > } > EXPORT_SYMBOL_GPL(amd_df_indirect_read); > > +struct pci_dev *get_root_devs(struct pci_dev *root,
static
> + const struct pci_device_id *root_ids, > + u16 roots_per_misc) > +{ > + u16 j; > + > + /* > + * If there are more PCI root devices than data fabric/ > + * system management network interfaces, then the (N) > + * PCI roots per DF/SMN interface are functionally the > + * same (for DF/SMN access) and N-1 are redundant. N-1 > + * PCI roots should be skipped per DF/SMN interface so > + * the following DF/SMN interfaces get mapped to > + * correct PCI roots. > + */ > + for (j = 0; j < roots_per_misc; j++) > + root = next_northbridge(root, root_ids); > + > + return root; > +} > + > int amd_cache_northbridges(void) > { > const struct pci_device_id *misc_ids = amd_nb_misc_ids; > @@ -237,10 +325,10 @@ int amd_cache_northbridges(void) > const struct pci_device_id *root_ids = amd_root_ids; > struct pci_dev *root, *misc, *link; > struct amd_northbridge *nb; > - u16 roots_per_misc = 0; > - u16 misc_count = 0; > - u16 root_count = 0; > - u16 i, j; > + u16 roots_per_misc = 0, gpu_roots_per_misc = 0; > + u16 misc_count = 0, gpu_misc_count = 0; > + u16 root_count = 0, gpu_root_count = 0; > + u16 i; > > if (amd_northbridges.num) > return 0; > @@ -252,15 +340,23 @@ int amd_cache_northbridges(void) > } > > misc = NULL; > - while ((misc = next_northbridge(misc, misc_ids)) != NULL) > - misc_count++; > + while ((misc = next_northbridge(misc, misc_ids)) != NULL) {
Just remove that redundant "!= NULL" at the end, while at it.
> + if (misc->device == PCI_DEVICE_ID_AMD_ALDEBARAN_DF_F3) > + gpu_misc_count++; > + else > + misc_count++; > + } > > if (!misc_count) > return -ENODEV; > > root = NULL; > - while ((root = next_northbridge(root, root_ids)) != NULL) > - root_count++; > + while ((root = next_northbridge(root, root_ids)) != NULL) { > + if (root->device == PCI_DEVICE_ID_AMD_ALDEBARAN_ROOT) > + gpu_root_count++; > + else > + root_count++; > + } > > if (root_count) { > roots_per_misc = root_count / misc_count; > @@ -275,33 +371,37 @@ int amd_cache_northbridges(void) > } > } > > - nb = kcalloc(misc_count, sizeof(struct amd_northbridge), GFP_KERNEL); > + /* > + * The number of miscs, roots and roots_per_misc might vary on different > + * nodes of a heterogeneous system. > + * Calculate roots_per_misc accordingly in order to skip the redundant > + * roots and map the DF/SMN interfaces to correct PCI roots. > + */
Reflow that comment so that it is a block.
> + if (gpu_root_count && gpu_misc_count) { > + int ret = amd_get_node_map(); > +
^ Superfluous newline.
> + if (ret) > + return ret; > + > + gpu_roots_per_misc = gpu_root_count / gpu_misc_count; > + } > +
-- Regards/Gruss, Boris.
https://people.kernel.org/tglx/notes-about-netiquette
| |