Received-SPF: pass (google.com: domain of linux-kernel+bounces-200997-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 4 Jun 2024 17:46:35 +0200
From: Borislav Petkov <bp@alien8.de>
To: Yazen Ghannam <yazen.ghannam@amd.com>
Cc: linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
	tony.luck@intel.com, x86@kernel.org, avadhut.naik@amd.com,
	john.allen@amd.com
Subject: Re: [PATCH 8/9] x86/mce/amd: Enable interrupt vectors once per-CPU
 on SMCA systems
Message-ID: <20240604154635.GTZl8222q7WAEVSJKH@fat_crate.local>
References: <20240523155641.2805411-1-yazen.ghannam@amd.com>
 <20240523155641.2805411-9-yazen.ghannam@amd.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240523155641.2805411-9-yazen.ghannam@amd.com>

On Thu, May 23, 2024 at 10:56:40AM -0500, Yazen Ghannam wrote:
>  static bool thresholding_irq_en;
>  static DEFINE_PER_CPU_READ_MOSTLY(mce_banks_t, mce_thr_intr_banks);
>  static DEFINE_PER_CPU_READ_MOSTLY(mce_banks_t, mce_dfr_intr_banks);
> +static DEFINE_PER_CPU_READ_MOSTLY(bool, smca_thr_intr_enabled);
> +static DEFINE_PER_CPU_READ_MOSTLY(bool, smca_dfr_intr_enabled);

So before you add those, we already have:

static DEFINE_PER_CPU_READ_MOSTLY(struct smca_bank[MAX_NR_BANKS], smca_banks);
static DEFINE_PER_CPU_READ_MOSTLY(u8[N_SMCA_BANK_TYPES], smca_bank_counts);
static DEFINE_PER_CPU(struct threshold_bank **, threshold_banks);
static DEFINE_PER_CPU(u64, bank_map);
static DEFINE_PER_CPU(u64, smca_misc_banks_map);

Please think of a proper struct which collects all that info in the
smallest possible format and unify everything.

It is a mess currently.

> +/*
> + * Enable the APIC LVT interrupt vectors once per-CPU. This should be done before hardware is
> + * ready to send interrupts.
> + *
> + * Individual error sources are enabled later during per-bank init.
> + */
> +static void smca_enable_interrupt_vectors(struct cpuinfo_x86 *c)
> +{
> +	u8 thr_offset, dfr_offset;
> +	u64 mca_intr_cfg;
> +
> +	if (!mce_flags.smca || !mce_flags.succor)
> +		return;
> +
> +	if (c == &boot_cpu_data) {
> +		mce_threshold_vector		= amd_threshold_interrupt;
> +		deferred_error_int_vector	= amd_deferred_error_interrupt;
> +	}

Nah, this should be done differently: you define a function
cpu_mca_init() which you call from early_identify_cpu(). In it, you do
the proper checks and assign those two vectors above. That in
a pre-patch.

Then, the rest becomes per-CPU code which you simply run in
mce_amd_feature_init(), dilligently, one thing after the other.

And then you don't need smca_{dfr,thr}_intr_enabled anymore because you
know that after having run setup_APIC_eilvt().

IOW, mce_amd_feature_init() does *all* per-CPU MCA init on AMD and it is
all concentrated in one place and not spread around.

I think this should be a much better cleanup.

Thx.

-- 
Regards/Gruss,
    Boris.

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