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

Yazen Ghannam
2022-03-23T17:19:39Z

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