[PATCH] x86/resctrl: Fix unneeded variable warning reported by kernel test robot

[PATCH] x86/resctrl: Fix unneeded variable warning reported by kernel test robot

[Babu Moger]

kernel test robot reported the following warning after the commit
54e35eb8611c ("x86/resctrl: Read supported bandwidth sources from CPUID").

cocci warnings: (new ones prefixed by >>)
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c:1621:5-8: Unneeded variable: "ret". Return "  0" on line 1655

Fix the warning by removing the variable "ret" and returning 0 directly.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202401241810.jbd8Ipa1-lkp@intel.com/
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 2b69e560b05f..6057f96df73f 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1618,7 +1618,6 @@ static int mbm_config_write_domain(struct rdt_resource *r,
 				   struct rdt_domain *d, u32 evtid, u32 val)
 {
 	struct mon_config_info mon_info = {0};
-	int ret = 0;
 
 	/*
 	 * Read the current config value first. If both are the same then
@@ -1652,7 +1651,7 @@ static int mbm_config_write_domain(struct rdt_resource *r,
 	resctrl_arch_reset_rmid_all(r, d);
 
 out:
-	return ret;
+	return 0;
 }
 
 static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
-- 
2.34.1

Re: [PATCH] x86/resctrl: Fix unneeded variable warning reported by kernel test robot

[Reinette Chatre]

Hi Babu,

Thank you for fixing this so promptly.

I think the subject can just be:
"x86/resctrl: Remove unneeded variable"

On 1/24/2024 9:52 AM, Babu Moger wrote:
> kernel test robot reported the following warning after the commit
> 54e35eb8611c ("x86/resctrl: Read supported bandwidth sources from CPUID").

This can be confusing since it implies that the patch you mention
introduces the issue but instead the variable has been unneeded since
the original:
92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config")

To help clarify you can mention this order of events and also add an
appropriate "Fixes:" tag.

> cocci warnings: (new ones prefixed by >>)
>>> arch/x86/kernel/cpu/resctrl/rdtgroup.c:1621:5-8: Unneeded variable: "ret". Return "  0" on line 1655
> 
> Fix the warning by removing the variable "ret" and returning 0 directly.

cocci warning was spot on*. This fix is not just a change to "make a
warning go away" but instead fixing an actual problem.
It can just be "Remove the unneeded variable and return 0 directly".

> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202401241810.jbd8Ipa1-lkp@intel.com/
> Signed-off-by: Babu Moger <babu.moger@amd.com>

Reinette

* I'll add a private setup with the goal to catch these earlier.

Re: [PATCH] x86/resctrl: Fix unneeded variable warning reported by kernel test robot

[Borislav Petkov]

On Wed, Jan 24, 2024 at 10:25:17AM -0800, Reinette Chatre wrote:
> This can be confusing since it implies that the patch you mention
> introduces the issue but instead the variable has been unneeded since
> the original:
> 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config")

What I said. :)

> To help clarify you can mention this order of events and also add an
> appropriate "Fixes:" tag.
> 
> > cocci warnings: (new ones prefixed by >>)
> >>> arch/x86/kernel/cpu/resctrl/rdtgroup.c:1621:5-8: Unneeded variable: "ret". Return "  0" on line 1655
> > 
> > Fix the warning by removing the variable "ret" and returning 0 directly.
> 
> cocci warning was spot on*. This fix is not just a change to "make a
> warning go away" but instead fixing an actual problem.
> It can just be "Remove the unneeded variable and return 0 directly".

I'll fix all up before applying.

> * I'll add a private setup with the goal to catch these earlier.

Except that it doesn't fire with the patch that added the code. It looks
like the cocci script needs adjustment...

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/resctrl: Fix unneeded variable warning reported by kernel test robot

[Reinette Chatre]

Hi Boris,

On 1/24/2024 10:31 AM, Borislav Petkov wrote:
> On Wed, Jan 24, 2024 at 10:25:17AM -0800, Reinette Chatre wrote:
>> This can be confusing since it implies that the patch you mention
>> introduces the issue but instead the variable has been unneeded since
>> the original:
>> 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config")
> 
> What I said. :)

Right from the start, yes.

> 
>> To help clarify you can mention this order of events and also add an
>> appropriate "Fixes:" tag.
>>
>>> cocci warnings: (new ones prefixed by >>)
>>>>> arch/x86/kernel/cpu/resctrl/rdtgroup.c:1621:5-8: Unneeded variable: "ret". Return "  0" on line 1655
>>>
>>> Fix the warning by removing the variable "ret" and returning 0 directly.
>>
>> cocci warning was spot on*. This fix is not just a change to "make a
>> warning go away" but instead fixing an actual problem.
>> It can just be "Remove the unneeded variable and return 0 directly".
> 
> I'll fix all up before applying.

Thank you very much. For what it is worth, I do agree with the actual fix
and you can add:
Acked-by: Reinette Chatre <reinette.chatre@intel.com>

> 
>> * I'll add a private setup with the goal to catch these earlier.
> 
> Except that it doesn't fire with the patch that added the code. It looks
> like the cocci script needs adjustment...

Reinette

Re: [PATCH] x86/resctrl: Fix unneeded variable warning reported by kernel test robot

[Borislav Petkov]

On Wed, Jan 24, 2024 at 10:51:49AM -0800, Reinette Chatre wrote:
> Thank you very much. For what it is worth, I do agree with the actual fix
> and you can add:
> Acked-by: Reinette Chatre <reinette.chatre@intel.com>

Ok, have a look at the below, pls, and lemme know if that's ok too.

mbm_config_write_domain() only returns 0 so it can be void. So the
callsite doesn't need to check retval either.

Thx.

---
From: Babu Moger <babu.moger@amd.com>
Date: Wed, 24 Jan 2024 11:52:56 -0600
Subject: [PATCH] x86/resctrl: Remove redundant variable in
 mbm_config_write_domain()

The kernel test robot reported the following warning after

  54e35eb8611c ("x86/resctrl: Read supported bandwidth sources from CPUID").

even though the issue is present even in the original patch which added
this function

  92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config")

  $ make C=1 CHECK=scripts/coccicheck arch/x86/kernel/cpu/resctrl/rdtgroup.o
  ...
  arch/x86/kernel/cpu/resctrl/rdtgroup.c:1621:5-8: Unneeded variable: "ret". Return "0" on line 1655

Remove the local variable 'ret'.

  [ bp: Massage commit message, make mbm_config_write_domain() void. ]

Fixes: 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config")
Closes: https://lore.kernel.org/oe-kbuild-all/202401241810.jbd8Ipa1-lkp@intel.com/
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Acked-by: Reinette Chatre <reinette.chatre@intel.com>
Link: https://lore.kernel.org/r/202401241810.jbd8Ipa1-lkp@intel.com
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 2b69e560b05f..c33eb77b6d70 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1614,11 +1614,10 @@ static void mon_event_config_write(void *info)
 	wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
 }
 
-static int mbm_config_write_domain(struct rdt_resource *r,
+static void mbm_config_write_domain(struct rdt_resource *r,
 				   struct rdt_domain *d, u32 evtid, u32 val)
 {
 	struct mon_config_info mon_info = {0};
-	int ret = 0;
 
 	/*
 	 * Read the current config value first. If both are the same then
@@ -1627,7 +1626,7 @@ static int mbm_config_write_domain(struct rdt_resource *r,
 	mon_info.evtid = evtid;
 	mondata_config_read(d, &mon_info);
 	if (mon_info.mon_config == val)
-		goto out;
+		return;
 
 	mon_info.mon_config = val;
 
@@ -1650,9 +1649,6 @@ static int mbm_config_write_domain(struct rdt_resource *r,
 	 * mbm_local and mbm_total counts for all the RMIDs.
 	 */
 	resctrl_arch_reset_rmid_all(r, d);
-
-out:
-	return ret;
 }
 
 static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
