Date:   Wed, 5 Jan 2022 16:12:08 +0000
From:   Yazen Ghannam <yazen.ghannam@amd.com>
To:     Borislav Petkov <bp@alien8.de>
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,
        william.roche@oracle.com
Subject: Re: [PATCH v2 1/2] EDAC/amd64: Check register values from all UMCs
Message-ID: <YdXDWIuuoY4D5cfO@yaz-ubuntu>
References: <20211215155309.2711917-1-yazen.ghannam@amd.com>
 <20211215155309.2711917-2-yazen.ghannam@amd.com>
 <YbotciKVDsH1Fl1H@zn.tnic>
 <YbtkcppejpW8gHqY@yaz-ubuntu>
 <Yc2ZzMT+Mg5xCvjI@zn.tnic>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Yc2ZzMT+Mg5xCvjI@zn.tnic>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?9JbAvBVnsZ6jKAu1CgqS8uU5AiHm8B39Ji1vzrZz/TVaUSTuMClaPBO1Lblr?=
 =?us-ascii?Q?XeLC188oAngcKezzXsEF8yYZx7qrYGKXsScIg8jK0nQW/M7qJxAPS946wfDL?=
 =?us-ascii?Q?cj6fLInEI/4Aeb0dPzJRBjTZbp2Atjd2Gnq2gQYoZIy8h4w9HPM9NzEn3zSQ?=
 =?us-ascii?Q?pIh6wRWpk3FqLp0yChhoMbroJZtjY6dK2r7XOxnBKzDCrXRP4OqV28OsMt+i?=
 =?us-ascii?Q?5vyqVFzIpfJPLa+TvK+WdKmwegHCxBFKsQ1Vk+jKTlW9AQ8MfRIeJP6tdNY/?=
 =?us-ascii?Q?yiF1+gcJG+RLX5Tvf14/HIEiWQCixwK/IH+rBRwBR2A4NomGlTmylqq841rn?=
 =?us-ascii?Q?7l1frtCLa4IhkorAqDE8CTkHeSVhqgwCtyIxDReuRYKGtah5muWVRl6dQ8tM?=
 =?us-ascii?Q?xbqwSJKZpUrQfeQp+OsySvaLIyWO/u7IQzZQOlLUZLDTqKOlnPqmP805aqSf?=
 =?us-ascii?Q?W9EC3Ua2dTzYDTyW4Rb8nRBmXYNK5yzqY+1UKBLZeqWrj2ZGt7Z+YAp1KmWp?=
 =?us-ascii?Q?qU0utOps5ZgpKsIlR4FMv5eSZKgTMhVHd8tccSSErxpXDqS19EeLR7XN3kkw?=
 =?us-ascii?Q?oljr1D33Ia8lcQzzz+LQVKe0ymtN6tHkIOG+uD1WhZ1YOZJqyHzQadePuvVu?=
 =?us-ascii?Q?GO1dczz/PfmWxRzy4CPKdgZum18gsZ+chnJJRCYnVkur/fpOmSYZ/X4yiFUE?=
 =?us-ascii?Q?RT7I1Yh3o+omBahspM5tD08ISZ3Qv9+7BhpYqP8M5OwnNr+Ys3rUAxZNzHK2?=
 =?us-ascii?Q?t/cpX0DbHZF1uLiRFfXFlHJYZBpQ5ABn4xdM42VcS0D6XjcXSujHEOWKgDht?=
 =?us-ascii?Q?MnTrEjm5hccD9gJ+iPd1eRTocrwBrGrjA10X6F6zKjyY9FuS6gnn0uRQA2ax?=
 =?us-ascii?Q?7SKEfepeFXU2IhD5spAuR36/9bCT4F1L5wYmauo5xyn0zR8mHouxgN0ElYx8?=
 =?us-ascii?Q?WdmFoodXwWNh55KExhMaOYLtrQkhL464Oyli+xgRl7sjezZzCpfkoYUzYacf?=
 =?us-ascii?Q?UcPN6O0T2hhvkZ8w9t7jnuwoOu2scdvC9V4Hye3OI+Oke/TWFBHCT/7Jp8cL?=
 =?us-ascii?Q?MZlshFEUZWV2vC1Z9NpLuj20j9I6iRXottz/bZh2VvyqL1/uKHw7C8HZaXCZ?=
 =?us-ascii?Q?QyiqbuiPvxY7Jt2cfPjwxxObKNxZOsk54SIVYoL7C8kSssDeMPPQfWXrgYc1?=
 =?us-ascii?Q?YGQh59BCPegxMsmKtQCpjdvEKKcpyVOm2jxSf4DMMRiWQfc29gnEqKZ92YvS?=
 =?us-ascii?Q?upgygJBfkJMlEIi3EyPwE/RikxRvB1OurTHxsr0JH7Bmqg+Bb+/ajmK9Em/7?=
 =?us-ascii?Q?pvrAspKCxWOrsfDevCF5Tcr8Kcv976cgR8ZF1MPB8N2YtzS8uPCawPBIWW+T?=
 =?us-ascii?Q?1fnyJ/iqmMBG3knd23C9KzERjGecWZgqJO0MGsj1NHgPz/9J3X1ViHhbvFqd?=
 =?us-ascii?Q?d7k2Lxudw2XKeUu0n9HbStZ4pgRkLb44D8QyTn4TMkXdhOGWQPTfwlf2ppTM?=
 =?us-ascii?Q?2Kacb520syegJmCICp/76blaYjBZEIaV4mkTs+AUgr7rOTfBh7O/bbLYh+Fn?=
 =?us-ascii?Q?5h9jEbsNsE/v/KF2b+CeUUEYl0lcN9LdWdJHp0y/tG/y9kcbRCjykOf+AJBT?=
 =?us-ascii?Q?pGPzIckYLb8/ykny1VyJ6IQ=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2f5e2be3-e294-40ad-78f0-08d9d066223f
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3108.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jan 2022 16:12:13.1181
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 3KqUzvjUBpjUzFre1QL7lKCI7JeH1x4hw0rqXOG+Nz8kpQvccA24YLzAqmepyzzu8BqQOzQfztzliRO2r7t92A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR12MB1746
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Dec 30, 2021 at 12:36:44PM +0100, Borislav Petkov wrote:
> On Thu, Dec 16, 2021 at 04:08:18PM +0000, Yazen Ghannam wrote:
> > No, that's a good question. And actually the assumption is incorrect. It is
> > allowed to have different DIMM types in a system though all DIMMs on a single
> > UMC must match.
> 
> Oh fun, really?!
> 
> So a single system can have DDR4 *and* DDR5 on the same board?!
>

