Re: [PATCH 01/14] EDAC/amd64: Move struct fam_type variables into struct amd64_pvt

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

On Mon, Feb 28, 2022 at 09:43:41PM +0530, Naveen Krishna Chatradhi wrote:
> From: Muralidhara M K <muralimk@amd.com>

...

> -static struct amd64_family_type *per_family_init(struct amd64_pvt *pvt)
> +static int per_family_init(struct amd64_pvt *pvt)
>  {
>  	pvt->ext_model  = boot_cpu_data.x86_model >> 4;
>  	pvt->stepping	= boot_cpu_data.x86_stepping;
>  	pvt->model	= boot_cpu_data.x86_model;
>  	pvt->fam	= boot_cpu_data.x86;
> +	pvt->max_mcs	= 2;
>  
>  	switch (pvt->fam) {
>  	case 0xf:
> -		fam_type	= &family_types[K8_CPUS];
> -		pvt->ops	= &family_types[K8_CPUS].ops;
> +		pvt->ctl_name				= (pvt->ext_model > K8_REV_F) ?
> +							  "K8 revF or later" : "K8 revE or earlier";
> +		pvt->f1_id				= PCI_DEVICE_ID_AMD_K8_NB_ADDRMAP;
> +		pvt->f2_id				= PCI_DEVICE_ID_AMD_K8_NB_MEMCTL;
> +		pvt->ops->early_channel_count		= k8_early_channel_count;
> +		pvt->ops->map_sysaddr_to_csrow		= k8_map_sysaddr_to_csrow;
> +		pvt->ops->dbam_to_cs			= k8_dbam_to_chip_select;
>  		break;
>  
>  	case 0x10:
> -		fam_type	= &family_types[F10_CPUS];
> -		pvt->ops	= &family_types[F10_CPUS].ops;
> +		pvt->ctl_name				= "F10h";
> +		pvt->f1_id				= PCI_DEVICE_ID_AMD_10H_NB_MAP;
> +		pvt->f2_id				= PCI_DEVICE_ID_AMD_10H_NB_DRAM;
> +		pvt->ops->early_channel_count		= f1x_early_channel_count;
> +		pvt->ops->map_sysaddr_to_csrow		= f1x_map_sysaddr_to_csrow;
> +		pvt->ops->dbam_to_cs			= f10_dbam_to_chip_select;
>  		break;
>  
>  	case 0x15:
>  		if (pvt->model == 0x30) {

I just noticed that families 15h and 16h aren't matching against a model
range. I think we should look into that.

...

> @@ -4142,10 +4012,13 @@ static int probe_one_instance(unsigned int nid)
>  	pvt->mc_node_id	= nid;
>  	pvt->F3 = F3;
>  
> +	pvt->ops = kzalloc(sizeof(*pvt->ops), GFP_KERNEL);
> +	if (!pvt->ops)
> +		goto err_out;
> +

This should goto err_enable so that pvt and above can be freed.

>  	ret = -ENODEV;
> -	fam_type = per_family_init(pvt);
> -	if (!fam_type)
> -		goto err_enable;
> +	if (per_family_init(pvt))
> +		goto err_out;
>

Same here, but pvt->ops should also be freed at this point too.
 
Thanks,
Yazen