@@ -1661,7 +1657,6 @@ static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
 	char *dom_str = NULL, *id_str;
 	unsigned long dom_id, val;
 	struct rdt_domain *d;
-	int ret = 0;
 
 next:
 	if (!tok || tok[0] == '\0')
@@ -1690,9 +1685,7 @@ static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
 
 	list_for_each_entry(d, &r->domains, list) {
 		if (d->id == dom_id) {
-			ret = mbm_config_write_domain(r, d, evtid, val);
-			if (ret)
-				return -EINVAL;
+			mbm_config_write_domain(r, d, evtid, val);
 			goto next;
 		}
 	}
-- 
2.43.0

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/resctrl: Fix unneeded variable warning reported by kernel test robot

[Moger, Babu]

Hi Boris,

On 1/24/24 13:14, Borislav Petkov wrote:
> On Wed, Jan 24, 2024 at 10:51:49AM -0800, Reinette Chatre wrote:
>> Thank you very much. For what it is worth, I do agree with the actual fix
>> and you can add:
>> Acked-by: Reinette Chatre <reinette.chatre@intel.com>
> 
> Ok, have a look at the below, pls, and lemme know if that's ok too.
> 
> mbm_config_write_domain() only returns 0 so it can be void. So the
> callsite doesn't need to check retval either.

