Message ID | 20240311155651.GAZe8pw0urOnUZj1y_@fat_crate.local (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | [GIT,PULL] EDAC updates for v6.9 | expand |
On Mon, 11 Mar 2024 at 08:57, Borislav Petkov <bp@alien8.de> wrote: > > - return topology_die_id(err->cpu) % amd_get_nodes_per_socket(); > + return topology_amd_node_id(err->cpu) % topology_amd_nodes_per_pkg(); Ho humm. Lookie here: static inline unsigned int topology_amd_nodes_per_pkg(void) { return 0; }; that's the UP case. Yeah, I'm assuming nobody tests this for UP, but it's clearly wrong to potentially do that modulus by zero. So I made the merge also change that UP case of topology_amd_nodes_per_pkg() to return 1. Because dammit, not only is a mod-by-zero wrong, a UP system most definitely has one node per package, not zero. Linus
The pull request you sent on Mon, 11 Mar 2024 16:57:11 +0100:
> git://git.kernel.org/pub/scm/linux/kernel/git/ras/ras.git tags/edac_updates_for_v6.9
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/b0402403e54ae9eb94ce1cbb53c7def776e97426
Thank you!
On 3/11/24 18:12, Linus Torvalds wrote: > On Mon, 11 Mar 2024 at 08:57, Borislav Petkov <bp@alien8.de> wrote: >> >> - return topology_die_id(err->cpu) % amd_get_nodes_per_socket(); >> + return topology_amd_node_id(err->cpu) % topology_amd_nodes_per_pkg(); > > Ho humm. Lookie here: > > static inline unsigned int topology_amd_nodes_per_pkg(void) > { return 0; }; > and there's an extra/trailing ';'. > that's the UP case. > > Yeah, I'm assuming nobody tests this for UP, but it's clearly wrong to > potentially do that modulus by zero. > > So I made the merge also change that UP case of > topology_amd_nodes_per_pkg() to return 1. > > Because dammit, not only is a mod-by-zero wrong, a UP system most > definitely has one node per package, not zero. > > Linus >
On Mon, 11 Mar 2024 at 19:24, Randy Dunlap <rdunlap@infradead.org> wrote: > > and there's an extra/trailing ';'. Ayup, I fixed that too while I was in there anyway. Linus
On Mon, Mar 11, 2024 at 06:12:54PM -0700, Linus Torvalds wrote: > Ho humm. Lookie here: > > static inline unsigned int topology_amd_nodes_per_pkg(void) > { return 0; }; > > that's the UP case. > > Yeah, I'm assuming nobody tests this for UP, Unless it gets randomly enabled in my randconfig builds once in a blue moon, I'd say pretty seldomly. I've heard people raise the question multiple times whether we should simply make CONFIG_SMP default y on x86 and frankly, it'll get rid of a whole bunch of stupid corner cases like that... > but it's clearly wrong to potentially do that modulus by zero. Yep. > So I made the merge also change that UP case of > topology_amd_nodes_per_pkg() to return 1. > > Because dammit, not only is a mod-by-zero wrong, a UP system most > definitely has one node per package, not zero. Yap, that's the the straight-forward thing to do, thanks for fixing it!
* Borislav Petkov <bp@alien8.de> wrote: > On Mon, Mar 11, 2024 at 06:12:54PM -0700, Linus Torvalds wrote: > > Ho humm. Lookie here: > > > > static inline unsigned int topology_amd_nodes_per_pkg(void) > > { return 0; }; > > > > that's the UP case. > > > > Yeah, I'm assuming nobody tests this for UP, > > Unless it gets randomly enabled in my randconfig builds once in a blue > moon, I'd say pretty seldomly. I've heard people raise the question > multiple times whether we should simply make CONFIG_SMP default y on x86 > and frankly, it'll get rid of a whole bunch of stupid corner cases like > that... Making it 'default y' in the Kconfig alone changes very little, as people & bots will still stumble on !SMP via allnoconfig or randconfig builds. If you mean forcing CONFIG_SMP via 'select SMP' on x86 on the other hand, that's worth considering - although I think there will be a ton of extra cross-build breakage as most patches still get created & tested on x86. In other words, the x86 UP build basically has the side-effect utility of covering a lot of UP cross-build scenarios in generic code. I think the most viable approach would be to make SMP the only model all across the kernel (and eventually removing the CONFIG_SMP option), while propagating UP data structures and locking primitives to the UP arch level, instead of having CONFIG_SMP #ifdefs in generic code. Maybe not today, but certainly in a few years. Thanks, Ingo
On Tue, Mar 12, 2024 at 10:16:10AM +0100, Ingo Molnar wrote: > If you mean forcing CONFIG_SMP via 'select SMP' on x86 on the other > hand, that's worth considering Yeah, that. > - although I think there will be a ton of extra cross-build breakage > as most patches still get created & tested on x86. I wanna say "this better be build-tested on the target architecture too" but I can certainly see the use case of having to cross-build a UP config. > I think the most viable approach would be to make SMP the only model > all across the kernel (and eventually removing the CONFIG_SMP option), > while propagating UP data structures and locking primitives to the UP > arch level, instead of having CONFIG_SMP #ifdefs in generic code. Right, UP is a SMP machine with only 1 CPU. It should just work. :-P > Maybe not today, but certainly in a few years. It makes sense to aim for such a model, yap. Let's do it. Thx.
On Mon, Mar 11 2024 at 18:12, Linus Torvalds wrote: > On Mon, 11 Mar 2024 at 08:57, Borislav Petkov <bp@alien8.de> wrote: >> >> - return topology_die_id(err->cpu) % amd_get_nodes_per_socket(); >> + return topology_amd_node_id(err->cpu) % topology_amd_nodes_per_pkg(); > > Ho humm. Lookie here: > > static inline unsigned int topology_amd_nodes_per_pkg(void) > { return 0; }; > > that's the UP case. > > Yeah, I'm assuming nobody tests this for UP, but it's clearly wrong to > potentially do that modulus by zero. Duh. I clearly was not thinking at all when I wrote this. Thanks for spotting it. tglx
diff --git a/drivers/ras/amd/atl/umc.c b/drivers/ras/amd/atl/umc.c index 08c6dbd44c62..59b6169093f7 100644 --- a/drivers/ras/amd/atl/umc.c +++ b/drivers/ras/amd/atl/umc.c @@ -315,7 +315,7 @@ static u8 get_die_id(struct atl_err *err) * For CPUs, this is the AMD Node ID modulo the number * of AMD Nodes per socket. */ - return topology_die_id(err->cpu) % amd_get_nodes_per_socket(); + return topology_amd_node_id(err->cpu) % topology_amd_nodes_per_pkg(); } #define UMC_CHANNEL_NUM GENMASK(31, 20)