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

[Yazen Ghannam]

On Mon, Oct 25, 2021 at 08:20:14PM +0530, Naveen Krishna Chatradhi wrote:
...
> +/* GPU Data Fabric ID Device 24 Function 1 */
> +#define PCI_DEVICE_ID_AMD_ALDEBARAN_DF_F1 0x14d1
> +
> +static struct pci_dev *get_gpu_df_f1(void)
> +{
> +	return pci_get_device(PCI_VENDOR_ID_AMD,
> +			      PCI_DEVICE_ID_AMD_ALDEBARAN_DF_F1, NULL);
> +}
> +
> +/* DF18xF1 registers on Aldebaran GPU */
> +#define REG_LOCAL_NODE_TYPE_MAP		0x144
> +#define REG_RMT_NODE_TYPE_MAP		0x148
> +
> +/*
> + * Newer AMD CPUs and GPUs whose data fabrics can be connected via custom xGMI
> + * links, comes with registers to gather local and remote node type map info.
> + *
> + * "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 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(struct pci_dev *pdev)
> +{
> +	struct amd_node_map *nodemap;
> +	u32 tmp;
> +
> +	nodemap = kmalloc(sizeof(*nodemap), GFP_KERNEL);
> +	if (!nodemap)
> +		return -ENOMEM;
> +
> +	pci_read_config_dword(pdev, REG_LOCAL_NODE_TYPE_MAP, &tmp);
> +	nodemap->gpu_node_start_id = tmp & 0xFFF;
> +
> +	pci_read_config_dword(pdev, REG_RMT_NODE_TYPE_MAP, &tmp);
> +	nodemap->cpu_node_count = tmp >> 16 & 0xFFF;
> +
> +	amd_northbridges.nodemap = nodemap;
> +	return 0;
> +}
> +
...
>  int amd_cache_northbridges(void)
>  {
>  	const struct pci_device_id *misc_ids = amd_nb_misc_ids;
>  	const struct pci_device_id *link_ids = amd_nb_link_ids;
>  	const struct pci_device_id *root_ids = amd_root_ids;
> -	struct pci_dev *root, *misc, *link;
> +	struct pci_dev *root, *misc, *link, *dff1;
...
> +	/*
> +	 * 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.
> +	 */
> +	if (gpu_root_count && gpu_misc_count) {
> +		dff1 = get_gpu_df_f1();

dff1 is only used in this section, so it can be declared here.

> +		if (!dff1) {
> +			pr_debug("Failed to gather GPU node info.\n");

This message doesn't make sense to me. The failure is that a particular PCI
device was not found. Gathering node info hasn't even been attempted yet.

> +			return -ENODEV;
> +		}
> +
> +		if (amd_get_node_map(dff1))
> +			return -ENOMEM;
> +

Also, why do these functions need to be split up? If it was because of the
return values, then you could have done the following.

		int ret = amd_get_node_map();

		if (ret)
			return ret;

Thanks,
Yazen