On Fri, Nov 16, 2018 at 08:54:26PM +0000, Moger, Babu wrote:
> Separate the call sequence for rdt_quirks and MBA feature.
> This is in preparation to handle vendor differences in these
> call sequences.
>
> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
> ---
> arch/x86/kernel/cpu/resctrl.c | 27 +++++++++++++++++++++------
> 1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl.c b/arch/x86/kernel/cpu/resctrl.c
> index 5d526dc45751..4cea275c7c57 100644
> --- a/arch/x86/kernel/cpu/resctrl.c
> +++ b/arch/x86/kernel/cpu/resctrl.c
> @@ -794,6 +794,14 @@ static bool __init rdt_cpu_has(int flag)
> return ret;
> }
Just nitpicks:
> +static __init bool rdt_mba_config(void)
That function doesn't have a verb in its name but it needs to have one
stating what it does. You could do
mv rdt_get_mem_config() -> __rdt_get_mem_config()
mv rdt_mba_config() -> rdt_get_mem_config()
to have the hierarchy of what calls what. And then the AMD variant will
be called __rdt_get_mem_config_amd().
Also, those are all static functions so you can just as well drop the
"rdt" prefix, I'd say.
> +{
> + if (rdt_cpu_has(X86_FEATURE_MBA))
> + return rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]);
> +
> + return false;
> +}
> +
> static __init bool get_rdt_alloc_resources(void)
> {
> bool ret = false;
> @@ -818,10 +826,9 @@ static __init bool get_rdt_alloc_resources(void)
> ret = true;
> }
>
> - if (rdt_cpu_has(X86_FEATURE_MBA)) {
> - if (rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]))
> - ret = true;
> - }
> + if (rdt_mba_config())
> + ret = true;
> +
> return ret;
> }
>
> @@ -840,7 +847,7 @@ static __init bool get_rdt_mon_resources(void)
> return !rdt_get_mon_l3_config(&rdt_resources_all[RDT_RESOURCE_L3]);
> }
>
> -static __init void rdt_quirks(void)
> +static __init void rdt_quirks_intel(void)
> {
> switch (boot_cpu_data.x86_model) {
> case INTEL_FAM6_HASWELL_X:
> @@ -855,9 +862,14 @@ static __init void rdt_quirks(void)
> }
> }
>
> +static __init void rdt_quirks(void)
Those functions also need to have a verb in the name stating what they
do.
> +{
> + if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
> + rdt_quirks_intel();
> +}
> +
> static __init bool get_rdt_resources(void)
> {
> - rdt_quirks();
> rdt_alloc_capable = get_rdt_alloc_resources();
> rdt_mon_capable = get_rdt_mon_resources();
>
> @@ -871,6 +883,9 @@ static int __init resctrl_late_init(void)
> struct rdt_resource *r;
> int state, ret;
>
> + /* Run quirks first */
> + rdt_quirks();
If the comment wasn't there, seeing "rdt_quirks();" doesn't say much and
makes me go look at what that function does.
> +
> if (!get_rdt_resources())
Unlike here, where it is clear that this gets the rdt resources.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