Well, I don't know about that specifically. There are some restrictions, but
you could have UDIMMs and RDIMMs of the same generation, at least.
 
> So then that
> 
> 	pvt->dram_type
> 
> is insufficient to store the DIMM type for a pvt. If you have multiple
> UMCs on a pvt and all have different type DIMMs, then you need the
> relevant DIMM type when you dump it in sysfs...
> 
> Which then means, you need ->dram_type to be per UMC...
> 
> And also, I'm assuming the hw already enforces that DIMMs on a single
> UMC must match - it simply won't boot if they don't so you don't have to
> enforce that, at least.
> 
> > Do you recommend a follow up patch or should this one be reworked?
> 
> This one is insufficient, I'm afraid.
> 
> One way to address this is, you could use pvt->umc at the places where
> dram_type is used and assign directly to the dimm->mtype thing. But then
> you'd need a way to map each struct dimm_info *dimm to the UMC so that
> you can determine the correct DIMM type.
> 

I did send a patch that did something like this.
https://lkml.kernel.org/r/20211228200615.412999-2-yazen.ghannam@amd.com

Though this got a build warning report, so I need to follow up on that.

> Which would make pvt->dram_type redundant and can be removed.
>

I kept this so as to not break legacy systems. But I'll look at it again. I
think you may be right.

Thanks,
Yazen