Yes. Looks good. Compile tested also. Thanks

> 
> Thx.
> 
> ---
> From: Babu Moger <babu.moger@amd.com>
> Date: Wed, 24 Jan 2024 11:52:56 -0600
> Subject: [PATCH] x86/resctrl: Remove redundant variable in
>  mbm_config_write_domain()
> 
> The kernel test robot reported the following warning after
> 
>   54e35eb8611c ("x86/resctrl: Read supported bandwidth sources from CPUID").
> 
> even though the issue is present even in the original patch which added
> this function
> 
>   92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config")
> 
>   $ make C=1 CHECK=scripts/coccicheck arch/x86/kernel/cpu/resctrl/rdtgroup.o
>   ...
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c:1621:5-8: Unneeded variable: "ret". Return "0" on line 1655
> 
> Remove the local variable 'ret'.
> 
>   [ bp: Massage commit message, make mbm_config_write_domain() void. ]
> 
> Fixes: 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config")
> Closes: https://lore.kernel.org/oe-kbuild-all/202401241810.jbd8Ipa1-lkp@intel.com/
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> Acked-by: Reinette Chatre <reinette.chatre@intel.com>
> Link: https://lore.kernel.org/r/202401241810.jbd8Ipa1-lkp@intel.com
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 2b69e560b05f..c33eb77b6d70 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1614,11 +1614,10 @@ static void mon_event_config_write(void *info)
>  	wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
>  }
>  
> -static int mbm_config_write_domain(struct rdt_resource *r,
> +static void mbm_config_write_domain(struct rdt_resource *r,
>  				   struct rdt_domain *d, u32 evtid, u32 val)
>  {
>  	struct mon_config_info mon_info = {0};
> -	int ret = 0;
>  
>  	/*
>  	 * Read the current config value first. If both are the same then
> @@ -1627,7 +1626,7 @@ static int mbm_config_write_domain(struct rdt_resource *r,
>  	mon_info.evtid = evtid;
>  	mondata_config_read(d, &mon_info);
>  	if (mon_info.mon_config == val)
> -		goto out;
> +		return;
>  
>  	mon_info.mon_config = val;
>  
> @@ -1650,9 +1649,6 @@ static int mbm_config_write_domain(struct rdt_resource *r,
>  	 * mbm_local and mbm_total counts for all the RMIDs.
>  	 */
>  	resctrl_arch_reset_rmid_all(r, d);
> -
> -out:
> -	return ret;
>  }
>  
>  static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
> @@ -1661,7 +1657,6 @@ static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
>  	char *dom_str = NULL, *id_str;
>  	unsigned long dom_id, val;
>  	struct rdt_domain *d;
> -	int ret = 0;
>  
>  next:
>  	if (!tok || tok[0] == '\0')
> @@ -1690,9 +1685,7 @@ static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
>  
>  	list_for_each_entry(d, &r->domains, list) {
>  		if (d->id == dom_id) {
> -			ret = mbm_config_write_domain(r, d, evtid, val);
> -			if (ret)
> -				return -EINVAL;
> +			mbm_config_write_domain(r, d, evtid, val);
>  			goto next;
>  		}
>  	}

-- 
Thanks
Babu Moger

Re: [PATCH] x86/resctrl: Fix unneeded variable warning reported by kernel test robot

[Borislav Petkov]

On Wed, Jan 24, 2024 at 01:39:01PM -0600, Moger, Babu wrote:
> Yes. Looks good. Compile tested also. Thanks

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/resctrl: Fix unneeded variable warning reported by kernel test robot

[Reinette Chatre]

Hi Boris,

On 1/24/2024 11:14 AM, Borislav Petkov wrote:
> On Wed, Jan 24, 2024 at 10:51:49AM -0800, Reinette Chatre wrote:
>> Thank you very much. For what it is worth, I do agree with the actual fix
>> and you can add:
>> Acked-by: Reinette Chatre <reinette.chatre@intel.com>
> 
> Ok, have a look at the below, pls, and lemme know if that's ok too.
> 
> mbm_config_write_domain() only returns 0 so it can be void. So the
> callsite doesn't need to check retval either.

Thank you very much for looking further and catching this.

You may already have taken care of this after sending this version, but I
just have a couple of nitpick comments intended to help this patch in case it
requires a clean slate from some checks. 

> 
> Thx.
> 
> ---
> From: Babu Moger <babu.moger@amd.com>
> Date: Wed, 24 Jan 2024 11:52:56 -0600
> Subject: [PATCH] x86/resctrl: Remove redundant variable in
>  mbm_config_write_domain()
> 
> The kernel test robot reported the following warning after
> 
>   54e35eb8611c ("x86/resctrl: Read supported bandwidth sources from CPUID").

I think a "commit" prefix is required here and below.

> 
> even though the issue is present even in the original patch which added
> this function
> 
>   92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config")
> 
>   $ make C=1 CHECK=scripts/coccicheck arch/x86/kernel/cpu/resctrl/rdtgroup.o
>   ...
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c:1621:5-8: Unneeded variable: "ret". Return "0" on line 1655
> 
> Remove the local variable 'ret'.
> 
>   [ bp: Massage commit message, make mbm_config_write_domain() void. ]
> 
> Fixes: 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config")
> Closes: https://lore.kernel.org/oe-kbuild-all/202401241810.jbd8Ipa1-lkp@intel.com/
> Reported-by: kernel test robot <lkp@intel.com>

I do not know if this is something tip prefers but the general patch checkers prefer
that "Reported-by:" is followed by "Closes:".

> Signed-off-by: Babu Moger <babu.moger@amd.com>
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> Acked-by: Reinette Chatre <reinette.chatre@intel.com>
> Link: https://lore.kernel.org/r/202401241810.jbd8Ipa1-lkp@intel.com
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 2b69e560b05f..c33eb77b6d70 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1614,11 +1614,10 @@ static void mon_event_config_write(void *info)
>  	wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
>  }
>  
> -static int mbm_config_write_domain(struct rdt_resource *r,
> +static void mbm_config_write_domain(struct rdt_resource *r,
>  				   struct rdt_domain *d, u32 evtid, u32 val)

This shifts the alignment slightly to no longer match the open parenthesis.

>  {
>  	struct mon_config_info mon_info = {0};
> -	int ret = 0;
>  
>  	/*
>  	 * Read the current config value first. If both are the same then
> @@ -1627,7 +1626,7 @@ static int mbm_config_write_domain(struct rdt_resource *r,
>  	mon_info.evtid = evtid;
>  	mondata_config_read(d, &mon_info);
>  	if (mon_info.mon_config == val)
> -		goto out;
> +		return;
>  
>  	mon_info.mon_config = val;
>  
> @@ -1650,9 +1649,6 @@ static int mbm_config_write_domain(struct rdt_resource *r,
>  	 * mbm_local and mbm_total counts for all the RMIDs.
>  	 */
>  	resctrl_arch_reset_rmid_all(r, d);
> -
> -out:
> -	return ret;
>  }
>  
>  static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
> @@ -1661,7 +1657,6 @@ static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
>  	char *dom_str = NULL, *id_str;
>  	unsigned long dom_id, val;
>  	struct rdt_domain *d;
> -	int ret = 0;
>  
>  next:
>  	if (!tok || tok[0] == '\0')
> @@ -1690,9 +1685,7 @@ static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
>  
>  	list_for_each_entry(d, &r->domains, list) {
>  		if (d->id == dom_id) {
> -			ret = mbm_config_write_domain(r, d, evtid, val);
> -			if (ret)
> -				return -EINVAL;
> +			mbm_config_write_domain(r, d, evtid, val);
>  			goto next;
>  		}
>  	}

The core changes look good to me. Thank you very much for creating this fix.

Reinette

Re: [PATCH] x86/resctrl: Fix unneeded variable warning reported by kernel test robot

[Borislav Petkov]

Hi,

On Wed, Jan 24, 2024 at 12:04:31PM -0800, Reinette Chatre wrote:
> >   54e35eb8611c ("x86/resctrl: Read supported bandwidth sources from CPUID").
> 
> I think a "commit" prefix is required here and below.

Yeah, but if you see a 12-char sha1 followed by a title in (" "), that
is a commit and nothing else, right?

If I say "commit" too it is kinda redundant.

> > Fixes: 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config")
> > Closes: https://lore.kernel.org/oe-kbuild-all/202401241810.jbd8Ipa1-lkp@intel.com/
> > Reported-by: kernel test robot <lkp@intel.com>
> 
> I do not know if this is something tip prefers but the general patch checkers prefer
> that "Reported-by:" is followed by "Closes:".

Good question. Documentation/process/maintainer-tip.rst doesn't clarify
that, lemme send a patch for it and see what my brethren in arms think.
:-)

> This shifts the alignment slightly to no longer match the open parenthesis.

Fixed.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/resctrl: Fix unneeded variable warning reported by kernel test robot

[Reinette Chatre]

Hi Boris,

On 1/24/2024 12:45 PM, Borislav Petkov wrote:
> Hi,
> 
> On Wed, Jan 24, 2024 at 12:04:31PM -0800, Reinette Chatre wrote:
>>>   54e35eb8611c ("x86/resctrl: Read supported bandwidth sources from CPUID").
>>
>> I think a "commit" prefix is required here and below.
> 
> Yeah, but if you see a 12-char sha1 followed by a title in (" "), that
> is a commit and nothing else, right?
> 
> If I say "commit" too it is kinda redundant.

I do not know the motivation for that requirement. From what I can tell the
change [1] that added that check went in as first version without discussion.
[1] starts by saying that the format is "preferred" so I assume there is
some history that I am not familiar with.

Reinette

[1] https://lore.kernel.org/all/976c6cdd680db4b55ae31b5fc2d1779da5c0dc66.camel@perches.com/

Re: [PATCH] x86/resctrl: Fix unneeded variable warning reported by kernel test robot

[Borislav Petkov]

On Wed, Jan 24, 2024 at 01:31:01PM -0800, Reinette Chatre wrote:
> I do not know the motivation for that requirement. From what I can tell the
> change [1] that added that check went in as first version without discussion.
> [1] starts by saying that the format is "preferred" so I assume there is
> some history that I am not familiar with.

