Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 11 Feb 2022 20:10:43 +0100
From:   Borislav Petkov <bp@alien8.de>
To:     Yazen Ghannam <yazen.ghannam@amd.com>
Cc:     linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
        mchehab@kernel.org, tony.luck@intel.com, james.morse@arm.com,
        rric@kernel.org, Smita.KoralahalliChannabasappa@amd.com
Subject: Re: [PATCH v4 01/24] EDAC/amd64: Define Data Fabric operations
Message-ID: <Yga0s+9cqtfB3Yxu@zn.tnic>
References: <20220127204115.384161-1-yazen.ghannam@amd.com>
 <20220127204115.384161-2-yazen.ghannam@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20220127204115.384161-2-yazen.ghannam@amd.com>
Precedence: bulk

On Thu, Jan 27, 2022 at 08:40:52PM +0000, Yazen Ghannam wrote:
> Define a stub to hold operations for different Data Fabric versions.
> This will be filled in following patches.
> 
> Set the operations at init-time as appropriate for each model/family
> group.
> 
> Also, start a glossary of acronyms used in the translation code.
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
> Link:
> https://lore.kernel.org/r/20211028175728.121452-6-yazen.ghannam@amd.com
> 
> v3->v4:
> * Started glossary.
> * Included pr_debug() for failing case.
> 
> v2->v3:
> * Was patch 6 in v2.
> * "df_ops" is set at init time.
> 
> v1->v2:
> * New in v2.
> 
>  drivers/edac/amd64_edac.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index fba609ada0e6..639dfbea3348 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -988,6 +988,12 @@ static int sys_addr_to_csrow(struct mem_ctl_info *mci, u64 sys_addr)
>  	return csrow;
>  }
>  
> +/*
> + * Glossary of acronyms used in address translation for Zen-based systems
> + *
> + * DF          =       Data Fabric

Yeah, "DF: Data Fabric" is probably easier to parse, without that weird
spacing and equals sign.

> + */
> +
>  /* Protect the PCI config register pairs used for DF indirect access. */
>  static DEFINE_MUTEX(df_indirect_mutex);
>  
> @@ -1058,6 +1064,14 @@ struct addr_ctx {
>  	u8 inst_id;
>  };
>  
> +struct data_fabric_ops {
> +};

I know that this is not the only example but we have struct definitions
interspersed with functions in the .c file while former should be all in
the header. It is a lot cleaner to have definitions and inline functions
in the header and the actual functionality in the C file but I leave it
up to you to decide what you prefer.

> +
> +struct data_fabric_ops df2_ops = {
> +};
> +
> +struct data_fabric_ops *df_ops;
> +
>  static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
>  {
>  	u64 dram_base_addr, dram_limit_addr, dram_hole_base;
> @@ -1072,6 +1086,11 @@ static int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr
>  
>  	struct addr_ctx ctx;
>  
> +	if (!df_ops) {
> +		pr_debug("Data Fabric Operations not set");

That probably wants to be a WARN_ON_ONCE() so that it is loud and
prominent when it happens...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette