Messages in this thread | | | Date | Wed, 9 Mar 2022 21:42:21 +0000 | From | Yazen Ghannam <> | Subject | Re: [PATCH v4 01/24] EDAC/amd64: Define Data Fabric operations |
| |
On Fri, Feb 11, 2022 at 08:10:43PM +0100, Borislav Petkov wrote: > > +/* > > + * 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. >
Okay, will change.
> > + */ > > + > > /* 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. >
Makes sense. I'll make the change.
> > + > > +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... >
I'd like to keep this a debug message because address translation updates will almost certainly lag general EDAC enablement. For example, Family 19h Model 10h is enabled now, but df_ops won't be set. The translation code will return early here, and the EDAC message will say that the tranlsation failed. But this isn't a bug that needs a WARNing. Rather it's just that the feature isn't enabled yet for new systems.
Thanks, Yazen
| |