My main goal with commit messages, code comments and every other *text*
you have in the code is to be as succinct and understandable as possible
for time considerations, clarity, etc.

If I see a 12-char sha1 followed by a title, to me that is a commit. No
need to say "commit" too. But as already said, if you prefer to have
"commit" there, I'll add it - no biggie.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/resctrl: Fix unneeded variable warning reported by kernel test robot

[Reinette Chatre]

Hi Boris,

On 1/24/2024 2:46 PM, Borislav Petkov wrote:
> On Wed, Jan 24, 2024 at 01:31:01PM -0800, Reinette Chatre wrote:
>> I do not know the motivation for that requirement. From what I can tell the
>> change [1] that added that check went in as first version without discussion.
>> [1] starts by saying that the format is "preferred" so I assume there is
>> some history that I am not familiar with.
> 
> My main goal with commit messages, code comments and every other *text*
> you have in the code is to be as succinct and understandable as possible
> for time considerations, clarity, etc.
> 
> If I see a 12-char sha1 followed by a title, to me that is a commit. No
> need to say "commit" too. But as already said, if you prefer to have
> "commit" there, I'll add it - no biggie.

I totally understand your sentiment. I am just the messenger here.

Without the "commit" this patch triggers a loud:

ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config")'
#15:
  92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config")

