RE: [PATCH V2] EDAC, amd64: Add Family 17h Model 10h support.

[Ghannam, Yazen]

> -----Original Message-----
> From: Michael Jin <mikhail.jin@xxxxxxxxx>
> Sent: Wednesday, August 15, 2018 6:41 AM
> To: Borislav Petkov <bp@xxxxxxx>; Ghannam, Yazen
> <Yazen.Ghannam@xxxxxxx>; Mauro Carvalho Chehab
> <mchehab@xxxxxxxxxx>
> Cc: linux-edac@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Michael Jin
> <mikhail.jin@xxxxxxxxx>; stable@xxxxxxxxxxxxxxx
> Subject: [PATCH V2] EDAC, amd64: Add Family 17h Model 10h support.
> 
> Add support for ECC error decoding on family 17h models 10h-2fh.
> 

Can you please make this more specific? Something like "Add PCI device IDs
for Fam17h Models 10h-2Fh so that amd64_edac_mod will load".

> Link: https://lkml.kernel.org/r/20180810193623.24629-1-
> mikhail.jin@xxxxxxxxx
> Cc: stable@xxxxxxxxxxxxxxx

This won't apply to all stable branches. EDAC support for Fam17h was added
in v4.10.

I'm not sure if the stable tag needs to be modified, or if the stable queues
automatically drop patches that don't apply.

> Signed-off-by: Michael Jin <mikhail.jin@xxxxxxxxx>
> ---
>  drivers/edac/amd64_edac.c | 15 +++++++++++++++
>  drivers/edac/amd64_edac.h |  3 +++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 18aeabb1d5ee..2d7b6d37d6ec 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -2200,6 +2200,15 @@ static struct amd64_family_type family_types[] = {
>  			.dbam_to_cs		= f17_base_addr_to_cs_size,
>  		}
>  	},
> +	[F17_M10H_CPUS] = {
> +		.ctl_name = "F17h_M10h",
> +		.f0_id = PCI_DEVICE_ID_AMD_17H_M10H_DF_F0,
> +		.f6_id = PCI_DEVICE_ID_AMD_17H_M10H_DF_F6,
> +		.ops = {
> +			.early_channel_count	= f17_early_channel_count,
> +			.dbam_to_cs		= f17_base_addr_to_cs_size,
> +		}
> +	},
>  };
> 
>  /*
> @@ -3188,6 +3197,12 @@ static struct amd64_family_type
> *per_family_init(struct amd64_pvt *pvt)
>  		break;
> 
>  	case 0x17:
> +		/* Check if CPU model is in range 10h-2fh */

This comment is not needed because it's obvious from the code.

> +		if (pvt->model >= 0x10 && pvt->model <= 0x2f) {
> +			fam_type = &family_types[F17_M10H_CPUS];
> +			pvt->ops = &family_types[F17_M10H_CPUS].ops;
> +			break;
> +		}
>  		fam_type	= &family_types[F17_CPUS];
>  		pvt->ops	= &family_types[F17_CPUS].ops;
>  		break;
> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
> index 1d4b74e9a037..4242f8e39c18 100644
> --- a/drivers/edac/amd64_edac.h
> +++ b/drivers/edac/amd64_edac.h
> @@ -115,6 +115,8 @@
>  #define PCI_DEVICE_ID_AMD_16H_M30H_NB_F2 0x1582
>  #define PCI_DEVICE_ID_AMD_17H_DF_F0	0x1460
>  #define PCI_DEVICE_ID_AMD_17H_DF_F6	0x1466
> +#define PCI_DEVICE_ID_AMD_17H_M10H_DF_F0 0x15e8
> +#define PCI_DEVICE_ID_AMD_17H_M10H_DF_F6 0x15ee
> 
>  /*
>   * Function 1 - Address Map
> @@ -281,6 +283,7 @@ enum amd_families {
>  	F16_CPUS,
>  	F16_M30H_CPUS,
>  	F17_CPUS,
> +	F17_M10H_CPUS,
>  	NUM_FAMILIES,
>  };
> 
> --

Other than those few nits this looks good to me.

Reviewed-by: Yazen Ghannam <yazen.ghannam@xxxxxxx>

Thanks,
Yazen