RE: [PATCH v6 0/6] AMD Pstate Fixes And Enhancements

[Deucher, Alexander]

[Public]

> -----Original Message-----
> From: Borislav Petkov <bp@xxxxxxxxx>
> Sent: Friday, February 9, 2024 10:51 AM
> To: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
> Cc: Yuan, Perry <Perry.Yuan@xxxxxxx>; rafael.j.wysocki@xxxxxxxxx;
> Limonciello, Mario <Mario.Limonciello@xxxxxxx>; viresh.kumar@xxxxxxxxxx;
> Huang, Ray <Ray.Huang@xxxxxxx>; Shenoy, Gautham Ranjal
> <gautham.shenoy@xxxxxxx>; Huang, Shimmer
> <Shimmer.Huang@xxxxxxx>; Du, Xiaojian <Xiaojian.Du@xxxxxxx>; Meng,
> Li (Jassmine) <Li.Meng@xxxxxxx>; linux-pm@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v6 0/6] AMD Pstate Fixes And Enhancements
>
> On Thu, Feb 08, 2024 at 11:09:47PM +0000, Deucher, Alexander wrote:
> > Why?
>
> Because we have those rules, you know. You should send about once a week,
> unless you've reworked your set fundamentally.
>

My reading of the rules is that you should wait before resending or pinging if you have not received feedback.  If you are actively receiving feedback, to me, it makes sense to rapidly iterate.  If a patch is reviewed and comments are addressed, it can land rather than waiting an extra week or two.

> Otherwise maintainers mailboxes would be a serious mess. Not that they're
> already such now...
>
> > In this case, there have been a number of review comments which have
> > been addressed in the new revisions of the patch set.  Why delay the
> > new revisions?
>
> See above.
>
> And see below for the "whopping" differences between the last two.
>
> > You'll ultimately get the same amount of email just with a much longer
> > latency.
>
> No, you'll have a lot less email. You send the set, sit tight and collect review
> feedback, work it in and send again after a week or so.

It also adds extra latency.  With my maintainer hat on, I'd like to have new revisions rapidly.  I guess it comes down to personal preference, but I don't do well with task switching.  When a patch set is fresh in my mind, I'd rather see it finished off and committed sooner rather than lingering and then a week or two later, I'd need to page the whole discussion back into my head to make sure everything was addressed and all the tags were collected.

Alex

>
> diff v5..v6 - definitely not resend one day later just to pick up tags.
> -----------
> --- v5_20240207_perry_yuan_amd_pstate_fixes_and_enhancements.mbx
>       2024-02-09 16:43:29.487104935 +0100
> +++ v6_20240208_perry_yuan_amd_pstate_fixes_and_enhancements.mbx
>       2024-02-09 16:42:55.671303380 +0100
> @@ -23,6 +23,7 @@ Reported-by: Gino Badouri <badouri.g@gma
>  Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218171
>  Fixes: fbd74d1689 ("ACPI: CPPC: Fix enabling CPPC on AMD systems with
> shared memory")
>  Signed-off-by: Perry Yuan <perry.yuan@xxxxxxx>
> +Reviewed-by: Gautham R. Shenoy <gautham.shenoy@xxxxxxx>
>  ---
>   arch/x86/kernel/acpi/cppc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-) @@ -293,6 +294,7 @@ Above
> message is not clear enough to ver
>
>  Reviewed-by: Mario Limonciello <mario.limonciello@xxxxxxx>
>  Signed-off-by: Perry Yuan <perry.yuan@xxxxxxx>
> +Reviewed-by: Gautham R. Shenoy <gautham.shenoy@xxxxxxx>
>  ---
>   drivers/acpi/cppc_acpi.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-) @@ -342,8 +344,9 @@ $ cat
> /sys/devices/system/cpu/cpu0/acpi_
>  $ cat /sys/devices/system/cpu/cpu0/acpi_cppc/nominal_freq
>  0
>
> -Signed-off-by: Perry Yuan <perry.yuan@xxxxxxx>
>  Reviewed-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> +Signed-off-by: Perry Yuan <perry.yuan@xxxxxxx>
> +Reviewed-by: Gautham R. Shenoy <gautham.shenoy@xxxxxxx>
>  ---
>   drivers/cpufreq/amd-pstate.c | 57
> ++++++++++++++++++++++++++++++++++--
>   include/linux/amd-pstate.h   |  6 ++++
>
>
> diff v4..v5 - *definitely* *not* warranting a new resend on the next day!
> -----------
> --- v4_20240206_perry_yuan_amd_pstate_fixes_and_enhancements.mbx
>       2024-02-09 16:43:53.922961536 +0100
> +++ v5_20240207_perry_yuan_amd_pstate_fixes_and_enhancements.mbx
>       2024-02-09 16:43:29.487104935 +0100
> @@ -343,13 +343,14 @@ $ cat /sys/devices/system/cpu/cpu0/acpi_
>  0
>
>  Signed-off-by: Perry Yuan <perry.yuan@xxxxxxx>
> +Reviewed-by: Mario Limonciello <mario.limonciello@xxxxxxx>
>  ---
> - drivers/cpufreq/amd-pstate.c | 59
> ++++++++++++++++++++++++++++++++++--
> + drivers/cpufreq/amd-pstate.c | 57
> ++++++++++++++++++++++++++++++++++--
>   include/linux/amd-pstate.h   |  6 ++++
> - 2 files changed, 63 insertions(+), 2 deletions(-)
> + 2 files changed, 61 insertions(+), 2 deletions(-)
>
>  diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c -
> index 77effc3caf6c..874d8b663790 100644
> +index 77effc3caf6c..ff4727c22dce 100644
>  --- a/drivers/cpufreq/amd-pstate.c
>  +++ b/drivers/cpufreq/amd-pstate.c
>  @@ -67,6 +67,7 @@ static struct cpufreq_driver amd_pstate_epp_driver;
> @@ -372,18 +373,16 @@ index 77effc3caf6c..874d8b663790 100644
> +static int __init dmi_matched_7k62_bios_bug(const struct dmi_system_id
> *dmi)  +{
>  +    /**
> -+     * match the broken bios for family 17h, model 31h processor
> ++     * match the broken bios for family 17h processor support CPPC V2
>  +     * broken BIOS lack of nominal_freq and lowest_freq capabilities
>  +     * definition in ACPI tables
>  +     */
> -+    if (boot_cpu_data.x86 == 0x17 && boot_cpu_data.x86_model ==
> 0x31 &&
> -+                    boot_cpu_has(X86_FEATURE_ZEN2)) {
> ++    if (boot_cpu_has(X86_FEATURE_ZEN2)) {
>  +            quirks = dmi->driver_data;
> -+            pr_info("hardware type %s found\n", dmi->ident);
> ++            pr_info("Overriding nominal and lowest frequencies for
> %s\n",
> ++dmi->ident);
>  +            return 1;
>  +    }
>  +
> -+
>  +    return 0;
>  +}
>  +
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette