Re: [PATCH v3 06/10] x86/mce: Break up __mcheck_cpu_apply_quirks()
From: Yazen Ghannam
Date: Wed Oct 30 2024 - 10:16:58 EST
On Wed, Oct 30, 2024 at 01:39:43AM +0000, Zhuo, Qiuxu wrote:
[...]
Thanks Qiuxu.
>
> > > +static void apply_quirks_intel(struct cpuinfo_x86 *c) {
> > > + struct mce_bank *mce_banks = this_cpu_ptr(mce_banks_array);
> > > + struct mca_config *cfg = &mca_cfg;
> >
> > Is there a benefit to this pointer? We use mca_cfg.FIELD in most other places.
>
> This could make the diff smaller for easier review, and I also believe that fewer direct
> uses of global variables in functions are better. Additionally, there are multiple uses of
> 'mca_cfg' in the function, the local variable 'cfg' is shorter and more convenient to use.
>
I don't think it would make the diff smaller here since the code is
already being moved.
Though you could say this is a separate logical change compared to just
moving the code as-is.
Also, I don't think the "shorter, more convenient" idea holds. It's not
that much shorter. And there are already cases of using the global
variables "mca_cfg" and "mce_flags".
Why is "...fewer direct uses of global variables in functions..." better?
> [ Certainly, if the global variable 'mca_cfg' is only used once in the function, directly
> using it might be more convenient. ]
>
There is one such case in your patch.
> Just from my perspective, no strong preference. 😊
>
Same here. I just figured this suggestion would be another possible
cleanup. :)
Thanks,
Yazen