I usually (unless, for example, following checkpatch.pl advice causes a change
to fall out of place with surrounding code) try to format my patches to get a
clean slate from checkpatch.pl with the goal to eliminate obstacles to the
patch getting included.
Since you are the one that decides the rules for inclusion you can make this
check to be one where checkpatch.pl can be ignored. No objection from me if
you choose to do so (and I will note the precedent for future patches).

Reinette

Re: [PATCH] x86/resctrl: Fix unneeded variable warning reported by kernel test robot

[Borislav Petkov]

On Wed, Jan 24, 2024 at 03:03:15PM -0800, Reinette Chatre wrote:
> Since you are the one that decides the rules for inclusion you can make this
> check to be one where checkpatch.pl can be ignored. No objection from me if
> you choose to do so (and I will note the precedent for future patches).

Nah, that's not nearly as important for you to change your workflow.

What I'd suggest, though, is to sanity-check what checkpatch suggests and
ask yourself whether it always makes sense.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/resctrl: Fix unneeded variable warning reported by kernel test robot

[Borislav Petkov]

On Thu, Jan 25, 2024 at 12:34:24AM +0100, Borislav Petkov wrote:
> On Wed, Jan 24, 2024 at 03:03:15PM -0800, Reinette Chatre wrote:
> > Since you are the one that decides the rules for inclusion you can make this
> > check to be one where checkpatch.pl can be ignored. No objection from me if
> > you choose to do so (and I will note the precedent for future patches).
> 
> Nah, that's not nearly as important for you to change your workflow.
> 
> What I'd suggest, though, is to sanity-check what checkpatch suggests and
> ask yourself whether it always makes sense.

Dammit, there's a reason I don't use this abomination:

$ cat /tmp/0001-x86-resctrl-Remove-redundant-variable-in-mbm_config_.patch | ./scripts/checkpatch.pl

WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
#9: 
      commit 54e35eb8611c ("x86/resctrl: Read supported bandwidth sources from CPUID").


WTF?!

I have a line underneath which is even one char longer:

"  commit 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config")"

but it doesn't complain about it.

What a bunch of crap.

I'm writing it as a maximally readable commit message and that's it.
Human-readable beats any script, any day of the week.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/resctrl: Fix unneeded variable warning reported by kernel test robot

[Reinette Chatre]

Hi Boris,

On 1/24/2024 3:34 PM, Borislav Petkov wrote:
> On Wed, Jan 24, 2024 at 03:03:15PM -0800, Reinette Chatre wrote:
>> Since you are the one that decides the rules for inclusion you can make this
>> check to be one where checkpatch.pl can be ignored. No objection from me if
>> you choose to do so (and I will note the precedent for future patches).
> 
> Nah, that's not nearly as important for you to change your workflow.
> 
> What I'd suggest, though, is to sanity-check what checkpatch suggests and
> ask yourself whether it always makes sense.

Will do. Thank you for considering and discussing this detail. The final
patch you just merged looks good to me. Thank you very much for fixing this.

Reinette