tree: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/cache
head: 54e35eb8611cce5550d3d7689679b1a91c864f28
commit: 54e35eb8611cce5550d3d7689679b1a91c864f28 [3/3] x86/resctrl: Read supported bandwidth sources from CPUID
config: x86_64-randconfig-102-20240124 (https://download.01.org/0day-ci/archive/20240124/[email protected]/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
cocci warnings: (new ones prefixed by >>)
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c:1621:5-8: Unneeded variable: "ret". Return " 0" on line 1655
vim +1621 arch/x86/kernel/cpu/resctrl/rdtgroup.c
92bd5a1390335b Babu Moger 2023-01-13 1616
92bd5a1390335b Babu Moger 2023-01-13 1617 static int mbm_config_write_domain(struct rdt_resource *r,
92bd5a1390335b Babu Moger 2023-01-13 1618 struct rdt_domain *d, u32 evtid, u32 val)
92bd5a1390335b Babu Moger 2023-01-13 1619 {
92bd5a1390335b Babu Moger 2023-01-13 1620 struct mon_config_info mon_info = {0};
92bd5a1390335b Babu Moger 2023-01-13 @1621 int ret = 0;
92bd5a1390335b Babu Moger 2023-01-13 1622
92bd5a1390335b Babu Moger 2023-01-13 1623 /*
92bd5a1390335b Babu Moger 2023-01-13 1624 * Read the current config value first. If both are the same then
92bd5a1390335b Babu Moger 2023-01-13 1625 * no need to write it again.
92bd5a1390335b Babu Moger 2023-01-13 1626 */
92bd5a1390335b Babu Moger 2023-01-13 1627 mon_info.evtid = evtid;
92bd5a1390335b Babu Moger 2023-01-13 1628 mondata_config_read(d, &mon_info);
92bd5a1390335b Babu Moger 2023-01-13 1629 if (mon_info.mon_config == val)
92bd5a1390335b Babu Moger 2023-01-13 1630 goto out;
92bd5a1390335b Babu Moger 2023-01-13 1631
92bd5a1390335b Babu Moger 2023-01-13 1632 mon_info.mon_config = val;
92bd5a1390335b Babu Moger 2023-01-13 1633
92bd5a1390335b Babu Moger 2023-01-13 1634 /*
92bd5a1390335b Babu Moger 2023-01-13 1635 * Update MSR_IA32_EVT_CFG_BASE MSR on one of the CPUs in the
92bd5a1390335b Babu Moger 2023-01-13 1636 * domain. The MSRs offset from MSR MSR_IA32_EVT_CFG_BASE
92bd5a1390335b Babu Moger 2023-01-13 1637 * are scoped at the domain level. Writing any of these MSRs
92bd5a1390335b Babu Moger 2023-01-13 1638 * on one CPU is observed by all the CPUs in the domain.
92bd5a1390335b Babu Moger 2023-01-13 1639 */
92bd5a1390335b Babu Moger 2023-01-13 1640 smp_call_function_any(&d->cpu_mask, mon_event_config_write,
92bd5a1390335b Babu Moger 2023-01-13 1641 &mon_info, 1);
92bd5a1390335b Babu Moger 2023-01-13 1642
92bd5a1390335b Babu Moger 2023-01-13 1643 /*
92bd5a1390335b Babu Moger 2023-01-13 1644 * When an Event Configuration is changed, the bandwidth counters
92bd5a1390335b Babu Moger 2023-01-13 1645 * for all RMIDs and Events will be cleared by the hardware. The
92bd5a1390335b Babu Moger 2023-01-13 1646 * hardware also sets MSR_IA32_QM_CTR.Unavailable (bit 62) for
92bd5a1390335b Babu Moger 2023-01-13 1647 * every RMID on the next read to any event for every RMID.
92bd5a1390335b Babu Moger 2023-01-13 1648 * Subsequent reads will have MSR_IA32_QM_CTR.Unavailable (bit 62)
92bd5a1390335b Babu Moger 2023-01-13 1649 * cleared while it is tracked by the hardware. Clear the
92bd5a1390335b Babu Moger 2023-01-13 1650 * mbm_local and mbm_total counts for all the RMIDs.
92bd5a1390335b Babu Moger 2023-01-13 1651 */
92bd5a1390335b Babu Moger 2023-01-13 1652 resctrl_arch_reset_rmid_all(r, d);
92bd5a1390335b Babu Moger 2023-01-13 1653
92bd5a1390335b Babu Moger 2023-01-13 1654 out:
92bd5a1390335b Babu Moger 2023-01-13 @1655 return ret;
92bd5a1390335b Babu Moger 2023-01-13 1656 }
92bd5a1390335b Babu Moger 2023-01-13 1657
:::::: The code at line 1621 was first introduced by commit
:::::: 92bd5a1390335bb3cc76bdf1b4356edbc94d408d x86/resctrl: Add interface to write mbm_total_bytes_config
:::::: TO: Babu Moger <[email protected]>
:::::: CC: Borislav Petkov (AMD) <[email protected]>
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Wed, Jan 24, 2024 at 06:31:41PM +0800, kernel test robot wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/cache
> head: 54e35eb8611cce5550d3d7689679b1a91c864f28
> commit: 54e35eb8611cce5550d3d7689679b1a91c864f28 [3/3] x86/resctrl: Read supported bandwidth sources from CPUID
> config: x86_64-randconfig-102-20240124 (https://download.01.org/0day-ci/archive/20240124/[email protected]/config)
> compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <[email protected]>
> | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> cocci warnings: (new ones prefixed by >>)
> >> arch/x86/kernel/cpu/resctrl/rdtgroup.c:1621:5-8: Unneeded variable: "ret". Return " 0" on line 1655
Well, AFAICT, even with the tree checked out at
92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config")
which is the first patch that added this function, ret is unneeded.
But scripts/coccinelle/misc/returnvar.cocci doesn't warn about it then,
only now that that hunk with the "return -EINVAL;" is removed in the
patch you're reporting this against.
Lemme add some cocci people to Cc for comment and leave the rest for
reference.
Thx.
> vim +1621 arch/x86/kernel/cpu/resctrl/rdtgroup.c
>
> 92bd5a1390335b Babu Moger 2023-01-13 1616
> 92bd5a1390335b Babu Moger 2023-01-13 1617 static int mbm_config_write_domain(struct rdt_resource *r,
> 92bd5a1390335b Babu Moger 2023-01-13 1618 struct rdt_domain *d, u32 evtid, u32 val)
> 92bd5a1390335b Babu Moger 2023-01-13 1619 {
> 92bd5a1390335b Babu Moger 2023-01-13 1620 struct mon_config_info mon_info = {0};
> 92bd5a1390335b Babu Moger 2023-01-13 @1621 int ret = 0;
> 92bd5a1390335b Babu Moger 2023-01-13 1622
> 92bd5a1390335b Babu Moger 2023-01-13 1623 /*
> 92bd5a1390335b Babu Moger 2023-01-13 1624 * Read the current config value first. If both are the same then
> 92bd5a1390335b Babu Moger 2023-01-13 1625 * no need to write it again.
> 92bd5a1390335b Babu Moger 2023-01-13 1626 */
> 92bd5a1390335b Babu Moger 2023-01-13 1627 mon_info.evtid = evtid;
> 92bd5a1390335b Babu Moger 2023-01-13 1628 mondata_config_read(d, &mon_info);
> 92bd5a1390335b Babu Moger 2023-01-13 1629 if (mon_info.mon_config == val)
> 92bd5a1390335b Babu Moger 2023-01-13 1630 goto out;
> 92bd5a1390335b Babu Moger 2023-01-13 1631
> 92bd5a1390335b Babu Moger 2023-01-13 1632 mon_info.mon_config = val;
> 92bd5a1390335b Babu Moger 2023-01-13 1633
> 92bd5a1390335b Babu Moger 2023-01-13 1634 /*
> 92bd5a1390335b Babu Moger 2023-01-13 1635 * Update MSR_IA32_EVT_CFG_BASE MSR on one of the CPUs in the
> 92bd5a1390335b Babu Moger 2023-01-13 1636 * domain. The MSRs offset from MSR MSR_IA32_EVT_CFG_BASE
> 92bd5a1390335b Babu Moger 2023-01-13 1637 * are scoped at the domain level. Writing any of these MSRs
> 92bd5a1390335b Babu Moger 2023-01-13 1638 * on one CPU is observed by all the CPUs in the domain.
> 92bd5a1390335b Babu Moger 2023-01-13 1639 */
> 92bd5a1390335b Babu Moger 2023-01-13 1640 smp_call_function_any(&d->cpu_mask, mon_event_config_write,
> 92bd5a1390335b Babu Moger 2023-01-13 1641 &mon_info, 1);
> 92bd5a1390335b Babu Moger 2023-01-13 1642
> 92bd5a1390335b Babu Moger 2023-01-13 1643 /*
> 92bd5a1390335b Babu Moger 2023-01-13 1644 * When an Event Configuration is changed, the bandwidth counters
> 92bd5a1390335b Babu Moger 2023-01-13 1645 * for all RMIDs and Events will be cleared by the hardware. The
> 92bd5a1390335b Babu Moger 2023-01-13 1646 * hardware also sets MSR_IA32_QM_CTR.Unavailable (bit 62) for
> 92bd5a1390335b Babu Moger 2023-01-13 1647 * every RMID on the next read to any event for every RMID.
> 92bd5a1390335b Babu Moger 2023-01-13 1648 * Subsequent reads will have MSR_IA32_QM_CTR.Unavailable (bit 62)
> 92bd5a1390335b Babu Moger 2023-01-13 1649 * cleared while it is tracked by the hardware. Clear the
> 92bd5a1390335b Babu Moger 2023-01-13 1650 * mbm_local and mbm_total counts for all the RMIDs.
> 92bd5a1390335b Babu Moger 2023-01-13 1651 */
> 92bd5a1390335b Babu Moger 2023-01-13 1652 resctrl_arch_reset_rmid_all(r, d);
> 92bd5a1390335b Babu Moger 2023-01-13 1653
> 92bd5a1390335b Babu Moger 2023-01-13 1654 out:
> 92bd5a1390335b Babu Moger 2023-01-13 @1655 return ret;
> 92bd5a1390335b Babu Moger 2023-01-13 1656 }
> 92bd5a1390335b Babu Moger 2023-01-13 1657
>
> :::::: The code at line 1621 was first introduced by commit
> :::::: 92bd5a1390335bb3cc76bdf1b4356edbc94d408d x86/resctrl: Add interface to write mbm_total_bytes_config
>
> :::::: TO: Babu Moger <[email protected]>
> :::::: CC: Borislav Petkov (AMD) <[email protected]>
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Hi Boris,
On 1/24/24 04:55, Borislav Petkov wrote:
> On Wed, Jan 24, 2024 at 06:31:41PM +0800, kernel test robot wrote:
>> tree: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/cache
>> head: 54e35eb8611cce5550d3d7689679b1a91c864f28
>> commit: 54e35eb8611cce5550d3d7689679b1a91c864f28 [3/3] x86/resctrl: Read supported bandwidth sources from CPUID
>> config: x86_64-randconfig-102-20240124 (https://download.01.org/0day-ci/archive/20240124/[email protected]/config)
>> compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
>>
>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>> the same patch/commit), kindly add following tags
>> | Reported-by: kernel test robot <[email protected]>
>> | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>>
>> cocci warnings: (new ones prefixed by >>)
>>>> arch/x86/kernel/cpu/resctrl/rdtgroup.c:1621:5-8: Unneeded variable: "ret". Return " 0" on line 1655
Hmm Interesting..
Looking at the code, I see the ret variable is really not required.
The following patch should fix the problem. Let me know if you want me to
send the patch officially. Thanks
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)
>
> Well, AFAICT, even with the tree checked out at
>
> 92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config")
>
> which is the first patch that added this function, ret is unneeded.
>
> But scripts/coccinelle/misc/returnvar.cocci doesn't warn about it then,
> only now that that hunk with the "return -EINVAL;" is removed in the
> patch you're reporting this against.
>
> Lemme add some cocci people to Cc for comment and leave the rest for
> reference.
>
> Thx.
>
>> vim +1621 arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>
>> 92bd5a1390335b Babu Moger 2023-01-13 1616
>> 92bd5a1390335b Babu Moger 2023-01-13 1617 static int mbm_config_write_domain(struct rdt_resource *r,
>> 92bd5a1390335b Babu Moger 2023-01-13 1618 struct rdt_domain *d, u32 evtid, u32 val)
>> 92bd5a1390335b Babu Moger 2023-01-13 1619 {
>> 92bd5a1390335b Babu Moger 2023-01-13 1620 struct mon_config_info mon_info = {0};
>> 92bd5a1390335b Babu Moger 2023-01-13 @1621 int ret = 0;
>> 92bd5a1390335b Babu Moger 2023-01-13 1622
>> 92bd5a1390335b Babu Moger 2023-01-13 1623 /*
>> 92bd5a1390335b Babu Moger 2023-01-13 1624 * Read the current config value first. If both are the same then
>> 92bd5a1390335b Babu Moger 2023-01-13 1625 * no need to write it again.
>> 92bd5a1390335b Babu Moger 2023-01-13 1626 */
>> 92bd5a1390335b Babu Moger 2023-01-13 1627 mon_info.evtid = evtid;
>> 92bd5a1390335b Babu Moger 2023-01-13 1628 mondata_config_read(d, &mon_info);
>> 92bd5a1390335b Babu Moger 2023-01-13 1629 if (mon_info.mon_config == val)
>> 92bd5a1390335b Babu Moger 2023-01-13 1630 goto out;
>> 92bd5a1390335b Babu Moger 2023-01-13 1631
>> 92bd5a1390335b Babu Moger 2023-01-13 1632 mon_info.mon_config = val;
>> 92bd5a1390335b Babu Moger 2023-01-13 1633
>> 92bd5a1390335b Babu Moger 2023-01-13 1634 /*
>> 92bd5a1390335b Babu Moger 2023-01-13 1635 * Update MSR_IA32_EVT_CFG_BASE MSR on one of the CPUs in the
>> 92bd5a1390335b Babu Moger 2023-01-13 1636 * domain. The MSRs offset from MSR MSR_IA32_EVT_CFG_BASE
>> 92bd5a1390335b Babu Moger 2023-01-13 1637 * are scoped at the domain level. Writing any of these MSRs
>> 92bd5a1390335b Babu Moger 2023-01-13 1638 * on one CPU is observed by all the CPUs in the domain.
>> 92bd5a1390335b Babu Moger 2023-01-13 1639 */
>> 92bd5a1390335b Babu Moger 2023-01-13 1640 smp_call_function_any(&d->cpu_mask, mon_event_config_write,
>> 92bd5a1390335b Babu Moger 2023-01-13 1641 &mon_info, 1);
>> 92bd5a1390335b Babu Moger 2023-01-13 1642
>> 92bd5a1390335b Babu Moger 2023-01-13 1643 /*
>> 92bd5a1390335b Babu Moger 2023-01-13 1644 * When an Event Configuration is changed, the bandwidth counters
>> 92bd5a1390335b Babu Moger 2023-01-13 1645 * for all RMIDs and Events will be cleared by the hardware. The
>> 92bd5a1390335b Babu Moger 2023-01-13 1646 * hardware also sets MSR_IA32_QM_CTR.Unavailable (bit 62) for
>> 92bd5a1390335b Babu Moger 2023-01-13 1647 * every RMID on the next read to any event for every RMID.
>> 92bd5a1390335b Babu Moger 2023-01-13 1648 * Subsequent reads will have MSR_IA32_QM_CTR.Unavailable (bit 62)
>> 92bd5a1390335b Babu Moger 2023-01-13 1649 * cleared while it is tracked by the hardware. Clear the
>> 92bd5a1390335b Babu Moger 2023-01-13 1650 * mbm_local and mbm_total counts for all the RMIDs.
>> 92bd5a1390335b Babu Moger 2023-01-13 1651 */
>> 92bd5a1390335b Babu Moger 2023-01-13 1652 resctrl_arch_reset_rmid_all(r, d);
>> 92bd5a1390335b Babu Moger 2023-01-13 1653
>> 92bd5a1390335b Babu Moger 2023-01-13 1654 out:
>> 92bd5a1390335b Babu Moger 2023-01-13 @1655 return ret;
>> 92bd5a1390335b Babu Moger 2023-01-13 1656 }
>> 92bd5a1390335b Babu Moger 2023-01-13 1657
>>
>> :::::: The code at line 1621 was first introduced by commit
>> :::::: 92bd5a1390335bb3cc76bdf1b4356edbc94d408d x86/resctrl: Add interface to write mbm_total_bytes_config
>>
>> :::::: TO: Babu Moger <[email protected]>
>> :::::: CC: Borislav Petkov (AMD) <[email protected]>
>>
>> --
>> 0-DAY CI Kernel Test Service
>> https://github.com/intel/lkp-tests/wiki
>
--
Thanks
Babu Moger
On Wed, Jan 24, 2024 at 08:58:27AM -0600, Moger, Babu wrote:
> The following patch should fix the problem.
Obviously.
> Let me know if you want me to send the patch officially.
Yes, please send a proper patch and add the Reported-by: and the Closes:
tags from the robot.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
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 <[email protected]>
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Signed-off-by: Babu Moger <[email protected]>
---
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
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 <[email protected]>
> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> Signed-off-by: Babu Moger <[email protected]>
Reinette
* I'll add a private setup with the goal to catch these earlier.
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
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 <[email protected]>
>
>> * 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
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 <[email protected]>
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 <[email protected]>
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/[email protected]/
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Babu Moger <[email protected]>
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
Acked-by: Reinette Chatre <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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
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 <[email protected]>
>
> 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 <[email protected]>
> 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/[email protected]/
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Babu Moger <[email protected]>
> Signed-off-by: Borislav Petkov (AMD) <[email protected]>
> Acked-by: Reinette Chatre <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> ---
> 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
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 <[email protected]>
>
> 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 <[email protected]>
> 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/[email protected]/
> Reported-by: kernel test robot <[email protected]>
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 <[email protected]>
> Signed-off-by: Borislav Petkov (AMD) <[email protected]>
> Acked-by: Reinette Chatre <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> ---
> 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
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/[email protected]/
> > Reported-by: kernel test robot <[email protected]>
>
> 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
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
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/[email protected]/
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
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
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
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
The following commit has been merged into the x86/cache branch of tip:
Commit-ID: fc747eebef734563cf68a512f57937c8f231834a
Gitweb: https://git.kernel.org/tip/fc747eebef734563cf68a512f57937c8f231834a
Author: Babu Moger <[email protected]>
AuthorDate: Wed, 24 Jan 2024 11:52:56 -06:00
Committer: Borislav Petkov (AMD) <[email protected]>
CommitterDate: Thu, 25 Jan 2024 00:41:59 +01:00
x86/resctrl: Remove redundant variable in mbm_config_write_domain()
The kernel test robot reported the following warning after commit
54e35eb8611c ("x86/resctrl: Read supported bandwidth sources from CPUID").
even though the issue is present even in the original commit
92bd5a139033 ("x86/resctrl: Add interface to write mbm_total_bytes_config")
which added this function. The reported warning is:
$ 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")
Reported-by: kernel test robot <[email protected]>
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Signed-off-by: Babu Moger <[email protected]>
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
Acked-by: Reinette Chatre <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 15 ++++-----------
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 2b69e56..aa24343 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,
- struct rdt_domain *d, u32 evtid, u32 val)
+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 @@ next:
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;
}
}
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