Re: [PATCH 02/14] EDAC/amd64: Add get_base_mask() into pvt->ops

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

On Mon, Feb 28, 2022 at 09:43:42PM +0530, Naveen Krishna Chatradhi wrote:
> From: Muralidhara M K <muralimk@amd.com>
> 
> Add function pointer for get_base_mask() in pvt->ops
> and assign family specific get_base_mask() definitions appropriately.
>

The commit message should include why the change is needed and not just what
the change is. A few of my patches have similar feedback, so it's fresh in my
mind. :)
 
> Signed-off-by: Muralidhara M K <muralimk@amd.com>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
> This patch is created by splitting the 5/12th patch in series
> [v7 5/12] https://patchwork.kernel.org/project/linux-edac/patch/20220203174942.31630-6-nchatrad@amd.com/
> 
>  drivers/edac/amd64_edac.c | 22 ++++++++++++++++------
>  drivers/edac/amd64_edac.h |  1 +
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index b21f43a3ec98..985c59d23a20 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -1570,11 +1570,6 @@ static void read_dct_base_mask(struct amd64_pvt *pvt)
>  {
>  	int cs;
>  
> -	prep_chip_selects(pvt);
> -
> -	if (pvt->umc)
> -		return read_umc_base_mask(pvt);
> -
>  	for_each_chip_select(cs, 0, pvt) {
>  		int reg0   = DCSB0 + (cs * 4);
>  		int reg1   = DCSB1 + (cs * 4);
> @@ -3287,7 +3282,9 @@ static void read_mc_regs(struct amd64_pvt *pvt)
>  	}
>  
>  skip:
> -	read_dct_base_mask(pvt);
> +	prep_chip_selects(pvt);
> +
> +	pvt->ops->get_base_mask(pvt);
>  
>  	determine_memory_type(pvt);
>  	edac_dbg(1, "  DIMM type: %s\n", edac_mem_types[pvt->dram_type]);
> @@ -3763,6 +3760,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		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;
> +		pvt->ops->get_base_mask			= read_dct_base_mask;
>  		break;
>  
>  	case 0x10:
> @@ -3772,6 +3770,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		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;
> +		pvt->ops->get_base_mask			= read_dct_base_mask;
>  		break;
>  
>  	case 0x15:
> @@ -3796,6 +3795,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		}
>  		pvt->ops->early_channel_count		= f1x_early_channel_count;
>  		pvt->ops->map_sysaddr_to_csrow		= f1x_map_sysaddr_to_csrow;
> +		pvt->ops->get_base_mask			= read_dct_base_mask;
>  		break;
>  
>  	case 0x16:
> @@ -3811,6 +3811,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		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			= f16_dbam_to_chip_select;
> +		pvt->ops->get_base_mask			= read_dct_base_mask;
>  		break;
>  
>  	case 0x17:
> @@ -3840,6 +3841,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  	case 0x18:
>  		pvt->ops->early_channel_count		= f17_early_channel_count;
>  		pvt->ops->dbam_to_cs			= f17_addr_mask_to_cs_size;
> +		pvt->ops->get_base_mask			= read_umc_base_mask;
>  
>  		if (pvt->fam == 0x18) {
>  			pvt->ctl_name			= "F18h";
> @@ -3875,6 +3877,7 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		}
>  		pvt->ops->early_channel_count		= f17_early_channel_count;
>  		pvt->ops->dbam_to_cs			= f17_addr_mask_to_cs_size;
> +		pvt->ops->get_base_mask			= read_umc_base_mask;

The function pointer is get_base_mask() and the helper functions are
read_{dct,umc}_base_mask(). I think the naming should be more consistent.
Either read_base_mask()/read_{dct,umc}_base_mask() or
get_base_mask()/get_{dct,umc}_base_mask().

>  		break;
>  
>  	default:
> @@ -3882,6 +3885,13 @@ static int per_family_init(struct amd64_pvt *pvt)
>  		return -ENODEV;
>  	}
>  
> +	/* ops required for all the families */
> +	if (!pvt->ops->early_channel_count || !pvt->ops->dbam_to_cs ||
> +	    !pvt->ops->get_base_mask) {
> +		edac_dbg(1, "Common helper routines not defined.\n");
> +		return -EFAULT;
> +	}
> +

I think it'd be clearer if we define an ops struct with default, "do nothing"
functions and statically include this in struct pvt. These can then be
overwritten in per_family_init(). I think this would avoid the need to check
that each function is set and the need to dynamically allocate the ops struct.

Thanks,
Yazen